Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

DisplayList tracks maximum render op depths #52070

Merged
merged 6 commits into from
Apr 17, 2024

Conversation

flar
Copy link
Contributor

@flar flar commented Apr 11, 2024

The DisplayList will now track the "render depth" of all rendering ops and provide the total content depth for DisplayList and save[Layer]/restore pairs to enable depth clipping in the back ends.

The depth accounting process is described in the class comment for DlOpReceiver

@flar flar requested a review from bdero April 11, 2024 21:49
@flar
Copy link
Contributor Author

flar commented Apr 12, 2024

I've updated the unit tests to double-check the max_depth values on the DisplayList in the existing unit tests. While I was in there I did some cleanup. There used to be 4 values on each "test snippet", as follows:

  • display list op count
  • display list byte count
  • dl->skia picture->dl recapture op count
  • dl->skia picture->dl recapture byte count

We haven't done "skia picture recapture" testing for a long time since we eliminated the need to convert Skia calls into DisplayList calls so the last 2 values have been obsolete for a while. I needed to add "depth" counts to all of the snippets, though, so while I was making that sweeping change I deleted the "sk recapture" numbers. The 3 numbers at the start of each snippet are now:

  • display list op count
  • display list byte count
  • display list depth

While doing that, I also deleted all comments related to fudging the numbers to satisfy the Skia recapture logic in the tests.

@flar flar marked this pull request as ready for review April 12, 2024 22:14
@bdero
Copy link
Member

bdero commented Apr 16, 2024

As an aside: I'm not sure there would be any utility in hooking this feature up to the current version of the DL dispatcher. We should hook this up to the new DL dispatcher as it's being formed, because

  1. Aiks/EntityPass will need to be almost entirely rewritten anyhow,
  2. we need to keep Aiks working without the DL until we're able to migrate them to use the DL.

builder.Translate(5, 5);
builder.DrawRect({10, 10, 20, 20}, DlPaint()); // depth 2

builder.SaveLayer(nullptr, nullptr); // lasts through depth 4
Copy link
Member

@bdero bdero Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this is something I should have specified. SaveLayers have a draw call that also needs a unique depth. But they are drawn after all of their children have been rendered, so the depth we use for them must be greater than all contained draws.

I recommend replicating this test against the DL depth values to ensure that:

  1. SaveLayers are reserved a depth value, and
  2. clips interact correctly with SaveLayers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that while I was updating dl_dispatcher.cc...

The odd thing is that they only allocate the depth on the restore after recording the max_depth, right? The clips inside them will be rendered to the "restore copy - 1" max depth and then the restore copy happens above that depth so it is only clipped by the enclosing clips.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I've accounted for this in the latest commit.

@flar
Copy link
Contributor Author

flar commented Apr 16, 2024

As an aside: I'm not sure there would be any utility in hooking this feature up to the current version of the DL dispatcher. We should hook this up to the new DL dispatcher as it's being formed, because

  1. Aiks/EntityPass will need to be almost entirely rewritten anyhow,
  2. we need to keep Aiks working without the DL until we're able to migrate them to use the DL.

I prototyped it, but now I'm working through adjusting the new max_depth testing in the DL unit tests to account for saveLayer depths.

And, in the end, the prototype was only useful for checking to see how it could be used. Since the impeller::Canvas class is used internally for a lot of unit tests which don't supply the new depth information, the new code isn't directly useful without a lot of modifications to the unit tests. It is possible we could do something in the future such that if the depth information was provided we keep it, but if not then we compute it, but it isn't important for this first step.

@flar flar requested review from bdero and jonahwilliams April 16, 2024 23:13
@flar flar force-pushed the dl-render-op-depth-tracking branch from eaf2736 to 03de2d3 Compare April 17, 2024 18:07
Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

builder.Restore();

builder.DrawRect({16, 16, 26, 26}, DlPaint()); // depth 10
builder.DrawRect({18, 18, 28, 28}, DlPaint()); // depth 11
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test that checks the depth of clip ops added to both the root and to the root and within a SaveLayer? That way we know they close with the correct depth (max depth of everything they affect).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the "depth of clip ops"? They aren't accounted in DL. That's an Impeller thing...?

All DL does is count how many render calls are within a save/restore or an entire DL. Clips are invisible to that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in the class comment in dl_op_receiver, the only places depth information is reported is the DL total_depth and the argument passed to save/saveLayer.

@flar flar added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 17, 2024
@auto-submit auto-submit bot merged commit cc3d18c into flutter:main Apr 17, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 18, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Apr 18, 2024
…146962)

flutter/engine@07f7532...20638b7

2024-04-17 [email protected] Roll Skia from a3a016537a8c to d221c1591d59 (2 revisions) (flutter/engine#52210)
2024-04-17 [email protected] DisplayList tracks maximum render op depths (flutter/engine#52070)

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 [email protected],[email protected],[email protected] 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
gilnobrega pushed a commit to gilnobrega/flutter that referenced this pull request Apr 22, 2024
…lutter#146962)

flutter/engine@07f7532...20638b7

2024-04-17 [email protected] Roll Skia from a3a016537a8c to d221c1591d59 (2 revisions) (flutter/engine#52210)
2024-04-17 [email protected] DisplayList tracks maximum render op depths (flutter/engine#52070)

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 [email protected],[email protected],[email protected] 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants