Closed Bug 1089538 Opened 10 years ago Closed 6 years ago

[Contacts] The thumbnail is slowly appeared when scrolls contact list

Categories

(Firefox OS Graveyard :: Gaia::Contacts, defect, P1)

ARM
Gonk (Firefox OS)

Tracking

(tracking-b2g:backlog)

RESOLVED WONTFIX
tracking-b2g backlog

People

(Reporter: edchen, Assigned: arcturus)

References

()

Details

(Keywords: perf, Whiteboard: [2.1-bug-bash][TPE][p=3][perf-wanted])

Attachments

(3 files)

*** Build Information
Gaia-Rev        1e48e3e40e0780c0cd07a3457e5fe2efeeb542d1
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/09fb60a37850
Build-ID        20141023001201
Version         34.0
Device-Name     flame
Base image: ***Please update this field to say if you're running the v180 or the v188 base image***


*** Description
The thumbnail is slowly appeared when scrolls contact list.

*** Steps to Reproduce
1. Flash the gaia
2. Make reference-workload-light
3. Launch contact app
4. Scroll the contact list

*** Expected Results
The thumbnail should be normal displayed. 

*** Actual Results
The thumbnail is slowly appeared when scrolls contact list.

*** Other Notes


*** Reproduction Frequency: 
100%
Do we have any numbers about the objectives in terms of performance for this feature?
Flags: needinfo?(francisco)
Whiteboard: [2.1-FC-bug-bash][TPE] → [2.1-bug-bash][TPE]
Yes, contacts list will never try to go under 60fps when scrolling.

Right now we are around 58-60.
Flags: needinfo?(francisco)
Keywords: perf
Edward, can you get a video of the perceived perception of this issue?   while francisco comments that scrolling is inline with 60 fps expectations, i'd like to see how bad the user behavior is.
Flags: needinfo?(edchen)
Tony - Uploaded video. http://youtu.be/CUq1aqbN3I8
Flags: needinfo?(edchen)
I think we should take a look to this bug in next sprint. At least see how much effort we need to put here.
Severity: normal → major
blocking-b2g: --- → backlog
Target Milestone: --- → 2.2 S9 (3apr)
Agree with backlog.  not showing the thumbnails while scrolling normally isnt a good user experience.  but i woudlnt block as it eventually does load.
Target Milestone: 2.2 S9 (3apr) → 2.1 S9 (21Nov)
Assignee: nobody → francisco
Whiteboard: [2.1-bug-bash][TPE] → [2.1-bug-bash][TPE][p=4]
Status: NEW → ASSIGNED
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Hei Carrie, Fang

could you take a look to how it looks the new way of loading images.

On the right you can see the current behaviour, on the left, default images appear instantly and then we preload any other image.

Do you agree with this behavior? Any suggestion?

I still need to properly measure memory and fps but to me it's looking promising
Attachment #8522314 - Flags: feedback?(fshih)
Attachment #8522314 - Flags: feedback?(cawang)
Comment on attachment 8522314 [details]
PoC for default thumbnail appearing fast

Hi Francisco, 

Of course the left one is much better. Actually, a similar issue happens in Email as well - the texts will slowly appear while scrolling. Hence, they display grey lines to replace the texts lines and then the real texts appear. I think our approach here is based on the same logic. It works fine to me. Thanks!
Attachment #8522314 - Flags: feedback?(cawang) → feedback+
Comment on attachment 8522314 [details]
PoC for default thumbnail appearing fast

For the visual part looks good to me as well! Thanks!
Attachment #8522314 - Flags: feedback?(fshih) → feedback+
Dear Francisco,
  Could you do me a faver to attach the patch in commet 7 to this bugzilla or email me, I can‘t reach location of www.youtube.com in my network. 
  Thanks a lot.
Flags: needinfo?(francisco)
Flags: needinfo?(francisco)
(In reply to Shiwei Zhang from comment #12)
> Dear Francisco,
>   Could you do me a faver to attach the patch in commet 7 to this bugzilla
> or email me, I can‘t reach location of www.youtube.com in my network. 
>   Thanks a lot.

Hi Shiwei,

just attached the current patch (not ready to land):

https://bug1089538.bugzilla.mozilla.org/attachment.cgi?id=8525958
Target Milestone: 2.1 S9 (21Nov) → 2.2 S1 (5dec)
Whiteboard: [2.1-bug-bash][TPE][p=4] → [2.1-bug-bash][TPE][p=3]
(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #14)
> 
> Hi Shiwei,
> 
> just attached the current patch (not ready to land):
> 
> https://bug1089538.bugzilla.mozilla.org/attachment.cgi?id=8525958

Dear Francisco,
  I havd a test on this patch, default-photo(initial photo) display synchronously, but thumbnails(pictures I selected from gallery) still display slowly. However, there is no similar issue on v1.4. 
  Is there any thing we can do to improve this patch?
Dear Francisco,
  Could you help me to review this patch(bug367054_contact_photo_display_slow.patch). I think and test patch_1089538 didn't realize the effect that item display default "group" background before displaying the real photo, but this patch realize it.
  In this patch, I make some changes, some of them are same with patch_1089538, and the other changes are mainly used to optimize the display speed and solve some problems. One problem is when we scroll contact list, we can find that default background appear faster than the photos, so there is a "blank" time for some items. To avoid the blank, I add and use interface displayDefaultPhoto() before finishing load the photo.

  Thanks a lot.
Attachment #8541991 - Attachment description: Display default background before load the thumbnail. → bug367054_contact_photo_display_slow.patch
Target Milestone: 2.2 S1 (5dec) → ---
A dupe of this bug has been filed recently. It seems the activity on this bug has stopped a couple of weeks ago. Francisco, do you think the PoC is good enough to be landed? Do you want some checks by QA before landing it?
Flags: needinfo?(francisco)
Hi Johan,

we have even a modification of the PoC from a contributor.

But we didn't have much time to work with it. Do you think is possible to have a QA test on that?
Flags: needinfo?(francisco)
Comment on attachment 8541991 [details] [diff] [review]
bug367054_contact_photo_display_slow.patch

I think I can help out here ;)
Attachment #8541991 - Flags: qa-approval?(jlorenzo)
Comment on attachment 8541991 [details] [diff] [review]
bug367054_contact_photo_display_slow.patch

Review of attachment 8541991 [details] [diff] [review]:
-----------------------------------------------------------------

This patch helps to reduce the time to load the default image but increase notably the time of the cold start of the app (see this video as a comparison: http://mzl.la/1ErcZ2s). It wasn't the case in the video called "PoC for default thumbnail appearing fast". So I would prefer to get the implementation demonstrated in this video rather than this one.

Also, the patch changes the color of the circle and leaves some ambiguous documentation.

Francisco, can we go with patch_1089538.diff?

::: apps/communications/contacts/js/views/list.js
@@ +1018,5 @@
> +    if (defaultImage) {
> +      renderDefaultPhoto(img, link, group);
> +      if (link.dataset.visited !== 'true') {
> +        // To avoid display blank, first display default picture.
> +        // modify for bug367054.

Please do not mention a bug which is not on bugzilla.mozilla.org. Bug 367054 exists and is not related to this change.

@@ +1035,5 @@
>        return;
>      }
>  
>      if (img) {
> +      //delete img.dataset.group;

Commented code.

@@ +1043,5 @@
>      }
>  
> +    //var figure = photoTemplate.cloneNode(true);
> +    //img = figure.children[0];
> +    //setImageURL(img, photo);

Commented code.

@@ +1048,2 @@
>  
> +    //link.insertBefore(figure, link.children[0]);

Commented code.

@@ +1119,5 @@
>      setImageURL(img, null);
>    };
>  
> +  // Display default group photo.
> +  // add for bug367054

Please do not mention a bug which is not on bugzilla.mozilla.org. Bug 367054 exists and is not related to this change.

::: apps/communications/contacts/style/contacts.css
@@ +765,4 @@
>    left: 0;
>    width: 100%;
>    height: 100%;
> +  color: #00f;

Why do we change the color of the text and the background?

::: shared/js/contacts/utilities/image_loader.js
@@ +113,5 @@
>          --imgsLoading;
>          image.style.backgroundImage = 'url(' + src + ')';
>          if (tmp.complete) {
> +          // Delete group background when item will display real photo.
> +          // modify for bug367054.

Please do not mention a bug which is not on bugzilla.mozilla.org. Bug 367054 exists and is not related to this change.

@@ +129,5 @@
>      }
>  
>      /**
> +     *  Display default background.
> +     *  add for bug367054.

Please do not mention a bug which is not on bugzilla.mozilla.org. Bug 367054 exists and is not related to this change.

@@ +134,5 @@
> +     */
> +    function displayDefaultBackground(item) {
> +      var image = item.querySelector('span[data-type=img][data-group]');
> +      if (image) {
> +        // var src = '/contacts/style/images/Imagery.png';

Commented code.

@@ +135,5 @@
> +    function displayDefaultBackground(item) {
> +      var image = item.querySelector('span[data-type=img][data-group]');
> +      if (image) {
> +        // var src = '/contacts/style/images/Imagery.png';
> +        // image.style.backgroundImage = 'url(' + src + ')';

Commented code.

@@ +199,5 @@
>  
> +        // If theItem has not display any picture, or
> +        // if theItem needs to display real photo but has still not display,
> +        // it need to call loadImage to display.
> +        // modify for bug367054.

Please do not mention a bug which is not on bugzilla.mozilla.org. Bug 367054 exists and is not related to this change.
Attachment #8541991 - Flags: qa-approval?(jlorenzo) → qa-approval-
See comment 23.
Flags: needinfo?(francisco)
I couldn't test patch_1089538.diff, it doesn't apply anymore on the current master.
Let me rebase the patch
Flags: needinfo?(francisco)
blocking-b2g: backlog → ---
Priority: -- → P1
Whiteboard: [2.1-bug-bash][TPE][p=3] → [2.1-bug-bash][TPE][p=3][perf-wanted]
Firefox OS is not being worked on
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: