-
Notifications
You must be signed in to change notification settings - Fork 29.8k
delay taking screenshot to allow GPU thread to render the frame #12896
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.
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
Conversation
HansMuller
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| // ------------------------------------------------------------------- | ||
| // UI : <-- build --> | ||
| // GPU : <-- rasterize --> | ||
| // Gap : | 1 second or more | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might as well say 2 seconds here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| // Gap : | 1 second or more | | ||
| // Driver: <-- screenshot --> | ||
| // | ||
| // The artificial gap should be long enough for the GPU thread to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds a little hypothetical. It might be better to say something like: a two second delay is long enough for any reasonable GPU request to be rasterized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| description: Flutter non-plugin UI integration tests. | ||
|
|
||
| dependencies: | ||
| image: 1.1.29 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please make sure to run flutter update-packages --force-upgrade when adding dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. We already depend on image: 1.1.29 in another package. This change has no intention to upgrade our packages to new versions. Perhaps we need another mode in the flutter update-packages command that completes the list of transitive dependencies without upgrading?
| // The two-second gap should be long enough for the GPU thread to | ||
| // finish rasterizing the frame, but not longer than necessary to keep | ||
| // driver tests as fast a possible. | ||
| await new Future<Null>.delayed(const Duration(seconds: 2)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we should just be able to wait until the GPU thread is done, instead.
@chinmaygarde is there a way for the engine to signal the driver (e.g. via the observatory port) that the GPU thread is now idle and taking a screenshot is safe? Or even better, is there a way to have _flutter.screenshot itself just wait for the GPU to be idle before taking the screenshot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My impression was that the engine-side change would need to be substantial enough that it's not worth making it just for taking a screenshot. That said, this PR also adds a test that reproduces the problem very reliably, so if we decide to pursue an engine-side fix, there's now an easy way to confirm that it works.
…ter#12896) * delay taking screenshot to allow GPU thread to render the frame * address comments
Fixes #11021
Fixes #12243