Add DlSamplingOptions & DlFilterMode #34024

JsouLiang · 2022-06-14T03:09:30Z

Using DlSamplingOptions and DlFilterMode to replace SkSamplingOptions and SkFilterMode

flar · 2022-06-14T19:51:42Z

I don't see an issue filed per the guidelines of flutter/flutter#95434

Filing the issue and linking it to the umbrella task is important so that we track who is working on what features at any time and can prepare for them in Impeller. We can also provide guidance for the task.

The current guidance in the umbrella issue is that we do not need the flexibility of SkSamplingOptions in DL or Impeller and so a simple enum will be fine. It would actually be far far preferable because we have no intention of implementing any options beyond the 4 enum values that Flutter uses in Impeller.

Please create the issue, link it to the line in the description in the umbrella issue above as is customary, and replace DlSamplingOptions with:

enum class DlImageSampling {
   kNearestNeighbor,
   kLinear,
   kMipmapLinear,
   kCubic,
};

For the methods which take an SkFilterMode, we should probably use the above enum and just treat Mipmap and Cubic as Linear.

flar

I commented with a few code cleanup issues, but there are also a couple of general issues to look at:

The new enums need unit tests in display_list_enum_unittests.cc
Nearly everywhere SkSamplingOptions was used as a parameter to a method it was qualified with const so that the object could not be modified. Now that it is an enum it is passed by value ad so the const qualifiers are no longer relevant.

(const is still needed where Sampling is a field in a class, though, to prevent modification)

display_list/display_list_sampling_options.h

testing/display_list_testing.cc

lib/ui/painting/fragment_program.cc

flar · 2022-06-15T20:15:03Z

Also, create an Issue for this work and link it in to the umbrella Issue as mentioned in flutter/flutter#95434 (comment)

flar

The only really critical change is that all fields in all structs defined in dl_ops.h need to be const so that the DL records are read-only, so those "const" modifiers need to be restored.

Everything else is a nit.

flar · 2022-06-16T21:59:34Z

display_list/display_list_ops.h

                                                                       \
    const SkPoint point;                                               \
-    const DlImageSampling sampling;                                    \
+    DlImageSampling sampling;                                          \


This is a field declaration. Leave const here so that the record cannot be modified.

(The const in the argument list is OK to remove, just not "const" on a variable or field declaration.)

flar · 2022-06-16T21:59:45Z

display_list/display_list_ops.h

  const SkRect src;
  const SkRect dst;
-  const DlImageSampling sampling;
+  DlImageSampling sampling;


Leave const here.

flar · 2022-06-16T21:59:57Z

display_list/display_list_ops.h

  const uint8_t has_colors;
  const uint8_t render_with_attributes;
-  const DlImageSampling sampling;
+  DlImageSampling sampling;


Leave const here.

flar · 2022-06-16T22:00:43Z

display_list/display_list_sampling_options.h

      return DlImageSampling::kMipmapLinear;
    }
  }
  if (so.filter == SkFilterMode::kNearest && so.mipmap == SkMipmapMode::kNone) {


This if statement isn't useful any more, now that the default return value below it is kNearest.

flar · 2022-06-16T22:06:55Z

display_list/display_list_canvas_unittests.cc

  };
  const sk_sp<SkImage> image = CanvasCompareTester::kTestImage;
-  const DlImageSampling sampling = DlImageSampling::kNearestNeighbor;
+  DlImageSampling sampling = DlImageSampling::kNearestNeighbor;


These 3 const modifiers should probably be left in because they are locking a local variable against modification. Only "const" in argument lists is unnecessary.

flar · 2022-06-16T22:07:09Z

display_list/display_list_canvas_unittests.cc

  };
  const sk_sp<SkImage> image = CanvasCompareTester::kTestImage;
-  const DlImageSampling sampling = DlImageSampling::kNearestNeighbor;
+  DlImageSampling sampling = DlImageSampling::kNearestNeighbor;


Restore const here.

flar · 2022-06-16T22:07:17Z

display_list/display_list_canvas_unittests.cc

  };
  const sk_sp<SkImage> image = CanvasCompareTester::kTestImage;
-  const DlImageSampling sampling = DlImageSampling::kLinear;
+  DlImageSampling sampling = DlImageSampling::kLinear;


Restore const here.

flar

LGTM

This reverts commit e25d1ce.

JsouLiang requested review from chinmaygarde and flar June 14, 2022 03:09

JsouLiang force-pushed the Add_DlSamplingOptions_DlFilterMode branch from 52d97e5 to b16b89b Compare June 14, 2022 03:33

Add DlSamplingOptions & DlFilterMode

5223b9b

JsouLiang force-pushed the Add_DlSamplingOptions_DlFilterMode branch from b16b89b to 5223b9b Compare June 14, 2022 06:38

Change DlSamplingOptions to DlImageSampling Enum

7ac683b

JsouLiang mentioned this pull request Jun 15, 2022

Create a DlImageSampling Object flutter/flutter#106013

Closed

flar suggested changes Jun 15, 2022

View reviewed changes

Add test cases and fix some comments

dbbefcf

flar suggested changes Jun 16, 2022

View reviewed changes

Restore const

dd52b62

JsouLiang requested a review from flar June 17, 2022 03:30

flar approved these changes Jun 17, 2022

View reviewed changes

flar mentioned this pull request Jun 17, 2022

PR stuck on ci.yaml validation flutter/flutter#106181

Closed

Restarting try builds

97fa08f

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

auto-submit bot merged commit e25d1ce into flutter:main Jun 17, 2022

iskakaushik mentioned this pull request Jun 17, 2022

Engine Tree red due to change in API while PR was in-progress flutter/flutter#106218

Closed

iskakaushik added a commit to iskakaushik/engine that referenced this pull request Jun 17, 2022

Revert "Add DlSamplingOptions & DlFilterMode (flutter#34024)"

aded564

This reverts commit e25d1ce.

engine-flutter-autoroll mentioned this pull request Jun 17, 2022

Roll Flutter Engine from 5540f885a806 to 9ee1525e20b8 (2 revisions) flutter/flutter#106222

Merged

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

e25d1ce Add DlSamplingOptions & DlFilterMode (flutter/engine#34024)

122f35b

houhuayong pushed a commit to houhuayong/engine that referenced this pull request Jun 21, 2022

Add DlSamplingOptions & DlFilterMode (flutter#34024)

5518dc1

bdero mentioned this pull request Nov 21, 2022

Replace the DlImageSampling enum with finer grained DlSamplingOptions struct flutter/flutter#115794

Open

Add DlSamplingOptions & DlFilterMode #34024

Add DlSamplingOptions & DlFilterMode #34024

Uh oh!

Conversation

JsouLiang commented Jun 14, 2022 • edited by flar Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flar commented Jun 14, 2022

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

flar commented Jun 15, 2022

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

flar Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

flar Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

flar Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

flar Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

flar Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

flar Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

flar Jun 16, 2022

Choose a reason for hiding this comment

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

JsouLiang commented Jun 14, 2022 •

edited by flar

Loading