Tab drawer animation should be smoother

RESOLVED FIXED in Firefox 19

Status

()

RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: ibarlow, Assigned: lucasr)

Tracking

(Blocks: 1 bug, {perf})

15 Branch
Firefox 19
ARM
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox15 affected, firefox16 affected, fennec18+)

Details

(Whiteboard: ui-responsiveness)

Attachments

(7 attachments, 2 obsolete attachments)

2.87 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
2.30 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
9.18 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
4.50 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
3.49 KB, patch
kats
: review+
Details | Diff | Splinter Review
41.27 KB, patch
mfinkle
: review+
sriram
: review+
Details | Diff | Splinter Review
1.93 KB, patch
snorp
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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

6 years ago
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 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.
tracking-fennec: --- → ?
Keywords: perf
(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

6 years ago
(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

6 years ago
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.
Depositing my vote into this bug bucket.
status-firefox15: --- → affected
status-firefox16: --- → affected

Updated

6 years ago
OS: Mac OS X → Android
Hardware: x86 → ARM
(Assignee)

Updated

6 years ago
Duplicate of this bug: 783193
(Assignee)

Updated

6 years ago
Assignee: nobody → lucasr.at.mozilla
tracking-fennec: ? → +
(Assignee)

Updated

6 years ago
tracking-fennec: + → ?
Whiteboard: ui-responsiveness
tracking-fennec: ? → 18+
(Assignee)

Comment 8

6 years ago
Created attachment 662330 [details] [diff] [review]
(1/5) Don't make assumptions about time passed in PropertyAnimator
Attachment #662330 - Flags: review?(mark.finkle)
(Assignee)

Comment 9

6 years ago
Created attachment 662331 [details] [diff] [review]
(2/5) Fix rounding issues in PropertyAnimator's interpolator
Attachment #662331 - Flags: review?(mark.finkle)
(Assignee)

Comment 10

6 years ago
Created attachment 662332 [details] [diff] [review]
(3/5) Take advantage of vsync'd drawing on post-JB in PropertyAnimator
Attachment #662332 - Flags: review?(mark.finkle)
(Assignee)

Comment 11

6 years ago
Created attachment 662333 [details] [diff] [review]
(4/5) Use faster interpolator for the tabs pane animation
Attachment #662333 - Flags: review?(mark.finkle)
(Assignee)

Comment 12

6 years ago
Created attachment 662334 [details] [diff] [review]
(5/5) Reimplement PropertyAnimator in terms of view proxies
Attachment #662334 - Flags: review?(mark.finkle)
(Assignee)

Comment 13

6 years ago
Created attachment 662338 [details] [diff] [review]
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.
Attachment #662338 - Flags: review?(bugmail.mozilla)
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.
Attachment #662338 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 15

6 years ago
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).
Attachment #662330 - Flags: review?(mark.finkle) → review+
Attachment #662331 - Flags: review?(mark.finkle) → review+
Attachment #662332 - Flags: review?(mark.finkle) → review+
Attachment #662333 - Flags: review?(mark.finkle) → review+
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.
Attachment #662334 - Flags: review?(sriram)
Attachment #662334 - Flags: review?(mark.finkle)
Attachment #662334 - Flags: review+
(Assignee)

Comment 17

6 years ago
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.
(Assignee)

Comment 18

6 years ago
Created attachment 664110 [details] [diff] [review]
(5/5) Reimplement PropertyAnimator in terms of view proxies

Here's the final patch with the correct the pressed/focused state in tablet UI.
Attachment #662334 - Attachment is obsolete: true
Attachment #662334 - Flags: review?(sriram)
Attachment #664110 - Flags: review?(sriram)
(Assignee)

Comment 19

6 years ago
(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 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.
(Assignee)

Comment 21

6 years ago
(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.
(Assignee)

Updated

6 years ago
Depends on: 792259
(Assignee)

Comment 22

6 years ago
Created attachment 666543 [details] [diff] [review]
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.
Attachment #664110 - Attachment is obsolete: true
Attachment #664110 - Flags: review?(sriram)
Attachment #666543 - Flags: review?(sriram)
Attachment #666543 - Flags: review?(mark.finkle)
(Assignee)

Comment 23

6 years ago
Created attachment 666561 [details] [diff] [review]
Disable TextureView support in LayerView for now
Attachment #666561 - Flags: review?(snorp)
Attachment #666561 - Flags: review?(snorp) → review+
(Assignee)

Updated

6 years ago
No longer depends on: 792259
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.
Attachment #666543 - Flags: review?(mark.finkle) → review+
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.
Attachment #666543 - Flags: review?(sriram) → review+
(Assignee)

Comment 26

6 years ago
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.
I think this or either bug 772940 has resulted in a black flash on startup. See bug 801477.

Updated

6 years ago
Depends on: 801477

Updated

6 years ago
Blocks: 802139
(Assignee)

Comment 30

6 years ago
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.
Attachment #662330 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 31

6 years ago
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.
Attachment #662331 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 32

6 years ago
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.
Attachment #662332 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 33

6 years ago
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.
Attachment #662333 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 34

6 years ago
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.
Attachment #662338 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 35

6 years ago
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.
Attachment #666543 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 36

6 years ago
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.
Attachment #666561 - Flags: approval-mozilla-aurora?
(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 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.
Attachment #662330 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-

Updated

6 years ago
Attachment #662331 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-

Updated

6 years ago
Attachment #662332 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-

Updated

6 years ago
Attachment #662333 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-

Updated

6 years ago
Attachment #662338 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-

Updated

6 years ago
Attachment #666543 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-

Updated

6 years ago
Attachment #666561 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
You need to log in before you can comment on or make changes to this bug.