[Impeller] opt into exp canvas. #54913

jonahwilliams · 2024-08-31T03:26:42Z

Switch back to new canvas implementation, which allows us to complete the display list/impeller interop arc of work. While we're at it, switch the subpass size rounding logic to round out if there is no image filter.

Fixes flutter/flutter#152366

Part of flutter/flutter#142054

flutter-dashboard · 2024-08-31T03:26:45Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

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. 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.

jonahwilliams · 2024-09-03T18:42:43Z

impeller/aiks/canvas.h

@@ -37,6 +37,8 @@ struct CanvasStackEntry {
  size_t num_clips = 0u;
  Scalar distributed_opacity = 1.0f;
  Entity::RenderingMode rendering_mode = Entity::RenderingMode::kDirect;
+  // Whether all entities in the current save should be skipped.
+  bool skipping = false;


When we encounter a saveLayer that should be skipped, we need a way to ensure that no child entities of that saveLayer get rendered. This is a quickish fix to make this work correctly.

In the future, we could change the dispatcher to use indicies and have a more explicit skip step.

We could also do a normal save; clip(Empty)

jonahwilliams · 2024-09-03T18:43:33Z

impeller/aiks/experimental_canvas.cc

  auto subpass_coverage = maybe_subpass_coverage.value();

+  // When an image filter is present, clamp to avoid flicking due to nearest
+  // sampled image. For other cases, round out to ensure than any geometry is
+  // not cut off.


This fixes flutter/flutter#152366

Yes, you will be filtering all of the subpass pixels, not just its theoretical coverage.

jonahwilliams · 2024-09-03T21:24:10Z

It looks like there is one failure in our test cases for a partiulcar depth watcher:

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8737784692171517121/+/u/test:_Host_Tests_for_host_debug_unopt_arm64/stdout

[ RUN      ] Play/AiksTest.MaskBlurVariantTestSolidTranslucent/Metal
[FATAL:flutter/impeller/display_list/dl_dispatcher.cc(90)] Check failed: canvas_.GetOpDepth() <= (old_depth_ + allowed_) && canvas_.GetOpDepth() <= old_max_. 
from ../../../flutter/impeller/display_list/dl_dispatcher.cc:988
old/allowed/current/max = 1/1/3/16777216

Looks like the depth values for mask blurs might be wrong sometimeS?

jonahwilliams · 2024-09-03T21:26:40Z

Also from goldens looks like there is a problem with BDF restore.

flar · 2024-09-03T22:57:35Z

Looks like the depth values for mask blurs might be wrong sometimeS?

The DL allows 2 per primitive that has a mask blur:

engine/display_list/dl_builder.cc

Line 350 in 8d7d5ff

render_op_depth_cost_ = 2u;

Ah, but the DepthWatcher logic always "allows" 1. It needs to bump that to 2 if there is a mask blur (but only for primitives that use it?)

jonahwilliams · 2024-09-03T23:21:25Z

Ah, so its just a depth watcher issue in this case? I think we can assume all primitives with a mask filter will use the mask filter - otherwise DL wouldn't record it, right?

flar · 2024-09-04T00:11:07Z

Ah, so its just a depth watcher issue in this case? I think we can assume all primitives with a mask filter will use the mask filter - otherwise DL wouldn't record it, right?

DL doesn't set it back to null if the primitive doesn't use it. It just assumes the backend will ignore it.

flar · 2024-09-04T00:13:18Z

Ah, so its just a depth watcher issue in this case? I think we can assume all primitives with a mask filter will use the mask filter - otherwise DL wouldn't record it, right?

DL doesn't set it back to null if the primitive doesn't use it. It just assumes the backend will ignore it.

On the plus side, though, DL will add the extra depth for all primitives whether they apply the mask filter or not. So, when there is a mask filter there is padding in the measured values for all primitives.

To that end, the macros could easily do "+1 if there is a mask filter" without exceeding the reported values, it just wouldn't be as accurate.

flutter-dashboard · 2024-09-05T02:51:27Z

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.

Changes reported for pull request #54913 at sha 1c0c46c

flutter-dashboard · 2024-09-05T17:25:50Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #54913 at sha 69884f9

flutter-dashboard · 2024-09-05T21:47:40Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #54913 at sha eb2bd85

flar

AFAICT from the goldens there is nothing glaring showing up so this is good to go even though we might continue refining it (and I have some comments that might lead to that added here). At some point we have to put the stake in the ground and see what breaks past the internal testing.

The one thing I might do differently is relabel this PR as "fixes so that we can opt-in to eCanvas" and move the change in the flag to a separate PR. That way we don't keep ping ponging these fixes if we run into trouble down the road with the flag flip - keeping the flag flip as its own PR with no other fixes minimizes revert/reland churn.

~~(In particular, I'm going to have to merge with this PR when it lands so I will make a revert of this PR difficult. I don't want my OpReceiver fixes to have to ping pong with it if we need to revert.)~~

D'oh! If we need to revert the opt-in, we can just do a new PR with a change of the flag, we don't have to do a physical revert of this PR...

flar · 2024-09-04T20:55:58Z

impeller/display_list/dl_dispatcher.cc

               int allowed)
      : file_(file),
        line_(line),
        canvas_(canvas),
-        allowed_(allowed),
+        allowed_(has_mask_blur ? allowed : (allowed + 1)),


That looks OK. We might fail to trigger on some ops that don't use MF if an MF is accidentally set, but we won't get spurious failures at least. In the long run I actually think we want to consolidate the depth accounting in Impeller. We need to do a pre-pass to manage the text cache and to plan the ordering of ops, so we could just as easily compute depths as well during that phase. Then the accounting is all in (basically) one class where it can't get out of synch.

that's not a bad idea, plus if we do any sort of sorting we'll have a prepass anyway - and the text cache isn't going away anytime soon.

flar · 2024-09-06T08:38:05Z

impeller/aiks/experimental_canvas.cc

+    subpass_size = ISize(subpass_coverage.GetSize());
+  } else {
+    subpass_size = ISize(IRect::RoundOut(subpass_coverage).GetSize());
+  }


Don't the variations of how you compute the subpass_size have a ramification for the origin? The subpass_coverage was relative to the global position I think? In which case if you just took its size and made a drawable for that then the global position would be the origin of the coverage. But if you rounded it out, then the global position would be the rounded down value of the origin of the coverage?

hmm, good point. Right now we're always rounding in https://github.com/flutter/engine/blob/main/impeller/aiks/experimental_canvas.cc#L524-L526

But I think instead we need to take into account whether we rounded in or out, so that we're not a pixel off.

flar · 2024-09-06T08:39:41Z

impeller/aiks/experimental_canvas.cc

@@ -670,6 +705,10 @@ void ExperimentalCanvas::DrawTextFrame(

 void ExperimentalCanvas::AddRenderEntityToCurrentPass(Entity entity,
                                                      bool reuse_depth) {
+  if (IsSkipping()) {


Should we test this earlier? Before we make the entity in the first place?

Eventually yes, as we won't ever make entities. But that logic is spread throughout the old and new canvas, so moving it earlier isn't easy as the old canvas doesn't use this check yet.

flar · 2024-09-06T08:40:04Z

impeller/aiks/experimental_canvas.cc

@@ -763,6 +801,10 @@ void ExperimentalCanvas::AddRenderEntityToCurrentPass(Entity entity,
 }

 void ExperimentalCanvas::AddClipEntityToCurrentPass(Entity entity) {
+  if (IsSkipping()) {
+    return;


Same as above, check this sooner?

flar · 2024-09-06T08:43:31Z

testing/dart/painting_test.dart

@@ -89,6 +89,7 @@ void main() {
    final SceneBuilder sceneBuilder = SceneBuilder();

    final Picture redClippedPicture = makePicture((Canvas canvas) {
+      canvas.drawPaint(Paint()..color = const Color(0xFFFFFFFF));


If I understand this comment correctly, you are saying that this new line is a new test of clear color optimization and since the RT is allocated after the clips, this is here to make sure that we clear the entire newly allocated RT despite that we may have seen clips in the meantime?

In other words, this is not "fixing a test to behave better with the code" which would indicate that we have a break in behavior, but rather "asking a test to try something that tests some functionality that we implemented"?

flar · 2024-09-06T08:48:03Z

Just realized that if we have to revert the flag change, we can do that with a simple flag-change-PR, we don't have to revert this entire PR - so ignore my suggestion there...

…into back_to_exp_canvas

flutter-dashboard · 2024-09-06T17:15:31Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #54913 at sha 90e2eb3

jonahwilliams · 2024-09-06T20:18:10Z

impeller/aiks/experimental_canvas.cc

-        (save_layer_state.coverage.GetOrigin() - GetGlobalPassPosition())
-            .Round();
+    Point subpass_texture_position;
+    if (transform_stack_.back().did_round_out) {


@flar to double check my math here...

I think rather than a flag it would be better to have the subpass record "I represent the pixels located at [rect]" in the long run, I think that's all the information you need and it doesn't require the back-half to know how the front-half did its math.

…154764) flutter/engine@419fb8c...b9f9015 2024-09-07 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[skwasm] use temporary RawPaint objects (#54917)" (flutter/engine#55018) 2024-09-06 jonahwilliams@google.com [engine] reland: always post tasks for platform channel responses. (flutter/engine#55006) 2024-09-06 skia-flutter-autoroll@skia.org Roll Skia from 4786936b4c0c to 06cd203d0607 (2 revisions) (flutter/engine#55014) 2024-09-06 paulberry@google.com [multiple] Avoid new `unreachable_switch_default` warning. (flutter/engine#54996) 2024-09-06 yjbanov@google.com [skwasm] use temporary RawPaint objects (flutter/engine#54917) 2024-09-06 flar@google.com [DisplayList] use DlScalar, DlPoint, DlRect exclusively in DlOpReceiver methods (flutter/engine#54982) 2024-09-06 skia-flutter-autoroll@skia.org Roll Skia from f38ea0134dba to 4786936b4c0c (4 revisions) (flutter/engine#55013) 2024-09-06 jonahwilliams@google.com [Impeller] opt into exp canvas. (flutter/engine#54913) 2024-09-06 skia-flutter-autoroll@skia.org Roll Skia from 8f62a6a4a299 to f38ea0134dba (4 revisions) (flutter/engine#55008) 2024-09-06 jonahwilliams@google.com [impeller] fake image for fake tests. (flutter/engine#54974) 2024-09-06 skia-flutter-autoroll@skia.org Roll Skia from 6ad117bd2efe to 8f62a6a4a299 (1 revision) (flutter/engine#55001) 2024-09-06 59215665+davidhicks980@users.noreply.github.com Change "there own" to "their own" in Flutter-GPU docs (flutter/engine#54921) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC codefu@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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

…visions) (#154764)" (#154765) Reverts: #154764 Initiated by: jonahwilliams Reason for reverting: failing post submit on mac module test. Original PR Author: engine-flutter-autoroll Reviewed By: {fluttergithubbot} This change reverts the following previous change: flutter/engine@419fb8c...b9f9015 2024-09-07 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[skwasm] use temporary RawPaint objects (#54917)" (flutter/engine#55018) 2024-09-06 jonahwilliams@google.com [engine] reland: always post tasks for platform channel responses. (flutter/engine#55006) 2024-09-06 skia-flutter-autoroll@skia.org Roll Skia from 4786936b4c0c to 06cd203d0607 (2 revisions) (flutter/engine#55014) 2024-09-06 paulberry@google.com [multiple] Avoid new `unreachable_switch_default` warning. (flutter/engine#54996) 2024-09-06 yjbanov@google.com [skwasm] use temporary RawPaint objects (flutter/engine#54917) 2024-09-06 flar@google.com [DisplayList] use DlScalar, DlPoint, DlRect exclusively in DlOpReceiver methods (flutter/engine#54982) 2024-09-06 skia-flutter-autoroll@skia.org Roll Skia from f38ea0134dba to 4786936b4c0c (4 revisions) (flutter/engine#55013) 2024-09-06 jonahwilliams@google.com [Impeller] opt into exp canvas. (flutter/engine#54913) 2024-09-06 skia-flutter-autoroll@skia.org Roll Skia from 8f62a6a4a299 to f38ea0134dba (4 revisions) (flutter/engine#55008) 2024-09-06 jonahwilliams@google.com [impeller] fake image for fake tests. (flutter/engine#54974) 2024-09-06 skia-flutter-autoroll@skia.org Roll Skia from 6ad117bd2efe to 8f62a6a4a299 (1 revision) (flutter/engine#55001) 2024-09-06 59215665+davidhicks980@users.noreply.github.com Change "there own" to "their own" in Flutter-GPU docs (flutter/engine#54921) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC codefu@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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

…154769) flutter/engine@419fb8c...7bf1169 2024-09-07 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[engine] reland: always post tasks for platform channel responses. (#55006)" (flutter/engine#55022) 2024-09-07 magder@google.com Turn off software rendering in iOS scenario golden tests (flutter/engine#55016) 2024-09-07 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[skwasm] use temporary RawPaint objects (#54917)" (flutter/engine#55018) 2024-09-06 jonahwilliams@google.com [engine] reland: always post tasks for platform channel responses. (flutter/engine#55006) 2024-09-06 skia-flutter-autoroll@skia.org Roll Skia from 4786936b4c0c to 06cd203d0607 (2 revisions) (flutter/engine#55014) 2024-09-06 paulberry@google.com [multiple] Avoid new `unreachable_switch_default` warning. (flutter/engine#54996) 2024-09-06 yjbanov@google.com [skwasm] use temporary RawPaint objects (flutter/engine#54917) 2024-09-06 flar@google.com [DisplayList] use DlScalar, DlPoint, DlRect exclusively in DlOpReceiver methods (flutter/engine#54982) 2024-09-06 skia-flutter-autoroll@skia.org Roll Skia from f38ea0134dba to 4786936b4c0c (4 revisions) (flutter/engine#55013) 2024-09-06 jonahwilliams@google.com [Impeller] opt into exp canvas. (flutter/engine#54913) 2024-09-06 skia-flutter-autoroll@skia.org Roll Skia from 8f62a6a4a299 to f38ea0134dba (4 revisions) (flutter/engine#55008) 2024-09-06 jonahwilliams@google.com [impeller] fake image for fake tests. (flutter/engine#54974) 2024-09-06 skia-flutter-autoroll@skia.org Roll Skia from 6ad117bd2efe to 8f62a6a4a299 (1 revision) (flutter/engine#55001) 2024-09-06 59215665+davidhicks980@users.noreply.github.com Change "there own" to "their own" in Flutter-GPU docs (flutter/engine#54921) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC codefu@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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

Switch back to new canvas implementation, which allows us to complete the display list/impeller interop arc of work. While we're at it, switch the subpass size rounding logic to round out if there is no image filter. Fixes flutter/flutter#152366 Part of flutter/flutter#142054

[Impeller] opt into exp canvas.

97160d4

github-actions bot added the e: impeller label Aug 31, 2024

jonahwilliams added 11 commits August 30, 2024 22:01

++

dd06f92

++

5181dd1

[impeller] fixes for subpass skipping.

9468843

++

e2f5268

better default.

d198704

fix offset alignment.

0292df8

++

bf7a396

++

08f6361

++

c85e2b4

++

df4e5e5

adjust flush location.

b894059

jonahwilliams requested a review from flar September 3, 2024 18:41

jonahwilliams commented Sep 3, 2024

View reviewed changes

jonahwilliams added 4 commits September 3, 2024 19:49

++

f50102e

back to old clip stack.

be9be26

use dcheck

06cbda0

++

1c0c46c

flutter-dashboard bot added the will affect goldens label Sep 5, 2024

++

c168b96

Merge branch 'main' into back_to_exp_canvas

eb2bd85

flar approved these changes Sep 6, 2024

View reviewed changes

jonahwilliams added 3 commits September 6, 2024 09:35

round out differences.

4248520

round in vs round out.

3938253

Merge branch 'back_to_exp_canvas' of github.com:jonahwilliams/engine …

90e2eb3

…into back_to_exp_canvas

jonahwilliams commented Sep 6, 2024

View reviewed changes

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 6, 2024

auto-submit bot merged commit 397987a into flutter:main Sep 6, 2024
30 checks passed

jonahwilliams deleted the back_to_exp_canvas branch September 6, 2024 20:43

engine-flutter-autoroll mentioned this pull request Sep 6, 2024

Roll Flutter Engine from 419fb8c0ab3e to 0fad4601a1fb (8 revisions) flutter/flutter#154760

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 6, 2024

397987aed [Impeller] opt into exp canvas. (flutter/engine#54913)

5908122

engine-flutter-autoroll mentioned this pull request Sep 6, 2024

Roll Flutter Engine from 419fb8c0ab3e to 2f1c8b84ec99 (10 revisions) flutter/flutter#154762

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 6, 2024

397987aed [Impeller] opt into exp canvas. (flutter/engine#54913)

01b41c4

engine-flutter-autoroll mentioned this pull request Sep 7, 2024

Roll Flutter Engine from 419fb8c0ab3e to b9f9015ff986 (12 revisions) flutter/flutter#154764

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 7, 2024

397987aed [Impeller] opt into exp canvas. (flutter/engine#54913)

29d1f13

auto-submit bot mentioned this pull request Sep 7, 2024

Reverts "Roll Flutter Engine from 419fb8c0ab3e to b9f9015ff986 (12 revisions) (#154764)" flutter/flutter#154765

Merged

engine-flutter-autoroll mentioned this pull request Sep 7, 2024

Roll Flutter Engine from 419fb8c0ab3e to b9f9015ff986 (12 revisions) flutter/flutter#154766

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 7, 2024

397987aed [Impeller] opt into exp canvas. (flutter/engine#54913)

037ba8d

engine-flutter-autoroll mentioned this pull request Sep 7, 2024

Roll Flutter Engine from 419fb8c0ab3e to 5b9d74fa67dd (13 revisions) flutter/flutter#154768

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 7, 2024

397987aed [Impeller] opt into exp canvas. (flutter/engine#54913)

3839d1b

engine-flutter-autoroll mentioned this pull request Sep 7, 2024

Roll Flutter Engine from 419fb8c0ab3e to 7bf1169715d6 (14 revisions) flutter/flutter#154769

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 7, 2024

397987aed [Impeller] opt into exp canvas. (flutter/engine#54913)

5b10032

[Impeller] opt into exp canvas. #54913

[Impeller] opt into exp canvas. #54913

Uh oh!

Conversation

jonahwilliams commented Aug 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Aug 31, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Sep 3, 2024

Uh oh!

jonahwilliams commented Sep 3, 2024

Uh oh!

flar commented Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams commented Sep 3, 2024

Uh oh!

flar commented Sep 4, 2024

Uh oh!

flar commented Sep 4, 2024

Uh oh!

flutter-dashboard bot commented Sep 5, 2024

Uh oh!

flutter-dashboard bot commented Sep 5, 2024

Uh oh!

flutter-dashboard bot commented Sep 5, 2024

Uh oh!

flar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flar commented Sep 6, 2024

Uh oh!

flutter-dashboard bot commented Sep 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonahwilliams commented Aug 31, 2024 •

edited

Loading

flar commented Sep 3, 2024 •

edited

Loading

flar left a comment •

edited

Loading