Create a mechanism to manage layer state #36458

flar · 2022-09-27T23:31:06Z

A new mechanism is added to manage all state between layers, including transforms, clips, and saveLayer attributes like opacity, ColorFilter and ImageFilter.

The advantages of this mechanism include:

Can eventually be merged with MutatorsStack for a single mechanism for both Preroll and Paint (future work TBD)
We no longer need to use an SkNWayCanvas or DisplayListMultiplexer to broadcast all state changes to the multiple recorders used for the embedder layers - one mechanism records all state changes and updates the "current embedder layer canvas" when the time comes to switch these recorders. Only one recorder (the one currently being rendered into) will receive state changes at a time and when we switch, the appropriate contextual state will be swapped into the new recorder.
Now that all state is collected into a common mechanism, it is easier to implement peephole-type implementations without having to create an obscure inter-layer communication system - the state mechanism itself will be the communication channel for all of that information as well as the focal point for future optimizations along those lines.

TBD:

The layer caching mechanisms may have broken here, we should instead rely on the ContainerLayer::PaintChildren method to implement that, but the work to switch that over hasn't been done yet. Some of the current layer caching might work, but I may have broken it - further testing and review will be needed to verify that. Testing seems to show that the layer caching is working fine, but I feel like I just edited it until it compiled OK rather than specifically designing how it would work with the new mechanism so a good set of eyes looking it over would be recommended.
More testing of attribute interaction needed in the unittests. For example, adding an opacity after an image filter vs before it, and other combinations. Verify that they combine - or don't combine - as we expect. I believe I added a decent number of tests of these conditions.
~~We should duplicate the layer_state_stack unittests as layer tests that exercise the same operations to ensure that the integration into the layers behaves as the mechanism intends.~~ Existing layer tests would confirm that the state is still applied as needed, though I didn't specifically write flutter::Layer based state-interaction tests.

skia-gold · 2022-09-27T23:47:25Z

Gold has detected about 4 new digest(s) on patchset 2.
View them at https://flutter-engine-gold.skia.org/cl/github/36458

display_list/display_list_color_filter.h

jonahwilliams

I've only taken a quick look through this so far, some thoughts:

Really like how it cleans up the Layer::Paint implementations and generalizes the opacity special cases.

I'm not sure I'm getting the distinction between apply/reapply/reapply_all?

As someone reading the code for the first time, I would appreciate more breadcrumbs between mutator.applyFoo() and where the canvas or display list builder receives a saveLayer. (Granted, its no longer 1-1 with this change)

jonahwilliams · 2022-09-29T04:36:28Z

flow/layers/opacity_layer.cc

  ContainerLayer::Preroll(context, child_matrix);
  // We store the inheritance ability of our children for |Paint|
-  set_children_can_accept_opacity(context->subtree_can_inherit_opacity);
+  set_children_can_accept_opacity((context->renderable_state_flags &


Do we use this for anything besides calling ShouldNotBeCached below?

A bunch of unit tests use it to make sure that the right decision was reached.

jonahwilliams · 2022-09-29T04:46:40Z

flow/layers/layer.h

+  // The state attribute flags that represent which attributes a
+  // layer can render if it will be rendering its content/children
+  // from a cached representation.
+  static constexpr int RASTER_CACHE_RENDER_FLAGS =


Could we also fold color filter or image filter into drawing from cache?

Layer caching was somewhat disrupted by this new mechanism. I'll have to resurrect it first.

Layer caching seems to be working OK, even though I thought I had left some wires hanging out of it...

My goal here is to achieve mostly parity with existing layer caching and leave enhancing the caching to new combinations for a future PR...

flar · 2022-09-29T22:53:20Z

The unit test failure is caused by the following problem:

LayerTree::Flatten only sets up the Builder as the target of the state_stack.
all state like clips and transforms then go directly to the builder, bypassing the recorder which is associated with it.
PaintChildren asks layers if they need to be painted. These layers rely on canvas->quickReject() to determine that.
Since the recorder (which is the SkCanvas in this case) was not involved in the clip/transform state recordings, it makes bad decisions in quickReject.

The same problems should also happen with the Impeller track since it records layers into a builder.

I'll need to move the quickReject checking into the LayerStateStack so that it is based on the common state rather than the state of the (possibly out-of-the-loop) canvas.

(a7b615b fixes this issue)

flar · 2022-09-30T00:14:31Z

I see a bug in the way I update the builder/canvas in platform_view_layer.cc. I'm going to try to write a test case that notices as I don't see any existing test cases that catch it... (c0f2262 fixes this issue)

flar · 2022-10-03T20:49:48Z

Things that I think remain here:

Layer caching, driven from the new state mechanism probably implemented in ContainerLayer::PaintChildren()
Echoing (duplicating?) a number of the LayerStateStack unit tests using the associated Layers to drive the cases

skia-gold · 2022-10-13T01:20:15Z

Gold has detected about 1 new digest(s) on patchset 10.
View them at https://flutter-engine-gold.skia.org/cl/github/36458

flar · 2022-10-14T23:39:35Z

This PR seems to be pretty much there, I'm mainly testing at this point to see what is broken that the tests don't show...

flar · 2022-10-17T18:07:51Z

@JsouLiang what do you think of how this is integrating into the existing layer and DL caching mechanisms. I tried to leave those mostly untouched except for what was needed to integrate with the new locations/holders for the state they need.

JsouLiang · 2022-10-19T02:37:52Z

@flar Most of the content is consistent with our previous discussion flow, and I think it is very clear. 👍 I need to take a closer look

dnfield

I've read through most of this and mostly just have nits. I'd like a little clarification around the flags question - I think there's probably a slightly cleaner way to expose what the caller is allowed to do without unintentionally resetting the flags, but that's more of a nitpick.

I'd really like to see a TAP run using this patch if possible. I'm poking at FRoB to see if I can get it to do what I want.

dnfield · 2022-10-25T21:10:26Z

display_list/display_list_builder.cc

+  SkMatrix matrix = getTransform();
+  // We don't need the inverse, but this method tells us if the matrix
+  // is singular in which case we can reject all rendering.
+  if (!matrix.invert(nullptr)) {


This will exclude Z perspective right? That's probably fine for this patch, but should we have an issue to follow up on this?

That's complicated. Since the bounds have no Z then you don't need the Z column. Since we drop the Z while rendering, and we're looking for a 2D bounds, you don't need the Z row either. So, it doesn't drop anything that affects this particular operation.

It does not drop perspective, though. The 3x3 matrix has perspective components. It just drops Z. The reason that down-converting a 4x4 to a 3x3 is bad is because you can no longer compose it accurately with other 4x4 matrices. But, for a given 2D rendering operation (and bounds operation) I don't think you need those elements of the matrix.

I should have said "for a leaf 2D operation". All you lose with the 3x3 is composing with future 4x4 matrices and ability to take or generate Z components. None of our geometry has Z and we don't output Z so the only thing that matters is the composability.

That composability is critical for TransformLayer because it could be nested with another TransformLayer, but not in single layer operations.

dnfield · 2022-10-25T21:10:38Z

display_list/display_list_canvas_unittests.cc

@@ -2040,6 +2040,7 @@ class CanvasCompareTester {
      const uint32_t* test_row = test_pixels->addr32(0, y);
      for (int x = 0; x < test_pixels->width(); x++) {
        if (ref_row[x] != test_row[x]) {
+          // FML_LOG(ERROR) << std::hex << ref_row[x] << " != " << test_row[x];


commented out code

Suggested change

// FML_LOG(ERROR) << std::hex << ref_row[x] << " != " << test_row[x];

True, it is one that I keep adding back in during testing, though. But, it's also easy to recreate when I need it. Maybe I should have it in contingent on "should_match".

dnfield · 2022-10-25T21:15:31Z

flow/layers/layer.h

+  // The state attribute flags that represent which attributes a
+  // layer can render if it plans to use a saveLayer call in its
+  // |Paint| method.
+  static constexpr int SAVE_LAYER_RENDER_FLAGS =


nit: here and elsewhere, constants should be kConstantName, e.g. kSaveLayerRenderFlags, kCallerCanApplyOpacity, etc.

https://google.github.io/styleguide/cppguide.html#Constant_Names

Done (for both these 2 constants and the ones in LayerStateStack)

dnfield · 2022-10-25T21:23:56Z

flow/layers/layer_state_stack.cc

+}
+
+void LayerStateStack::set_initial_cull_rect(const SkRect& cull_rect) {
+  FML_CHECK(is_empty()) << "set_initial_cull_rect() must be called before any "


DCHECK is probably more appropriate for these.

A mistake here will cause an application crash that will be harder for developers to work through and report. We could consider logging a warning in this case if it's actionable by the developer, but this seems like it'd be a mistake in the engine if we allow this and that the developer would just have to file a bug to let us know. DCHECKs will still fire in unit tests and make them fail.

Done - DCHECK

dnfield · 2022-10-25T21:26:18Z

flow/layers/layer_state_stack.cc

+  layer_state_stack_->restore_to_count(stack_restore_count_);
+}
+
+AutoRestore LayerStateStack::applyState(const SkRect& bounds,


Since this is all new code and it's not sitting next to other code that has a mix of cases, let's just try to follow the style guide. Here and elsewhere: the methods in classes in this file should use a consistent naming scheme with the style guide. That means snake_case for simple accessors/mutators, otherwise PascalCase

https://google.github.io/styleguide/cppguide.html#Function_Names

Inside the mutator we have "saveLayer" with the capitalization from everywhere else that method appears. Then we have its variant methods like "applyOpacity" which are meant to mirror it. This capitalization scheme then starts leaking into this code. I'm trying to figure out where the line is.

About all I've done so far is get_draw_checkerboard -> draw_checkerboard since it is an accessor, but unfortunately that makes it sound like an operation rather than a getter... Sigh. Maybe I'll rename it to "checkerboard_renderer".

applyState could probably be renamed to resolve or protect as it conditionally applies a protective saveLayer only if there are outstanding incompatible attributes...?

In Impeller it's Canvas::SaveLayer. We only have saveLayer from either Skia or legacy Blink code.

dnfield · 2022-10-25T21:27:55Z

flow/layers/layer_state_stack.cc

+    // We could do a 4-point long-form conversion, but since this is
+    // only used for culling, let's just return a non-constricting
+    // cull rect.


FWIW, the Web engine does this or something like it regardless of perspective. It seems to work fine. That said, it'd be fine to do this in a separate patch to avoid new variance.

dnfield · 2022-10-25T21:56:02Z

flow/layers/layer_tree.cc

+      .state_stack                   = state_stack,
+      .canvas                        = frame.canvas(),
+      .builder                       = builder,


This is so much better.

dnfield · 2022-10-25T21:57:31Z

flow/layers/layer_tree.cc

+      .state_stack                   = state_stack,
+      .canvas                        = frame.canvas(),
+      .builder                       = builder,


dnfield · 2022-10-25T22:01:36Z

flow/paint_utils.cc

+  if (canvas) {
+    DrawCheckerboard(canvas, rect);
+  }
+  if (builder) {


That may be true, and true of any number of methods in this file, but very soon one of these variations will be eliminated entirely so I'd like to leave the lines separated. Originally I allowed both until I realized that we should only be updating a single delegate at a time.

Perhaps in another pass I could switch these to use a DelegateHandler with 3 variants for SkCanvas, DLBuilder, and MutatorsStack...

dnfield · 2022-10-25T22:07:08Z

flow/layers/layer_state_stack.h

+  static constexpr int CALLER_CAN_APPLY_OPACITY = 0x1;
+  static constexpr int CALLER_CAN_APPLY_COLOR_FILTER = 0x2;
+  static constexpr int CALLER_CAN_APPLY_IMAGE_FILTER = 0x4;
+  static constexpr int CALLER_CAN_APPLY_ANYTHING = 0x7;


Should we make this an enum for some more type safety where these get used?

Should we bother having these public instead of just having method(s) on the class that let you set/clear them? For example, it looks like TextureLayer sets the OPACITY flag and clears others, whereas other layers use |= to add flags. It's not entirely clear to me whether that's intentional or not, whereas methods would be a bit clearer.

They are bit fields. Enum would not be appropriate since they are combined (see the ANYTHING value). I could probably make that more obvious by having the ANYTHING be a bit-OR of the 3 values if constexpr allows me to do that.

Changed these to kConstant naming and the Anything version is now a bit-OR.

This part of the mechanism needs more work that goes beyond just translating existing support into a new mechanism. I would like to remove the state flags from the PrerollContext and manage the congregate values inside LSS itself, but I don't want to make that part of this initial design shift.

Some of the cases involve flags = kCallerCan... because in those cases the code knows that those flags are exactly what can be handled in the Paint method. Some involve |= kCallerCan... because those pieces of code are suggesting that someone else may have made an initial determination of what can be handled and the code in question is trying to say "whatever that other code determined, I now know enough additional information to state that the following attribute can also be handled". If I come up with an interface that manages these flags via method calls, then those method calls would have to be rich enough to describe all of these situations.

For now, I'm hoping to just barely capture existing functionality in a simple form and create a platform from which we can get more sophisticated through additional work.

Sounds good.

…t tests

flar · 2022-10-26T00:00:20Z

@dnfield I think I addressed all or most of your concerns in my last commit. Let me know if I missed something.

Also, I was at a loss to apply the naming conventions to most of the methods in LSS. They talk about using lower_case for getters and mutators which covers a lot of ground. They give an example of using it for a setter in later recommendations, but aren't clear what a mutator is. I also used it for state methods which can appear to be getters and I think it is better for methods like content_culled which are queries. Most of the methods are mixedCase because they model what we've used in the graphics classes in other cases. I'm torn between familiarity with what we've done and vagueness in the style guide. Can you provide specific suggestions?

dnfield · 2022-10-26T16:58:47Z

I think we should in general avoid new code that introduces camelCase method names, and just use PascalCase or snake_case as appropriate. This is a nit and easily cleaned up later though. Unfortunately the code base is not consistent on this.

dnfield

LGTM

flar · 2022-10-26T18:31:24Z

I filed flutter/flutter#114089 to update not only the APIs here, but throughout DL. Soon, hopefully, we will have very Skia API usage within the engine sources and so we will have little legacy naming conventions to confuse things.

)

This reverts commit 705939b.

* Revert "Fix a clang-tidy warning in display_list_canvas_unittests.cc (#37062)" This reverts commit 871de2f. * Revert "Create a mechanism to manage layer state (#36458)" This reverts commit 705939b.

flar requested review from JsouLiang, jonahwilliams and dnfield September 27, 2022 23:31

flar mentioned this pull request Sep 27, 2022

Layer state stack #35685

Closed

ColdPaleLight reviewed Sep 28, 2022

View reviewed changes

display_list/display_list_color_filter.h Outdated Show resolved Hide resolved

jonahwilliams reviewed Sep 29, 2022

View reviewed changes

ColdPaleLight mentioned this pull request Sep 30, 2022

[Impeller] Defer applying opacity when saving layer #36279

Merged

8 tasks

chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label Oct 6, 2022

ColdPaleLight mentioned this pull request Oct 12, 2022

Make 'Transform' widget work as expected when 'transform' param is a 2D translation flutter/flutter#111856

Closed

8 tasks

flar force-pushed the layer-state-stack-2 branch from 83b81d5 to 373c30a Compare October 13, 2022 01:00

flar removed the Work in progress (WIP) Not ready (yet) for review! label Oct 17, 2022

flar requested a review from cyanglaz October 18, 2022 19:35

flar mentioned this pull request Oct 25, 2022

Transparency/Opacity destroys depth/z-translation flutter/flutter#113346

Closed

flar force-pushed the layer-state-stack-2 branch from d112837 to a2e56eb Compare October 25, 2022 20:22

dnfield reviewed Oct 25, 2022

View reviewed changes

flar added 7 commits October 25, 2022 15:46

recreate previous prototype on more recent source commit

dfc22b4

update recently added unit tests for new behavior

ab3af3d

fix translation rejection bug

6c92f50

lint

2168b5c

fix issue with layer bounds rejection when rendering a tree to a DL

c45f326

add tests for state tracking after a platform layer and fix a bug

20ed976

rename some methods, consolidate state, add attribute interaction uni…

0255507

…t tests

flar added 9 commits October 25, 2022 15:46

tests to verify saveLayer attribute commutability assumptions

cf27cad

shift Preroll from MutatorsStack to LayerStateStack

09db174

move cull_rect and preroll transform into layer state stack

001228b

fix initial matrix/clip when reapplying for new canvas/builder

24413fb

don't use a reference to store what could be a temp local value

7d5254f

add canvas to delegate when rendering layers for layer cache

862dab6

render ImageFilterLayer children with transformed filter

9094ab0

ignore set_delegate calls that set the same delegate

c648e4a

add design/usage comment to LayerStateStack

894bf2f

flar force-pushed the layer-state-stack-2 branch from a2e56eb to 894bf2f Compare October 25, 2022 23:06

review feedback, mostly naming

e658ff8

dnfield approved these changes Oct 26, 2022

View reviewed changes

flar mentioned this pull request Oct 26, 2022

Modify DisplayList and LayerStateStack naming conventions to match Flutter style guidelines flutter/flutter#114089

Open

add back automatic saveLayer tracing calls

7632996

flar added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 26, 2022

auto-submit bot merged commit 705939b into flutter:main Oct 26, 2022

engine-flutter-autoroll mentioned this pull request Oct 27, 2022

Roll Flutter Engine from a772d20c7550 to 871de2f904a4 (6 revisions) flutter/flutter#114119

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 27, 2022

705939bc1 Create a mechanism to manage layer state (flutter/engine#36458

1a66e26

)

flar added a commit to flar/engine that referenced this pull request Oct 27, 2022

Revert "Create a mechanism to manage layer state (flutter#36458)"

891f0c0

This reverts commit 705939b.

flar mentioned this pull request Oct 27, 2022

Revert layer state stack #37090

Merged

flar mentioned this pull request Oct 29, 2022

Reland layer state stack #37135

Merged

b3nni97 mentioned this pull request Oct 31, 2022

Rendering glitches (Blur and BorderRadius suddenly not working properly) flutter/flutter#114359

Closed

flar mentioned this pull request Nov 7, 2022

iOS platform views render the entire scene multiple times per frame flutter/flutter#114712

Closed

Create a mechanism to manage layer state #36458

Create a mechanism to manage layer state #36458

Conversation

flar commented Sep 27, 2022 • edited Loading

skia-gold commented Sep 27, 2022

jonahwilliams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flar commented Sep 29, 2022 • edited Loading

flar commented Sep 30, 2022 • edited Loading

flar commented Oct 3, 2022

skia-gold commented Oct 13, 2022

flar commented Oct 14, 2022

flar commented Oct 17, 2022

JsouLiang commented Oct 19, 2022 • edited Loading

dnfield left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flar Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flar commented Oct 26, 2022

dnfield commented Oct 26, 2022

dnfield left a comment

Choose a reason for hiding this comment

flar commented Oct 26, 2022

flar commented Sep 27, 2022 •

edited

Loading

flar commented Sep 29, 2022 •

edited

Loading

flar commented Sep 30, 2022 •

edited

Loading

JsouLiang commented Oct 19, 2022 •

edited

Loading

flar Oct 25, 2022 •

edited

Loading