Repository navigation
Fix Impeller Vulkan sampling of explicitly supplied manual mip chains - #189966
Ram-Dawson wants to merge 2 commits into
Conversation
Allow Flutter GPU callers that provide every mip level explicitly to opt out of the Adreno mip-generation workaround. Keep ordinary samplers on base mip when the workaround is active, while preserving nearest and linear mip filtering for the explicit opt-in. Add descriptor key coverage and Vulkan sampler-library coverage for both paths.
github-actions
Bot
added
engine
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds an allow_manual_mip_sampling option to SamplerDescriptor and exposes it through the Flutter GPU Dart and C++ APIs. This option allows manual mipmap sampling to bypass the Vulkan backend's mipmap generation workarounds. Relevant unit tests have been added to verify the key generation and the workaround behavior. There are no review comments, and I have no feedback to provide.
|
So rather than landing #189897 or #189966 and exposing hackery to flutter_gpu users, I would much rather land something like #190186 as a backstop to allow sampling in cases where there are manually uploaded mip levels, since the bug that lead to the kBase clamping only affects automatic mipmap generation, which I intentionally chose not to expose through flutter_gpu's API. But better than all of these things would be to just reopen #161283, fix this the right way and make automatic mip generation just work on all devices, and delete the hackery... We should do what the other production HALs do to get around this Adreno mip generation bug and add an alternative rendering fallback for it. That'd also make it so Impeller's 2D renderer can actually safely rely on automatic mipmap generation in general. Get rid of a huge invisible footgun. Reopened it and am prototyping a fix... |
|
Thanks, that makes sense. #190186 keeps the generated-mip workaround in place while restoring sampling for hand-uploaded mip chains without exposing an Adreno-specific opt-in through the Flutter GPU API. That is a better boundary than #189966. The I agree that #190186 should supersede this proposal. I will close #189966 in favor of #190186 and keep the standalone repro and device evidence available for validation. |
Related to #189965 and #189897
Summary
On affected Adreno Vulkan devices, the
broken_mipmap_generationworkaround forces ordinary mip samplers to base mip. This is appropriate for generated mipmaps, but it also affects complete mip chains supplied explicitly by Flutter GPU callers.This change proposes an explicit
allowManualMipSamplingopt-in. The default remainsfalse, so ordinary samplers continue to use the existing base-mip workaround. Callers that supply every mip level explicitly can preserve their requested nearest or linear mip filtering.#189897 is the safe capability correction: it reports manually mipped textures as unsupported while the workaround is active, allowing capability-aware clients to fall back. This PR is not a replacement for that behavior. It is a follow-up proposal for maintainers to evaluate whether complete manually supplied mip chains should have an explicit opt-in path.
Evidence
The standalone reproduction from #189965 isolates the behavior without
flutter_scene, scene lighting, shadow maps, or a material system.The matching roughness 0.45 probes are available in the public reproduction repository.
Tests
out/host_debug_unopt/impeller_unittests \ --gtest_filter='SamplerDescriptorTest.ToKeyIncludesAllFields:SamplerLibraryVK.WorkaroundsCanDisableReadingFromMipLevels:SamplerLibraryVK.WorkaroundsPreserveExplicitManualMipSampling:SamplerLibraryVK.WorkaroundsPreserveExplicitManualNearestMipSampling'The four focused tests pass. Device validation and local-engine builds were completed against framework revision
ea1bc5dc4f6fedce69957417b6907e45615146e2, the master revision used for the investigation.masterhas advanced since that validation; this PR's CI provides corresponding coverage against the current target branch.Scope
This changes Vulkan sampler selection only. It does not change the Android shadow path or address other rendering artifacts. If the added Flutter GPU API is not considered worthwhile or safe to expose, this PR should be closed in favor of the capability correction in #189897.
Pre-launch Checklist
///).