Closed Bug 793721 Opened 8 years ago Closed 7 years ago

about:home scrolling is not smooth

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 19
Tracking Status
firefox19 --- affected

People

(Reporter: cwiiis, Unassigned)

Details

(Whiteboard: ui-responsiveness)

Attachments

(2 files, 1 obsolete file)

When scrolling up and down on the start page, it is visibly less smooth than scrolling on web-pages (somewhere in the region of 30fps or less). This is on a Galaxy Nexus.

As other, more complex apps and web-pages manage to scroll more smoothly, I think this should also scroll smoothly. It feels inconsistent otherwise.
This is greatly exaggerated now with it seems with the thumbnail enhancements on mozilla-inbound; about:home scrolling is painful.
I think the clipping is causing pain. I'll look at ways to simplify it, but we can turn that off easily if we need to.
I've noticed that opening the tab tray too with a full page of thumbnails even opens the menu slower
(In reply to Aaron Train [:aaronmt] from comment #3)
> I've noticed that opening the tab tray too with a full page of thumbnails
> even opens the menu slower

Yes, the new thumbnail stuff in about:home is causing performance issues for the tabs tray animation (i.e. dropping almost all frames). Let's either fix or disable the clipping asap. Serious regression.
Attached patch Patch (obsolete) — Splinter Review
This improves scrolling quite a bit.
Attachment #671921 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 671921 [details] [diff] [review]
Patch

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

I think you should only create the hw layer or drawing cache when you need it (i.e. while scrolling or animating). Keep in mind that you'll also need to cover the tabs tray animation case. Creating a hardware layer or drawing cache for the about:home content before/after the animation should work. See onPropertyAnimationStart/onPropertyAnimationEnd in BrowserApp.

::: mobile/android/base/AboutHomeContent.java.in
@@ +151,5 @@
>          mInflater.inflate(R.layout.abouthome_content, this);
>  
>          mTopSitesGrid = (TopSitesGridView)findViewById(R.id.top_sites_grid);
> +        if (android.os.Build.VERSION.SDK_INT >= 11)
> +            mTopSitesGrid .setLayerType(View.LAYER_TYPE_HARDWARE, null);

Hardware layers consume video memory and, ideally, should not be kept around indefinitely. What you probably want is to set the layer for the whole abouthome layout (not only the grid) while scrolling because painting can become very slow if about:home has too many children views.

@@ +153,5 @@
>          mTopSitesGrid = (TopSitesGridView)findViewById(R.id.top_sites_grid);
> +        if (android.os.Build.VERSION.SDK_INT >= 11)
> +            mTopSitesGrid .setLayerType(View.LAYER_TYPE_HARDWARE, null);
> +        else
> +            mTopSitesGrid .setDrawingCacheEnabled(true);

Same here: the drawing cache creates a bitmap that consumes memory. You should only use the cache when you need (e.g. while scrolling or animating). You can probably call setChildrenDrawnWithCacheEnabled() in AboutHomeContent when scrolling. Have a look at:

 http://developer.android.com/reference/android/view/ViewGroup.html#setChildrenDrawnWithCacheEnabled%28boolean%29
Attachment #671921 - Flags: review?(lucasr.at.mozilla) → review-
(In reply to Lucas Rocha (:lucasr) from comment #6)
> Comment on attachment 671921 [details] [diff] [review]
> Patch
> 
> Review of attachment 671921 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think you should only create the hw layer or drawing cache when you need
> it (i.e. while scrolling or animating). Keep in mind that you'll also need
> to cover the tabs tray animation case. Creating a hardware layer or drawing
> cache for the about:home content before/after the animation should work. See
> onPropertyAnimationStart/onPropertyAnimationEnd in BrowserApp.

Fly-by comment - if this is done, won't we incur a penalty at the beginning of every scroll and animation? Is the creation so quick that it doesn't have a noticeable effect on interactivity?
(In reply to Chris Lord [:cwiiis] from comment #7)
> (In reply to Lucas Rocha (:lucasr) from comment #6)
> > Comment on attachment 671921 [details] [diff] [review]
> > Patch
> > 
> > Review of attachment 671921 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I think you should only create the hw layer or drawing cache when you need
> > it (i.e. while scrolling or animating). Keep in mind that you'll also need
> > to cover the tabs tray animation case. Creating a hardware layer or drawing
> > cache for the about:home content before/after the animation should work. See
> > onPropertyAnimationStart/onPropertyAnimationEnd in BrowserApp.
> 
> Fly-by comment - if this is done, won't we incur a penalty at the beginning
> of every scroll and animation? Is the creation so quick that it doesn't have
> a noticeable effect on interactivity?

I've tested this quite a bit while working on the tabs tray animation. Enabling drawing cache and creating a hw layer doesn't seem to cause any problems to the animation. We're already doing that on the tabs tray animation, see:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#554
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/PropertyAnimator.java#124

This is actually what the official Android docs recommends:
http://developer.android.com/guide/topics/graphics/hardware-accel.html#layers-anims

In fact, this is exactly how ListView works: when scrolling starts, it will enable drawing cache on the scrolled views to avoid drawing overhead (on non-hw accelerated devices). See
https://github.com/android/platform_frameworks_base/blob/master/core/java/android/widget/AbsListView.java#L4867
(In reply to Lucas Rocha (:lucasr) from comment #6)
> I think you should only create the hw layer or drawing cache when you need
> it (i.e. while scrolling or animating).

AFAICT, there is no simple way to determine when scrolling ends on a ScrollView (we could cheat and override onScroll and potentially some other scroll methods to ensure we get the start event, but I seriously hate that. ListViews do it inside their implementation). Digging around for something better.

I think we would probably be better just to ensure this isn't enabled when about:home isn't showing. When it is showing, I think its probably worth using a small bit of extra memory.
(In reply to Wesley Johnston (:wesj) from comment #9)
> (In reply to Lucas Rocha (:lucasr) from comment #6)
> > I think you should only create the hw layer or drawing cache when you need
> > it (i.e. while scrolling or animating).
> 
> AFAICT, there is no simple way to determine when scrolling ends on a
> ScrollView (we could cheat and override onScroll and potentially some other
> scroll methods to ensure we get the start event, but I seriously hate that.
> ListViews do it inside their implementation). Digging around for something
> better.
> 
> I think we would probably be better just to ensure this isn't enabled when
> about:home isn't showing. When it is showing, I think its probably worth
> using a small bit of extra memory.

The problem is that when drawing cache or hardware layer are enabled, each invalidate() call will involve recreating them, which is not very efficient and might cause other responsiveness issues. For example, tapping on a top site item will invalidate the layer or cache.

Anyway, it's probably something worth trying. It's especially important to test this approach on lower end phones as (re)creating drawing caches is much slower on them.
After talking to mfinkle today, I think we're going to just remove the rounded corners at this point. They were a "bonus" feature for UX, and the time required to make them work vs the payout isn't that big.
(In reply to Wesley Johnston (:wesj) from comment #11)
> After talking to mfinkle today, I think we're going to just remove the
> rounded corners at this point. They were a "bonus" feature for UX, and the
> time required to make them work vs the payout isn't that big.

Rather than applying the mask live, couldn't we apply it to the Bitmap itself once? Also, even without this change, the about:home screen still wasn't fast :( (perhaps the attached patch fixes that?)
(In reply to Chris Lord [:cwiiis] from comment #12)
> (In reply to Wesley Johnston (:wesj) from comment #11)
> > After talking to mfinkle today, I think we're going to just remove the
> > rounded corners at this point. They were a "bonus" feature for UX, and the
> > time required to make them work vs the payout isn't that big.
> 
> Rather than applying the mask live, couldn't we apply it to the Bitmap
> itself once? Also, even without this change, the about:home screen still
> wasn't fast :( (perhaps the attached patch fixes that?)

Making it fast will probably be about enabling drawing cache or hardware layer when scrolling. It's probably slow right now because the view that is scrolled is too complex and involves drawing too many inner views.
Status: NEW → ASSIGNED
(In reply to Chris Lord [:cwiiis] from comment #12)
> Rather than applying the mask live, couldn't we apply it to the Bitmap
> itself once? Also, even without this change, the about:home screen still
> wasn't fast :( (perhaps the attached patch fixes that?)

Thats basically the same thing as turning these on, right?
(In reply to Wesley Johnston (:wesj) from comment #14)
> (In reply to Chris Lord [:cwiiis] from comment #12)
> > Rather than applying the mask live, couldn't we apply it to the Bitmap
> > itself once? Also, even without this change, the about:home screen still
> > wasn't fast :( (perhaps the attached patch fixes that?)
> 
> Thats basically the same thing as turning these on, right?

I've not read the code, so maybe I'm making incorrect assumptions here - I assumed this was slow because it was being applied on each draw - so I assume that we're already keeping a Bitmap around for the thumbnail, then masking, drawing the title on it, etc. - I'm suggesting that we should draw the title, mask, save that into a Bitmap and use that exclusively, without holding the thumbnail in memory.

It's basically the same as turning these on, but with lower memory use and without the impact/complication of choosing when to turn them on (I'd hope).
Attached patch Patch v2Splinter Review
This kills off the rounded corners and the shadow for now. I decided to look at Chris' suggestion and have a WIP that does our own caching of thumbnails (as well as some changes to make us show about:home faster). I'll post it as well, but it needs a bit more polish, and I want to track down some other bugs first. In the mean time, I'm fine with this fix.
Attachment #671921 - Attachment is obsolete: true
Attachment #673429 - Flags: review?(mark.finkle)
Attached patch WIP PatchSplinter Review
This does the caching by hand basically. I can't imagine it uses more memory than our current implementation which also caches the thumbnail Bitmap. I took care to make sure that isn't cached twice.

It also delays the thumbnail query so that we can quickly show about:home and fill in thumbnails later.

Because of the cache, if you leave about:home and come back we quickly show the cached thumbs. When the topSites query is updated we update thumbs and everything moves. I need to make that transition less jarring.
Comment on attachment 673429 [details] [diff] [review]
Patch v2

I'm fine with this to improve performance now. We can always add it back if we find another solution, like the caching WIP.
Attachment #673429 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea132a82593c

Leave this open for the real fix.
Whiteboard: ui-responsiveness → ui-responsiveness leave-open
Wes, is there anything left to do here with the new about:home or can it be resolved?
Flags: needinfo?(wjohnston)
Sriram has been working on this in some other bugs. I'll leave it up to him if he wants to keep this one open.
Flags: needinfo?(wjohnston) → needinfo?(sriram)
We are tracking this in Bug 919777. We can close this.
Flags: needinfo?(sriram)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: ui-responsiveness leave-open → ui-responsiveness
Target Milestone: --- → Firefox 19
You need to log in before you can comment on or make changes to this bug.