Repository navigation
Conversation
The Adreno mipmap workaround rewrites every sampler to base-mip-only, but SupportsManuallyMippedTextures still returned true, so clients that upload their own mip chains sample only the base level with no way to detect it. Record the workaround in CapabilitiesVK::ApplyWorkarounds and report the capability truthfully.
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 updates Impeller's Vulkan backend to disable support for manually mipped textures on devices where the mipmap generation workaround is active (such as Adreno GPUs), and adds corresponding unit tests and documentation. Feedback on the changes highlights a potential buffer overflow or memory read issue in the test helper where std::string_view::copy is used without ensuring proper null-termination of the destination buffer.
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
LGTM, just a couple style notes
| } | ||
|
|
||
| bool CanSampleManuallyWrittenMips(std::string_view driver_name, | ||
| bool qc = true) { |
There was a problem hiding this comment.
| bool qc = true) { | |
| bool is_qualcom = true) { |
| TEST(DriverInfoVKTest, ManuallyMippedTexturesDisabledWithMipWorkaround) { | ||
| // The mipmap workaround rewrites every sampler to base-mip-only, so the | ||
| // capability must not advertise hand-written mip chains on these devices. | ||
| EXPECT_FALSE(CanSampleManuallyWrittenMips("Adreno (TM) 540", true)); |
There was a problem hiding this comment.
| EXPECT_FALSE(CanSampleManuallyWrittenMips("Adreno (TM) 540", true)); | |
| EXPECT_FALSE(CanSampleManuallyWrittenMips("Adreno (TM) 540", /*is_qualcom=*/true)); |
Please add labels to these literal arguments.
Rename the qc parameter to is_qualcomm and label the literal arguments at the call sites in the manually-mipped capability test.
|
I don't think this is the right workaround. It's just pushing a preexisting hack along. The actual bug is that automatic mip generation doesn't work on some devices, and we worked around it in Impeller by silently rewriting all samplers to kBase, which is just wrong behavior. We should implement an alternative rendering path for MIPS on these affected devices (#190264) and/or stop propagating the hack to flutter_gpu's samplers, since flutter_gpu intentionally doesn't even support automatic mip generation (#190186). |
On Adreno GPUs the mipmap workaround rewrites every sampler to
MipFilter::kBase, so nothing can sample past the base mip level, butCapabilitiesVK::SupportsManuallyMippedTexturesstill returned an unconditionaltrue. Clients that check the capability and upload their own mip chains sample only the base level, with no way to detect it. Flutter GPU exposes the capability asdoesSupportManuallyMippedTextures; prefiltered environment maps that store one roughness band per mip level render every band fully sharp on these devices, reported downstream in bdero/flutter_scene#241.This change records the workaround in
CapabilitiesVK::ApplyWorkaroundsand reports manually mipped textures as unsupported while it is active, so capability checks fall back the same way they do on backends that lack the feature. The capability docs are updated to match, and a new unit test verifies the capability is off for Adreno devices and on for Mali.Pre-launch Checklist
///).