Skip to content

Read back images on the raster thread on OpenGLES - #195927

Merged
auto-submit[bot] merged 3 commits into
flutter:masterfrom
b-luk:gles-readback-raster-thread
Nov 10, 2026
Merged

Read back images on the raster thread on OpenGLES#195927
auto-submit[bot] merged 3 commits into
flutter:masterfrom
b-luk:gles-readback-raster-thread

Conversation

@b-luk

@b-luk b-luk commented Nov 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fix for #193428.

On Impeller OpenGLES, Image.toByteData() on IO-thread-owned images performs texture readback on the IO thread (determined by image_encoding_impeller's ConvertImageToRaster). When this is done while the raster thread is using the same texture, this may cause deadlocks and ANRs on some drivers.

This PR makes the readback always happen on the raster thread on OpenGLES. This matches the Skia backend behavior in image_encoding_skia.cc's ConvertImageToRasterSkia, where the readback is done on the raster thread, with a warning about concurrent usage on the IO and raster threads:

flutter/engine/src/flutter/lib/ui/painting/image_encoding_skia.cc

Lines 70 to 73 in b9b2802

// Cross-context images do not support makeRasterImage. Convert these images
// by drawing them into a surface. This must be done on the raster thread
// to prevent concurrent usage of the image on both the IO and raster threads.
raster_task_runner->PostTask([dl_image, encode_task = std::move(encode_task),

Verified on a moto g 5g (2022) that #193428 (comment) repros without this fix, and does not repro with this fix.

This change is implemented for all OpenGLES devices rather than only the affected MediaTek/Mali devices from #193428. The issue only affects certain devices, but we choose to apply the fix to all devices. The downside is that readbacks for IO-thread-owned images takes time on the raster thread, so frequent readbacks can stall rendering. The upside is consistent behavior across devices, without having to maintain a list of devices that need the workaround.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

@flutter-dashboard flutter-dashboard Bot added the CICD Run CI/CD label Nov 3, 2026
gemini-code-assist[bot]
gemini-code-assist Bot reviewed Nov 3, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the image encoding process under Impeller to avoid reading back textures on the IO thread when using the OpenGLES backend, mitigating potential driver deadlocks. It also introduces unit tests to verify this behavior across different backends. The review feedback identifies a thread affinity issue in the new tests where snapshot_delegate is default-constructed on the main thread but assigned on the raster thread, which will trigger an assertion failure in debug builds. It is recommended to wrap snapshot_delegate in a std::unique_ptr to defer its initialization to the raster thread.

Comment thread engine/src/flutter/lib/ui/painting/image_encoding_unittests.cc Resolved
Comment thread engine/src/flutter/lib/ui/painting/image_encoding_unittests.cc Resolved
@github-actions github-actions Bot added the engine flutter/engine related. See also e: labels. label Nov 3, 2026
@b-luk b-luk mentioned this pull request Nov 4, 2026
Closed
@b-luk
b-luk requested a review from gaaclarke November 7, 2026 00:32
gaaclarke
gaaclarke requested changes Nov 9, 2026
View reviewed changes

@gaaclarke gaaclarke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there is a race in this implementation. Previously the uploading and reading happened on the same thread so the ordering was was implicit. Now there is a risk that someone could do a toByteData before the image is drawn we could get a corrupt read.

Gemini thinks it can be addressed by adding a wait on the fence in TextureGLES::SetAsFramebufferAttachment.

Here's the comment it added:

// Blits and readbacks reference the texture through a framebuffer rather
// than Bind, so they must also wait for any pending upload from another
// context (#193428).

@github-actions github-actions Bot added the e: impeller Impeller rendering backend issues and features requests label Nov 9, 2026
@b-luk

b-luk commented Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

I believe there is a race in this implementation. Previously the uploading and reading happened on the same thread so the ordering was was implicit. Now there is a risk that someone could do a toByteData before the image is drawn we could get a corrupt read.

Gemini thinks it can be addressed by adding a wait on the fence in TextureGLES::SetAsFramebufferAttachment.

Here's the comment it added:

// Blits and readbacks reference the texture through a framebuffer rather
// than Bind, so they must also wait for any pending upload from another
// context (#193428).

You're right. I factored the fence wait out of Bind into a new WaitForFence. Both Bind and SetAsFramebufferAttachment wait for the fence now.

@b-luk
b-luk requested a review from gaaclarke November 9, 2026 21:31
gaaclarke
gaaclarke approved these changes Nov 9, 2026
View reviewed changes

@gaaclarke gaaclarke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Can you just update the description to explain why we chose this way instead of doing a device specific workaround. The potential downside to this approach is readbacks can stall rendering. The upside is consistency across devices and not having to play wackamole with devices that need the workaround.

@b-luk

b-luk commented Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

LGTM. Can you just update the description to explain why we chose this way instead of doing a device specific workaround. The potential downside to this approach is readbacks can stall rendering. The upside is consistency across devices and not having to play wackamole with devices that need the workaround.

Done. Updated the description to mention this.

@b-luk b-luk added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 9, 2026
@auto-submit
auto-submit Bot added this pull request to the merge queue Nov 10, 2026
Merged via the queue into flutter:master with commit 031636e Nov 10, 2026
20 of 21 checks passed
@flutter-dashboard flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 10, 2026
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 10, 2026
Merged
auto-submit Bot pushed a commit to flutter/packages that referenced this pull request Nov 10, 2026
@engine-flutter-autoroll
Roll Flutter from d46382112aa6 to fffd8c22ae64 (21 revisions) (#13614)
6fbd01a 
flutter/flutter@d463821...fffd8c2

2026-11-10 [email protected] Roll Fuchsia Test Scripts from 8QcsP_SmbsT09VgUs... to PXc5D7S_q6sIYpfFJ... (flutter/flutter#196273)
2026-11-10 [email protected] [web] Accept the capped letter spacing line shift in kitchensink (flutter/flutter#196148)
2026-11-10 [email protected] Fix Vulkan MSAA diagonal seam in FramebufferBlend (flutter/flutter#196197)
2026-11-10 [email protected] Read back images on the raster thread on OpenGLES (flutter/flutter#195927)
2026-11-10 [email protected] [web] Unify ui.Paint across CanvasKit and Skwasm (flutter/flutter#196071)
2026-11-10 [email protected] Roll Fuchsia Test Scripts from p3N3B-Kchh1bthB6n... to 8QcsP_SmbsT09VgUs... (flutter/flutter#196249)
2026-11-09 [email protected] [Impeller] Reland clipped BackdropFilter square corners fix (flutter/flutter#195716)
2026-11-09 [email protected] [web] Recreate canvas and context on CkOffscreenSurface resize to fix WebGL clipping (flutter/flutter#192769)
2026-11-09 [email protected] Roll Dart SDK from ee0f66fa0ad2 to a2baae273bb2 (1 revision) (flutter/flutter#196245)
2026-11-09 [email protected] Add Mac web_tool_tests_1_2 and _2_2 in bringup (flutter/flutter#196241)
2026-11-09 [email protected] Roll pub packages (flutter/flutter#196246)
2026-11-09 [email protected] Increase number of 'Windows tool_integration_tests' subshards (flutter/flutter#196243)
2026-11-09 [email protected] Roll Fuchsia Test Scripts from Jo-ZgQSNDuPsjlZ4s... to p3N3B-Kchh1bthB6n... (flutter/flutter#196238)
2026-11-09 [email protected] Skip UIScene pre-migration test cases on Xcode 27 (flutter/flutter#194755)
2026-11-09 [email protected] [flutter_tools] Share Analytics.isBot and make isRunningOnBot sync (flutter/flutter#194744)
2026-11-09 [email protected] [flutter_tools] Retry chromedriver startup when its port is taken (flutter/flutter#196150)
2026-11-09 [email protected] Remove unused vulkan_provider.cc (flutter/flutter#195513)
2026-11-09 [email protected] Roll Dart SDK from 3b174b7217fe to ee0f66fa0ad2 (13 revisions) (flutter/flutter#196217)
2026-11-09 [email protected] Goma is dead, long live RBE! (Take 2) (flutter/flutter#196212)
2026-11-09 [email protected] Split DevTools customer tests into its own customer_testing_devtools shard (flutter/flutter#195871)
2026-11-09 [email protected] Split mac_host_{debug,profile,release}_arm64_tests into one step per type (flutter/flutter#196031)

If this roll has caused a breakage, revert this CL and set the roller
to dry run mode using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@dfdgsdfg

dfdgsdfg commented Nov 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Adding native-only reference material for the cross-context readback hazard discussed in this PR:

With completed producer glFinish and immutable sources, the same-source AA condition stalls on Galaxy A15 / Mali-G57 MC2 r32p1 and Pixel 6 / Mali-G78 r38p1. Changing only Raster's source object (AB) sustains 600 seconds on both. The AA endpoints show IO glReadPixels, the other context's first glBindTexture, and a Mali backend mutex wait.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gaaclarke gaaclarke gaaclarke approved these changes
+1 more reviewer
@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CICD Run CI/CD e: impeller Impeller rendering backend issues and features requests engine flutter/engine related. See also e: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@b-luk @dfdgsdfg @gaaclarke