Skip to content

Conversation

@yjbanov
Copy link
Contributor

@yjbanov yjbanov commented Nov 6, 2017

Fixes #11021
Fixes #12243

Copy link
Contributor

@HansMuller HansMuller left a 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 |
Copy link
Contributor

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.

Copy link
Contributor Author

@yjbanov yjbanov Nov 7, 2017

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
Copy link
Contributor

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.

Copy link
Contributor Author

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
Copy link
Contributor

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

Copy link
Contributor Author

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));
Copy link
Contributor

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?

Copy link
Contributor Author

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.

DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request May 14, 2018
…ter#12896)

* delay taking screenshot to allow GPU thread to render the frame

* address comments
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

flutter_driver's waitUntilNoTransientCallbacks doesn't await Image asset loading FlutterDriver: no way to wait until a screenshot is ready

4 participants