Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Material page transition is slow when we enable fade #13736

Closed
Hixie opened this issue Dec 21, 2017 · 50 comments · Fixed by flutter/engine#5420
Closed

Material page transition is slow when we enable fade #13736

Hixie opened this issue Dec 21, 2017 · 50 comments · Fixed by flutter/engine#5420
Assignees
@liyuqian
Labels
c: performance Relates to speed or footprint issues (see "perf:" labels) customer: gallery Relating to flutter/gallery repository. Please transfer non-framework issues there. customer: mulligan (g3) engine flutter/engine repository. See also e: labels. f: material design flutter/packages/flutter/material repository. f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels.
Projects
Milestone 3 Performance Debugging Experience
Milestone
bucket9

Comments

@Hixie
Copy link
Contributor

Hixie commented Dec 21, 2017

No description provided.

@Hixie Hixie added c: performance Relates to speed or footprint issues (see "perf:" labels) f: material design flutter/packages/flutter/material repository. c: regression It was better in the past than it is now labels Dec 21, 2017
@Hixie Hixie added this to the 3: Current Milestone milestone Dec 21, 2017
@Hixie Hixie self-assigned this Dec 21, 2017
@Hixie
Copy link
Contributor Author

Hixie commented Jan 18, 2018

Specifically, this seems to cause a ~2x regression on the GPU side.
See flutter_gallery__transition_perf average_frame_rasterizer_time_millis around 2017-12-20.

@Hixie
Copy link
Contributor Author

Hixie commented Jan 20, 2018

Here's what it looks like on Nexus 5 today:

MaterialPageRoute.debugEnableFadingRoutes = false
screenshot_20180119-173834

MaterialPageRoute.debugEnableFadingRoutes = true
screenshot_20180119-173951

@Hixie Hixie removed their assignment Jan 20, 2018
@Hixie Hixie added engine flutter/engine repository. See also e: labels. and removed c: regression It was better in the past than it is now labels Jan 20, 2018
@Hixie
Copy link
Contributor Author

Hixie commented Jan 20, 2018

cc @cbracken and @chinmaygarde for the engine side. @abarth was suggesting one trick we could use is noticing that a particular layer hasn't changed from frame to frame and not redrawing it when we change the opacity, or some such; I forget the precise details.

@abarth
Copy link
Contributor

abarth commented Jan 20, 2018

The idea is to be able to cache intermediate layers (in this case the one that gets blended back into the main buffer) rather than just leaves as we do today.

The framework could take each layer with a unique id (similar to the unique id we use for SkPicture). The compositor could then notice with a subtree had the same id as before and consider caching it.

@Hixie Hixie added this to To Do in Milestone 3 Jan 26, 2018
@Hixie Hixie moved this from To Do to Material Widgets in Milestone 3 Jan 26, 2018
@Hixie Hixie modified the milestones: 3: Current Milestone, 4: Next milestone Jan 26, 2018
@Hixie Hixie added this to Page Load Performance (chris) in Milestone 3 Jan 30, 2018
@sethladd
Copy link
Contributor

sethladd commented Feb 8, 2018

Can we turn the default back to "use opacity" in the meantime? I believe the perf hit is less bad than the current situation now.

@sethladd sethladd added the customer: gallery Relating to flutter/gallery repository. Please transfer non-framework issues there. label Feb 8, 2018
@Hixie
Copy link
Contributor Author

Hixie commented Feb 9, 2018

I'm happy to set the default to whatever you think is best. Please make sure you've tested it with it on and off on a low-end device before making a final decision though; the effect on low-end devices can be dramatic (as in, halving the frame rate, if the transition is only barely making 60Hz without the fade).

@sethladd
Copy link
Contributor

sethladd commented Mar 7, 2018

I wonder if this report is the same thing as this bug: https://stackoverflow.com/questions/49145258/flutter-routing-stutter

@xster
Copy link
Member

xster commented Mar 7, 2018

Unlikely since that example didn't turn on any opacity animations as far as I can tell. There are some complains on the Play Store as well. I wonder if we're missing anything with our transition benchmark tests (different Android devices for instance).

@xqwzts
Copy link
Contributor

xqwzts commented Mar 8, 2018

This looks like a regression, reported by @RobertBrunhage here: #15294

@Hixie Hixie added the f: routes Navigator, Router, and related APIs. label Mar 8, 2018
@Hixie Hixie mentioned this issue Mar 14, 2018
Closed
@mit-mit
Copy link
Member

mit-mit commented Mar 15, 2018

cc @Hixie per email discussion, yes as a temporary solution it would be great to enable "use opacity" for Gallery until we have a fix for this. I took a quick look myself, and not quite sure how to do it.

@liyuqian liyuqian mentioned this issue Jun 22, 2018
Closed
@alanrussian
Copy link
Contributor

@mehmetf Roberto is currently out. Do you expect there are issues here that are specific to mulligan? Once you all achieve the desired frame-rate for your sample app/performance test (not sure what you're using to measure it), shouldn't we be OK too?

@mehmetf
Copy link
Contributor

mehmetf commented Jul 10, 2018 •
edited

@alanrussian Potentially. My understanding is that Gallery ships with the fading routes option because it is an acceptable fix. However, Roberto said the fading routes was still janky on the Mulligan app and thus was not a acceptable fix. That's why I requested Mulligan to measure and report performance (such as where frames are being dropped).

@alanrussian
Copy link
Contributor

alanrussian commented Jul 10, 2018 •
edited

We still have to look more into this but one specific issue for now: if you push a route with an autofocus form field it's really janky. I'm guessing the cause is the combination of the keyboard opening (which animates the size of the pushed screen) and the screen transition. cc @johnfesa

@mehmetf
Copy link
Contributor

mehmetf commented Jul 13, 2018

@Hixie @HansMuller Why are we stalled on this issue? Is it because we don't have more evidence/use cases to go on?

@liyuqian
Copy link
Contributor

@mehmetf My understanding is that the fade transition is much faster now, but there's still room for further speedup. In particular, we're trying to enable retained layers. @chinmaygarde is actively working on that. Once it's done, I should be able to send a small patch to speedup animating the opacity.

@InMatrix InMatrix added this to Not much the user can do in Performance Debugging Experience Jul 13, 2018
@mehmetf
Copy link
Contributor

mehmetf commented Jul 13, 2018

Removing customer blocker tag as per internal discussion.

@Hixie
Copy link
Contributor Author

Hixie commented Jul 16, 2018

This continues to be our top priority for 1.0; we're not stalled, just working hard. :-)

@Hixie Hixie modified the milestones: bucket6, bucket8, bucket9 Jul 17, 2018
najeira pushed a commit to najeira/flutter-engine that referenced this issue Jul 19, 2018
@liyuqian @najeira
Remove unnecessary saveLayer (flutter#5420)
200bd06 
saveLayer is slow so we should avoid it as much as possible. If
there are artifacts without saveLayer, then we should report that
to Skia for the fix instead of slowing the performance with
saveLayer.

This PQ makes average rasterizer time drop from 25ms to 18ms in
flutter_gallery transitions perf test on a Nexus 5X.

This partially fixes flutter/flutter#13736 .
We probably still need some work in the opacity layer to squize
all the performance improvements.
@zoechi zoechi added the framework flutter/packages/flutter repository. See also f: labels. label Jul 22, 2018
@amirh amirh mentioned this issue Aug 5, 2018
Closed
@liyuqian liyuqian mentioned this issue Aug 8, 2018
Closed
@noelmansour noelmansour mentioned this issue Aug 29, 2018
Closed
@liyuqian liyuqian mentioned this issue Sep 4, 2018
Merged
liyuqian added a commit to liyuqian/flutter that referenced this issue Sep 5, 2018
@liyuqian
Enable fading animation during page transitions.
3ba34dd 
The average frame time of page transitions on Moto G4 is now very
close to 16ms (the last 10 measurements on our dashboard are
between 15.5ms to 16.7ms and half of them are below 16ms).

It is now much faster than when we disabled it (which was at about
35ms). So I think that we should be able to enable it by default.
I'll leave the flag there until we implement the retained rendering
to bring the frame time comfortably below 16ms.

See flutter#13736
liyuqian added a commit that referenced this issue Sep 6, 2018
@liyuqian
Enable fading animation during page transitions. (#21394)
6c4b38b 
The average frame time of page transitions on Moto G4 is now very
close to 16ms (the last 10 measurements on our dashboard are
between 15.5ms to 16.7ms and half of them are below 16ms).

It is now much faster than when we disabled it (which was at about
35ms). So I think that we should be able to enable it by default.
I'll leave the flag there until we implement the retained rendering
to bring the frame time comfortably below 16ms.

See #13736
@liyuqian liyuqian mentioned this issue Sep 12, 2018
Closed
@Hixie
Copy link
Contributor Author

Hixie commented Sep 25, 2018

I'm closing this for now. The remaining work will be tracked in #21756.

@Hixie Hixie closed this as completed Sep 25, 2018
@zoechi zoechi mentioned this issue Dec 4, 2018
Closed
@liyuqian liyuqian mentioned this issue Mar 10, 2020
Closed
@github-actions
Copy link

github-actions bot commented Sep 1, 2021

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@liyuqian liyuqian

Labels
c: performance Relates to speed or footprint issues (see "perf:" labels) customer: gallery Relating to flutter/gallery repository. Please transfer non-framework issues there. customer: mulligan (g3) engine flutter/engine repository. See also e: labels. f: material design flutter/packages/flutter/material repository. f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels.
Projects
Performance Debugging Experience
Not much the user can do
Milestone 3
Page Load Performance (chris)
Milestone
bucket9
Development

Successfully merging a pull request may close this issue.

Remove unnecessary saveLayer liyuqian/engine