engine
engine copied to clipboard
Create a mechanism to manage layer state
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.
- 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.
- 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.
Gold has detected about 4 new digest(s) on patchset 2. View them at https://flutter-engine-gold.skia.org/cl/github/36458
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.
(https://github.com/flutter/engine/pull/36458/commits/a7b615b275a17c41eefbaddeb30a9425eae63eb8 fixes this issue)
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... (https://github.com/flutter/engine/pull/36458/commits/c0f2262e39d9f8cf49b8532f2db774a362e2dc92 fixes this issue)
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
Gold has detected about 1 new digest(s) on patchset 10. View them at https://flutter-engine-gold.skia.org/cl/github/36458
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...
@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.
@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 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?
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.
I filed https://github.com/flutter/flutter/issues/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.