793721 - about:home scrolling is not smooth

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Lord [:cwiiis]]

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.

Comment 1

[Aaron Train [:aaronmt]]

This is greatly exaggerated now with it seems with the thumbnail enhancements on mozilla-inbound; about:home scrolling is painful.

Comment 2

[Wesley Johnston (:wesj)]

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.

Comment 3

[Aaron Train [:aaronmt]]

I've noticed that opening the tab tray too with a full page of thumbnails even opens the menu slower

Comment 4

[Lucas Rocha (:lucasr)]

(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.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch

This improves scrolling quite a bit.

Comment 6

[Lucas Rocha (:lucasr)]

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

Comment 7

[Chris Lord [:cwiiis]]

(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?

Comment 8

[Lucas Rocha (:lucasr)]

(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

Comment 9

[Wesley Johnston (:wesj)]

(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.

Comment 10

[Lucas Rocha (:lucasr)]

(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.

Comment 11

[Wesley Johnston (:wesj)]

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.

Comment 12

[Chris Lord [:cwiiis]]

(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?)

Comment 13

[Lucas Rocha (:lucasr)]

(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.

Comment 14

[Wesley Johnston (:wesj)]

(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?

Comment 15

[Chris Lord [:cwiiis]]

(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).

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Patch v2

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.

Comment 17

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

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 18

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 19

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea132a82593c

Leave this open for the real fix.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d40649836ffa

Comment 21

[Peter Retzer (:pretzer)]

Wes, is there anything left to do here with the new about:home or can it be resolved?

Comment 22

[Wesley Johnston (:wesj)]

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.

Comment 23

[Sriram Ramasubramanian [:sriram]]

We are tracking this in Bug 919777. We can close this.