Make the filter_ in mutator shared_ptr #34944

cyanglaz · 2022-07-27T20:04:58Z

Storing raw filter_ pointers of DlImageFilter in union caused some issue where all the pointers are pointing to the same address in the stack.

This PR moves the filter_ object out of union so we can simply making the filter_ a shared_ptr.
This is to unblock flutter/flutter#43902
The real fix for Mutators should be: flutter/flutter#108470

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flar

Just a couple of nits about unnecessary calls to shared() in places that don't need them. You should only have to call "shared()" when you initially accept the DlImageFilter& reference into the system and after that just keep sharing/transferring/using the same shared_ptr.

flar · 2022-07-27T20:15:07Z

flow/embedded_views.h

@@ -56,7 +56,7 @@ class Mutator {
        alpha_ = other.alpha_;
        break;
      case kBackdropFilter:
-        filter_ = other.filter_;
+        filter_ = other.filter_->shared();


For the copy constructor you can just copy the shared_ptr without having to recreate it with the shared() method.

What do you mean by copy the shared_ptr?

filter_ = other.filter_

Why do you call shared() there? You already have a shared_ptr.

Does filter_ = other.filter_ deep copy the raw pointer inside the shared_ptr? The idea of this copy constructor is that the filters are different objects, changing the value of the original filter will not affect the value of the copied filter.

No deep copy. It points to a new filter. It no longer points to the old filter.

flar · 2022-07-27T20:16:49Z

flow/embedded_views.h

@@ -119,15 +119,18 @@ class Mutator {
 private:
  MutatorType type_;

+  // TODO(cyanglaz): Remove union.
+  //  https://github.com/flutter/flutter/issues/108470


Is the SkPath object guaranteed to survive the lifecycle of the mutator? It might be safer to copy the SkPath using a copy constructor since the SkPath copy constructor is documented to be very fast and copy-safe.

https://api.skia.org/classSkPath.html#aa30a9363d49c092958d48e30608df367

The SkPath object is 16 bytes I think so the object takes up similar room to SkRect and much less than SkRRect or SkMatrix.

Is the SkPath object guaranteed to survive the lifecycle of the mutator? It might be safer to copy the SkPath using a copy constructor since the SkPath copy constructor is documented to be very fast and copy-safe.

Yeah storing a copy of SkPath is fine with me, but I think shared_ptr for all the them is probably more consistent and fast/light weight too.

SkPath is kind of its own shared_ptr for all of the data that matters...

flar · 2022-07-27T20:20:11Z

flow/embedded_views.h

@@ -98,7 +98,7 @@ class Mutator {
      case kOpacity:
        return alpha_ == other.alpha_;
      case kBackdropFilter:
-        return *filter_ == *other.filter_;
+        return *filter_ == *other.filter_->shared();


No need to call shared here as far as I can tell...?

cyanglaz · 2022-07-27T22:53:19Z

Updated per review comments. @flar PTAL

flar

LGTM, you might want to run it by @chinmaygarde or @jason-simmons as some of the places you copy the shared_ptr might benefit from an "std::move" which will avoid unnecessary inc/decreffing.

cyanglaz · 2022-07-27T23:49:11Z

The same filters are also used to filter none-PlatformView contents so I don't think std::move would be safe. Using std::move will make the code to be dependent on making sure the none-PlatformView part is done with the filter before passing the filter to the stack.

flar · 2022-07-27T23:59:59Z

The same filters are also used to filter none-PlatformView contents so I don't think std::move would be safe. Using std::move will make the code to be dependent on making sure the none-PlatformView part is done with the filter before passing the filter to the stack.

It doesn't move the original. The call to your method already did one incref, a move macro just avoids a second incref/decref pair by "moving" your copy to the field. You hand off your ref to the field rather than create a new ref for the field and then drop your ref separately.

It's a very minor optimization if this code isn't used that often.

making filter shared

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

fa99add

cyanglaz requested review from JTKryptic and flar July 27, 2022 20:05

cyanglaz marked this pull request as ready for review July 27, 2022 20:05

flar suggested changes Jul 27, 2022

View reviewed changes

Chris Yang added 3 commits July 27, 2022 14:11

review

1e1c470

review

dafe9dc

update tests

a6fffa6

flar approved these changes Jul 27, 2022

View reviewed changes

cyanglaz added the autosubmit label Jul 27, 2022

auto-submit bot merged commit bbd1997 into flutter:main Jul 27, 2022

cyanglaz deleted the mutator_stack_image_filter_out_of_union branch July 27, 2022 23:52

engine-flutter-autoroll mentioned this pull request Jul 28, 2022

Roll Flutter Engine from 4becf4942e8d to bbd1997dc91e (2 revisions) flutter/flutter#108501

Merged

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

bbd1997 Make the filter_ in mutator shared_ptr (flutter/engine#34944)

e7c63e8

betrevisan pushed a commit to betrevisan/engine that referenced this pull request Jul 29, 2022

Make the filter_ in mutator shared_ptr (flutter#34944)

d23c7c8

cyanglaz mentioned this pull request Aug 2, 2022

Mutator stack should store a shared reference to the backdrop filter flutter/flutter#107763

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the filter_ in mutator shared_ptr #34944

Make the filter_ in mutator shared_ptr #34944

cyanglaz commented Jul 27, 2022

flar left a comment

flar Jul 27, 2022

cyanglaz Jul 27, 2022

flar Jul 27, 2022

cyanglaz Jul 27, 2022 •

edited

Loading

flar Jul 27, 2022

flar Jul 27, 2022

flar Jul 27, 2022

cyanglaz Jul 27, 2022

flar Jul 27, 2022

flar Jul 27, 2022

cyanglaz commented Jul 27, 2022

flar left a comment

cyanglaz commented Jul 27, 2022

flar commented Jul 27, 2022 •

edited

Loading

Make the filter_ in mutator shared_ptr #34944

Make the filter_ in mutator shared_ptr #34944

Conversation

cyanglaz commented Jul 27, 2022

Pre-launch Checklist

flar 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

cyanglaz Jul 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz commented Jul 27, 2022

flar left a comment

Choose a reason for hiding this comment

cyanglaz commented Jul 27, 2022

flar commented Jul 27, 2022 • edited Loading

cyanglaz Jul 27, 2022 •

edited

Loading

flar commented Jul 27, 2022 •

edited

Loading