Create DlLocalMatrixImageFilter #34878

JsouLiang · 2022-07-25T08:43:53Z

Currently this technique relies on SkImageFilter::makeWithLocalMatrix, but that operation is not supported in DlImageFilter.
This pr is try to implement the DlLocalMatrixImageFilter
cc @flar

flar

Did you run tests on how SkImageFilter deals with local matrices? How is the filtered output affected? The bounds methods?

flar · 2022-07-25T16:37:17Z

display_list/display_list_image_filter.h

+
+  SkRect* map_local_bounds(const SkRect& input_bounds,
+                           SkRect& output_bounds) const override {
+    output_bounds = matrix_.mapRect(input_bounds);


This should just be return image_filter_->map_local_bounds(...);

flar · 2022-07-25T16:38:01Z

display_list/display_list_image_filter.h

+                             const SkMatrix& ctm,
+                             SkIRect& output_bounds) const override {
+    return image_filter_->map_device_bounds(
+        input_bounds, SkMatrix::Concat(ctm, matrix_), output_bounds);


The input bounds are relative to ctm, but not to matrix_. You need to return bounds relative to only ctm.

Actually, I'm wondering what Skia does with this. Have you run tests to see how the bounds methods behave with a local matrix?

I added a test case:

auto blur_filter = SkImageFilters::Blur(5.0, 6.0, SkTileMode::kRepeat, nullptr); auto local_filter = blur_filter->makeWithLocalMatrix(SkMatrix::Scale(2, 2)); auto inputBounds = SkIRect::MakeLTRB(20, 20, 80, 80); auto rect = local_filter->filterBounds(inputBounds, SkMatrix::I(), SkImageFilter::MapDirection::kForward_MapDirection); auto dl_color_filter = std::make_shared<DlBlurImageFilter>(5.0, 6.0, DlTileMode::kRepeat); auto local_matrix_filter = DlLocalMatrixImageFilter(SkMatrix::Scale(2, 2), dl_color_filter); SkIRect out_bounds; local_matrix_filter.map_device_bounds(inputBounds, SkMatrix::I(),out_bounds); ASSERT_EQ(out_bounds, rect);

the out_bouns is equal the rect when we use SkMatrix::Concat(ctm, matrix_)

This is a very basic test of some of the cases that have the fewest quirks in them.

Try something like:

For each filter type: filter = instantiate that type; For each transform type local_matrix: transformed_filter = filter->makeWithLocal(local_matrix) For each transform type map_matrix: compare output of transformed_filter.map(..., map_matrix, ...);

Transform types should be:

translate

scale + translate

rotate + translate

perspective

any others?

flar · 2022-07-25T16:38:52Z

display_list/display_list_image_filter.h

+  SkIRect* get_input_device_bounds(const SkIRect& output_bounds,
+                                   const SkMatrix& ctm,
+                                   SkIRect& input_bounds) const override {
+    return image_filter_->get_input_device_bounds(


Same as map_device_bounds - which coordinate system should the returned bounds be relative to?

flar · 2022-07-25T16:43:05Z

display_list/display_list_image_filter.h

+  explicit DlLocalMatrixImageFilter(const DlLocalMatrixImageFilter* filter)
+      : DlLocalMatrixImageFilter(filter->matrix_, filter->image_filter_) {}
+  DlLocalMatrixImageFilter(const DlLocalMatrixImageFilter& filter)
+      : DlLocalMatrixImageFilter(&filter) {}


There is no protection against a null filter in the constructors so everywhere image_filter_ is used here has to protect against null.

display_list/display_list_image_filter.h

JsouLiang · 2022-07-25T17:12:58Z

Did you run tests on how SkImageFilter deals with local matrices? How is the filtered output affected? The bounds methods?

I will add some tests to test it. Current version some method maybe is wrong

flar · 2022-07-27T04:53:57Z

display_list/display_list_builder.cc

+            filter->asLocalMatrix();
+        FML_DCHECK(local_matrix_filter);
+        void* pod = Push<SetPodImageFilterOp>(local_matrix_filter->size(), 0);
+        new (pod) DlLocalMatrixImageFilter(local_matrix_filter);


pod allocations must be raw data, no pointers. This filter is incompatible with pod storage.

flar · 2022-07-27T05:12:40Z

display_list/display_list_image_filter.h

+                             const SkMatrix& ctm,
+                             SkIRect& output_bounds) const override {
+    return image_filter_->map_device_bounds(
+        input_bounds, SkMatrix::Concat(ctm, matrix_), output_bounds);


This is a very basic test of some of the cases that have the fewest quirks in them.

Try something like:

For each filter type: filter = instantiate that type; For each transform type local_matrix: transformed_filter = filter->makeWithLocal(local_matrix) For each transform type map_matrix: compare output of transformed_filter.map(..., map_matrix, ...);

Transform types should be:

translate

scale + translate

rotate + translate

perspective

any others?

flar · 2022-07-27T05:13:51Z

display_list/display_list_image_filter.h

                                           SkIRect& input_bounds) const = 0;

+  virtual MatrixCapability get_matrix_capability() const {
+    return MatrixCapability::kScaleTranslate;


Where did you get information about the compatibility here?

In Skia, the ImageFilter has a method onGetCTMCapability that will return the MatrixCapability.

flar · 2022-07-28T23:04:18Z

display_list/display_list_builder.cc

        Push<SetSharedImageFilterOp>(0, 0, filter);
        break;
      }
+      case DlImageFilterType::kLocalMatrixFilter:


This should be added to the previous set of cases in this switch so we add it with a shared pointer rather than converting to an opaque Skia object.

(See SetSharedImageFilterOp implementation)

flar · 2022-07-28T23:15:17Z

display_list/display_list_image_filter.h

+    // until that's updated. if (this->cropRectIsSet()) {
+    //     result = std::min(result, MatrixCapability::kScaleTranslate);
+    // }
+    for (auto* filter : filters()) {


This feels a little like overkill for our one type that has sub-filters. I would just make this virtual and have Compose return "min(outer->..., inner->...).

Done.
Another question is the Skia has a logic is check the cropRectIsSet, sorry I don't get what it means.

Should we need to add this check?

If it's the code I think you are referring to, I think they have a way to inject a crop rectangle on the inputs that we don't necessarily have (?yet?)?

flar · 2022-07-28T23:17:02Z

display_list/display_list_image_filter.h

+    return MatrixCapability::kScaleTranslate;
+  }
+
+  virtual std::vector<const DlImageFilter*> filters() const { return {this}; }


This is actually unnecessary for child-less filters - their own method was already called before this vector is considered so this default implementation just forces their own method to be called twice. An empty vector would be less redundant.

But, really, I think this vector solution is overkill for our current minimal tree.

This is now unused, please delete it.

Why is this method still here? It is unused. Please delete it.

display_list/display_list_image_filter.h

display_list/display_list_image_filter_unittests.cc

display_list/display_list_image_filter.cc

display_list/display_list_image_filter.h

flar · 2022-07-29T21:54:08Z

display_list/display_list_image_filter.h

+    return MatrixCapability::kScaleTranslate;
+  }
+
+  virtual std::vector<const DlImageFilter*> filters() const { return {this}; }


This is now unused, please delete it.

display_list/display_list_image_filter_unittests.cc

flar · 2022-08-08T19:25:19Z

display_list/display_list_image_filter.h

+    return MatrixCapability::kScaleTranslate;
+  }
+
+  virtual std::vector<const DlImageFilter*> filters() const { return {this}; }


Why is this method still here? It is unused. Please delete it.

display_list/display_list_image_filter.h

display_list/display_list_image_filter_unittests.cc

display_list/display_list_ops.h

flar

Looks good to go. Make sure we have appropriate Skia bug(s) and Flutter Issue(s) for the problem in the unit tests and add them to the code before pushing...

display_list/display_list_image_filter_unittests.cc

bdero · 2022-09-12T21:05:11Z

I'm having trouble understanding this implementation. Do the bounds mapping functions correctly mimic Skia's behavior?

The skia_object() implementation returns skia_object->makeWithLocalMatrix(matrix_);.
The device bounds mapping routines look like the intent is for the matrix to be applied within the CTM space before drawing takes place. In practice, the matrix seems to be applied in the parent space: https://fiddle.skia.org/c/338f53b8b36d4ba30e38cd91f0564c9d

If the bounds mapping code in this implementation was correct, I would expect the Skia fiddle above to translate the blurred image 600 pixels to the right -- but in practice, it only translates 100 pixels. Swapping over to the SkImageFilters::MatrixTransform implementation changes nothing about the result.

JsouLiang · 2022-09-13T02:29:07Z

@bdero I found this issue when I wrote the unit test cases, and I have put forward an issus, there has been some discussion with @flar in the issus context
Maybe they're related.
This is the issue: flutter/flutter#108693

JsouLiang requested a review from flar July 25, 2022 08:44

flar suggested changes Jul 25, 2022

View reviewed changes

JsouLiang requested a review from flar July 27, 2022 02:08

flar suggested changes Jul 27, 2022

View reviewed changes

JsouLiang requested a review from flar July 27, 2022 12:25

JsouLiang force-pushed the Create-DlLocalMatrixImageFilter branch from 1f91b6f to 56557ed Compare July 28, 2022 08:45

flar suggested changes Jul 28, 2022

View reviewed changes

JsouLiang force-pushed the Create-DlLocalMatrixImageFilter branch from 56557ed to a730139 Compare July 29, 2022 07:46

JsouLiang requested a review from flar July 29, 2022 07:47

flar suggested changes Jul 29, 2022

View reviewed changes

flar mentioned this pull request Aug 5, 2022

Use DlImageFilter for ImageFilterLayer #34837

Merged

JsouLiang requested a review from flar August 8, 2022 06:26

flar suggested changes Aug 8, 2022

View reviewed changes

JsouLiang added 7 commits August 9, 2022 11:22

Create DlLocalMatrixImageFilter

d48807e

implement localMatrixImageFilter

e19c17b

update test cases

8c91fb3

test cases

de99e7d

fix some comments

c323d42

update some comments

9f48c9c

fix some comments

abd475b

JsouLiang force-pushed the Create-DlLocalMatrixImageFilter branch from a137693 to abd475b Compare August 9, 2022 03:49

JsouLiang requested a review from flar August 9, 2022 03:49

flar approved these changes Aug 11, 2022

View reviewed changes

display_list/display_list_image_filter_unittests.cc Outdated Show resolved Hide resolved

Add issue tracking

97864c6

JsouLiang added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 11, 2022

auto-submit bot merged commit f2fce02 into flutter:main Aug 11, 2022

engine-flutter-autoroll mentioned this pull request Aug 11, 2022

Roll Flutter Engine from c9a31b7ea4fe to cf8d7c01d66d (10 revisions) flutter/flutter#109394

Merged

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

f2fce02 Create DlLocalMatrixImageFilter (flutter/engine#34878)

7abeeae

emilyabest pushed a commit to emilyabest/engine that referenced this pull request Aug 12, 2022

Create DlLocalMatrixImageFilter (flutter#34878)

698ffe8

flar mentioned this pull request Nov 7, 2022

DlImageFilter should implement something similar to SkImageFilter.makeWithLocalMatrix() flutter/flutter#108263

Closed

Create DlLocalMatrixImageFilter #34878

Create DlLocalMatrixImageFilter #34878

Uh oh!

Conversation

JsouLiang commented Jul 25, 2022

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JsouLiang commented Jul 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bdero commented Sep 12, 2022

Uh oh!

JsouLiang commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

JsouLiang commented Sep 13, 2022 •

edited

Loading