Closed Bug 1028216 Opened 10 years ago Closed 10 years ago

Async animation fps while zooming in has regressed

Categories

(Core :: Graphics, defect, P1)

Product:
Core
Component:
Graphics
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla33
Project Flags:
blocking-b2g 2.0+
Tracking Flags:
Tracking Status
firefox32 --- fixed
firefox33 --- fixed
b2g-v2.0 --- verified
b2g-v2.1 --- verified

People

(Reporter: vingtetun, Assigned: mattwoodrow)

References

Details

(Keywords: perf, regression, Whiteboard: [c=regression p= s= u=2.0])

Attachments

(1 file)

Backout of bug 952721
2.86 KB, patch
lmandel
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
The lock screen animation, or the new inline animation, or even the homescreen animation when an app is launched have regressed and we often missed them.

I'm pretty sure the regression is because of bug 952721 that use the current frame size instead of the reference size. Currently rebuilding by locally reverting this change to see if that's it.

Pretty sure this should block 2.0.
Yep. Reverting the changes from bug 952721 fix the issue for me.

Milan, is it already a known bug, and if not do you have resources to work on it ?
Flags: needinfo?(milan)
So, regressed since ... 1.4/30?  Do you have any numbers on what we had before what we have now, etc., triage will likely need it for the blocking status.
Matt, the "second" change to bug 952721 seemingly removed the 4k x 4k limit, but I imagine that's only at a first glance?  Or, is there anything else you can think of that could be a cause of a slowdown Vivien describes?
Flags: needinfo?(milan) → needinfo?(matt.woodrow)
(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #1)
> Yep. Reverting the changes from bug 952721 fix the issue for me.

Is this about the first patch of that bug, which was uplifted even to 30, or about the second, which only landed on 32?
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #4)
> (In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails,
> needinfo? please) from comment #1)
> > Yep. Reverting the changes from bug 952721 fix the issue for me.
> 
> Is this about the first patch of that bug, which was uplifted even to 30, or
> about the second, which only landed on 32?

About the second.
(In reply to Milan Sreckovic [:milan] from comment #2)
> So, regressed since ... 1.4/30?  Do you have any numbers on what we had
> before what we have now, etc., triage will likely need it for the blocking
> status.

I don't have numbers - but if you unlock the phone with/without the patch, you will see it instantly. With the patch the animation is choppy, without it, its butter-smooth.
(In reply to Milan Sreckovic [:milan] from comment #3)
> Matt, the "second" change to bug 952721 seemingly removed the 4k x 4k limit,
> but I imagine that's only at a first glance?  Or, is there anything else you
> can think of that could be a cause of a slowdown Vivien describes?

The previous version of the code was using the reference size to decide if the element needs to be pre-rendered or not. The new one looks for the current size AFAICT.

As a result, scaling an application which takes the size of the screen by default, result into the layer beeing pre-rendered at first, and then the code enter again this function during the animation, and, I think cancel the pre-rendering.
blocking-b2g: 2.0? → 2.0+
Blocks: 952721
Seems wrong to block on something that I can't tell when it's done, short of asking Vivien to run and approve, so qawanted to give us the performance numbers before and after.  It would also be good to formalize the regression range (comment 1 suggests somewhere around May 2nd.)
Keywords: qawanted
Mike, do we have any performance tests or anything that would catch regressions like this in less than 6 weeks it took to see this one?
Flags: needinfo?(mlee)
(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #7)
> ...
> As a result, scaling an application which takes the size of the screen by
> default, result into the layer beeing pre-rendered at first, and then the
> code enter again this function during the animation, and, I think cancel the
> pre-rendering.

Right, because it is now smaller on the screen (the change makes us look at the size on the screen).  Matt would know the reasoning for the original change, but it's quite possible that approach used in better suited for zoom in (when the image takes more in screen space than the original), rather than the zooming out (when the image takes less space than the original), which is the case here.
(In reply to Milan Sreckovic [:milan] from comment #11)
> Vivien, Flame device?

Yes.
Ok, I think we should probably back the second patch from bug 952721 out for now.

The change that bug made was to only prerender transformed content if the post-transformed size was approximately the screen size or smaller. Since we try to draw transformed content at it's final resolution, this correlates to the texture size we have to allocate.

This bug isn't really because of pre-rendering, but because we use the same test to determine if we can do async animations. For the app launch animation, we only ever move content offscreen (by animating scale), so pre-rendering the currently offscreen content isn't actually necessary. We don't have code currently to analyze the animation to determine this though.

I think the best way to fix this is to go back to our old code for determining when to pre-render, and instead limit the resolution we apply to the layers. This will mean that if you scale something up massively (such that it would be larger than the max texture size), and it hits the pre-render path, then you'll get it draw at a lower resoltion and scaled up at composition time.
Flags: needinfo?(matt.woodrow)
Thanks Matt - we'll need to uplift this to Aurora.

I think I can sort of see what Vivien is talking about, but it isn't easy with the rebuild in between (really need the side by side comparison).
Assignee: nobody → matt.woodrow
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/c0e27620432b
Keywords: leave-open
Whiteboard: [leave open]
Priority: -- → P1
Whiteboard: [c=regression p= s= u=]
Severity: normal → blocker
Whiteboard: [c=regression p= s= u=] → [c=regression p= s= u=2.0]
Blocks: 1018592
Not sure how to reconcile leave-open and needing uplift - Matt, are we OK with Aurora uplift of this patch?
Flags: needinfo?(matt.woodrow)
Approval Request Comment
[Feature/regressing bug #]: Bug 952721
[User impact if declined]: Homescreen FPS drop on b2g
[Describe test coverage new/current, TBPL]:
[Risks and why]: Low risk, just backing out an existing change.
[String/UUID change made/needed]: None
Attachment #8446883 - Flags: approval-mozilla-aurora?
Flags: needinfo?(matt.woodrow)
Leaving the bug open because I want to remember to have another shot at fixing the underlying issue.
Comment on attachment 8446883 [details] [diff] [review]
Backout of bug 952721

Aurora approval granted.
Attachment #8446883 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I think the best solution here is to allow pre-rendering of most transforms, and adjust the ThebesLayer resolution within the transform to be the highest we can manage without ridiculous sizes.

Unfortunately we don't know the size of ThebesLayers until we've finished adding all the items into it, and by that time we've already made decisions based on the resolution we chose during creation (like checking if component alpha items are on top of fully opaque pixels).

We can get an upper-bound estimate by taking the visible rect of the current display list, which I think would be reasonable to use for this purpose.

Then we can see what allocation size will be needed for the requested resolution, and use a scale transform on the layer if it's too big. Need to be careful not to regress tiled scrollable layers in the process.

roc, how does that sound? I don't think your ComputeVisibility changes should affect this too much.
Flags: needinfo?(roc)
(In reply to Matt Woodrow (:mattwoodrow) from comment #21)
> We can get an upper-bound estimate by taking the visible rect of the current
> display list, which I think would be reasonable to use for this purpose.

Yes, that sounds good.

> Then we can see what allocation size will be needed for the requested
> resolution, and use a scale transform on the layer if it's too big. Need to
> be careful not to regress tiled scrollable layers in the process.
> 
> roc, how does that sound? I don't think your ComputeVisibility changes
> should affect this too much.

Yeah. This sounds great.
Flags: needinfo?(roc)
(In reply to Milan Sreckovic [:milan] from comment #8)
> Seems wrong to block on something that I can't tell when it's done, short of
> asking Vivien to run and approve, so qawanted to give us the performance
> numbers before and after.  It would also be good to formalize the regression
> range (comment 1 suggests somewhere around May 2nd.)

QA Wanted will need a clear STR (including expected behavior and actual behavior), on what device/branch this bug is occurring, and/or on what device/branch you want this tested. Comment 0 mentioned 'The lockscreen animation' which is not very specific on actual bug behavior.
Flags: needinfo?(milan)
STR: Unlock the screen.  Observe the zoom animation that gets us to the home screen.
Expected: Smooth animation.
Observed: Not smooth.
Flags: needinfo?(milan)
(In reply to Milan Sreckovic [:milan] from comment #24)
> STR: Unlock the screen.  Observe the zoom animation that gets us to the home
> screen.
> Expected: Smooth animation.
> Observed: Not smooth.

Not to be confused with bug 1024483 of course.
I *think* I can reproduce this... but am not 100% positive so I made a video on latest Flame 2.0 Aurora.
Please verify if this is indeed reproducing the bug.

Video: https://www.youtube.com/watch?v=OpaW6BMBj5c

Tested on:
Device: Flame
Build ID: 20140702035037
Gaia: 085cdbf16dfd5249786016ac8ceafa1a54e88497
Gecko: a6e69640a00b
Version: 32.0a2 (2.0)
Firmware Version: B1TC00011220

(In reply to Milan Sreckovic [:milan] from comment #8)
> qawanted to give us the performance numbers before and after.

By performance numbers do you mean enabling FPS in Developer HUD and observe the numbers?
And by "before and after" could you specify before what and after what? (if you still need this information)
Flags: needinfo?(milan)
QA Contact: pcheng
Hi Pi Wei, you may need to go back to an older build, and something landed in this bug that would improve performance.  Here's a more detailed STR, with particular builds to compare.  I'm sure you will be able to observe the difference "by eye" and it would be great if you can find some numbers that describe that difference (FPS if that does it?):
Compare these two builds:

https://pvtbuilds.mozilla.org/pvt/mozilla.org/b2gotoro/nightly/mozilla-central-flame-eng/2014/06/2014-06-22-16-02-03/

and

https://pvtbuilds.mozilla.org/pvt/mozilla.org/b2gotoro/nightly/mozilla-central-flame-eng/2014/06/2014-06-30-16-02-02/

For each of those, install 18-20 apps from the app store (doesn't seem to matter which ones.)
Make sure you stay in the 3 per row icon setup.
Scroll to the bottom of the homescreen.
(Lock the phone.  Unlock and observe the smoothness of the animation ) x repeat

Let me know how this works out.
Flags: needinfo?(milan)
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → fixed
status-firefox32: --- → affected
status-firefox33: --- → fixed
Whiteboard: [c=regression p= s= u=2.0] → [c=regression p= s= u=2.0] [checkin-needed-aurora]
Once this lands on Aurora, we will close this bug.  Good thoughts from comment 21 and comment 22 can go into a separate, non-2.0 blocker so that we don't lose track of it.
https://hg.mozilla.org/releases/mozilla-aurora/rev/d850cfb6a2ff

Agreed 100% that follow-up work is best suited for a follow-up bug rather than cramming it into this one.
Status: NEW → RESOLVED
Closed: 10 years ago
status-b2g-v2.0: affected → fixed
status-firefox32: affected → fixed
Keywords: leave-open
Resolution: --- → FIXED
Whiteboard: [c=regression p= s= u=2.0] [checkin-needed-aurora] → [c=regression p= s= u=2.0]
Target Milestone: --- → mozilla33
Removing QA wanted due to bug being marked as fixed.

(In reply to Milan Sreckovic [:milan] from comment #27)
> For each of those, install 18-20 apps from the app store (doesn't seem to
> matter which ones.)
> Make sure you stay in the 3 per row icon setup.
> Scroll to the bottom of the homescreen.
> (Lock the phone.  Unlock and observe the smoothness of the animation ) x repeat

I installed 18 apps on the provided 6/22 build, scrolled to bottom, lock & unlock for 10+ times and it unlocks smoothly, I think perhaps even more smooth than the video I did at comment 26.

The same goes for 6/30 build, smooth unlocking, as smooth as 6/22 build.

Now that the qawanted tag has been removed, let me know if you still want this worked on.

NI Josh for qawanted lead triage/review.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Keywords: qawanted
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
Flags: needinfo?(mlee)
Verified the issue is fixed on 2.1 and 2.0
As per steps provided in 27, the animation is smooth when lock and unlock the device with downloaded 20 +apps

Device: Flame 2.1
BuildID: 20141029001202
Gaia: eb0aab0f13c78c7ac378ad860e865c4b6eaf669f
Gecko: 318019f80a8e
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Flame 2.0 KK
BuildID: 20141029000205
Gaia: 9f5b6f025e528fabfcc068782cb9b492cb51a7f9
Gecko: de8cfd54bf93
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 32.0 (2.0)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
status-b2g-v2.0: fixed → verified
status-b2g-v2.1: fixed → verified
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: