Displaylist ColorFilter objects#31491
Conversation
|
I'll need to wait for the associated changes in flutter/impeller#12 to be merged before I can update the DEPS file so that these changes will compile. |
flar
force-pushed
the
displaylist-colorfilter-objects
branch
from
789ee11 to
9969ac4
Compare
| void onSetShader(sk_sp<SkShader> shader); | ||
| void onSetImageFilter(sk_sp<SkImageFilter> filter); | ||
| void onSetColorFilter(sk_sp<SkColorFilter> filter); | ||
| void onSetColorFilter(const DlColorFilter* filter); |
There was a problem hiding this comment.
Is there any reason to not use a shared_ptr here? Even, for example, a shared_ptr?
There was a problem hiding this comment.
If we do that, then when the dispatcher wants to dispatch the op from the record in the DL, it has to reinstantiate the object.
Alternately, I suppose I could back off to storing the shared_ptr in the buffer, but then equals and destructor have to visit each node and manually compare or manually destruct. One of my goals was to embed the info directly in the DL buffer so that we could mass compare and mass free the buffers.
| } | ||
|
|
||
| std::shared_ptr<DlColorFilter> shared() const override { | ||
| return std::make_shared<DlBlendColorFilter>(this); |
There was a problem hiding this comment.
Why isn't this unsafe? Each call to this will create new shared pointers that don't know about each other. One of them could end up dangling.
There was a problem hiding this comment.
make_shared(...) calls the constructor on T that matches the args. It returns a shared_ptr to a brand new instance of the DlColorFilter (DlBlendColorFilter in that particular case).
shared()->get() != this
*shared()->get() == *this
Perhaps I should add tests for that first condition to the unittests?
There was a problem hiding this comment.
My concept was that the DlFoo objects would be lightweight, immutable, and copyable without loosing any functionality. This function takes advantage of those design constraints.
There was a problem hiding this comment.
The only unsafe (perhaps surprising) aspect of this method is that the address in the shared_ptr will not be the address of the original. That might be unexpected, but I don't think this is unsafe wrt memory management and since the objects are immutable, it shouldn't be unsafe with respect to sharing modifications.
There was a problem hiding this comment.
I added those unit tests to verify that the shared ptr returned is distinct from the original.
|
I added a bunch of comments to the d_l_color_filter header with a design philosophy comment at the top and then method comments on the methods in the base class. |
flar
added
the
waiting for tree to go green
5 participants
Fixes flutter/flutter#98715
This PR creates an inspectable object for DisplayLists to store ColorFilters. These objects can also be stored inline in the DisplayList buffer of records without requiring a dangling pointer that needs to be searched and freed when a DL is disposed.
The class can implement any of the ui.ColorFilter objects used in Dart ui code, and can recapture any of those ColorFilter types from an SkColorFilter as well for when we need to recapture rendering ops from code that only speaks to an SkCanvas (for the time being just the Paragraph code).
This PR can be compared and contrasted to the previous PR which accomplished this objective by creating a number of different DL records and Dispatcher.setFooColorFilter(args) methods. It feels a bit simpler to have these DlColorFilter objects instead of the plethora of dispatch methods. This PR also provides a ready storage format for keeping a shared DlColorFilter* reference where needed.