[Impeller] Allow sampling textures with manually-uploaded mip levels#187729
Conversation
Remove the bind-time NeedsMipmapGeneration() check from the Metal and OpenGL ES backends. It rejected sampling any texture whose mip chain was uploaded via Texture::overwrite(mipLevel:) (rather than the GenerateMipmap blit) by returning false at bind, so the texture was never bound. The check was inconsistent: Vulkan has no equivalent and the Metal check was already skipped on iOS. The mipmap_generated_ flag stays, since the 2D renderer's Gaussian blur and render-target mip generation still read it; only the redundant bind-time validation is removed. The 2D renderer generates mips before sampling by construction and never tripped the check, so this only unblocks external HAL consumers (Flutter GPU) sampling manually-mipped textures such as prefiltered IBL atlases and compressed-texture mip chains.
CanSampleManuallyMippedTexture populates a mip_count=2 texture's base level by hand (via SetContents, no GenerateMipmap blit) and samples it at an explicit LOD of 0 through the mipmaps shader. It exercises the bind-time mipmap validation removed in the previous commit: before that change the bind failed on desktop Metal and OpenGL ES, so the golden would have been blank.
There was a problem hiding this comment.
Code Review
This pull request removes bind-time mipmap validation checks from the OpenGL ES and Metal backends, allowing textures with manually populated mip chains to be sampled. A new golden test is added to verify this behavior. Feedback suggests initializing the entire mip chain in the test by populating the second mip level to prevent potential driver-specific completeness issues.
Upload the second mip level by hand via a blit copy so the texture is mipmap-complete on backends that require it (OpenGL ES samples a mipmapped texture as incomplete otherwise). The blit copy does not set mipmap_generated_, so NeedsMipmapGeneration() stays true and the test still exercises the removed bind-time check.
|
An existing Git SHA, To re-trigger presubmits after closing or re-opeing a PR, or pushing a HEAD commit (i.e. with |
|
you have to push an empty commit to this to kick the bots. it seems to happen when the cicd label gets confused |
|
@b-luk can you give this a review please? |
b-luk
left a comment
There was a problem hiding this comment.
Generally looks good.
The mac_unopt test failure seems like a real failure caused by this change, so we should make sure it gets fixed. https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8679457603160578785/+/u/test:_Impeller-golden_for_host_release/stdout
The linux_unopt test is just a formatting issue: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8679457623519549473/+/u/test:_test:_Check_formatting/stdout.
I want to make sure the golden output is what we expect too. Right now I think those two other failures are blocking golden results from getting created.
| VS::BindFrameInfo(pass, host_buffer->EmplaceUniform(frame_info)); | ||
|
|
||
| FS::FragInfo frag_info; | ||
| frag_info.lod = 0.0f; |
There was a problem hiding this comment.
Should we also have another test to make sure lod = 1 renders as expected? If you add this, we should change it to be something other than all white.
There was a problem hiding this comment.
Added CanSampleManuallyMippedTextureLod1, which samples the second level at LOD 1, and changed that level to solid orange so it is distinct from the base and from an empty level. Done in 0cdc8e9.
Refactor the manually-mipped golden into a helper parameterized by LOD and add a second test that samples the hand-uploaded mip level at LOD 1. The second level is now solid orange rather than white, so it is distinct from the base and from an empty level. Also wraps the AddCopy call to satisfy clang-format.
The new test on mac_unopt is still failing: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8679443838533653025/+/u/test:_Impeller-golden_for_host_release/stdout From asking Gemini about the failure, it says: The test failure occurs because If it's not making things up, that seems like a reasonable theory to me. |
AddCopy defaults the destination region to the full base size, so uploading the 4x4 second level with no region expected 256 bytes against a 64 byte source and tripped the bounds validation. Pass an explicit 4x4 destination region.
|
Yes, GLES is pain. GLES 2 leaves GL_TEXTURE_MAX_LEVEL at its default of 1000, which made the partial mip chain mipmap incomplete and sample as black... so 3e6570c clamps it to the texture's declared mip count at bind time. Now let's see those device lab goldens. 🤞 |
|
Looks good to me, as long as the golden tests come back fine. The linux_fuchsia failure is an unrelated infrastructure issue that is being worked on. Unfortunately it'll block the rest of the CI from running until it gets resolved. |
|
The linux_fuchsia tests should be fixed by #187813. After that is in master, merge past it and try CI again. |
ShaderLibrary.fromAsset is synchronous, so the texture pipeline helper drops the stray await and matches the unlit helper. Also annotates the non-obvious locals per specify_nonobvious_local_variable_types and uses var where the type is obvious.
…into bdero/impeller-mipmap-sampling
|
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 Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
@b-luk the stars have aligned and the goldens are all looking as expected 🙌 |
b-luk
left a comment
There was a problem hiding this comment.
LGTM! Thanks for the revisions, Brandon. Sorry about the CI issues.
flutter/flutter@8bdce07...b7cb925 2026-06-12 engine-flutter-autoroll@skia.org Roll Skia from cadbde1ec4b7 to 8c89bf2b0ee3 (5 revisions) (flutter/flutter#187926) 2026-06-12 engine-flutter-autoroll@skia.org Roll Packages from 1b56cde to b78ad83 (5 revisions) (flutter/flutter#187928) 2026-06-12 engine-flutter-autoroll@skia.org Roll Dart SDK from f3441f2067ae to f6c31f4c3a63 (17 revisions) (flutter/flutter#187924) 2026-06-12 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 2KosSR4ONUjIB7tP_... to A3eaUn9mQ_EkSNxVI... (flutter/flutter#187923) 2026-06-12 engine-flutter-autoroll@skia.org Roll Skia from a2228b926c68 to cadbde1ec4b7 (9 revisions) (flutter/flutter#187921) 2026-06-12 kustermann@google.com Remove dynamic module loading code in flutter web engine (flutter/flutter#187777) 2026-06-12 matt.kosarek@canonical.com Remove EnableTransparentWindowBackground because it did nothing important and because Windows 10 does not support DWMWA_SYSTEMBACKDROP_TYPE (flutter/flutter#187848) 2026-06-12 engine-flutter-autoroll@skia.org Roll Fuchsia Test Scripts from dQ4PjIJB5kZFU8Y32... to EmfiOMUge_nnNS33B... (flutter/flutter#187912) 2026-06-12 mvincentong@gmail.com Clarify RichText selection docs (flutter/flutter#186844) 2026-06-12 1063596+reidbaker@users.noreply.github.com Custom KGP task and migration to AGP api for geting kgp version (flutter/flutter#182788) 2026-06-12 engine-flutter-autoroll@skia.org Roll Skia from f61acb31edf8 to a2228b926c68 (5 revisions) (flutter/flutter#187896) 2026-06-12 bdero@google.com [Flutter GPU] Expose ASTC HDR texture formats (flutter/flutter#187715) 2026-06-12 awolff@google.com Expand coverage of android_hardware_smoke_test. Add image, text, blend mode, and blur tests. (flutter/flutter#187600) 2026-06-11 bdero@google.com [Flutter GPU] Add blit operations (flutter/flutter#187289) 2026-06-11 bkonyi@google.com [flutter_tools] Fix version cache git fallback performance regression (flutter/flutter#187400) 2026-06-11 nshahan@google.com Rewrite `-d web-server` hot reload/restart tests (flutter/flutter#187453) 2026-06-11 bdero@google.com [Impeller] Allow sampling textures with manually-uploaded mip levels (flutter/flutter#187729) 2026-06-11 154381524+flutteractionsbot@users.noreply.github.com Sync CHANGELOG.md from stable (flutter/flutter#187884) 2026-06-11 47866232+chunhtai@users.noreply.github.com iOS a11y sets header trait based on heading level (flutter/flutter#186916) 2026-06-11 engine-flutter-autoroll@skia.org Roll Skia from 9f02102df298 to f61acb31edf8 (19 revisions) (flutter/flutter#187869) 2026-06-11 engine-flutter-autoroll@skia.org Roll ICU from ee5f27adc28b to d578f2e8b7bd (8 revisions) (flutter/flutter#187829) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC louisehsu@google.com,stuartmorgan@google.com 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
…lutter#187729) Removes the bind-time `NeedsMipmapGeneration()` check in the Metal and OpenGL ES backends, which rejected sampling any `mip_count > 1` texture whose mips were uploaded by hand (`Texture.overwrite(mipLevel:)`) rather than produced by the `GenerateMipmap` blit. Why remove it: - It's inconsistent across backends. Vulkan has no equivalent, and the Metal check is already skipped on iOS, so manually-mipped textures already sample fine there; only desktop Metal and GLES failed. - Makes stuff like prefiltered IBL atlases & compressed-texture mip chains impossible. The `mipmap_generated_`/`NeedsMipmapGeneration()` state tracking is kept, the Gaussian blur and render-target mip generation still read it; only the bind-time validation is removed. Adds the `CanSampleManuallyMippedTexture` render golden. ## 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 documentation (doc comments with `///`). - [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. <!-- Links --> [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/ [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Removes the bind-time
NeedsMipmapGeneration()check in the Metal and OpenGL ES backends, which rejected sampling anymip_count > 1texture whose mips were uploaded by hand (Texture.overwrite(mipLevel:)) rather than produced by theGenerateMipmapblit.Why remove it:
The
mipmap_generated_/NeedsMipmapGeneration()state tracking is kept, the Gaussian blur and render-target mip generation still read it; only the bind-time validation is removed.Adds the
CanSampleManuallyMippedTexturerender golden.Pre-launch Checklist
///).