Convert a dart:ui Paint to a DisplayList DlPaint#36629
Convert a dart:ui Paint to a DisplayList DlPaint#36629auto-submit[bot] merged 1 commit intoflutter:mainfrom
Conversation
flar
left a comment
flar
left a comment
There was a problem hiding this comment.
Some comments about avoiding attribute->get()->shared() duplicate copies, but otherwise looks good...
lib/ui/painting/paint.cc
Outdated
| if (!Dart_IsNull(color_filter)) { | ||
| ColorFilter* decoded = | ||
| tonic::DartConverter<ColorFilter*>::FromDart(color_filter); | ||
| paint.setColorFilter(decoded->dl_filter()); |
There was a problem hiding this comment.
The version of this in the SkPaint method will assume that filter() returns a valid shared ptr (it calls ->skia_object() without any null check). The dl_filter() method will protect against nullptr which seems to be unnecessary based on how the SkPaint version is written, but it will also de-share the pointer - returning DlCF* - which then needs to be re-shared in the setColorFilter method. I think overall that "paint.setColorFilter(decoded->filter())" would be faster as it can transfer the shared ptr without de/re-sharing it and it avoids what seems like an unnecessary null check.
lib/ui/painting/paint.cc
Outdated
| if (Shader* decoded = tonic::DartConverter<Shader*>::FromDart(shader)) { | ||
| auto sampling = | ||
| ImageFilter::SamplingFromIndex(uint_data[kFilterQualityIndex]); | ||
| paint.setColorSource(decoded->shader(sampling).get()); |
There was a problem hiding this comment.
Remove .get() - it un-shares the shared_ptr which requires the method to recreate a shared version. Better to transfer the shared version directly.
lib/ui/painting/paint.cc
Outdated
| if (!Dart_IsNull(image_filter)) { | ||
| ImageFilter* decoded = | ||
| tonic::DartConverter<ImageFilter*>::FromDart(image_filter); | ||
| paint.setImageFilter(decoded->dl_filter()); |
There was a problem hiding this comment.
Similar comment to the CF code - probably better to use decoded->filter()
lib/ui/painting/paint.cc
Outdated
| double sigma = float_data[kMaskFilterSigmaIndex]; | ||
| DlBlurMaskFilter dl_filter(blur_style, sigma); | ||
| if (dl_filter.skia_object()) { | ||
| paint.setMaskFilter(&dl_filter); |
There was a problem hiding this comment.
This creates a copy. You could construct a shared_ptr using make_shared instead and then just allow that shared ref to be adopted into the paint.
| return true; | ||
| } | ||
|
|
||
| void Paint::toDlPaint(DlPaint& paint) const { |
There was a problem hiding this comment.
Just noting a difference here...
The SkPaint version returns a pointer that can be null if there is an issue. That feature doesn't really help in the current usage in text_style, it was mainly useful in the way that Canvas used to use it - and since we deleted the Sk code from the Canvas object, we're left with "might have a null value if the Dart object was null" that doesn't seem to apply to the use in text_style.
So, this call signature makes sense. I just wanted to double check that I went through all of the proper decision-vetting on that difference.
jason-simmons
force-pushed
the
dl_convert_to_dlpaint
branch
3 times, most recently
from
e43f531 to
d7faf6e
Compare
|
Done - PTAL |
flar
left a comment
flar
left a comment
There was a problem hiding this comment.
Looks fine, modulo suggestion on using == for attribute comparisons.
lib/ui/painting/paint_unittests.cc
Outdated
|
|
||
| ASSERT_EQ(dl_paint.getBlendMode(), DlBlendMode::kModulate); | ||
| ASSERT_EQ(static_cast<uint32_t>(dl_paint.getColor()), 0x11223344u); | ||
| std::shared_ptr<const DlColorFilter> color_filter = dl_paint.getColorFilter(); |
There was a problem hiding this comment.
Note that DlColorFilter and DlMaskFilter support ==. There are also Equals() and NotEquals() templates in dl_comparable.h that make sure the objects are compared regardless of whether they are pointers, shared_ptrs, const or non-const, or you could just do == on the dereferenced pointers.
There was a problem hiding this comment.
Updated the test to compare the filter objects
jason-simmons
force-pushed
the
dl_convert_to_dlpaint
branch
from
d7faf6e to
44a8d2e
Compare
lib/ui/painting/paint_unittests.cc
Outdated
|
|
||
| ASSERT_EQ(dl_paint.getBlendMode(), DlBlendMode::kModulate); | ||
| ASSERT_EQ(static_cast<uint32_t>(dl_paint.getColor()), 0x11223344u); | ||
| ASSERT_EQ(*dl_paint.getColorFilter()->asBlend(), |
There was a problem hiding this comment.
You don't need asBlend() and asBlur() here. The base objects support a type-safe ==.
jason-simmons
force-pushed
the
dl_convert_to_dlpaint
branch
from
44a8d2e to
d033b70
Compare
jason-simmons
added
the
autosubmit
2 participants
No description provided.