Use DlImageFilter for ImageFilterLayer #34837

JsouLiang · 2022-07-22T09:36:22Z

Now we can use DlImageFilter to replace SkImageFilter which can help us to enable some optimizations such as removing saveLayer objects for things like a matrix transform image filter on a compatible group of children.

flar · 2022-07-25T04:52:25Z

display_list/display_list_image_filter.h

    return From(sk_filter.get());
  }

+  std::shared_ptr<DlImageFilter> makeWithLocalMatrix(const SkMatrix&) const {


I'm going to look into implementing this for real rather than just stubbing it out here. I think with this one addition to the DlImageFilter class hierarchy we can get rid of a few of the further uses of SkImageFilter down below.

I filed flutter/flutter#108263 for this with a fairly high priority. I'll look to try to address it in the next couple of days.

flar · 2022-07-25T04:53:20Z

flow/layers/image_filter_layer.cc

+    PaintChildren(context);
+    context.leaf_nodes_builder->restore();
+  } else {
+    cache_paint.setImageFilter(filter_ ? filter_->skia_object() : nullptr);


Let's try to standardize with just the Dl variant of this method as mentioned in the ColorFilter version of this PR. I realize that will mean implementing makeWithLocalMatrix which I'll put on a high priority.

flar · 2022-07-25T04:56:10Z

flow/layers/image_filter_layer_unittests.cc

-  auto layer = std::make_shared<ImageFilterLayer>(sk_sp<SkImageFilter>());
+  auto sk_image_filter = sk_sp<SkImageFilter>();
+  auto layer =
+      std::make_shared<ImageFilterLayer>(DlImageFilter::From(sk_image_filter));


Similar comment as in the ColorFilter version of this PR. Unless a unit test has a specific reason to use one of the SkImageFilter variants that we don't support, migrate these to using a basic DlImageFilter object instead. I haven't looked through each and every unit test here, but I would be surprised if any of them actually care what kind of filter they are using (except some of their bounds answers might be tailored to the filter they originally chose and may need some adjustment to match a new type of filter.

zanderso · 2022-08-04T20:12:08Z

@JsouLiang Are you interested in continuing with this PR? Moving to WIP since it sounds like @flar is requesting some bigger changes.

flar · 2022-08-05T00:04:14Z

@JsouLiang Are you interested in continuing with this PR? Moving to WIP since it sounds like @flar is requesting some bigger changes.

That must have been a misunderstanding. There are no "bigger changes", this work is simply dependent on another mechanism being added under #34878

flar

A lot of kudos in the review comments, but at least one point of concern (the deleted line) and a couple of suggestions...

flar · 2022-08-11T18:28:05Z

flow/layers/image_filter_layer.cc


  SkRect child_bounds = SkRect::MakeEmpty();
  PrerollChildren(context, matrix, &child_bounds);
-  context->subtree_can_inherit_opacity = true;


Why is this line deleted?

It repeats with the next line

flar · 2022-08-11T18:30:40Z

flow/layers/image_filter_layer.cc

+    context.leaf_nodes_builder->saveLayer(&child_paint_bounds(),
+                                          cache_paint.dl_paint());
+    PaintChildren(context);
+    context.leaf_nodes_builder->restore();


(At some point we should probably create a new DL-based AutoSave object?)

AutoSaveLayer support DisplayListBuilder

flow/layers/layer.h

flow/layers/image_filter_layer_unittests.cc

flar · 2022-08-11T18:37:28Z

flow/layers/image_filter_layer_unittests.cc

+        image_filter_paint.setColor(opacity_alpha << 24);
+        image_filter_paint.setImageFilter(dl_image_filter.get());
+        expected_builder.saveLayer(&child_path.getBounds(),
+                                   &image_filter_paint);


flow/layers/image_filter_layer.cc

flow/layers/layer.h

flar · 2022-08-15T04:43:03Z

flow/layers/layer.cc

+}
+
 Layer::AutoSaveLayer::~AutoSaveLayer() {
+  if (type_ == Type::kDisplayList) {


Can't we do checkerboarding? Ah, I see an issue. Another comment below would fix this...

flar · 2022-08-15T04:44:23Z

flow/layers/layer.cc

+  if (type_ == Type::kDisplayList) {
+    dl_builder_->restore();
+    return;
+  }


In the following lines we always use canvas_ to checkerboard, but this could potentially be a nodes_canvas which would broadcast the checkerboarding to all embedded slices. We should probably always use leaf_canvas here regardless of the "mode" of the SkCanvas saveLayer.

flow/layers/layer.h

flow/layers/layer.cc

flar

My apologies for distracting this PR with the idea of a DisplayList AutoSaveLayer, let's not tackle that issue in this PR which should be about converting IFLayer to DlImageFilter.

flar · 2022-08-15T20:40:38Z

display_list/display_list_image_filter.h

  SkRect* map_local_bounds(const SkRect& input_bounds,
                           SkRect& output_bounds) const override {
+    if (!sk_filter_) {
+      return nullptr;


You must fill in output_bounds anyway per the contract of the method.

We should write a test to make sure that this happens.

display_list/display_list_image_filter.h

flow/layers/backdrop_filter_layer.cc

flar · 2022-08-15T21:25:03Z

There is something even more troubling about the DlAutoSaveLayer concept and even our existing DL-based saveLayers...

They aren't executed on the internal_nodes_canvas which means they aren't propagated to other embedding layer canvas objects. There is a lot more thought to be done on how we're managing the saveLayer stack in the presence of DisplayListBuilders that goes far beyond this PR. For now, just continue with the same paradigm of using leaf_nodes_builder.saveLayer in the builder pathways.

flar · 2022-08-15T23:07:39Z

Sadly, it looks like this PR and the similar one for DlColorFilter introduced a bug. I haven't written any code to exploit it, but if we ever have a ColorFilterLayer or ImageFilterLayer that includes a PlatformView in its children then the filter will only apply to its pre-embedded children since we bypass the internal_nodes_canvas with the saveLayer calls. I think we have to revert the CF-de-skia-fying PR as well as hold off on this PR until we can come up with a new plan for how a post-Skia API world will deal with the stratifying of the layers due to embedded views. It isn't too hard to think "well, we can just convert everything to DisplayList and use a solution like flutter/flutter#109563 to replace it" but such a solution would impact our rendering using the Skia backend greatly as we would then be recording the scene into a DisplayList and playing it back to Skia rather than direct rendering to Skia. Until Impeller is our default rendering backend we'll have to make sure that our solution to this problem doesn't impact the Skia backend.

flar · 2022-08-15T23:20:46Z

flutter/flutter#109576

JsouLiang · 2022-08-16T03:00:05Z

Waiting for this #35421 to merge.

flar

LGTM

JsouLiang requested a review from flar July 22, 2022 09:36

JsouLiang force-pushed the Change-ImageFilterLayer-Use-SkImageFilter-To-DlImageFilter branch from e8f28df to 24450da Compare July 25, 2022 03:14

JsouLiang mentioned this pull request Jul 25, 2022

Switch layers to DlImageFiter, DlColorFilter, and DlColorSource objects flutter/flutter#100454

Closed

flar suggested changes Jul 25, 2022

View reviewed changes

zanderso added the Work in progress (WIP) Not ready (yet) for review! label Aug 4, 2022

JsouLiang force-pushed the Change-ImageFilterLayer-Use-SkImageFilter-To-DlImageFilter branch from 24450da to daed8cd Compare August 11, 2022 10:42

JsouLiang requested a review from flar August 11, 2022 10:43

flar suggested changes Aug 11, 2022

View reviewed changes

JsouLiang requested a review from flar August 15, 2022 02:58

flar suggested changes Aug 15, 2022

View reviewed changes

JsouLiang force-pushed the Change-ImageFilterLayer-Use-SkImageFilter-To-DlImageFilter branch from dcaa63c to 09b18bc Compare August 15, 2022 06:55

JsouLiang requested a review from flar August 15, 2022 06:56

JsouLiang force-pushed the Change-ImageFilterLayer-Use-SkImageFilter-To-DlImageFilter branch from 09b18bc to 1b0d384 Compare August 15, 2022 06:57

flar suggested changes Aug 15, 2022

View reviewed changes

flow/layers/layer.h Show resolved Hide resolved

flow/layers/layer.cc Outdated Show resolved Hide resolved

JsouLiang requested a review from flar August 15, 2022 08:28

flar suggested changes Aug 15, 2022

View reviewed changes

JsouLiang added 5 commits August 16, 2022 13:53

draft for SkImageFilter to DlImageFilter

6346810

Formatting Code

d2f85cd

complete the logic

9e62af2

fix some comments

c89cf15

formatting code

de1b257

JsouLiang force-pushed the Change-ImageFilterLayer-Use-SkImageFilter-To-DlImageFilter branch from f7a3cbf to de1b257 Compare August 16, 2022 12:58

JsouLiang requested a review from flar August 16, 2022 12:59

fill out_bounds

d598833

flar approved these changes Aug 16, 2022

View reviewed changes

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

auto-submit bot merged commit c007c40 into flutter:main Aug 16, 2022

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

Roll Flutter Engine from 688624b057db to 69ee0c7094f3 (12 revisions) flutter/flutter#109634

Merged

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

c007c40 Use DlImageFilter for ImageFilterLayer (flutter/engine#34837)

6ed2aa3

moffatman mentioned this pull request Sep 2, 2022

Fix crash on ImageFilter.blur with very small sigma #35890

Merged

Use DlImageFilter for ImageFilterLayer #34837

Use DlImageFilter for ImageFilterLayer #34837

Uh oh!

Conversation

JsouLiang commented Jul 22, 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!

zanderso commented Aug 4, 2022

Uh oh!

flar commented Aug 5, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

flar commented Aug 15, 2022

Uh oh!

flar commented Aug 15, 2022

Uh oh!

flar commented Aug 15, 2022

Uh oh!

JsouLiang commented Aug 16, 2022

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants