[Impeller] Vulkan: don't drop user-supplied viewport X, Y, and depth range#185886
Merged
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request fixes a bug in the Vulkan backend where SetViewport ignored X/Y offsets and depth ranges by implementing a ToVkViewport helper function. It also adds regression tests for GLES and Vulkan and updates the Vulkan mock to track recorded viewports. Review feedback suggests using triple-slash documentation comments to follow the style guide and simplifying the unit tests by removing a redundant variable.
7e67ca2 to
2186641
Compare
Member
I think you just dismiss the approval on the old goldens. |
gaaclarke
reviewed
May 1, 2026
|
|
||
| /// @brief Returns the viewports passed to `vkCmdSetViewport` calls on the | ||
| /// given command buffer, in call order. | ||
| std::vector<VkViewport>& GetRecordedViewports(VkCommandBuffer buffer); |
Member
There was a problem hiding this comment.
Suggested change
| std::vector<VkViewport>& GetRecordedViewports(VkCommandBuffer buffer); | |
| const std::vector<VkViewport>& GetRecordedViewports(VkCommandBuffer buffer) const; |
I didn't see a reason why this has to be a non-const function.
Member
|
(friendly ping: I think you addressed my feedback but didn't hit the "re-request review" button) |
|
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
|
Closing/reopening to retrigger CI. Current failures look like upstream pub.dev advisories format issue, not this PR. |
Contributor
|
An existing Git SHA, To re-trigger presubmits after closing or re-opeing a PR, or pushing a HEAD commit (i.e. with |
…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
Callers only read from the returned vector, so a const reference is the right type. Update the unittest's local alias to match.
1fef736 to
b2138a0
Compare
This was referenced May 10, 2026
auto-submit Bot
pushed a commit
to flutter/packages
that referenced
this pull request
May 14, 2026
Roll Flutter from 707dbc0420a3 to 23f6f5853f50 (149 revisions) flutter/flutter@707dbc0...23f6f58 2026-05-12 737941+loic-sharma@users.noreply.github.com Add 'cp: review' label to the manual cherrypick process (flutter/flutter#186158) 2026-05-12 engine-flutter-autoroll@skia.org Roll Packages from 19ec8b8 to 93cbed6 (3 revisions) (flutter/flutter#186401) 2026-05-12 30870216+gaaclarke@users.noreply.github.com Removes SDF option for macOS (always enabled) (flutter/flutter#186265) 2026-05-12 nico.reiab@gmail.com docs: fix typos in flutter_tools comments (flutter/flutter#186321) 2026-05-12 15619084+vashworth@users.noreply.github.com Pass XcodeBasedProject instead of String to functions in XcodeProjectInterpreter (flutter/flutter#186378) 2026-05-12 jason-simmons@users.noreply.github.com Update iOS scenario app test goldens to match changes from flutter/flutter#182662 (flutter/flutter#186390) 2026-05-12 engine-flutter-autoroll@skia.org Roll Skia from ad0aff15b9fa to 77a21bc723dc (2 revisions) (flutter/flutter#186396) 2026-05-12 32538273+ValentinVignal@users.noreply.github.com Migrate focus_node.unfocus.0.dart to use `RadioGroup` (flutter/flutter#183979) 2026-05-12 engine-flutter-autoroll@skia.org Roll Skia from 91d3c1e730af to ad0aff15b9fa (7 revisions) (flutter/flutter#186391) 2026-05-12 bdero@google.com [Flutter GPU] Allow customizing the vertex layout on a RenderPipeline (flutter/flutter#186310) 2026-05-12 97480502+b-luk@users.noreply.github.com Fix `EmbedderTest.CanRenderTextWithImpellerMetal` test breakage (flutter/flutter#186262) 2026-05-12 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from rFhU-YPqdCRCtCz7b... to z7ICmPtn4hspu02zk... (flutter/flutter#186384) 2026-05-12 bdero@google.com [Impeller] GLES: lazily allocate texture mip levels on first per-level write (flutter/flutter#186302) 2026-05-12 bdero@google.com [Android] Propagate --enable-flutter-gpu Intent extra to engine args (flutter/flutter#186298) 2026-05-11 47866232+chunhtai@users.noreply.github.com [ci] update no-response workflow to also look for old label name in e… (flutter/flutter#186373) 2026-05-11 bdero@google.com [ImpellerC] Write a depfile when --shader-bundle is in use (flutter/flutter#186341) 2026-05-11 nico.reiab@gmail.com docs: fix doubled-word typos in comments (flutter/flutter#186320) 2026-05-11 engine-flutter-autoroll@skia.org Roll Skia from 32281401997e to 91d3c1e730af (4 revisions) (flutter/flutter#186368) 2026-05-11 15619084+vashworth@users.noreply.github.com Show SwiftPM warnings right before iOS/macOS build (flutter/flutter#185984) 2026-05-11 15619084+vashworth@users.noreply.github.com Convert rebuilding-flutter-tool script to dart (flutter/flutter#185089) 2026-05-11 15619084+vashworth@users.noreply.github.com Use Xcode's LLDB (flutter/flutter#186273) 2026-05-11 mr-peipei@web.de Remove `currentMainUri` from `generateMainDartWithPluginRegistrant` (flutter/flutter#185907) 2026-05-11 engine-flutter-autoroll@skia.org Roll Skia from 2514f6b5f92b to 32281401997e (1 revision) (flutter/flutter#186349) 2026-05-11 engine-flutter-autoroll@skia.org Roll Packages from 92552b1 to 19ec8b8 (4 revisions) (flutter/flutter#186350) 2026-05-11 1063596+reidbaker@users.noreply.github.com Check for absolute paths in skills. (flutter/flutter#185632) 2026-05-11 engine-flutter-autoroll@skia.org Roll Skia from 9fb7d2814642 to 2514f6b5f92b (1 revision) (flutter/flutter#186347) 2026-05-11 engine-flutter-autoroll@skia.org Roll Skia from 8cafb209e836 to 9fb7d2814642 (4 revisions) (flutter/flutter#186335) 2026-05-10 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from sOBiPJb0xznDBZlf5... to rFhU-YPqdCRCtCz7b... (flutter/flutter#186328) 2026-05-10 engine-flutter-autoroll@skia.org Roll Skia from 05a03f99c74e to 8cafb209e836 (1 revision) (flutter/flutter#186315) 2026-05-10 bdero@google.com [Impeller] Vulkan: don't drop user-supplied viewport X, Y, and depth range (flutter/flutter#185886) 2026-05-09 mbrase@google.com Update Fuchsia tests to subpackage their child components (flutter/flutter#186259) 2026-05-09 victorsanniay@gmail.com Fix SelectableText crash with inline lambda contextMenuBuilder (flutter/flutter#184990) 2026-05-09 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 5_TnhTsHSqtCx37o6... to sOBiPJb0xznDBZlf5... (flutter/flutter#186289) 2026-05-09 engine-flutter-autoroll@skia.org Roll Skia from dc78d4bd2efb to 05a03f99c74e (2 revisions) (flutter/flutter#186283) 2026-05-09 22373191+Hari-07@users.noreply.github.com Improve non rect platform view rendering (flutter/flutter#182662) 2026-05-08 engine-flutter-autoroll@skia.org Roll Skia from 31521f8508c7 to dc78d4bd2efb (1 revision) (flutter/flutter#186278) 2026-05-08 30870216+gaaclarke@users.noreply.github.com Moves wide_gamut_macos to arm64 (flutter/flutter#186214) 2026-05-08 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[iOS] Migrate VSyncClient to a pure Obj-C implementation (#186166)" (flutter/flutter#186266) 2026-05-08 engine-flutter-autoroll@skia.org Roll Skia from a00db8749edb to 31521f8508c7 (2 revisions) (flutter/flutter#186264) 2026-05-08 97480502+b-luk@users.noreply.github.com Optimize compatible `DrawDiffRoundRect` calls to use `DrawRoundRect` (flutter/flutter#186203) 2026-05-08 bdero@google.com [triage] Add Flutter GPU as a triage team (flutter/flutter#186263) 2026-05-08 dmgr@google.com doc: Unified Check-Run User manual (flutter/flutter#186210) 2026-05-08 engine-flutter-autoroll@skia.org Roll Skia from 5f7adf4403d6 to a00db8749edb (1 revision) (flutter/flutter#186257) 2026-05-08 engine-flutter-autoroll@skia.org Roll Packages from 0411f1d to 92552b1 (1 revision) (flutter/flutter#186256) 2026-05-08 34871572+gmackall@users.noreply.github.com Add logging to figure out jvm crashes for `hot_mode_tests` (flutter/flutter#186107) 2026-05-08 engine-flutter-autoroll@skia.org Roll Skia from 926c09741ce2 to 5f7adf4403d6 (3 revisions) (flutter/flutter#186242) ...
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
3 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.
Fixes #185885
RenderPassVK::SetViewportbuilds avk::Viewportvia chained setters that never invoke.setX(...). Becausevk::Viewport()default-initializesxto0.0f, the user's X is silently dropped. Y is also effectively dropped (the existing code callssetY(rect.GetHeight())to perform the negative-height Y-flip, never readingrect.GetY()), andminDepth/maxDepthare hardcoded to 0/1, ignoring the user'sDepthRange.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.pngwill need to be re-uploaded on Skia Gold to match the corrected output. Theflutter_gpu_test_viewporttest ingpu_test.dartis currently disabled on GLES (re-enabled in #185879); both backends will produce the same correct render after this PR plus #185879 land.