[Impeller] Commands that don't specify their own viewports get the viewport of the render pass. by chinmaygarde · Pull Request #177473 · flutter/flutter

chinmaygarde · 2025-10-23T20:30:06Z

flutter-dashboard · 2025-10-23T20:30:10Z

It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

chinmaygarde · 2025-10-23T20:31:08Z

This is still a WIP as I am not able to create a reduced test case for the regression spotted in the linked issue using just the DisplayList layer. Pushing this speculative fix to test against existing goldens while I work on reasoning about the report.

gemini-code-assist

Code Review

This pull request refactors the viewport handling logic in RenderPassGLES. A new helper function, EncodeViewport, is introduced to consolidate setting the OpenGL viewport and depth range. This change fixes a bug where commands with a specific viewport did not have it applied correctly. The new implementation ensures that if a command specifies a viewport, it is used; otherwise, it defaults to the render pass's viewport. Additionally, it introduces an optimization to avoid redundant glViewport calls if the viewport has not changed between commands. The changes improve code clarity, correctness, and efficiency.

flutter-dashboard · 2025-11-12T20:35:30Z

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 package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Piinks · 2026-02-03T17:16:54Z

Greetings from stale PR triage! 👋
Is this change still on your radar?

zanderso · 2026-03-05T15:31:09Z

@jtmcdole or @gaaclarke It seems like this one is close to the finish line, but it needs some more thought about testing. cc'ing you here so it doesn't fall through the cracks.

gaaclarke

There is a variable that is passed in as what appears to be a cache or some sort but it is on the stack. It looks like the idea of the PR isn't fully implemented.

Plus it needs tests.

gaaclarke · 2026-03-05T18:54:51Z

I've looked at this closer and I think this should work, it just needs a test like chinmay said.

gaaclarke

I added 2 tests, one that makes sure we coalesce duplicate viewports and one for reverting to the render pass viewport.

auto-submit · 2026-03-05T22:11:21Z

autosubmit label was removed for flutter/flutter/177473, because This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.

Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

gaaclarke · 2026-03-05T22:13:06Z

Doh, thought chinmay got a pass there haha.

jtmcdole · 2026-03-05T22:22:00Z

+  // Viewport should only be called twice. Once for the fallback, once for the
+  // first override. We set a catch-all to 0 to ensure no other calls occur.
+  EXPECT_CALL(mock_gl_impl_ref, Viewport(_, _, _, _)).Times(0);
+  EXPECT_CALL(mock_gl_impl_ref, Viewport(0, 0, 100, 100)).Times(1);


took me a second to realize this was the first render_pass call and it was falling back to the created render pass 100,100. The expectation is far enough away, is it worth noting on 189 "// uses default viewport"

this is a nit comment and can be resolved if you don't agree.

Yea, it's kind of a bummer. I think there an alternative way to assert which is more like sequential versus how many times something is called. It would probably be more clear but I didn't think it was worth complicating.

zanderso · 2026-03-05T22:53:07Z

If @chinmaygarde would like, @gaaclarke you can sponsor adding him back to flutter-hackers on Discord.

…ewport of the render pass. Fixes #176945

pulled out some dupe code

auto-submit · 2026-04-13T18:49:01Z

autosubmit label was removed for flutter/flutter/177473, because This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.

Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a member of flutter-hackers before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

gaaclarke · 2026-04-13T18:49:18Z

@jtmcdole I had to rebase this because autosubmit failed and it got stale, can I get a reapproval?

…get the viewport of the render pass. (flutter/flutter#177473)

flutter/flutter@2fa45e0...c1b14e9 2026-04-14 15619084+vashworth@users.noreply.github.com Rebuild flutter tool skill (flutter/flutter#184975) 2026-04-14 engine-flutter-autoroll@skia.org Roll Skia from 0851d988db03 to 4c382df6a25a (1 revision) (flutter/flutter#185025) 2026-04-14 engine-flutter-autoroll@skia.org Roll Dart SDK from 5504504b38c2 to ee5afcef0596 (1 revision) (flutter/flutter#185024) 2026-04-14 dacoharkes@google.com [ci] Split up integration.shard record_use_test.dart (flutter/flutter#185022) 2026-04-14 engine-flutter-autoroll@skia.org Roll Skia from d34c84df4c37 to 0851d988db03 (3 revisions) (flutter/flutter#185012) 2026-04-14 dacoharkes@google.com [record_use] Add recorded uses to link hooks (flutter/flutter#184869) 2026-04-14 engine-flutter-autoroll@skia.org Roll Skia from 0e98a9c635bb to d34c84df4c37 (5 revisions) (flutter/flutter#185009) 2026-04-14 engine-flutter-autoroll@skia.org Roll Dart SDK from ef28089d6533 to 5504504b38c2 (3 revisions) (flutter/flutter#185008) 2026-04-14 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from K_2AkZL3Drs6cGE1q... to rB8LAuZL_DwHMssTU... (flutter/flutter#185007) 2026-04-14 rmacnak@google.com [fuchsia] Replace ambient-replace-as-executable with VmexResource. (flutter/flutter#184967) 2026-04-13 chinmaygarde@google.com [Impeller] Commands that don't specify their own viewports get the viewport of the render pass. (flutter/flutter#177473) 2026-04-13 engine-flutter-autoroll@skia.org Roll Skia from bc1df263ff3f to 0e98a9c635bb (1 revision) (flutter/flutter#184995) 2026-04-13 737941+loic-sharma@users.noreply.github.com Update autosubmit guide with the emergency label (flutter/flutter#184993) 2026-04-13 69043738+aNOOBisTheGod@users.noreply.github.com [flutter_tools] Cache pubspec reads and share PackageGraph/PackageConfig across workspace packages during pub get post-processing (flutter/flutter#184528) 2026-04-13 15619084+vashworth@users.noreply.github.com Fix codesign verification test for SwiftPM Add to App (flutter/flutter#184980) 2026-04-13 87962825+kyungilcho@users.noreply.github.com Preprovision Android NDK for flavored builds and reuse matching unflavored NDKs (flutter/flutter#183555) 2026-04-13 15619084+vashworth@users.noreply.github.com Reland "Disable async mode with LLDB" (flutter/flutter#184970) 2026-04-13 engine-flutter-autoroll@skia.org Roll Skia from 55ddd6bb8be5 to bc1df263ff3f (6 revisions) (flutter/flutter#184968) 2026-04-13 matej.knopp@gmail.com Expose platform specific handles for multi-window API (flutter/flutter#184662) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC boetger@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…ewport of the render pass. (flutter#177473) Fixes flutter#176945 --------- Co-authored-by: gaaclarke <30870216+gaaclarke@users.noreply.github.com>

…r#11506) flutter/flutter@2fa45e0...c1b14e9 2026-04-14 15619084+vashworth@users.noreply.github.com Rebuild flutter tool skill (flutter/flutter#184975) 2026-04-14 engine-flutter-autoroll@skia.org Roll Skia from 0851d988db03 to 4c382df6a25a (1 revision) (flutter/flutter#185025) 2026-04-14 engine-flutter-autoroll@skia.org Roll Dart SDK from 5504504b38c2 to ee5afcef0596 (1 revision) (flutter/flutter#185024) 2026-04-14 dacoharkes@google.com [ci] Split up integration.shard record_use_test.dart (flutter/flutter#185022) 2026-04-14 engine-flutter-autoroll@skia.org Roll Skia from d34c84df4c37 to 0851d988db03 (3 revisions) (flutter/flutter#185012) 2026-04-14 dacoharkes@google.com [record_use] Add recorded uses to link hooks (flutter/flutter#184869) 2026-04-14 engine-flutter-autoroll@skia.org Roll Skia from 0e98a9c635bb to d34c84df4c37 (5 revisions) (flutter/flutter#185009) 2026-04-14 engine-flutter-autoroll@skia.org Roll Dart SDK from ef28089d6533 to 5504504b38c2 (3 revisions) (flutter/flutter#185008) 2026-04-14 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from K_2AkZL3Drs6cGE1q... to rB8LAuZL_DwHMssTU... (flutter/flutter#185007) 2026-04-14 rmacnak@google.com [fuchsia] Replace ambient-replace-as-executable with VmexResource. (flutter/flutter#184967) 2026-04-13 chinmaygarde@google.com [Impeller] Commands that don't specify their own viewports get the viewport of the render pass. (flutter/flutter#177473) 2026-04-13 engine-flutter-autoroll@skia.org Roll Skia from bc1df263ff3f to 0e98a9c635bb (1 revision) (flutter/flutter#184995) 2026-04-13 737941+loic-sharma@users.noreply.github.com Update autosubmit guide with the emergency label (flutter/flutter#184993) 2026-04-13 69043738+aNOOBisTheGod@users.noreply.github.com [flutter_tools] Cache pubspec reads and share PackageGraph/PackageConfig across workspace packages during pub get post-processing (flutter/flutter#184528) 2026-04-13 15619084+vashworth@users.noreply.github.com Fix codesign verification test for SwiftPM Add to App (flutter/flutter#184980) 2026-04-13 87962825+kyungilcho@users.noreply.github.com Preprovision Android NDK for flavored builds and reuse matching unflavored NDKs (flutter/flutter#183555) 2026-04-13 15619084+vashworth@users.noreply.github.com Reland "Disable async mode with LLDB" (flutter/flutter#184970) 2026-04-13 engine-flutter-autoroll@skia.org Roll Skia from 55ddd6bb8be5 to bc1df263ff3f (6 revisions) (flutter/flutter#184968) 2026-04-13 matej.knopp@gmail.com Expose platform specific handles for multi-window API (flutter/flutter#184662) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC boetger@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

github-actions Bot added engine flutter/engine related. See also e: labels. e: impeller Impeller rendering backend issues and features requests labels Oct 23, 2025

gemini-code-assist Bot reviewed Oct 23, 2025

View reviewed changes

jtmcdole reviewed Mar 5, 2026

View reviewed changes

Comment thread engine/src/flutter/impeller/renderer/backend/gles/render_pass_gles.cc

gaaclarke requested changes Mar 5, 2026

View reviewed changes

Comment thread engine/src/flutter/impeller/renderer/backend/gles/render_pass_gles.cc

gaaclarke force-pushed the staleviewport branch from 28317f0 to c6aa6f8 Compare March 5, 2026 22:04

gaaclarke previously approved these changes Mar 5, 2026

View reviewed changes

gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 5, 2026

auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 5, 2026

jtmcdole previously approved these changes Mar 5, 2026

View reviewed changes

gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 5, 2026

chinmaygarde and others added 2 commits April 13, 2026 11:31

[Impeller] Commands that don't specify their own viewports get the vi…

9ecac2d

…ewport of the render pass. Fixes #176945

added test

b05f3c5

pulled out some dupe code

gaaclarke dismissed stale reviews from jtmcdole and themself via b05f3c5 April 13, 2026 18:48

gaaclarke force-pushed the staleviewport branch from c6aa6f8 to b05f3c5 Compare April 13, 2026 18:48

gaaclarke approved these changes Apr 13, 2026

View reviewed changes

gaaclarke requested a review from jtmcdole April 13, 2026 18:48

auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 13, 2026

gaaclarke added the CICD Run CI/CD label Apr 13, 2026

jason-simmons approved these changes Apr 13, 2026

View reviewed changes

jtmcdole approved these changes Apr 13, 2026

View reviewed changes

gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 13, 2026

auto-submit Bot added this pull request to the merge queue Apr 13, 2026

Merged via the queue into master with commit dc1c2c7 Apr 14, 2026
205 checks passed

auto-submit Bot deleted the staleviewport branch April 14, 2026 04:32

flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 14, 2026

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 14, 2026

dc1c2c700 [Impeller] Commands that don't specify their own viewports …

88a71cc

…get the viewport of the render pass. (flutter/flutter#177473)

engine-flutter-autoroll mentioned this pull request Apr 14, 2026

Roll Flutter from 2fa45e01aa5c to 252d635a01de (18 revisions) flutter/packages#11503

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 14, 2026

dc1c2c700 [Impeller] Commands that don't specify their own viewports …

3fcd2ef

…get the viewport of the render pass. (flutter/flutter#177473)

engine-flutter-autoroll mentioned this pull request Apr 14, 2026

Roll Flutter from 2fa45e01aa5c to c1b14e92dcfb (19 revisions) flutter/packages#11506

Merged

Uh oh!

Conversation

chinmaygarde commented Oct 23, 2025

Uh oh!

flutter-dashboard Bot commented Oct 23, 2025

Uh oh!

chinmaygarde commented Oct 23, 2025

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

flutter-dashboard Bot commented Nov 12, 2025

Uh oh!

Piinks commented Feb 3, 2026

Uh oh!

zanderso commented Mar 5, 2026

Uh oh!

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gaaclarke commented Mar 5, 2026

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

auto-submit Bot commented Mar 5, 2026

Uh oh!

gaaclarke commented Mar 5, 2026

Uh oh!

jtmcdole Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

jtmcdole Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

gaaclarke Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

zanderso commented Mar 5, 2026

Uh oh!

auto-submit Bot commented Apr 13, 2026

Uh oh!

gaaclarke commented Apr 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants