ColorFilterLayer change SkColorFilter to DLColorFilter #34744

JsouLiang · 2022-07-19T09:35:33Z

Now we can use DlColorFilter and DlImageFilter to replace SkColorFilter/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.

SkColorFilter to DlColorFilter

display_list/display_list_canvas_dispatcher.cc

flar

Many of these comments point out that the old unit tests would use an SkLightingFilter because it was just as reasonable as any other SkColorFilter object, but now that we are focusing on Dl versions of the objects - that turns into an unknown DlColorFilter - which is unnecessary and requires these tests to continue to construct SkColorFilter objects even as we are purging them from the rest of the source base. Please migrate the unit tests to supported DlColorFilter types unless they have a specific reason to be instantiating the odd variant of SkColorFilter that they chose, which I didn't really see any cases where they actually cared what type of filter was being used...?

flow/layers/layer.h

flar · 2022-07-25T04:31:18Z

flow/layers/color_filter_layer_unittests.cc

@@ -82,10 +87,10 @@ TEST_F(ColorFilterLayerTest, SimpleFilter) {
  const SkRect child_bounds = SkRect::MakeLTRB(5.0f, 6.0f, 20.5f, 21.5f);
  const SkPath child_path = SkPath().addRect(child_bounds);
  const SkPaint child_paint = SkPaint(SkColors::kYellow);
-  auto layer_filter =
-      SkColorMatrixFilter::MakeLightingFilter(SK_ColorGREEN, SK_ColorYELLOW);
+  auto dl_color_filter = DlColorFilter::From(


I don't think this test was testing the behavior of a Lighting filter, it's just a basic test of using the color filter layer with a basic filter. It would be good to switch it to some variation that we support in DlColorFilter.

Using DlColorFilter

flar · 2022-07-25T04:31:23Z

flow/layers/color_filter_layer.cc

@@ -3,12 +3,15 @@
 // found in the LICENSE file.

 #include "flutter/flow/layers/color_filter_layer.h"
+#include <cstddef>


Is this needed? I didn't see any use of it below, but maybe I missed something.

flar · 2022-07-25T04:31:57Z

flow/layers/color_filter_layer.cc

    if (layer_raster_cache_item_->IsCacheChildren()) {
-      cache_paint.setColorFilter(filter_);
+      cache_paint.setColorFilter(filter_->skia_object());


Can we switch these all to use the non-skia version of cache_paint.setColorFilter()?

Using DlColorFilter

flar · 2022-07-25T04:32:24Z

flow/layers/color_filter_layer.cc

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


Same as earlier - use the Dl version of this method so we can delete the sk version.

flar · 2022-07-25T04:39:11Z

flow/layers/color_filter_layer_unittests.cc

  auto mock_layer1 = std::make_shared<MockLayer>(child_path1, child_paint1);
  auto mock_layer2 = std::make_shared<MockLayer>(child_path2, child_paint2);
-  auto layer = std::make_shared<ColorFilterLayer>(layer_filter);
+  auto dl_color_filter = DlColorFilter::From(
+      SkColorMatrixFilter::MakeLightingFilter(SK_ColorGREEN, SK_ColorYELLOW));


Switch to a basic DlColorFilter.

flar · 2022-07-25T04:39:36Z

flow/layers/color_filter_layer_unittests.cc

-  auto layer2 = std::make_shared<ColorFilterLayer>(layer_filter2);
+  auto dl_color_filter1 = DlColorFilter::From(
+      SkColorMatrixFilter::MakeLightingFilter(SK_ColorGREEN, SK_ColorYELLOW));
+  auto layer1 = std::make_shared<ColorFilterLayer>(dl_color_filter1);


Don't propagate the use of Sk Lighting Filters as per other cases.

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

flow/layers/color_filter_layer_unittests.cc

@@ -234,7 +269,8 @@ TEST_F(ColorFilterLayerTest, CacheChild) {
  const SkPath child_path = SkPath().addRect(SkRect::MakeWH(5.0f, 5.0f));
  SkPaint paint = SkPaint();
  auto mock_layer = std::make_shared<MockLayer>(child_path);
-  auto layer = std::make_shared<ColorFilterLayer>(layer_filter);
+  auto layer =


Use a basic DlColorFilter here.

flar · 2022-07-25T04:40:42Z

flow/layers/color_filter_layer_unittests.cc

@@ -276,7 +312,8 @@ TEST_F(ColorFilterLayerTest, CacheChildren) {
  const SkPath child_path2 = SkPath().addRect(SkRect::MakeWH(5.0f, 5.0f));
  auto mock_layer1 = std::make_shared<MockLayer>(child_path1);
  auto mock_layer2 = std::make_shared<MockLayer>(child_path2);
-  auto layer = std::make_shared<ColorFilterLayer>(layer_filter);
+  auto layer =
+      std::make_shared<ColorFilterLayer>(DlColorFilter::From(layer_filter));


Use basic DlColorFilter.

flar · 2022-07-25T04:40:57Z

flow/layers/color_filter_layer_unittests.cc

@@ -322,7 +359,8 @@ TEST_F(ColorFilterLayerTest, CacheColorFilterLayerSelf) {
  const SkPath child_path2 = SkPath().addRect(SkRect::MakeWH(5.0f, 5.0f));
  auto mock_layer1 = std::make_shared<MockLayer>(child_path1);
  auto mock_layer2 = std::make_shared<MockLayer>(child_path2);
-  auto layer = std::make_shared<ColorFilterLayer>(layer_filter);
+  auto layer =


Use basic DlColorFilter.

flar

Mostly code simplification suggestions, but a few of them are for future maintainability. This is essentially "approve, but I really want new code to focus on the builder methods that use DlPaint" so I marked it as "request changes".

flow/layers/color_filter_layer.cc

flow/layers/color_filter_layer_unittests.cc

flar · 2022-08-03T23:49:23Z

flow/layers/color_filter_layer_unittests.cc

+      /* MockLayer::Paint() */ {
+        expected_builder.setColor(SkColors::kYellow.toSkColor());
+        expected_builder.setColorFilter(nullptr);
+        expected_builder.drawPath(child_path);


This would look simpler if you use the drawPath(..., &dl_paint) variant. You wouldn't have to manually reset the attributes in the builder (assuming you used different paint objects in each call). The same thing could be said for the saveLayer, though it wouldn't be any fewer lines of code. I'm steering the builder interface and its internal uses towards the call variants that take a DlPaint and may eventually make the stateful calls a hidden implementation detail not visible from the Builder API.

Note that the set methods on DlPaint are chainable for easy initialization and more compact code, as in:

DlPaint paint = DlPaint().setColor(DlColor.kYellow());

flar · 2022-08-03T23:52:45Z

flow/layers/color_filter_layer_unittests.cc

-  auto layer = std::make_shared<ColorFilterLayer>(layer_filter);
+  auto dl_color_filter = DlSrgbToLinearGammaColorFilter::instance;
+  auto layer = std::make_shared<ColorFilterLayer>(
+      DlSrgbToLinearGammaColorFilter::instance);


Already loaded into a local variable.

flar · 2022-08-03T23:53:09Z

flow/layers/color_filter_layer_unittests.cc

+  DisplayListBuilder expected_builder;
+  /* ColorFilterLayer::Paint() */ {
+    expected_builder.setColorFilter(dl_color_filter.get());
+    expected_builder.saveLayer(&children_bounds, true);


saveLayer(&children_bounds, &paint)

flow/layers/color_filter_layer_unittests.cc

flar

Looks good, but I don't think reusing local variable names is a good style practice.

flow/layers/color_filter_layer_unittests.cc

flar

Approved, but please fix the duplicate local variable names in the tests.

flow/layers/color_filter_layer_unittests.cc

…er/engine#34744)

* change SkColorFilter to DlColorFilter * draft for SkColorFilter to DlColorFilter * draft for SkColorFilter to DlColorFilter * Formatting code * Use DlColorFilter for ColorFilterLayer; Change the test cases * Use DlObject for test cases * fix some nits * update code formats * update the local variable name

…er/engine#34744)

* 7a536ee Roll Dart SDK from 0ab07c889f74 to 16cbbc9d5e4b (1 revision) (flutter/engine#35177) * e1e4a28 [Impeller] Enforce shader resource limits in impellerc (flutter/engine#35120) * d6931cc Roll Fuchsia Mac SDK from CUZLMQ1lB... to b8WQvI4f0... (flutter/engine#35179) * c1bb5d7 Roll Dart SDK from 16cbbc9d5e4b to aada0a67f81e (1 revision) (flutter/engine#35180) * d45c4c4 ColorFilterLayer change SkColorFilter to DLColorFilter (flutter/engine#34744) * 1a678d2 Roll Fuchsia Linux SDK from BRTz21cLl... to kURZcohuz... (flutter/engine#35182) * cdccc60 Roll Dart SDK from aada0a67f81e to 344a7d12b413 (1 revision) (flutter/engine#35181) * 2c7b4f6 Update setAssetDirectory service extension to fail if provided path is invalid (flutter/engine#35178) * ce3397f fix analysis error (flutter/engine#35187) * eaeae8e Make it possible to obtain `FontWeight` integer value (flutter/engine#35183) * 1f9d87d Analyze all dart code on CI (flutter/engine#35147) * 1cf7023 [impeller] [vulkan] Support textures backed by `vk::Image`s (flutter/engine#35163) * 57f43bd Roll Skia from 098c234c05f7 to f1245dcd35f8 (23 revisions) (flutter/engine#35189)

…09063) * 7a536ee Roll Dart SDK from 0ab07c889f74 to 16cbbc9d5e4b (1 revision) (flutter/engine#35177) * e1e4a28 [Impeller] Enforce shader resource limits in impellerc (flutter/engine#35120) * d6931cc Roll Fuchsia Mac SDK from CUZLMQ1lB... to b8WQvI4f0... (flutter/engine#35179) * c1bb5d7 Roll Dart SDK from 16cbbc9d5e4b to aada0a67f81e (1 revision) (flutter/engine#35180) * d45c4c4 ColorFilterLayer change SkColorFilter to DLColorFilter (flutter/engine#34744) * 1a678d2 Roll Fuchsia Linux SDK from BRTz21cLl... to kURZcohuz... (flutter/engine#35182) * cdccc60 Roll Dart SDK from aada0a67f81e to 344a7d12b413 (1 revision) (flutter/engine#35181) * 2c7b4f6 Update setAssetDirectory service extension to fail if provided path is invalid (flutter/engine#35178) * ce3397f fix analysis error (flutter/engine#35187) * eaeae8e Make it possible to obtain `FontWeight` integer value (flutter/engine#35183) * 1f9d87d Analyze all dart code on CI (flutter/engine#35147) * 1cf7023 [impeller] [vulkan] Support textures backed by `vk::Image`s (flutter/engine#35163) * 57f43bd Roll Skia from 098c234c05f7 to f1245dcd35f8 (23 revisions) (flutter/engine#35189)

* change SkColorFilter to DlColorFilter * draft for SkColorFilter to DlColorFilter * draft for SkColorFilter to DlColorFilter * Formatting code * Use DlColorFilter for ColorFilterLayer; Change the test cases * Use DlObject for test cases * fix some nits * update code formats * update the local variable name

JsouLiang requested a review from flar July 19, 2022 09:35

JsouLiang added the Work in progress (WIP) Not ready (yet) for review! label Jul 19, 2022

flar suggested changes Jul 21, 2022

View reviewed changes

display_list/display_list_canvas_dispatcher.cc Outdated Show resolved Hide resolved

JsouLiang force-pushed the change-SkColorFilter-SkImageFilter-to-DlColorFilter-DlImageFilter branch from 3786c49 to e242a80 Compare July 21, 2022 08:34

JsouLiang changed the title ~~SkColorFilter/ImageFilter to DLColorFilter/ImageFilter~~ SkColorFilter to DLColorFilter Jul 21, 2022

JsouLiang changed the title ~~SkColorFilter to DLColorFilter~~ ColorFilterLayer change SkColorFilter to DLColorFilter Jul 21, 2022

JsouLiang added 4 commits July 25, 2022 11:06

change SkColorFilter to DlColorFilter

ade0b1c

draft for SkColorFilter to DlColorFilter

9289fee

draft for SkColorFilter to DlColorFilter

64b84e8

Formatting code

8b3a620

JsouLiang force-pushed the change-SkColorFilter-SkImageFilter-to-DlColorFilter-DlImageFilter branch from 268d5e4 to 8b3a620 Compare July 25, 2022 03:09

JsouLiang requested a review from flar July 25, 2022 03:16

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

JsouLiang added 2 commits July 25, 2022 13:15

Use DlColorFilter for ColorFilterLayer; Change the test cases

a6b673e

Use DlObject for test cases

c859e82

JsouLiang requested a review from flar July 25, 2022 05:46

JsouLiang removed the Work in progress (WIP) Not ready (yet) for review! label Aug 1, 2022

flar suggested changes Aug 3, 2022

View reviewed changes

fix some nits

e2eaa1b

JsouLiang requested a review from flar August 4, 2022 03:05

flar suggested changes Aug 4, 2022

View reviewed changes

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

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

update code formats

7d1e8b7

JsouLiang requested a review from flar August 5, 2022 02:25

flar approved these changes Aug 5, 2022

View reviewed changes

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

update the local variable name

68d4b60

JsouLiang merged commit d45c4c4 into flutter:main Aug 5, 2022

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

Roll Flutter Engine from 25bad51f09d6 to d45c4c42dac5 (5 revisions) flutter/flutter#109041

Closed

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

d45c4c4 ColorFilterLayer change SkColorFilter to DLColorFilter (flutt…

a8b79f7

…er/engine#34744)

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

Roll Flutter Engine from 25bad51f09d6 to 1a678d2ce6b9 (6 revisions) flutter/flutter#109047

Closed

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

d45c4c4 ColorFilterLayer change SkColorFilter to DLColorFilter (flutt…

bf5b8bb

…er/engine#34744)

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

Roll Flutter Engine from 25bad51f09d6 to cdccc60ca53d (7 revisions) flutter/flutter#109048

Closed

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

d45c4c4 ColorFilterLayer change SkColorFilter to DLColorFilter (flutt…

793ca04

…er/engine#34744)

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

Roll Flutter Engine from 25bad51f09d6 to ce3397f950a4 (9 revisions) flutter/flutter#109051

Closed

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

d45c4c4 ColorFilterLayer change SkColorFilter to DLColorFilter (flutt…

d3de2b9

…er/engine#34744)

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

Roll Flutter Engine from 25bad51f09d6 to eaeae8e83702 (10 revisions) flutter/flutter#109056

Closed

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

d45c4c4 ColorFilterLayer change SkColorFilter to DLColorFilter (flutt…

7f55c93

…er/engine#34744)

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

Roll Flutter Engine from 25bad51f09d6 to 1cf7023e2637 (12 revisions) flutter/flutter#109058

Closed

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

d45c4c4 ColorFilterLayer change SkColorFilter to DLColorFilter (flutt…

1474ac3

…er/engine#34744)

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

Roll Flutter Engine from 25bad51f09d6 to 57f43bd3a901 (13 revisions) flutter/flutter#109061

Closed

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

d45c4c4 ColorFilterLayer change SkColorFilter to DLColorFilter (flutt…

af5e5c1

…er/engine#34744)

iskakaushik mentioned this pull request Aug 5, 2022

Roll Flutter Engine from 25bad51f09d6 to 1cf7023e2637 (12 revisions) flutter/flutter#109062

Closed

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

Roll Flutter Engine from 25bad51f09d6 to 57f43bd3a901 (13 revisions) flutter/flutter#109063

Merged

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

d45c4c4 ColorFilterLayer change SkColorFilter to DLColorFilter (flutt…

c5d830e

…er/engine#34744)

ColorFilterLayer change SkColorFilter to DLColorFilter #34744

ColorFilterLayer change SkColorFilter to DLColorFilter #34744

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

flar left a comment

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!

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!

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JsouLiang commented Jul 19, 2022 •

edited

Loading