Repository navigation
[Impeller] Scope the broken mipmap workaround to the textures it affects - #190186
Conversation
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 introduces an allow_manual_mip_sampling flag to SamplerDescriptor to prevent hand-uploaded mipmaps from being clamped to the base mip level by the Adreno workaround, and adds corresponding unit tests. Feedback suggests that using NeedsMipmapGeneration() to detect hand-uploaded mipmaps relies on an implementation detail, and recommends explicitly tracking manual mipmaps or adding a dedicated method to improve maintainability.
|
Working on an alternative to this approach at the moment. See comment: #189966 (comment) |
|
considering you mentioned you are still working on this, moved to draft |
bdero
changed the title
f8e4001 to
c9c8bdc
Compare
There was a problem hiding this comment.
Code Review
This pull request implements a workaround for Vulkan drivers that generate corrupt mipmap levels by creating and sampling through an image view restricted to the base mip level, replacing the previous sampler-level workaround. The review feedback identifies a potential resource leak in 'allocator_vk.cc' if the base mip image view creation fails and returns early, as well as a compilation error on 32-bit platforms in 'mock_vulkan.cc' due to casting a 64-bit atomic integer to a pointer type.
I am having trouble creating individual review comments. Click here to see my feedback.
engine/src/flutter/impeller/renderer/backend/vulkan/allocator_vk.cc (472-478)
If device.createImageViewUnique fails, returning early here will leak the allocated VkImage and VmaAllocation because they have not yet been wrapped in the RAII UniqueImageVMA container (which only happens at line 481 inside ImageResource).\n\nSince the base mip image view is a workaround for sampling on specific drivers, a failure to create it should not fail the entire texture allocation. We can safely log the error and proceed without the base mip view, falling back to the main image view.
auto [base_result, base_view] = device.createImageViewUnique(view_info);\n if (base_result == vk::Result::eSuccess) {\n base_mip_image_view = std::move(base_view);\n } else {\n VALIDATION_LOG << \"Unable to create a base mip image view: \"\n << vk::to_string(base_result);\n }engine/src/flutter/impeller/renderer/backend/vulkan/test/mock_vulkan.cc (488-489)
On 32-bit platforms (such as 32-bit ARM, which Flutter still supports), VkImageView is a 32-bit pointer type. Casting a 64-bit integer (std::atomic_uint64_t) to a 32-bit pointer using reinterpret_cast will trigger a compilation error (-Wint-to-pointer-cast).\n\nUsing uintptr_t ensures that the integer is always the same size as a pointer on the target architecture, preventing compilation failures on 32-bit builds.
static std::atomic<uintptr_t> next_handle = 0xFEE1DEAD;\n *pView = reinterpret_cast<VkImageView>(next_handle.fetch_add(1));
|
@gaaclarke I revisited this to make the workaround for this bug less far reaching, so that it doesn't unnecessarily touch mip chains that have nothing to do with automatic mip generation (and are thus not affected by the Adreno bug). I think we should land this workaround scope reduction before considering something like #190264, since adding an alternative rendering path for automatic mips is a much more involved change (and we might be better off doing that once I have compute pipelines in a better place anyway). |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a workaround for Vulkan drivers that generate corrupt mipmap levels by creating and sampling from an image view restricted to the base mip level. Feedback on these changes highlights a potential resource leak in allocator_vk.cc if the base mip image view creation fails and returns early, suggesting instead to degrade gracefully. Additionally, it is recommended to move the new texture_vk_unittests.cc file into the test/ subdirectory to maintain consistency with the project's directory structure.
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
There are some clang-tidy failures:
❌ Failures for clang-tidy on /b/s/w/ir/cache/builder/engine/src/flutter/impeller/renderer/backend/vulkan/texture_vk_unittests.cc:
../../../flutter/impeller/renderer/backend/vulkan/test/mock_vulkan.h:114:5: error: Address of stack memory associated with local variable 'physical_properties_callback' returned to caller [clang-analyzer-core.StackAddressEscape,-warnings-as-errors]
114 | return *this;
| ^
/b/s/w/ir/cache/builder/engine/src/flutter/impeller/renderer/backend/vulkan/texture_vk_unittests.cc:77:24: note: Calling 'MakeContext'
77 | auto const context = MakeContext("Adreno (TM) 750", kQualcommVendorID);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/b/s/w/ir/cache/builder/engine/src/flutter/impeller/renderer/backend/vulkan/texture_vk_unittests.cc:27:10: note: Calling 'MockVulkanContextBuilder::SetPhysicalPropertiesCallback'
27 | return MockVulkanContextBuilder()
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
28 | .SetPhysicalPropertiesCallback(
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
29 | [device_name, vendor_id](VkPhysicalDevice device,
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
30 | VkPhysicalDeviceProperties* prop) {
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
31 | prop->vendorID = vendor_id;
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~
32 | device_name.copy(prop->deviceName, device_name.size());
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
33 | prop->deviceType = VK_PHYSICAL_DEVICE_TYPE_INTEGRATED_GPU;
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
34 | })
| ~~
../../../flutter/impeller/renderer/backend/vulkan/test/mock_vulkan.h:114:5: note: Address of stack memory associated with local variable 'physical_properties_callback' returned to caller
110 | std::function<void(VkPhysicalDevice device,
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
111 | VkPhysicalDeviceProperties* physicalProperties)>
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
112 | physical_properties_callback) {
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
113 | physical_properties_callback_ = std::move(physical_properties_callback);
114 | return *this;
| ^ ~~~~~
Suppressed 11894 warnings (11800 in non-user code, 94 NOLINT).
Use -header-filter=.* or leave it as default to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning treated as error
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. 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. |
e41dc15 to
e695cbc
Compare
|
Rebased onto master and fixed the clang-tidy failure in e695cbc. |
|
(don't forget to hit "Re-request review" or I might miss these =) |
|
Rebased onto master to pick up a fix for the unrelated ANGLE failure. |
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
Thanks for taking the time to explain, lgtm!
gaaclarke
added
the
autosubmit
auto-submit
Bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/190186, because - The status or check suite Dashboard Checks has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
I'm not sure why the presubmit is failing. I've filed an issue: #193175 |
The broken-mipmap workaround clamps every Vulkan sampler to the base mip level so corrupt blit-generated chains are never read. That also collapsed textures whose mip levels were uploaded by hand, which are valid. Skip the clamp for those, detected via NeedsMipmapGeneration, so Flutter GPU manual mip chains sample correctly while generated chains stay clamped.
Only textures whose mips came from the broken generation blit get clamped to the base level, instead of every sampler in the process.
The workaround now lives with the texture whose levels are corrupt. Drivers that generate bad mips get a base-level-only image view, used when sampling a texture those drivers mipped. Samplers, the sampler descriptor, and Flutter GPU no longer know about it.
clang-tidy traced a stack address escaping through the builder's callback. Plain function pointers carry no stack state, and a named builder keeps the reference it returns out of the caller.
c708e63 to
05407e9
Compare
bdero
added
CICD
flutter-dashboard
Bot
removed
the
autosubmit
Fixes bdero/flutter_scene#241. Related to #161283.
Adreno drivers corrupt blit-generated mip chains. The workaround for that rewrote every Vulkan sampler to
MipFilter::kBase, so nothing in the process could be sampled past its base mip level. But the corruption belongs to the texture whose levels were generated, not to the sampler, so textures with hand-uploaded mip chains (prefiltered environment maps, for one) lost their mip filtering for no reason. Flutter GPU is hit hardest, since it exposes no mipmap generation at all and every mip chain it binds was uploaded by the app.The workaround now lives with the texture. On affected drivers a mipped sampled texture also gets an image view covering only the base level, and
TextureVK::GetSampledImageViewhands that view out for textures those drivers mipped. Generated chains stay unreadable past the base level, every other texture samples as requested, and the sampler library,SamplerDescriptor, andlib/gpuno longer carry any of it.A texture with
mip_count > 1whose levels were never written is no longer masked on these devices, which matches its behavior on every other platform. #190264 fixes the generation itself and will delete what is left here.Pre-launch Checklist
///).