767980 - Tab drawer animation should be smoother

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ian Barlow (:ibarlow)]

Opening and closing the tab drawer still feels pretty janky, we should try to at least double the frame rate we are getting, to make it feel smoother.

Comment 1

[Chris Lord [:cwiiis]]

Imho, I think we should settle for nothing less than 60fps UI animations, or at least they need to be as smooth as our page scrolling update (which, on most hardware, is 60fps).

We had a quick look at the code in the office, and the problem here is that the animation is relayouting on every frame - this isn't really a tenable way of getting smooth animations (at least, not on Android...)

As reference, Android's animation framework operates separately to layout. This is a common feature of UI toolkits that have animation. In ICS (and I assume Honeycomb?), you can animate transformations and opacity.

The tabs animation, for example, could be implemented by doing a y-coordinate translation transformation on the widgets stacked above the tab-picker (i.e. the awesomebar and page), then relayouting once this animated transform has finished.

Comment 2

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

(In reply to Chris Lord [:cwiiis] from comment #1)

> The tabs animation, for example, could be implemented by doing a
> y-coordinate translation transformation on the widgets stacked above the
> tab-picker (i.e. the awesomebar and page), then relayouting once this
> animated transform has finished.

This is the ICS phone animation, where everything slides down. On tablets, where things slide to the right, we translate the browser area and the tabs sidebar, but the browser toolbar is resized. When the animation is over, we resize the browser area too, since having the tabs sidebar visible should not affect the ability to use the browser.

On phones, the browser can'r be used while the tabs area is visible. Any attempt to use a menu or enter a URL will cause the tabs area to disappear.

Comment 3

[Sriram Ramasubramanian [:sriram]]

(In reply to Chris Lord [:cwiiis] from comment #1)
> Imho, I think we should settle for nothing less than 60fps UI animations, or
> at least they need to be as smooth as our page scrolling update (which, on
> most hardware, is 60fps).
> 
> We had a quick look at the code in the office, and the problem here is that
> the animation is relayouting on every frame - this isn't really a tenable
> way of getting smooth animations (at least, not on Android...)
> 
> As reference, Android's animation framework operates separately to layout.
> This is a common feature of UI toolkits that have animation. In ICS (and I
> assume Honeycomb?), you can animate transformations and opacity.
> 
> The tabs animation, for example, could be implemented by doing a
> y-coordinate translation transformation on the widgets stacked above the
> tab-picker (i.e. the awesomebar and page), then relayouting once this
> animated transform has finished.

In phones and tablets, to get a better feel, there is a "translation". On tablets, a transformation of the "browser size" happens, only after translation. The black portion popping up on tablets is because of the speed the tablets have. And I don't think frame rate can help us here. I am using a 10ms interval, which is the default interval for animation in Android.

Comment 4

[Chris Lord [:cwiiis]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #3)
> (In reply to Chris Lord [:cwiiis] from comment #1)
> > Imho, I think we should settle for nothing less than 60fps UI animations, or
> > at least they need to be as smooth as our page scrolling update (which, on
> > most hardware, is 60fps).
> > 
> > We had a quick look at the code in the office, and the problem here is that
> > the animation is relayouting on every frame - this isn't really a tenable
> > way of getting smooth animations (at least, not on Android...)
> > 
> > As reference, Android's animation framework operates separately to layout.
> > This is a common feature of UI toolkits that have animation. In ICS (and I
> > assume Honeycomb?), you can animate transformations and opacity.
> > 
> > The tabs animation, for example, could be implemented by doing a
> > y-coordinate translation transformation on the widgets stacked above the
> > tab-picker (i.e. the awesomebar and page), then relayouting once this
> > animated transform has finished.
> 
> In phones and tablets, to get a better feel, there is a "translation". On
> tablets, a transformation of the "browser size" happens, only after
> translation. The black portion popping up on tablets is because of the speed
> the tablets have. And I don't think frame rate can help us here. I am using
> a 10ms interval, which is the default interval for animation in Android.

I don't see frame-rates anywhere near 60fps (to quantify, in the absolute best case I see 30, and usually what looks like sub-15), so I guess we're doing too much work on each frame - also, why animate at a 10ms interval? The refresh on every android device I've used has been 60Hz, so a 16ms interval would make more sense - even better, make the animation time-based and just request a draw (there's some left-over code that may be useful for this in bug 711959)

It's really jarring to have UI animations running at a lower frame-rate than panning/zooming on the content area.

Comment 5

[Chris Lord [:cwiiis]]

No activity on this?

I see this as a release blocker, personally - I'm fairly sure a large proportion of users are going to change tabs at some point, and currently it feels really bad.

Comment 6

[Aaron Train [:aaronmt]]

Depositing my vote into this bug bucket.

Comment 8

[Lucas Rocha (:lucasr)]

Attachment: (1/5) Don't make assumptions about time passed in PropertyAnimator

Comment 9

[Lucas Rocha (:lucasr)]

Attachment: (2/5) Fix rounding issues in PropertyAnimator's interpolator

Comment 10

[Lucas Rocha (:lucasr)]

Attachment: (3/5) Take advantage of vsync'd drawing on post-JB in PropertyAnimator

Comment 11

[Lucas Rocha (:lucasr)]

Attachment: (4/5) Use faster interpolator for the tabs pane animation

Comment 12

[Lucas Rocha (:lucasr)]

Attachment: (5/5) Reimplement PropertyAnimator in terms of view proxies

Comment 13

[Lucas Rocha (:lucasr)]

Attachment: Create SurfaceView/TextureView after view tree is created

This patch correctly enables TextureView when we're running on >= ICS in a hardware accelerated window. The check was being done too early and always returning false.

FYI: Apparently, enabling TextureView causes a visible panning performance regression. We need TextureView to be able to run animations in our main UI using hardware layers (ideal for super smooth performance). I filed bug 792259 to track this. snorp will be having at look at it in the next days.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 662338 [details] [diff] [review]
Create SurfaceView/TextureView after view tree is created

>diff --git a/mobile/android/base/gfx/LayerView.java b/mobile/android/base/gfx/LayerView.java
>+    public void initializeView(EventDispatcher eventDispatcher) {
>+        // This check should not done while the view tree is still being created

should not *be* done

>+        // as hardware acceleration will not be enabled at this point.
>+        // createLayerClient() is called on the initialization phase of GeckoApp,

s/createLayerClient()/initializeView()/

LGTM otherwise.

Comment 15

[Lucas Rocha (:lucasr)]

Here's a quick overview of what these patches do:

- On pre-ICS phones, the animation will be only slightly improved with my fixes to the interpolation code. Unfortunately, we can't animate using drawing caches (using the pre-Honeycomb animation framework) due to the use of SurfaceView for web rendering. We need to keep the scrolling-based animation which is not as smooth, especially on older phones. There's still some room for improvement here (e.g. using drawing caches whenever possible). I'll file a follow-up.

- On post-ICS phones, animations will be hardware accelerated and are super-smooth.

- On tablets, I replaced the animation based on frequent relayouts (impossible to be smooth) with a translation animation in the contents of the browser toolbar. This means the UI animation is very smooth. We still need to find a way to resize the web content area that doesn't look bad. Cross fading is an option but might not be trivial to do. Will file a follow-up.

Honeycomb is a weird case because it does provide have hardware acceleration for animations but we can't use it because we don't have TextureView on pre-ICS Android. In this case, we use the scrolling-based animation which hopefully doesn't look too bad on tablets (Honeycomb is not a big part of our user base anyway).

Comment 16

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

Comment on attachment 662334 [details] [diff] [review]
(5/5) Reimplement PropertyAnimator in terms of view proxies

This looks OK to me. Let's get Sriram to look over the toolbar changes too.

Comment 17

[Lucas Rocha (:lucasr)]

I just noticed that I forgot to handle the different states (default and pressed) for the fake right edge on the toolbar. I'll send a new patch to fix this once I'm back.

Comment 18

[Lucas Rocha (:lucasr)]

Attachment: (5/5) Reimplement PropertyAnimator in terms of view proxies

Here's the final patch with the correct the pressed/focused state in tablet UI.

Comment 19

[Lucas Rocha (:lucasr)]

(In reply to Lucas Rocha (:lucasr) from comment #15)
> Here's a quick overview of what these patches do:
> 
> - On pre-ICS phones, the animation will be only slightly improved with my
> fixes to the interpolation code. Unfortunately, we can't animate using
> drawing caches (using the pre-Honeycomb animation framework) due to the use
> of SurfaceView for web rendering. We need to keep the scrolling-based
> animation which is not as smooth, especially on older phones. There's still
> some room for improvement here (e.g. using drawing caches whenever
> possible). I'll file a follow-up.

Filed bug 793771
 
> - On tablets, I replaced the animation based on frequent relayouts
> (impossible to be smooth) with a translation animation in the contents of
> the browser toolbar. This means the UI animation is very smooth. We still
> need to find a way to resize the web content area that doesn't look bad.
> Cross fading is an option but might not be trivial to do. Will file a
> follow-up.

Filed bug 793769

Comment 20

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 664110 [details] [diff] [review]
(5/5) Reimplement PropertyAnimator in terms of view proxies

>> mGeckoLayout.setPadding(0, 0, 0, 0);
Do we need this? If so, it should be margin I believe. Padding will extend the background (just in case we have one).

>> mTabsPanel.setLayerType(View.LAYER_TYPE_HARDWARE, null);
I thought we enabled Hardware acceleration at application level, and hence all individual views will have it by default. Do we need to set/reset it?

>> private boolean shouldEnableHardwareLayer(ElementHolder element) {
Seems like this can be cleaned up a bit.

>>  <ImageView android:id="@+id/address_bar_bg"
>> <FrameLayout android:id="@+id/awesome_bar_right_edge"
Why do we need these?

>> private View mAwesomeBarRightEdge;
If we want the above one, a shadow copy of these should be created in other 5 browser_toolbar.xml.in's. This would ensure it being not null.

Comment 21

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #20)
> Comment on attachment 664110 [details] [diff] [review]
> (5/5) Reimplement PropertyAnimator in terms of view proxies
> 
> >> mGeckoLayout.setPadding(0, 0, 0, 0);
> Do we need this? If so, it should be margin I believe. Padding will extend
> the background (just in case we have one).

Good point, will change that.
 
> >> mTabsPanel.setLayerType(View.LAYER_TYPE_HARDWARE, null);
> I thought we enabled Hardware acceleration at application level, and hence
> all individual views will have it by default. Do we need to set/reset it?

This is not about set/reset hardware acceleration. Turning hardware acceleration at app level means that drawing operation on canvas will be hw accelerated. That's fine for general drawing but when it comes to animations of complex views (which is the case here), you want to some extra drawing optimization.

Hw layers will draw the view into a GL texture that is kept in video memory. It's the hw accelerated version of a drawing cache (see View's drawing cache methods for reference). Using layers will drastically reduce the number of drawing operations per frame. This is especially important if the animation involves any sort of blending while animating (which is the case in our tabs panel animation).

So, in practice, my patch is creating two layers: one for the tabs panel and another for the main layout. This means that we'll be simply redrawing two GL textures while animating, which is why the animation becomes super-smooth.

If you want to know more about this, have a look at:
  http://developer.android.com/guide/topics/graphics/hardware-accel.html#layers-anims

> >> private boolean shouldEnableHardwareLayer(ElementHolder element) {
> Seems like this can be cleaned up a bit.

What do you mean? I split the conditions in different ifs to make them easier to read.
 
> >>  <ImageView android:id="@+id/address_bar_bg"
> >> <FrameLayout android:id="@+id/awesome_bar_right_edge"
> Why do we need these?

I should have probably added a comment about this. The point of the specific changes in the tablet toolbar is to allow us to animate it without causing relayouts on each frame (which will not be smooth). I refactored the animation in terms of translating the inner elements of the tablet toolbar to give the *impression* of resizing. In order to do this, I added a "fake" right edge for the entry border which is kept in the same position during the animation (hence the negative translation value) while the elements on the left (favicon, back, forware, lock icon, title, ...) slide "underneath" it.

I'll add the description above to the code for future reference.

> >> private View mAwesomeBarRightEdge;
> If we want the above one, a shadow copy of these should be created in other
> 5 browser_toolbar.xml.in's. This would ensure it being not null.

There's no need to copy it to other layouts as this will only be used on the tablet UI during the horizontal slide animation (which only runs when we have a tabs sidebar). There's no need to ensure it's not null on phone and large-screen UIs as it will be simply ignored.

Comment 22

[Lucas Rocha (:lucasr)]

Attachment: Reimplement PropertyAnimator in terms of view proxies

mfinkle, I factored out the AnimatorProxy code into a self-contained class to be able to use it in other parts of our code (I plan to use it in the swipe-to-dismiss behaviour on the tabs tray). Worth getting a second look.

sriram, I added a couple extensive comments describing the animation on the tablet toolbar. I hope they clarify the layout changes.

Comment 23

[Lucas Rocha (:lucasr)]

Attachment: Disable TextureView support in LayerView for now

Comment 24

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

Comment on attachment 666543 [details] [diff] [review]
Reimplement PropertyAnimator in terms of view proxies

Looks ok. Sriram might have some feedback related to changes he is making for personas. Not sure if these changes might cause some bitrot for him.

Comment 25

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 666543 [details] [diff] [review]
Reimplement PropertyAnimator in terms of view proxies

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

This looks good to me.

Comment 26

[Lucas Rocha (:lucasr)]

FYI: The changes in these patches are relatively risky (especially on tablets) to land with such a short time before the merge. I'll only land these patches after the merge next week.

Comment 27

[Lucas Rocha (:lucasr)]

Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1ea82d15f11
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f085751610f
https://hg.mozilla.org/integration/mozilla-inbound/rev/b992d80a7998
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2dabf55ea41
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e416d28e933
https://hg.mozilla.org/integration/mozilla-inbound/rev/913e25c339f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/9990a1074771

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c1ea82d15f11
https://hg.mozilla.org/mozilla-central/rev/0f085751610f
https://hg.mozilla.org/mozilla-central/rev/b992d80a7998
https://hg.mozilla.org/mozilla-central/rev/d2dabf55ea41
https://hg.mozilla.org/mozilla-central/rev/2e416d28e933
https://hg.mozilla.org/mozilla-central/rev/913e25c339f8
https://hg.mozilla.org/mozilla-central/rev/9990a1074771

Comment 29

[Aaron Train [:aaronmt]]

I think this or either bug 772940 has resulted in a black flash on startup. See bug 801477.

Comment 30

[Lucas Rocha (:lucasr)]

Comment on attachment 662330 [details] [diff] [review]
(1/5) Don't make assumptions about time passed in PropertyAnimator

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: This patch series hugely improve our tabs UX by making animations much smoother, especially on devices with hw acceleration support (ICS+). The difference is bigger on phones.
Testing completed (on m-c, etc.): These patches have been in m-c for a few weeks now with no issues.
Risk to taking this patch (and alternatives if risky): Given how long this has been in m-c without issues, I think the risk is relatively low.
String or UUID changes made by this patch: none.

Comment 31

[Lucas Rocha (:lucasr)]

Comment on attachment 662331 [details] [diff] [review]
(2/5) Fix rounding issues in PropertyAnimator's interpolator

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: This patch series hugely improve our tabs UX by making animations much smoother, especially on devices with hw acceleration support (ICS+). The difference is bigger on phones.
Testing completed (on m-c, etc.): These patches have been in m-c for a few weeks now with no issues.
Risk to taking this patch (and alternatives if risky): Given how long this has been in m-c without issues, I think the risk is relatively low.
String or UUID changes made by this patch: none.

Comment 32

[Lucas Rocha (:lucasr)]

Comment on attachment 662332 [details] [diff] [review]
(3/5) Take advantage of vsync'd drawing on post-JB in PropertyAnimator

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: This patch series hugely improve our tabs UX by making animations much smoother, especially on devices with hw acceleration support (ICS+). The difference is bigger on phones.
Testing completed (on m-c, etc.): These patches have been in m-c for a few weeks now with no issues.
Risk to taking this patch (and alternatives if risky): Given how long this has been in m-c without issues, I think the risk is relatively low.
String or UUID changes made by this patch: none.

Comment 33

[Lucas Rocha (:lucasr)]

Comment on attachment 662333 [details] [diff] [review]
(4/5) Use faster interpolator for the tabs pane animation

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: This patch series hugely improve our tabs UX by making animations much smoother, especially on devices with hw acceleration support (ICS+). The difference is bigger on phones.
Testing completed (on m-c, etc.): These patches have been in m-c for a few weeks now with no issues.
Risk to taking this patch (and alternatives if risky): Given how long this has been in m-c without issues, I think the risk is relatively low.
String or UUID changes made by this patch: none.

Comment 34

[Lucas Rocha (:lucasr)]

Comment on attachment 662338 [details] [diff] [review]
Create SurfaceView/TextureView after view tree is created

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: This patch series hugely improve our tabs UX by making animations much smoother, especially on devices with hw acceleration support (ICS+). The difference is bigger on phones.
Testing completed (on m-c, etc.): These patches have been in m-c for a few weeks now with no issues.
Risk to taking this patch (and alternatives if risky): Given how long this has been in m-c without issues, I think the risk is relatively low.
String or UUID changes made by this patch: none.

Comment 35

[Lucas Rocha (:lucasr)]

Comment on attachment 666543 [details] [diff] [review]
Reimplement PropertyAnimator in terms of view proxies

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: This patch series hugely improve our tabs UX by making animations much smoother, especially on devices with hw acceleration support (ICS+). The difference is bigger on phones.
Testing completed (on m-c, etc.): These patches have been in m-c for a few weeks now with no issues.
Risk to taking this patch (and alternatives if risky): Given how long this has been in m-c without issues, I think the risk is relatively low.
String or UUID changes made by this patch: none.

Comment 36

[Lucas Rocha (:lucasr)]

Comment on attachment 666561 [details] [diff] [review]
Disable TextureView support in LayerView for now

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: This patch series hugely improve our tabs UX by making animations much smoother, especially on devices with hw acceleration support (ICS+). The difference is bigger on phones.
Testing completed (on m-c, etc.): These patches have been in m-c for a few weeks now with no issues.
Risk to taking this patch (and alternatives if risky): Given how long this has been in m-c without issues, I think the risk is relatively low.
String or UUID changes made by this patch: none.

Comment 37

[bhavana bajaj [:bajaj]]

(In reply to Lucas Rocha (:lucasr) from comment #36)
> Comment on attachment 666561 [details] [diff] [review]
> Disable TextureView support in LayerView for now
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): n/a
> User impact if declined: This patch series hugely improve our tabs UX by
> making animations much smoother, especially on devices with hw acceleration
> support (ICS+). The difference is bigger on phones.
> Testing completed (on m-c, etc.): These patches have been in m-c for a few
> weeks now with no issues.
> Risk to taking this patch (and alternatives if risky): Given how long this
> has been in m-c without issues, I think the risk is relatively low.
> String or UUID changes made by this patch: none.

Can we maintain the status quo for this release and let these ride the trains instead of having them on aurora ? with so many patches I am concerned about the possible regressions

Comment 38

[bhavana bajaj [:bajaj]]

Comment on attachment 662330 [details] [diff] [review]
(1/5) Don't make assumptions about time passed in PropertyAnimator

As discussed offline with Lucas and Release Management team, we will have the enitire tab UX improvements ride the trains.