[Flutter GPU] Carry vec_size & columns in shader bundle uniform metadata#185879
Merged
bdero merged 1 commit intoMay 1, 2026
Merged
Conversation
1ff45f2 to
5541dec
Compare
Flutter GPU shader bundles serialize uniform struct field metadata via a flatbuffer schema (`shader_bundle.fbs`) that carries per-field type and size information, but does not carry the underlying vector/column dimensions. This is enough to bind uniforms on Metal and Vulkan, but it isn't enough to distinguish shapes that share the same `element_size_in_bytes`, most notably `vec4` vs `mat2`. The GLES backend uses `ShaderStructMemberMetadata::float_type` to pick the right `glUniform*` call, and the bundle loader had no way to populate it. PR flutter#182229 ("Turns on most of fragment_shader_test.dart for opengles") introduced `ShaderFloatType`, the GLES validation that requires it, and the fix for runtime effect shaders (`runtime_effect_contents.cc`), but the equivalent fix for the Flutter GPU shader bundle loader (`lib/gpu/shader_library.cc`) was not made. As a result, every flutter_gpu app with a uniform block crashes the GLES backend on all platforms (Windows ANGLE, Linux GLES, Android GLES) with: Float uniform should have a float type. The size-only signal carried by the bundle is not sufficient to derive `float_type` correctly for `mat2` and `mat3`, which would otherwise collide with `vec2` arrays and `vec3` arrays. To fix this without regressing matrix-typed uniforms, this change extends the schema to carry `vec_size` and `columns` (matching what `ShaderInput` already does for vertex inputs), and uses them to derive `float_type` in the bundle loader via the same shape-based dispatch that runtime effects already use. The shader bundle format version is bumped from 1 to 2 because old bundles do not carry the new fields. Old bundles loaded against a new engine fail with a clear error pointing at the impellerc/SDK version mismatch, rather than silently rendering with the wrong glUniform call. Tests: - `shader_bundle_unittests.cc`: verify the bundle now carries `vec_size` and `columns` for the existing `mvp` (mat4) and `color` (vec4) test fixtures, and exhaustively cover `DeriveShaderFloatType` for float scalars, all vector sizes, all square matrices, non-square shapes, non-float types, and zero (legacy bundle) values. - `buffer_bindings_gles_unittests.cc`: add a regression guard asserting that the GLES backend rejects a float uniform that arrives without `float_type` populated, so a future change that forgets to populate it (in any code path) fails at unit test time. - `gpu_test.dart`: re-enable the eight Flutter GPU tests that were disabled for the opengles backend pending this fix (triangle, MSAA triangle, polygon mode, indexed triangle, stencil, cull mode, scissor, viewport). Fixes flutter#184953 Fixes flutter#183530 May also resolve flutter#176875, flutter#164438, and possibly flutter#164745. To be verified against the linked reproducers after merge.
5541dec to
731a0c3
Compare
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the shader bundle format to version 2, adding vec_size and columns fields to ShaderUniformStructField to disambiguate shader types with identical byte sizes. It introduces a DeriveShaderFloatType utility in shader_types.h and updates the reflector, shader bundle data, and shader library to support these new dimensions. Additionally, a regression test was added to the GLES backend, and several Dart GPU tests were re-enabled for OpenGLES. I have no feedback to provide.
Member
Author
|
Re-enables the previously failing GLES Flutter GPU tests. A good chunk of this diff is caused by changes to the Dart test formatting via |
|
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. |
Member
Author
|
The golden change is caused by a preexisting bug on the Vulkan backend. The new OpenGLES golden being uploaded by this PR is the correct render; the existing Vulkan reference golden is buggy. Tracking in #185885, will fix.
|
bdero
added a commit
to bdero/flutter
that referenced
this pull request
May 1, 2026
…range
`RenderPassVK::SetViewport` builds a `vk::Viewport` via chained setters
that never invoke `.setX(...)`. Because `vk::Viewport()` default-
initializes `x` to `0.0f`, the user's X is silently dropped. Y is also
effectively dropped (the existing code calls `setY(rect.GetHeight())`
to perform the negative-height Y-flip, never reading `rect.GetY()`),
and `minDepth` / `maxDepth` are hardcoded to 0/1, ignoring the user's
`DepthRange`.
The bug has been present since `e7be989feb1c` ("Reland: Encode
directly to command buffer.", 2024-01-19) and stayed hidden because:
- No Vulkan unit test exercised non-zero `x`, `y`, or a non-default
depth range in `SetViewport`.
- The `Can render portion of the triangle using viewport` test in
`gpu_test.dart` was running on Vulkan and producing a buggy golden
once, then comparing against itself forever.
- The same test was disabled on the GLES backend via
flutter#183530 due to an upstream
`float_type` regression, so the GLES rendering (which honors X
correctly) never ran to expose the discrepancy.
The `float_type` fix in
flutter#185879 re-enables the GLES
test, which produces a (correct) centered render that does not match
the (buggy) Vulkan reference.
This fix:
- Adds `.setX(viewport.rect.GetX())`.
- Replaces `.setY(viewport.rect.GetHeight())` with
`.setY(viewport.rect.GetY() + viewport.rect.GetHeight())` so a
non-zero Y offset propagates through the negative-height flip.
- Replaces hardcoded `setMinDepth(0.0f)` / `setMaxDepth(1.0f)` with
the user's `viewport.depth_range.z_near` / `z_far`.
- Extracts the conversion into a `ToVkViewport` static helper so the
initial-viewport setup at construction and `SetViewport` share a
single formula.
Tests:
- `RenderPassVK.SetViewportPropagatesAllUserSuppliedFields` is a new
regression guard. It uses an extension to the Vulkan mock
(`recorded_viewports_` on `MockCommandBuffer`, exposed via
`GetRecordedViewports`) that records the actual `VkViewport`
parameters passed to `vkCmdSetViewport`. The test asserts that all
six fields (`x`, `y`, `width`, `height`, `minDepth`, `maxDepth`)
reach the GPU with the correct values for a viewport with non-zero
X, Y, and a non-default depth range.
- `RenderPassGLESViewportTest.ViewportWithNonZeroXOffsetReachesGL`
is a sibling regression guard for the GLES backend (which has
always been correct, but was previously not asserted with a
non-zero X) so a future change can't silently regress it.
Once this lands, the existing Vulkan golden for
`flutter_gpu_test_viewport.png` will need to be re-uploaded on Skia
Gold to match the corrected output.
Fixes flutter#185885
This was referenced May 2, 2026
bdero
added a commit
to bdero/flutter
that referenced
this pull request
May 10, 2026
…range
`RenderPassVK::SetViewport` builds a `vk::Viewport` via chained setters
that never invoke `.setX(...)`. Because `vk::Viewport()` default-
initializes `x` to `0.0f`, the user's X is silently dropped. Y is also
effectively dropped (the existing code calls `setY(rect.GetHeight())`
to perform the negative-height Y-flip, never reading `rect.GetY()`),
and `minDepth` / `maxDepth` are hardcoded to 0/1, ignoring the user's
`DepthRange`.
The bug has been present since `e7be989feb1c` ("Reland: Encode
directly to command buffer.", 2024-01-19) and stayed hidden because:
- No Vulkan unit test exercised non-zero `x`, `y`, or a non-default
depth range in `SetViewport`.
- The `Can render portion of the triangle using viewport` test in
`gpu_test.dart` was running on Vulkan and producing a buggy golden
once, then comparing against itself forever.
- The same test was disabled on the GLES backend via
flutter#183530 due to an upstream
`float_type` regression, so the GLES rendering (which honors X
correctly) never ran to expose the discrepancy.
The `float_type` fix in
flutter#185879 re-enables the GLES
test, which produces a (correct) centered render that does not match
the (buggy) Vulkan reference.
This fix:
- Adds `.setX(viewport.rect.GetX())`.
- Replaces `.setY(viewport.rect.GetHeight())` with
`.setY(viewport.rect.GetY() + viewport.rect.GetHeight())` so a
non-zero Y offset propagates through the negative-height flip.
- Replaces hardcoded `setMinDepth(0.0f)` / `setMaxDepth(1.0f)` with
the user's `viewport.depth_range.z_near` / `z_far`.
- Extracts the conversion into a `ToVkViewport` static helper so the
initial-viewport setup at construction and `SetViewport` share a
single formula.
Tests:
- `RenderPassVK.SetViewportPropagatesAllUserSuppliedFields` is a new
regression guard. It uses an extension to the Vulkan mock
(`recorded_viewports_` on `MockCommandBuffer`, exposed via
`GetRecordedViewports`) that records the actual `VkViewport`
parameters passed to `vkCmdSetViewport`. The test asserts that all
six fields (`x`, `y`, `width`, `height`, `minDepth`, `maxDepth`)
reach the GPU with the correct values for a viewport with non-zero
X, Y, and a non-default depth range.
- `RenderPassGLESViewportTest.ViewportWithNonZeroXOffsetReachesGL`
is a sibling regression guard for the GLES backend (which has
always been correct, but was previously not asserted with a
non-zero X) so a future change can't silently regress it.
Once this lands, the existing Vulkan golden for
`flutter_gpu_test_viewport.png` will need to be re-uploaded on Skia
Gold to match the corrected output.
Fixes flutter#185885
pull Bot
pushed a commit
to Mattlk13/flutter
that referenced
this pull request
May 10, 2026
…range (flutter#185886) Fixes flutter#185885 `RenderPassVK::SetViewport` builds a `vk::Viewport` via chained setters that never invoke `.setX(...)`. Because `vk::Viewport()` default-initializes `x` to `0.0f`, the user's X is silently dropped. Y is also effectively dropped (the existing code calls `setY(rect.GetHeight())` to perform the negative-height Y-flip, never reading `rect.GetY()`), and `minDepth` / `maxDepth` are hardcoded to 0/1, ignoring the user's `DepthRange`. The bug has been present since `e7be989feb1c` ("Reland: Encode directly to command buffer.", 2024-01-19). ## Goldens Once this lands, the existing Vulkan golden for `flutter_gpu_test_viewport.png` will need to be re-uploaded on Skia Gold to match the corrected output. The `flutter_gpu_test_viewport` test in `gpu_test.dart` is currently disabled on GLES (re-enabled in flutter#185879); both backends will produce the same correct render after this PR plus flutter#185879 land.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Flutter GPU shader bundles serialize uniform struct field metadata via a flatbuffer schema (
shader_bundle.fbs) that carries per-field type and size information, but does not carry the underlying vector/column dimensions. This is enough to bind uniforms on Metal and Vulkan, but it isn't enough to distinguish shapes that share the sameelement_size_in_bytes, most notablyvec4vsmat2. The GLES backend usesShaderStructMemberMetadata::float_typeto pick the rightglUniform*call, and the bundle loader had no way to populate it.PR #182229 ("Turns on most of fragment_shader_test.dart for opengles") introduced
ShaderFloatType, the GLES validation that requires it, and the fix for runtime effect shaders (runtime_effect_contents.cc), but the equivalent fix for the Flutter GPU shader bundle loader (lib/gpu/shader_library.cc) was not made. As a result, every flutter_gpu app with a uniform block crashes the GLES backend on all platforms (Windows ANGLE, Linux GLES, Android GLES) with:Approach
The size-only signal already carried by the bundle is not sufficient to derive
float_typecorrectly format2andmat3, which would otherwise collide withvec2arrays andvec3arrays. (impellerc serializes amat2uniform withelement_size_in_bytes = 8andarray_elements = 2, which is the same shape as a length-2vec2array. Same story format3.) To fix this without regressing matrix-typed uniforms, this PR extends the schema to carryvec_sizeandcolumns(matching whatShaderInputalready does for vertex inputs), and uses them to derivefloat_typein the bundle loader via the same shape-based dispatch that runtime effects already use.The shader bundle format version is bumped from 1 to 2 because old bundles do not carry the new fields. Old bundles loaded against a new engine fail with a clear error pointing at the impellerc/SDK version mismatch, rather than silently rendering with the wrong
glUniform*call.Tests
shader_bundle_unittests.cc: verify the bundle carriesvec_sizeandcolumnsfor the existingmvp(mat4) andcolor(vec4) test fixtures. Exhaustively coverDeriveShaderFloatTypefor float scalars, all vector sizes, all square matrices, non-square shapes, non-float types, and zero (legacy bundle) values.buffer_bindings_gles_unittests.cc: regression guard asserting that the GLES backend rejects a float uniform that arrives withoutfloat_typepopulated. If a future change forgets to populatefloat_typein any code path (shader bundle loader, runtime effects, or anything else), this test catches it at unit-test time instead of at runtime.gpu_test.dart: the eight re-enabled tests cover triangle rendering, polygon mode, MSAA, indexed draw, stencil, cull mode, scissor, and viewport on the opengles backend, end-to-end through the bundle loader.Issues
Fixes #184953
Fixes #183530
May also resolve the following issues, all of which surface as GLES uniform binding failures and share the same root-cause family. To be verified against the linked reproducers after merge:
This supersedes the smaller fix in
bdero/flutter-gpu-float-types(draft #185874), which derivedfloat_typefromelement_size_in_bytesonly and would have misclassifiedmat2/mat3asvec2/vec3.