Surgically remove `.dither.` from the Engine #46750

matanlurey · 2023-10-11T00:51:47Z

We no longer support any user-visible configuration around dithering. It is unconditionally applied for gradients (in both the Skia and Impeller backends), and never applied elsewhere. After this change, I'll update https://docs.flutter.dev/release/breaking-changes/paint-enableDithering accordingly.

~~Requires #46746 to land successfully~~ Done.

Alright, this appears ready to review!

jonahwilliams

LGTM

flar

Extra credit - the dither flags in dl_op_flags.[h,cc] can be deleted.

flar · 2023-10-30T21:22:21Z

display_list/display_list_unittests.cc

@@ -387,7 +386,6 @@ TEST_F(DisplayListTest, BuildRestoresAttributes) {
  builder.Build();
  check_defaults(builder, cull_rect);

-  receiver.setDither(true);


The two lines following that should also be removed. They have nothing to check if the receiver wasn't modified.

flar · 2023-10-30T21:23:32Z

display_list/dl_paint.h

@@ -61,12 +61,6 @@ class DlPaint {
    return *this;
  }

-  bool isDither() const { return is_dither_; }
-  DlPaint& setDither(bool isDither) {
-    is_dither_ = isDither;


The field can be deleted as well.

flar · 2023-10-30T21:26:14Z

display_list/skia/dl_sk_conversions.cc

    sk_paint.setShader(ToSk(color_source));
+  } else {
+    sk_paint.setDither(false);


sk_paint is a brand new object allocated above with defaults (dither is false by default).

flar · 2023-10-30T21:30:58Z

display_list/skia/dl_sk_paint_dispatcher.h

    //
    // See https://github.com/flutter/flutter/issues/112498.
-    paint_.setDither(color_source_gradient_ && dither_);
+    paint_.setDither(color_source_gradient_);


This could be handled in setColorSource directly now without having to pass a flag to this method. (color_source_gradient_ field is obsolete compared to just setting the dither flag on the paint_ object)

flar · 2023-10-30T21:45:34Z

The failures in the rendering tests are likely due to setting up an sk_paint with a gradient, but not setting its dither flag (look for calls to setShader and make sure they set the dither flag appropriately).

The entire if (testP.uses_gradient()) { block which used to test the difference between dither and non-dither is likely obsolete now that the parameterization is not any different between the env.init_ref and the RenderWith calls.

matanlurey · 2023-10-30T23:12:36Z

Thanks @jonahwilliams and especially @flar!

I'm working on some MSAA stuff for the rest of the day, and I'll address feedback tomorrow AM.

flar

The latest changes look good. The failures are likely due to setting a gradient in the Skia code in the rendering tests without setting the dithering flag on the SkPaint. For example:

engine/display_list/testing/dl_rendering_unittests.cc

Line 1782 in 29bc90a

ctx.paint.setShader(sk_gradient);

I don't think any tests outside of the dl_rendering_unittests.cc will run into this, but these tests are measuring "DL output vs the equivalent SK commands" so any time a gradient is set, it has to also include setting the dither flag in the Sk version. There are a number of calls to setShader in this file, but it's not clear which ones are setting gradients or some other color source (the one I linked above is definitely setting a gradient).

Note that you can run the rendering tests manually to make sure you've caught all of these cases using out/<build>/display_list_rendertests

matanlurey · 2023-10-31T19:01:55Z

The latest changes look good. The failures are likely due to setting a gradient in the Skia code in the rendering tests without setting the dithering flag on the SkPaint. For example:

engine/display_list/testing/dl_rendering_unittests.cc

Line 1782 in 29bc90a

ctx.paint.setShader(sk_gradient);

I don't think any tests outside of the dl_rendering_unittests.cc will run into this, but these tests are measuring "DL output vs the equivalent SK commands" so any time a gradient is set, it has to also include setting the dither flag in the Sk version. There are a number of calls to setShader in this file, but it's not clear which ones are setting gradients or some other color source (the one I linked above is definitely setting a gradient).

Note that you can run the rendering tests manually to make sure you've caught all of these cases using out/<build>/display_list_rendertests

It looks like the failures are because of this change:

engine/display_list/skia/dl_sk_conversions.cc

Lines 46 to 56 in 7a2bdf8

    
           if (color_source) { 
        
             // On the Impeller backend, we will only support dithering of *gradients*, 
        
             // and it will be enabled by default (without the option to disable it). 
        
             // Until Skia support is completely removed, we only want to respect the 
        
             // dither flag for gradients (otherwise it will also apply to, for example, 
        
             // images, which is not supported in Impeller). 
        
             // 
        
             // See https://github.com/flutter/flutter/issues/112498. 
        
             sk_paint.setDither(color_source->isGradient()); 
        
             sk_paint.setShader(ToSk(color_source)); 
        
           }

... we're not checking for a requested dither before setting dithering (which is intended by this PR).

The part I'm confused on is what exactly to do about it given all of the wrapping in these tests. By adding some logging, I've observed that the following test case:

engine/display_list/testing/dl_rendering_unittests.cc

Lines 3432 to 3448 in 7a2bdf8

    
           TEST_F(DisplayListRendering, DrawImageRectLinear) { 
        
             SkRect src = SkRect::MakeIWH(kRenderWidth, kRenderHeight).makeInset(5, 5); 
        
             SkRect dst = kRenderBounds.makeInset(10.5, 10.5); 
        
             CanvasCompareTester::RenderAll(  // 
        
                 TestParameters( 
        
                     [=](const SkRenderContext& ctx) { 
        
                       ctx.canvas->drawImageRect( 
        
                           ctx.image, src, dst, SkImageSampling::kLinear,  // 
        
                           &ctx.paint, SkCanvas::kFast_SrcRectConstraint); 
        
                     }, 
        
                     [=](const DlRenderContext& ctx) {  // 
        
                       ctx.canvas->DrawImageRect( 
        
                           ctx.image, src, dst, DlImageSampling::kLinear,  // 
        
                           &ctx.paint, DlCanvas::SrcRectConstraint::kFast); 
        
                     }, 
        
                     kDrawImageRectWithPaintFlags)); 
        
           }

... only ends up setting the isGradient branch once:

$ ../out/host_debug_unopt_arm64/display_list_rendertests --gtest_filter="*ImageRectLinear"

INFO:test_timeout_listener.cc(76)] Test timeout of 300 seconds per test case will be enforced.
Note: Google Test filter = *ImageRectLinear
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from DisplayListRendering
[INFO:dl_rendering_unittests.cc(2822)] Running tests on [ Software ]
[ RUN      ] DisplayListRendering.DrawImageRectLinear
[INFO:dl_rendering_unittests.cc(213)] No Impeller output tests on Software
[ERROR:flutter/display_list/skia/dl_sk_conversions.cc(50)] color_source->isGradient() = 0
[ERROR:flutter/display_list/skia/dl_sk_conversions.cc(50)] color_source->isGradient() = 0
[ERROR:flutter/display_list/skia/dl_sk_conversions.cc(50)] color_source->isGradient() = 0
[ERROR:flutter/display_list/skia/dl_sk_conversions.cc(50)] color_source->isGradient() = 0
[ERROR:flutter/display_list/skia/dl_sk_conversions.cc(50)] color_source->isGradient() = 0
[ERROR:flutter/display_list/skia/dl_sk_conversions.cc(50)] color_source->isGradient() = 0
[ERROR:flutter/display_list/skia/dl_sk_conversions.cc(50)] color_source->isGradient() = 0
[ERROR:flutter/display_list/skia/dl_sk_conversions.cc(50)] color_source->isGradient() = 0
[ERROR:flutter/display_list/skia/dl_sk_conversions.cc(50)] color_source->isGradient() = 0
[ERROR:flutter/display_list/skia/dl_sk_conversions.cc(50)] color_source->isGradient() = 0
[ERROR:flutter/display_list/skia/dl_sk_conversions.cc(50)] color_source->isGradient() = 0
[ERROR:flutter/display_list/skia/dl_sk_conversions.cc(50)] color_source->isGradient() = 1
[ERROR:flutter/display_list/testing/dl_rendering_unittests.cc(2619)] e020bf00 != e01fbf00
[ERROR:flutter/display_list/testing/dl_rendering_unittests.cc(2619)] d828af00 != d827af00
[ERROR:flutter/display_list/testing/dl_rendering_unittests.cc(2619)] 97692f00 != 97682f00
[ERROR:flutter/display_list/testing/dl_rendering_unittests.cc(2619)] e817cf00 != e818cf00
[ERROR:flutter/display_list/testing/dl_rendering_unittests.cc(2619)] a7584f00 != a7594f00
../../flutter/display_list/testing/dl_rendering_unittests.cc:2626: Failure
Expected equality of these values:
  pixels_different
    Which is: 992
  0
Software: DrawImageRectLinear (LinearGradient GYB) (DlCanvas output matches SkCanvas)
[ERROR:flutter/display_list/testing/dl_rendering_unittests.cc(2619)] e020bf00 != e01fbf00
[ERROR:flutter/display_list/testing/dl_rendering_unittests.cc(2619)] d828af00 != d827af00
[ERROR:flutter/display_list/testing/dl_rendering_unittests.cc(2619)] 97692f00 != 97682f00
[ERROR:flutter/display_list/testing/dl_rendering_unittests.cc(2619)] e817cf00 != e818cf00
[ERROR:flutter/display_list/testing/dl_rendering_unittests.cc(2619)] a7584f00 != a7594f00
../../flutter/display_list/testing/dl_rendering_unittests.cc:2626: Failure
Expected equality of these values:
  pixels_different
    Which is: 992
  0
Software: DrawImageRectLinear (LinearGradient GYB) (DlCanvas DL output matches Builder Dl output)
../../flutter/display_list/testing/dl_rendering_unittests.cc:2702: Failure
Expected equality of these values:
  pixels_different
    Which is: 992
  0
Software: DrawImageRectLinear (LinearGradient GYB) (DisplayList built directly -> surface)
[  FAILED  ] DisplayListRendering.DrawImageRectLinear (2668 ms)
[----------] 1 test from DisplayListRendering (2668 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (2668 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] DisplayListRendering.DrawImageRectLinear

That means in order to be consistent, I'd have to set ctx.paint.setDither(true) exactly once, but I'm not sure how to do that because of the wrapped invocations. I'm guessing it's one of these inner calls:

engine/display_list/testing/dl_rendering_unittests.cc

Lines 1288 to 1297 in 7a2bdf8

    
           SkSetup sk_backdrop_setup = [=](const SkSetupContext& ctx) { 
        
             SkPaint setup_p; 
        
             setup_p.setShader(MakeColorSource(ctx.image)); 
        
             ctx.canvas->drawPaint(setup_p); 
        
           }; 
        
           DlSetup dl_backdrop_setup = [=](const DlSetupContext& ctx) { 
        
             DlPaint setup_p; 
        
             setup_p.setColorSource(MakeColorSource(ctx.image)); 
        
             ctx.canvas->DrawPaint(setup_p); 
        
           };

... but I'm not sure which one, yet.

matanlurey · 2023-10-31T19:10:10Z

... but I'm not sure which one, yet.

I assumed it was "LinearGradient GYB", I'm not familiar with this test suite though:

        RenderWith(testP, env, tolerance,
                   CaseParameters(
                       "LinearGradient GYB",
                       [=](const SkSetupContext& ctx) {
+                        ctx.paint.setDither(true);
                         ctx.paint.setShader(sk_gradient);
                       },
                       [=](const DlSetupContext& ctx) {
                         ctx.paint.setColorSource(dl_gradient);
                       }));

This didn't actually fix the failure, though commenting out the { ... } clause does show this is the offending case. I'll keep digging/playing around.

matanlurey · 2023-10-31T19:17:02Z

Added some more logging:

[ERROR:flutter/display_list/skia/dl_sk_conversions.cc(50)] color_source->isGradient() = 0
[ERROR:flutter/display_list/testing/dl_rendering_unittests.cc(1782)] dl.setColorSource(gradient)
[ERROR:flutter/display_list/skia/dl_sk_conversions.cc(50)] color_source->isGradient() = 1
[ERROR:flutter/display_list/testing/dl_rendering_unittests.cc(1782)] dl.setColorSource(gradient)

It appears that isGradient() either isn't covering a case, or there is something I misunderstand.

flar · 2023-10-31T19:18:37Z

I'll bring over the branch and look.

matanlurey · 2023-10-31T19:22:50Z

Note that the following patch also fails, so maybe it's something else:

        RenderWith(testP, env, tolerance,
                   CaseParameters(
                       "LinearGradient GYB",
                       [=](const SkSetupContext& ctx) {
+                        ctx.paint.setDither(dl_gradient->isGradient());
                         ctx.paint.setShader(sk_gradient);
                       },
                       [=](const DlSetupContext& ctx) {
                         ctx.paint.setColorSource(dl_gradient);
                       }));

flar · 2023-10-31T19:37:02Z

Ah, I see the problem. Just because we set a gradient in the sk_paint doesn't mean the operation will draw a gradient. The drawImage family of rendering calls ignores the shader so the dither flag will affect it even though it is not drawing a gradient - thus causing a difference. Let me look at a better solution here.

matanlurey · 2023-10-31T19:38:35Z

Ah, I see the problem. Just because we set a gradient in the sk_paint doesn't mean the operation will draw a gradient. The drawImage family of rendering calls ignores the shader so the dither flag will affect it even though it is not drawing a gradient - thus causing a difference. Let me look at a better solution here.

Sounds good, let me know how I can help!

flar · 2023-10-31T21:18:28Z

display_list/skia/dl_sk_paint_dispatcher.h

+    // and it will be enabled by default (without the option to disable it).
+    // Until Skia support is completely removed, we only want to respect the
+    // dither flag for gradients (otherwise it will also apply to, for
+    // example, image ops and image sources, which are not supported in


Oops, should probably say "not dithered in" otherwise it looks like we don't implement drawImage...

…ine#46750)

jonahwilliams · 2023-11-01T01:03:27Z

There were some golden diffs on an engine roll that I suspect were due to this change. i accepted them to unblock the roll, but they can be seen here:

https://flutter-gold.skia.org/search?crs=github&issue=137651&patchsets=5&positive=true

…137651) flutter/engine@406b7d7...db06c2e 2023-10-31 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from rg_Ici1tgAVF93cn9... to x2NzYMNhyodIwDl0I... (flutter/engine#47528) 2023-10-31 30870216+gaaclarke@users.noreply.github.com [Impeller] updated ios benchmark link (flutter/engine#47515) 2023-10-31 30870216+gaaclarke@users.noreply.github.com Made clang tidy use arm64 if on an arm64 mac. (flutter/engine#47494) 2023-10-31 bdero@google.com [Impeller] Allow 3D scenes to render when MSAA is not supported. (flutter/engine#47217) 2023-10-31 matanlurey@users.noreply.github.com Surgically remove `.*dither.*` from the Engine (flutter/engine#46750) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from rg_Ici1tgAVF to x2NzYMNhyodI If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

matanlurey added 3 commits October 10, 2023 17:45

WIP.

3a5678b

More WIP.

b27c9df

Finish removing.

543b2ae

github-actions bot added the e: impeller label Oct 11, 2023

chinmaygarde removed the e: impeller label Oct 11, 2023

Merge branch 'main' into engine-removeSetDither

879c6a1

github-actions bot added the e: impeller label Oct 30, 2023

matanlurey added 2 commits October 30, 2023 13:42

Remove removed isDither call for << operator.

8f0a989

Remove unused field.

e17ddfe

matanlurey requested review from jonahwilliams and flar October 30, 2023 21:07

matanlurey marked this pull request as ready for review October 30, 2023 21:08

matanlurey added 2 commits October 30, 2023 14:12

Finish cleanup.

9a40ed4

Format.

8fc1180

matanlurey changed the title ~~Surgically remove .*dither.* from non-fragment code in the Engine~~ Surgically remove .*dither.* from the Engine Oct 30, 2023

jonahwilliams approved these changes Oct 30, 2023

View reviewed changes

flar suggested changes Oct 30, 2023

View reviewed changes

Address comments.

7a2bdf8

matanlurey requested a review from flar October 31, 2023 17:20

flar approved these changes Oct 31, 2023

View reviewed changes

factor operation type into whether to set dither flag

9dc5f11

flar reviewed Oct 31, 2023

View reviewed changes

matanlurey added 2 commits October 31, 2023 14:37

Tweak but still failing.

0907ecd

Tweak comment.

17031b9

matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 31, 2023

auto-submit bot merged commit fe3882e into flutter:main Oct 31, 2023

matanlurey deleted the engine-removeSetDither branch October 31, 2023 22:44

engine-flutter-autoroll mentioned this pull request Nov 1, 2023

Roll Flutter Engine from 406b7d71ca37 to db06c2e10459 (5 revisions) flutter/flutter#137651

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 1, 2023

fe3882e03 Surgically remove .*dither.* from the Engine (flutter/eng…

c50388f

…ine#46750)

Surgically remove .*dither.* from the Engine #46750

Surgically remove .*dither.* from the Engine #46750

Uh oh!

Conversation

matanlurey commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

flar Oct 30, 2023

Choose a reason for hiding this comment

Uh oh!

flar Oct 30, 2023

Choose a reason for hiding this comment

Uh oh!

flar Oct 30, 2023

Choose a reason for hiding this comment

Uh oh!

flar Oct 30, 2023

Choose a reason for hiding this comment

Uh oh!

flar commented Oct 30, 2023

Uh oh!

matanlurey commented Oct 30, 2023

Uh oh!

flar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matanlurey commented Oct 31, 2023

Uh oh!

matanlurey commented Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matanlurey commented Oct 31, 2023

Uh oh!

flar commented Oct 31, 2023

Uh oh!

matanlurey commented Oct 31, 2023

Uh oh!

flar commented Oct 31, 2023

Uh oh!

matanlurey commented Oct 31, 2023

Uh oh!

flar Oct 31, 2023

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Nov 1, 2023

Uh oh!

Uh oh!

Surgically remove `.dither.` from the Engine #46750

Surgically remove `.dither.` from the Engine #46750

matanlurey commented Oct 11, 2023 •

edited

Loading

flar left a comment •

edited

Loading

matanlurey commented Oct 31, 2023 •

edited

Loading