Fix supports_decal check in framebuffer_blend.frag by zanderso · Pull Request #196227 · flutter/flutter

zanderso · 2026-11-09T18:21:42Z

The callers of this shader only ever pass 0.0 or 1.0 for supports_decal, so prior to this change supports_decal > 1 was always false. Even when the hardware sampler was configured with SamplerAddressMode::kDecal, the shader always took the fallback branch and called IPSampleDecal.

Prior to this change, a user-visible difference could only occur when all of the following happen at once:

The device supports both frame buffer fetch and decal sampling (iOS/macOS Metal, most Android Vulkan GPUs, or Android GLES with both extensions).
An image is drawn directly via TextureContents::RenderToSnapshot's passthrough fast-path.
The image is downscaled using both an advanced BlendMode and mipmapped minification filtering like FilterQuality.medium.

Fixes #196266

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.

gemini-code-assist

Code Review

This pull request modifies the framebuffer_blend.frag shader to check if supports_decal > 0.0 instead of > 1 to ensure correct behavior when hardware decal is supported, and adds a corresponding unit test in aiks_dl_blend_unittests.cc. The review feedback suggests adding assertions to ensure that created device buffers and readback contents are not null before they are accessed in the test, and recommends using supports_decal > 0.5 in the shader to avoid potential floating-point precision issues.

flutter-dashboard · 2026-11-09T19:22:15Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #196227 at sha 8d033d9

flutter-dashboard · 2026-11-09T23:23:14Z

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #196227 at sha fa5cb29

gaaclarke

can you link an issue in the description please =)

zanderso · 2026-11-10T02:30:49Z

can you link an issue in the description please =)

Done!

gaaclarke

I don't find that the provided test or golden changes provide a clear demonstration of correct behavior. I worry that we don't have an appropriate backstop for this change.

Also, Gemini suggest that while the branch was dead code it locked in desirable behavior. It made it so DrawImage with kScreen and kSrcOver have the same sharpness. It recommends keeping the old bias:

    if (supports_decal > 0.0) {
      return texture(texture_sampler, texture_coords, kDefaultMipBias);
    }

The bot also suggests that drawing those 2 things side by side will be a good golden test to show that they have the same crispness. I'm not sure how easy that will be to actually see, but worth a try.

Can we land a PR before this that enables malioc for this shader so we can show that this is an improvement?

zanderso · 2026-11-10T23:08:20Z

Can we land a PR before this that enables malioc for this shader so we can show that this is an improvement?

Comments in the build files indicate that we can't?

flutter/engine/src/flutter/impeller/entity/BUILD.gn

Line 101 in df147b9

# malioc does not support analyzing shaders that use the framebuffer fetch

jtmcdole · 2026-11-10T23:38:49Z

+  // Mip 0 (8x8) is transparent black (0); mip 1 (4x4) is opaque white (255).
+  // Drawing at 0.5x scale selects LOD = 1.0 (white) when framebuffer_blend.frag
+  // takes the unbiased hardware decal path (`supports_decal > 0.0`). With the
+  // old `supports_decal > 1` check, `1.0 > 1` was false and `IPSampleDecal`


old will only get older after this PR lands. Can you link to the "before" hash/line for future code archivists?

jtmcdole

lgtm, assuming Aaron doesn't have anything else.

gaaclarke · 2026-11-11T00:30:54Z

Comments in the build files indicate that we can't?

Comments in build files: "Why would I deceive you?"

My bot seemed to think it was feasible, maybe with tweaking the target device? I didn't pull on that thread.

zanderso · 2026-11-11T01:52:05Z

Welp. Our bots agree with each other. #196343 seems to work.

…ter#196343) Analyze framebuffer_blend with malioc. For review comment flutter#196227 (review) ## 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/CONTRIBUTING.ai.md [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

flutter-dashboard · 2026-11-11T22:26:57Z

It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If both you and the PR author believe this PR qualifies for a test exemption, escalate to the tech lead for the changed area to review and apply the test-exempt label. All reviewers should feel empowered to ask for tests before escalating.

zanderso · 2026-11-12T00:06:02Z

Changed to:

    if (supports_decal > 0.0) {
      return texture(texture_sampler, texture_coords, kDefaultMipBias);
    }

Malioc diff is here: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8665389924786194881/+/u/test:_malioc_diff/stdout

Everything appears to be moving in the right direction except two things Gemini says don't matter:

shortest_path_cycles texture: 0.0 → 0.25 (both Vulkan and GLES):
- malioc computes shortest_path_cycles purely by static control-flow graph analysis.
- Previously, IPSampleDecal had an early-return branch (if (coords < 0 || coords >= 1) return vec4(0);) that skipped texture(...). Even though that branch is never taken at runtime (since v_src_texture_coords is always in (0, 1)), malioc treated that early return as the "shortest path" (texture = 0.0).
- With supports_decal > 0.0, the bounds-check branch is eliminated and the texture sample is unconditional, so shortest_path_cycles and longest_path_cycles both report the single 0.25-cycle texture fetch.
work_registers_used: 21 → 25 (GLES only):
- Because the texture2D fetch is no longer gated behind the IPSampleDecal bounds check, the Mali GLES compiler hoists the texture2D instruction earlier to hide texture latency behind ReadDestination() and destination unpremultiply, keeping the vec4 result live across those instructions (+4 32-bit registers).
- On Mali-G78 (Valhall), full 100% thread occupancy is maintained up to 32 work registers, so going from 21 to 25 registers has zero impact on occupancy.

gaaclarke · 2026-11-12T00:22:00Z

Everything appears to be moving in the right direction except two things Gemini says don't matter:

yea those are fine

…ant test

flutter-dashboard · 2026-11-12T03:57:31Z

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #196227 at sha 971b1af

zanderso · 2026-11-12T05:33:49Z

The golden diffs are expected:

IPSampleDecal vs. hardware SamplerAddressMode::kDecal at v = 1.001984:

Before (right image, IPSampleDecal): IPSampleDecal has a hard cutoff if (... || coords.y >= 1.0) return vec4(0); before calling texture(...). Because v = 1.001984 ≥ 1.0, it immediately returned vec4(0), leaving row y = 159 completely unblended at background RGB(169, 169, 169).
With this PR (left image, hardware kDecal): Hardware SamplerAddressMode::kDecal applies bilinear filtering across the border between the last texel center and the first border texel center. At mip level 1 (H₁ = 189), 1.0 + 0.5/189 ≈ 1.00265 > 1.001984, so the sampler returns a small non-zero bilinear weight on the bottom texel row, and the 1 covered MSAA subsample resolves to RGB(167..168) against the RGB(169) background.

gaaclarke

lgtm!

test-exempt: improvement in static performance evaluation

auto-submit · 2026-11-12T22:27:11Z

auto label is removed for flutter/flutter/196227, Failed to enqueue flutter/flutter/196227 with HTTP 400: Pull request Required status check "Check Code Freeze" is queued..

…13653) Manual roll Flutter from 9c427b5d7b1c to 1b9585481095 (32 revisions) Manual roll requested by [email protected] flutter/flutter@9c427b5...1b95854 2026-11-13 [email protected] Roll pub packages (flutter/flutter#196541) 2026-11-13 [email protected] Do not route PRs for review without cla, convert to draft (flutter/flutter#196341) 2026-11-13 [email protected] Roll Skia from d87a6c04da74 to 11f46cfe8838 (13 revisions) (flutter/flutter#196533) 2026-11-13 [email protected] [flutter_tools, flutter_test] Deprecate --experimental-faster-testing (flutter/flutter#196123) 2026-11-13 [email protected] [flutter_tools] Handle RPCError in ScreenshotCommand.runSkia (flutter/flutter#196132) 2026-11-13 [email protected] Roll Dart SDK from a8b961fd48bb to c91e8b6674e2 (2 revisions) (flutter/flutter#196502) 2026-11-13 [email protected] Roll Skia from 0c5ce4960cef to d87a6c04da74 (12 revisions) (flutter/flutter#196497) 2026-11-13 [email protected] Don't call `skia_image()` for raster-owned images in `DlImage::Equals` (flutter/flutter#196473) 2026-11-13 [email protected] [flutter_tools] Require FlutterCommandRunner DI fields and extract findCommandName (flutter/flutter#196331) 2026-11-13 [email protected] [web] Unify ui.Canvas behind a shared EngineCanvas frontend (flutter/flutter#196348) 2026-11-13 [email protected] Link to MediaQuery.displayFeaturesOf from DisplayFeatures (flutter/flutter#193728) 2026-11-13 [email protected] Removes reference to unnecessary ensureSemantics call in doc (flutter/flutter#195831) 2026-11-13 [email protected] [flutter_tools] Register trackWidgetCreation in AppleBuildOptionsBundle (flutter/flutter#196078) 2026-11-12 [email protected] [Android] Skip the HardwareBuffer defect fallback for platform views (flutter/flutter#193441) 2026-11-12 [email protected] Fix supports_decal check in framebuffer_blend.frag (flutter/flutter#196227) 2026-11-12 [email protected] [ios] Update the screen in didUpdateEffectiveGeometry (flutter/flutter#195508) 2026-11-12 [email protected] [flutter_tools] Add `Cache-Control` headers for web test assets (flutter/flutter#196414) 2026-11-12 [email protected] Roll Skia from 3b71ec08218f to 0c5ce4960cef (16 revisions) (flutter/flutter#196472) 2026-11-12 [email protected] [web] Render bounded ImageFilter.blur on wimp (flutter/flutter#196081) 2026-11-12 [email protected] Roll Dart SDK from 69567bd3c427 to a8b961fd48bb (2 revisions) (flutter/flutter#196464) 2026-11-12 [email protected] [engine] Release the Skia OpenGL resource context on shutdown (flutter/flutter#193592) 2026-11-12 [email protected] [web] Fix decoding of still AVIF images on Chrome (flutter/flutter#195958) 2026-11-12 [email protected] Gate SurfaceControl context setting on API 34+ (flutter/flutter#196394) 2026-11-12 [email protected] [flutter_tools] Only pass on a valid root tool in DASH__TOOL (flutter/flutter#196228) 2026-11-12 [email protected] Support GL_TEXTURE_EXTERNAL_OES embedder textures in Impeller (flutter/flutter#195857) 2026-11-12 [email protected] Stop building linux/wasm_release in local_engine_builds (flutter/flutter#196345) 2026-11-12 [email protected] Split flame out of Linux customer_testing (flutter/flutter#196347) 2026-11-12 Jiwon Lee [Windows] Fix Space input after Alt+Space with previous Microsoft IME (flutter/flutter#192105) 2026-11-12 [email protected] [flutter_tools] Add ToolExtensionException, exit() guard, graceful shutdown, and base modifiers (flutter/flutter#195908) 2026-11-12 [email protected] Roll Packages from 653628e to 14f9933 (9 revisions) (flutter/flutter#196450) 2026-11-12 [email protected] Roll Dart SDK from e5add7a25b32 to 69567bd3c427 (2 revisions) (flutter/flutter#196437) 2026-11-12 [email protected] Roll Skia from 9aa2513a4ac9 to 3b71ec08218f (1 revision) (flutter/flutter#196436) 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 ...

flutter-dashboard Bot added the CICD Run CI/CD label Nov 9, 2026

gemini-code-assist Bot reviewed Nov 9, 2026

View reviewed changes

Comment thread engine/src/flutter/impeller/display_list/aiks_dl_blend_unittests.cc Resolved

Comment thread engine/src/flutter/impeller/display_list/aiks_dl_blend_unittests.cc Resolved

Comment thread engine/src/flutter/impeller/entity/shaders/blending/framebuffer_blend.frag Resolved

flutter-dashboard Bot added the will affect goldens Changes to golden files label Nov 9, 2026

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

zanderso requested review from gaaclarke and jtmcdole November 9, 2026 23:55

gaaclarke requested changes Nov 10, 2026

View reviewed changes

zanderso requested a review from gaaclarke November 10, 2026 02:31

gaaclarke requested changes Nov 10, 2026

View reviewed changes

jtmcdole reviewed Nov 10, 2026

View reviewed changes

jtmcdole previously approved these changes Nov 10, 2026

View reviewed changes

zanderso mentioned this pull request Nov 11, 2026

[Impeller] Enable malioc analysis for framebuffer_blend shaders #196343

Merged

11 tasks done

zanderso added 2 commits November 11, 2026 13:57

Fix supports_decal check in framebuffer_blend.frag

1bfaebb

Address review feedback

7c3c4bf

zanderso dismissed jtmcdole’s stale review via cbd566a November 11, 2026 22:26

zanderso force-pushed the supports-decal-fix branch from fa5cb29 to cbd566a Compare November 11, 2026 22:26

flutter-dashboard Bot added the missing-tests label Nov 11, 2026

Keep kDefaultMipBias in supports_decal > 0.0 branch and remove redund…

971b1af

…ant test

zanderso force-pushed the supports-decal-fix branch from cbd566a to 971b1af Compare November 12, 2026 02:48

zanderso requested a review from gaaclarke November 12, 2026 05:34

gaaclarke approved these changes Nov 12, 2026

View reviewed changes

gaaclarke added test-exempt Exempt from tests. Overrides `missing-tests` label autosubmit Merge PR when tree becomes green via auto submit App labels Nov 12, 2026

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

zanderso added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 12, 2026

auto-submit Bot added this pull request to the merge queue Nov 12, 2026

Merged via the queue into flutter:master with commit 29625ca Nov 13, 2026
28 of 29 checks passed

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

Conversation

zanderso commented Nov 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

flutter-dashboard Bot commented Nov 9, 2026

Uh oh!

flutter-dashboard Bot commented Nov 9, 2026

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

zanderso commented Nov 10, 2026

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

zanderso commented Nov 10, 2026

Uh oh!

jtmcdole Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

jtmcdole left a comment

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Nov 11, 2026

Uh oh!

zanderso commented Nov 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard Bot commented Nov 11, 2026

Uh oh!

zanderso commented Nov 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaaclarke commented Nov 12, 2026

Uh oh!

flutter-dashboard Bot commented Nov 12, 2026

Uh oh!

zanderso commented Nov 12, 2026

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

auto-submit Bot commented Nov 12, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zanderso commented Nov 9, 2026 •

edited

Loading

zanderso commented Nov 11, 2026 •

edited

Loading

zanderso commented Nov 12, 2026 •

edited

Loading