Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Make the filter_ in mutator shared_ptr #34944

Merged
merged 4 commits into from
Jul 27, 2022
Merged

Make the filter_ in mutator shared_ptr #34944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fa99add
making filter shared
Jul 27, 2022
1e1c470
review
Jul 27, 2022
dafe9dc
review
Jul 27, 2022
a6fffa6
update tests
Jul 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion flow/embedded_views.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ void MutatorsStack::PushOpacity(const int& alpha) {
vector_.push_back(element);
};

void MutatorsStack::PushBackdropFilter(const DlImageFilter& filter) {
void MutatorsStack::PushBackdropFilter(
std::shared_ptr<const DlImageFilter> filter) {
std::shared_ptr<Mutator> element = std::make_shared<Mutator>(filter);
vector_.push_back(element);
};
Expand Down
11 changes: 7 additions & 4 deletions flow/embedded_views.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ class Mutator {
explicit Mutator(const SkMatrix& matrix)
: type_(kTransform), matrix_(matrix) {}
explicit Mutator(const int& alpha) : type_(kOpacity), alpha_(alpha) {}
explicit Mutator(const DlImageFilter& filter)
: type_(kBackdropFilter), filter_(&filter) {}
explicit Mutator(std::shared_ptr<const DlImageFilter> filter)
: type_(kBackdropFilter), filter_(filter) {}

const MutatorType& GetType() const { return type_; }
const SkRect& GetRect() const { return rect_; }
Expand Down Expand Up @@ -119,15 +119,18 @@ class Mutator {
private:
MutatorType type_;

// TODO(cyanglaz): Remove union.
// https://github.com/flutter/flutter/issues/108470
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

union {
SkRect rect_;
SkRRect rrect_;
SkMatrix matrix_;
SkPath* path_;
int alpha_;
const DlImageFilter* filter_;
};

std::shared_ptr<const DlImageFilter> filter_;

}; // Mutator

// A stack of mutators that can be applied to an embedded platform view.
Expand All @@ -148,7 +151,7 @@ class MutatorsStack {
void PushClipPath(const SkPath& path);
void PushTransform(const SkMatrix& matrix);
void PushOpacity(const int& alpha);
void PushBackdropFilter(const DlImageFilter& filter);
void PushBackdropFilter(std::shared_ptr<const DlImageFilter> filter);

// Removes the `Mutator` on the top of the stack
// and destroys it.
Expand Down
20 changes: 15 additions & 5 deletions flow/mutators_stack_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,11 +91,21 @@ TEST(MutatorsStack, PushOpacity) {

TEST(MutatorsStack, PushBackdropFilter) {
MutatorsStack stack;
auto filter = DlBlurImageFilter(5, 5, DlTileMode::kClamp);
stack.PushBackdropFilter(filter);
auto iter = stack.Bottom();
ASSERT_TRUE(iter->get()->GetType() == MutatorType::kBackdropFilter);
ASSERT_TRUE(iter->get()->GetFilter() == filter);
const int num_of_mutators = 10;
for (int i = 0; i < num_of_mutators; i++) {
auto filter = std::make_shared<DlBlurImageFilter>(i, 5, DlTileMode::kClamp);
stack.PushBackdropFilter(filter);
}

auto iter = stack.Begin();
int i = 0;
while (iter != stack.End()) {
ASSERT_EQ(iter->get()->GetType(), MutatorType::kBackdropFilter);
ASSERT_EQ(iter->get()->GetFilter().asBlur()->sigma_x(), i);
++iter;
++i;
}
ASSERT_EQ(i, num_of_mutators);
}

TEST(MutatorsStack, Pop) {
Expand Down