Removes feedback loop from advanced filters without offscreen msaa and framebufferfetch by gaaclarke · Pull Request #193306 · flutter/flutter

gaaclarke · 2026-09-24T20:18:45Z

My fork of #193178 to avoid OP's open PR limit.

Coauthored with @krll-kov

Fixes #192918

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.

…-debug test builds

…-kov/flutter into impeller-backdrop-no-msaa-2

gemini-code-assist

Code Review

This pull request prevents a rendering feedback loop in Impeller's backdrop rendering by checking if the backdrop texture is already present in the color attachment before drawing it. It also introduces GLES-specific canvas unit tests and expands the GLES mock framework to support these tests. Feedback on the changes highlights potential issues in the mock GLES implementation, including out-of-bounds access, buffer overflow, and null pointer dereferences in GetActiveUniform, as well as unintended map insertions in Uniform1i.

gemini-code-assist · 2026-09-24T20:20:19Z

+    ON_CALL(gl, GetActiveUniform(_, _, _, _, _, _, _))
+        .WillByDefault([](GLuint program, GLuint index, GLsizei buf_size,
+                          GLsizei* length, GLint* size, GLenum* type,
+                          GLchar* name) {
+          std::string_view uniform = kSamplerUniforms[index];
+          std::memcpy(name, uniform.data(), uniform.size());
+          name[uniform.size()] = '\0';
+          *length = uniform.size();
+          *size = 1;
+          *type = GL_SAMPLER_2D;
+        });


The mock implementation of GetActiveUniform does not check if index is within the bounds of kSamplerUniforms, which can lead to out-of-bounds access. Additionally, it does not respect buf_size when copying the uniform name, which can cause a buffer overflow if the buffer is too small. It also dereferences length, size, and type without checking if they are null, which is a potential null pointer dereference since length can be null according to the OpenGL ES specification.

ON_CALL(gl, GetActiveUniform(_, _, _, _, _, _, _)) .WillByDefault([](GLuint program, GLuint index, GLsizei buf_size, GLsizei* length, GLint* size, GLenum* type, GLchar* name) { if (index >= kSamplerUniforms.size()) { return; } std::string_view uniform = kSamplerUniforms[index]; if (name != nullptr && buf_size > 0) { size_t copy_size = std::min(static_cast<size_t>(buf_size - 1), uniform.size()); std::memcpy(name, uniform.data(), copy_size); name[copy_size] = '\\0'; if (length != nullptr) { *length = copy_size; } } if (size != nullptr) { *size = 1; } if (type != nullptr) { *type = GL_SAMPLER_2D; } });

gemini-code-assist · 2026-09-24T20:20:19Z

+    ON_CALL(gl, Uniform1i(_, _))
+        .WillByDefault([this](GLint location, GLint v0) {
+          std::scoped_lock lock(mutex_);
+          sampled_textures_.insert(texture_units_[v0]);
+        });


Using operator[] on texture_units_ with v0 will default-insert an entry into the map if v0 is not already present. It is safer and cleaner to use find to avoid polluting the map with default-constructed entries.

ON_CALL(gl, Uniform1i(_, _)) .WillByDefault([this](GLint location, GLint v0) { std::scoped_lock lock(mutex_); auto it = texture_units_.find(v0); if (it != texture_units_.end()) { sampled_textures_.insert(it->second); } });

krll-kov · 2026-09-24T20:43:20Z

@gaaclarke looks like CI glitched, the engine builds were canceled. Can you rerun it?

gaaclarke · 2026-09-24T20:44:20Z

@gaaclarke looks like CI glitched, the engine builds were canceled. Can you rerun it?

Yea, I was just waiting for a new commit at HEAD, easiest fix is just to merge main.

b-luk · 2026-09-24T22:28:14Z


+  // Drawing input_texture into itself is a feedback loop, and ANGLE on D3D11
+  // samples zeros. Without offscreen MSAA the pass writes to input_texture and
+  // LoadAction::kLoad already put the backdrop there. DidLoadPreviousContents()


DidLoadPreviousContents() doesn't seem to exist.

DidLoadPreviousContents() doesn't seem to exist.

That was discussed approach in issue with proposed Gemini fix, I explained why it wasn't used , but yes, it isn't helpful for anybody without context of that discussion

done, removed

b-luk · 2026-09-24T22:29:11Z

+  const ColorAttachment color0 = render_passes_.back()
+                                     .GetEntityPassTarget()
+                                     ->GetRenderTarget()


Is render_passes_.back().GetEntityPassTarget()->GetRenderTarget() just current_render_pass.GetRenderTarget()? If so, the latter seems more readable.

Yes, both fix problem identically. Either works, the original makes it explicit that we compare against the entity pass target. If you prefer the shorter form

const ColorAttachment color0 = current_render_pass.GetRenderTarget().GetColorAttachment(0);

b-luk · 2026-09-24T22:39:42Z

+  // Drawing input_texture into itself is a feedback loop, and ANGLE on D3D11
+  // samples zeros. Without offscreen MSAA the pass writes to input_texture and
+  // LoadAction::kLoad already put the backdrop there. DidLoadPreviousContents()
+  // would not say which texture the pass writes to, so we compare attachment 0.


I think this comment is both overly specific and also not very helpful in understanding what's happening especially for someone without context of this specific bug.

This is just checking whether the current pass was created already containing the backdrop (which happens when not using MSAA), in which case the below "eager restore" is unnecessary and may result in invalid rendering. I think we should just say something simpler like that.

Agreed, proposed rewrite:

Suggested change

// Drawing input_texture into itself is a feedback loop, and ANGLE on D3D11

// samples zeros. Without offscreen MSAA the pass writes to input_texture and

// LoadAction::kLoad already put the backdrop there. DidLoadPreviousContents()

// would not say which texture the pass writes to, so we compare attachment 0.

// If current pass already contains the backdrop, which only happens when

// MSAA is not available, the eager restore below is unnecessary and would

// draw the texture into itself, which may result in invalid rendering.

reworded with a tweak on krll-kov's suggestion

auto-submit · 2026-09-25T17:13:23Z

autosubmit label was removed for flutter/flutter/193306, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

auto-submit · 2026-09-25T17:45:07Z

autosubmit label was removed for flutter/flutter/193306, because - The status or check suite Dashboard Checks has failed. Please fix the issues identified (or deflake) before re-applying this label.

gaaclarke · 2026-09-25T18:46:22Z

Ugh, formatting problem. I saw that diff thinking it was weird and i ran ./bin/et format but it didn't grab it.

b-luk · 2026-09-25T18:48:50Z

Ugh, formatting problem. I saw that diff thinking it was weird and i ran ./bin/et format but it didn't grab it.

I've seen this same thing. For my case, et format was using an older configuration. Running gclient sync before et format made it grab the latest configuration and got the proper formatting.

@krll-kov

…d framebufferfetch (flutter#193306) My fork of flutter#193178 to avoid OP's open PR limit. Coauthored with @krll-kov Fixes flutter#192918 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant in-code documentation (doc comments with `///`). - [x] If this PR introduces a new feature or capability, I created and linked a website documentation issue or PR in [flutter/website] (or verified none is needed). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. 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](https://developers.google.com/gemini-code-assist/docs/review-github-code). 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.  [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [flutter/website]: https://github.com/flutter/website [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Kyrylo K. <[email protected]>

@krll-kov

…d framebufferfetch (flutter#193306) My fork of flutter#193178 to avoid OP's open PR limit. Coauthored with @krll-kov Fixes flutter#192918 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant in-code documentation (doc comments with `///`). - [x] If this PR introduces a new feature or capability, I created and linked a website documentation issue or PR in [flutter/website] (or verified none is needed). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. 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](https://developers.google.com/gemini-code-assist/docs/review-github-code). 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.  [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [flutter/website]: https://github.com/flutter/website [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Kyrylo K. <[email protected]>

…#13060) Manual roll Flutter from 4fcd90be0045 to c5a061b18fa2 (222 revisions) Manual roll requested by [email protected] flutter/flutter@4fcd90b...c5a061b 2026-09-28 [email protected] [cross imports] navigator_test.dart (flutter/flutter#190954) 2026-09-28 [email protected] Revert: Reland: Only render views that need to be rendered (flutter/flutter#193360) 2026-09-28 [email protected] ci(bringup): android_intent_security_test is green (flutter/flutter#193469) 2026-09-28 [email protected] Sync CHANGELOG.md from stable (flutter/flutter#193020) 2026-09-28 [email protected] Roll Fuchsia Linux SDK from ukukV5lEkKabtOATk... to QdgqP02_cYpRQQQNN... (flutter/flutter#193454) 2026-09-28 [email protected] Roll pub packages (flutter/flutter#193460) 2026-09-28 [email protected] Un-nest sceneBuildDuration and windowRenderDuration in web SceneBuilderRecorder (flutter/flutter#193260) 2026-09-28 [email protected] Fix unresolved doc comment references in dev/integration_tests and dev/benchmarks (flutter/flutter#193433) 2026-09-28 [email protected] Roll Packages from e55e7ac to ba0364a (9 revisions) (flutter/flutter#193450) 2026-09-28 [email protected] [tool] Require explicit dependency injection for FlutterDevice and FlutterDevice.create (flutter/flutter#192830) 2026-09-28 [email protected] Roll pub packages (flutter/flutter#193442) 2026-09-28 [email protected] [flutter_tools] Include data assets from hooks when pubspec.yaml is empty (flutter/flutter#193434) 2026-09-28 [email protected] Roll pub packages (flutter/flutter#193438) 2026-09-28 [email protected] Fix unresolved doc comment references in Material and Cupertino (flutter/flutter#193283) 2026-09-28 [email protected] Fix doc references to Material and Cupertino in the widgets library and its tests (flutter/flutter#193334) 2026-09-26 [email protected] wasm: enforce WasmGC opt-in capability checks and Firefox < 147 guard (flutter/flutter#193180) 2026-09-26 [email protected] [Android] Refuse external setters of engine entrypoint and cached engine arguments via `Intent`s (flutter/flutter#190249) 2026-09-26 [email protected] [web] Unskip TextPainter.getWordBoundary test (flutter/flutter#193378) 2026-09-26 [email protected] Specify non-obvious types in pattern variable declarations (flutter/flutter#192632) 2026-09-26 [email protected] Removes feedback loop from advanced filters without offscreen msaa and framebufferfetch (flutter/flutter#193306) 2026-09-26 [email protected] [web] Unskip 8 passing image tests in painting and rendering (flutter/flutter#193377) 2026-09-26 [email protected] Revert: Allow resetting test invariants in `addTearDown` (flutter/flutter#193383) 2026-09-25 [email protected] [tool] Migrate CoverageCollector to modular dependency injection (flutter/flutter#192924) 2026-09-25 [email protected] Fix more unresolved doc comment references (flutter/flutter#193336) 2026-09-25 [email protected] Allow resetting test invariants in `addTearDown` (flutter/flutter#192082) 2026-09-25 [email protected] [a11y] Add a semantics role for slider (flutter/flutter#193324) 2026-09-25 [email protected] Fix plugin tests after Pigeon plugin template changes (flutter/flutter#193302) 2026-09-25 [email protected] [Android 17] Bump Standard Test Apps in flutter/flutter to AGP 9.3.1 (flutter/flutter#193263) 2026-09-25 [email protected] SSBO-based gradients in UberSDF (flutter/flutter#192962) 2026-09-25 [email protected] [Linux] Handle FlView being destroyed before rendering is complete. (flutter/flutter#193268) 2026-09-25 [email protected] [flutter_tools] Handle Windows reserved characters in test target path (flutter/flutter#191900) 2026-09-25 [email protected] docs(tools): nit: say hook/build.dart in CMake native assets comment (flutter/flutter#192205) 2026-09-25 [email protected] [web] Enable Firefox Skwasm UI CI suites, configure COI configs, and fail fast on loader rejections (flutter/flutter#193187) 2026-09-25 [email protected] [Android 17] Bumped Engine Dependencies to 9.3.1 (flutter/flutter#193265) 2026-09-25 [email protected] [web] Omit group role on menu scrollables and assign region role to named routes (flutter/flutter#192965) 2026-09-25 [email protected] Roll Skia from 8eedeed98e79 to f441ca223b2b (5 revisions) (flutter/flutter#193353) 2026-09-25 [email protected] Roll Fuchsia Linux SDK from EbPpoJW-Lnsu-8dyZ... to ukukV5lEkKabtOATk... (flutter/flutter#193326) 2026-09-25 [email protected] Roll Packages from 431ea69 to e55e7ac (6 revisions) (flutter/flutter#193351) 2026-09-25 [email protected] Support DynamicLibrary.codeAsset in Flutter (flutter/flutter#188947) 2026-09-25 [email protected] Roll Dart SDK from 75d9e87d4e3d to 0e7642b85457 (1 revision) (flutter/flutter#193346) 2026-09-25 [email protected] Exclude iOS PRs from macOS link (flutter/flutter#193315) 2026-09-25 [email protected] Roll Skia from 6357608543ec to 8eedeed98e79 (4 revisions) (flutter/flutter#193341) 2026-09-25 [email protected] Roll Dart SDK from d3aedb186aab to 75d9e87d4e3d (3 revisions) (flutter/flutter#193335) 2026-09-25 [email protected] Cache the paint offset adjusted line metrics in TextPainter (flutter/flutter#191223) ...

krll-kov and others added 7 commits September 22, 2026 22:16

Don't draw the backdrop into itself when the pass has no offscreen MSAA

6013fce

Merge branch 'flutter:master' into impeller-backdrop-no-msaa-2

dee524e

Fixed "error: no member named 'label' in 'impeller::Command'" for non…

fc3a664

…-debug test builds

Merge branch 'impeller-backdrop-no-msaa-2' of https://github.com/krll…

660769a

…-kov/flutter into impeller-backdrop-no-msaa-2

Merge branch 'flutter:master' into impeller-backdrop-no-msaa-2

f4199cb

Merge branch 'impeller-backdrop-no-msaa-2' of https://github.com/krll…

dd6c1e3

…-kov/flutter into impeller-backdrop-no-msaa-2

switched test

9d46430

flutter-dashboard Bot added the CICD Run CI/CD label Sep 24, 2026

github-actions Bot added engine flutter/engine related. See also e: labels. e: impeller Impeller rendering backend issues and features requests labels Sep 24, 2026

Merge branch 'master' into my-pr-193178

6d451cb

gaaclarke requested review from andywolff and b-luk September 24, 2026 20:19

gaaclarke mentioned this pull request Sep 24, 2026

[Impeller] Don't draw the backdrop into itself when the pass has no offscreen MSAA #193178

Closed

8 of 11 tasks

gemini-code-assist Bot reviewed Sep 24, 2026

View reviewed changes

Merge branch 'master' into my-pr-193178

e9a4ff2

b-luk requested changes Sep 24, 2026

View reviewed changes

bluk feedback

e385d5c

gaaclarke requested a review from b-luk September 25, 2026 16:25

b-luk previously approved these changes Sep 25, 2026

View reviewed changes

gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 25, 2026

auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 25, 2026

gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 25, 2026

auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 25, 2026

format

1dfcd03

gaaclarke dismissed b-luk’s stale review via 1dfcd03 September 25, 2026 18:50

gaaclarke requested a review from b-luk September 25, 2026 18:50

b-luk approved these changes Sep 25, 2026

View reviewed changes

andywolff approved these changes Sep 25, 2026

View reviewed changes

gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 25, 2026

auto-submit Bot added this pull request to the merge queue Sep 26, 2026

Merged via the queue into flutter:master with commit 95feccb Sep 26, 2026
23 checks passed

flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 26, 2026

This was referenced Sep 28, 2026

Roll Flutter from 4fcd90be0045 to 9345dd20564f (216 revisions) flutter/packages#13055

Closed

Manual roll Flutter from 4fcd90be0045 to c5a061b18fa2 (222 revisions) flutter/packages#13060

Merged

RickyvdBerg mentioned this pull request Oct 2, 2026

feat(impeller): add bounded coverage rendering and resource reports avio-os/avio-flutter#1

Merged

eduardohr-muniz mentioned this pull request Oct 2, 2026

[Windows] Use an unsized texture format for backing stores on OpenGL ES 2 #192917

Merged

2 of 11 tasks

Conversation

gaaclarke commented Sep 24, 2026

Pre-launch Checklist

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

krll-kov commented Sep 24, 2026

Uh oh!

gaaclarke commented Sep 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krll-kov Sep 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

auto-submit Bot commented Sep 25, 2026

Uh oh!

auto-submit Bot commented Sep 25, 2026

Uh oh!

gaaclarke commented Sep 25, 2026

Uh oh!

b-luk commented Sep 25, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

krll-kov Sep 25, 2026 •

edited

Loading