Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update (flipping the default from false -> true) and deprecate Paint.enableDithering. #44705

Merged

Conversation

matanlurey
Copy link
Contributor

@matanlurey matanlurey commented Aug 14, 2023

Update: Blocked on #44912 landing, and merging into google3.


Partial work towards flutter/flutter#112498.

tl;dr: In Impeller's backend we intend to always dither gradients, and never allow any changes long-term (i.e., write your own shader if you want different behavior) with the assumption that dithered gradients look better most of the time, and don't typically hurt elsewhere.

Note that, at the time of this writing, I couldn't find a single case of this being set explicitly to true inside Google, and there are between 100 and 200 publicly accessible on GitHub, of which virtually all are setting it explicitly to true.

There are some (valid) concerns this would cause a lot of golden-file image diffs, so I'm going to seek explicit approval from the Google testing team as well as someone from the framework team before landing this commit.

@matanlurey
Copy link
Contributor Author

It appears this will break at least 1 framework test:

flutter/test/painting/text_style_test.dart: backgroundColor [E]                                                                                   
  Expected: contains 'background: Paint(Color(0xff00ff00))'
    Actual: 'TextStyle(color: unspecified, decoration: unspecified, decorationColor: unspecified, decorationStyle: unspecified, decorationThickness: unspecified, fontWeight: unspecified, fontStyle: unspecified, textBaseline: unspecified, fontFamily: unspecified, fontFamilyFallback: unspecified, fontSize: unspecified, letterSpacing: unspecified, wordSpacing: unspecified, height: unspecified, leadingDistribution: unspecified, locale: unspecified, background: Paint(Color(0xff00ff00); dither: true), foreground: unspecified, shadows: unspecified, fontFeatures: unspecified, fontVariations: unspecified)'
     Which: does not contain 'background: Paint(Color(0xff00ff00))'
  
  package:matcher                                     expect
  package:flutter_test/src/widget_tester.dart 458:18  expect
  test/painting/text_style_test.dart 462:5            main.<fn>

... and a number of engine tests will need to be updated. One example:

flutter/shell/platform/embedder/tests/embedder_gl_unittests.cc:1469: Failure
Value of: ImageMatchesFixture( FixtureNameForBackend(backend, "gradient.png"), rendered_scene)
  Actual: false
Expected: true
�[0;31m[  FAILED  ] �[mEmbedderTestGlVk/EmbedderTestMultiBackend.CanRenderGradientWithCompositorOnNonRootLayer/1, where GetParam() = 4-byte object <03-00 00-00> (15612 ms)
�[0;32m[----------] �[m1 test from EmbedderTestGlVk/EmbedderTestMultiBackend (15613 ms total)

�[0;32m[----------] �[mGlobal test environment tear-down
�[0;32m[==========] �[m1 test from 1 test suite ran. (15614 ms total)
�[0;32m[  PASSED  ] �[m0 tests.
�[0;31m[  FAILED  ] �[m1 test, listed below:
�[0;31m[  FAILED  ] �[mEmbedderTestGlVk/EmbedderTestMultiBackend.CanRenderGradientWithCompositorOnNonRootLayer/1, where GetParam() = 4-byte object <03-00 00-00>

A couple of options:

  1. I could hardcode this off in a test environment(s)
  2. I could update the tests to be less brittle, i.e. allow dithering/check less pixels
  3. I could update the tests to always assume dithering

Thoughts?

@matanlurey matanlurey self-assigned this Aug 15, 2023
auto-submit bot pushed a commit that referenced this pull request Aug 16, 2023
#44730)

In the Impeller backend, we **only** support dithering of _gradients_. In addition, it will be the default (and only option).

In the [process of enabling dithering by default](#44705), i.e.
```diff
class Paint {
-  static bool enableDithering = false;
+  static bool enableDithering = true;
}
```

... we realized with internal Google testing this will now apply dithering on more than just gradients, i.e. images in the Skia backend. Since we won't support dithering of images in the Impeller backend, this PR gives a "hint" on whether the `colorSource` (if one is set) can be dithered by the contrived rules we've created.
@chinmaygarde
Copy link
Member

This will definitely blow up Scubas. But if we think this is the right change, we shouldn't let the pain of updating Scubas stop us. @CaseyHillers indicated that he has a workflow for dealing with a lot of Scubas. And it makes sense to exercise that playbook on a non-on-fire change.

@CaseyHillers
Copy link
Contributor

Preferably, can we update this outside of the roll process? @LongCatIsLooong recently did this with a build flag, and was able to update all the scubas without blocking the roll.

@matanlurey
Copy link
Contributor Author

@chinmaygarde:

This will definitely blow up Scubas

After disabling Skia image diffing, I'm actually not sure it will anymore. There are virtually no gradients internally.

@CaseyHillers:

Preferably, can we update this outside of the roll process?

Yes. I'm going to make sure google3 looks green (or at least, 99% green) before I land this.

@chinmaygarde
Copy link
Member

Looks like there was a blocker that is progressing. Just wanted to unblock this.

@matanlurey
Copy link
Contributor Author

Blocked by flutter/flutter#133375.

gaaclarke pushed a commit to gaaclarke/engine that referenced this pull request Aug 30, 2023
flutter#44730)

In the Impeller backend, we **only** support dithering of _gradients_. In addition, it will be the default (and only option).

In the [process of enabling dithering by default](flutter#44705), i.e.
```diff
class Paint {
-  static bool enableDithering = false;
+  static bool enableDithering = true;
}
```

... we realized with internal Google testing this will now apply dithering on more than just gradients, i.e. images in the Skia backend. Since we won't support dithering of images in the Impeller backend, this PR gives a "hint" on whether the `colorSource` (if one is set) can be dithered by the contrived rules we've created.
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Aug 30, 2023
See flutter/engine#44705 where it fails a framework smoke test.

Partial work towards #112498.
@matanlurey matanlurey merged commit 489c399 into flutter:main Sep 1, 2023
@matanlurey matanlurey deleted the engine-deprecate-paint-enableDithering branch September 1, 2023 17:20
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 1, 2023
fluttermirroringbot pushed a commit to flutter/flutter that referenced this pull request Sep 2, 2023
…133879)

flutter/engine@d00b69a...489c399

2023-09-01 [email protected] Update (flipping the default from false -> true) and deprecate Paint.enableDithering. (flutter/engine#44705)
2023-09-01 [email protected] [Impeller] EntityPass::Clone needs to clone harder (flutter/engine#45313)
2023-09-01 [email protected] Reland "ios: remove shared_application and support app extension build #44732" (flutter/engine#45351)

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 [email protected],[email protected],[email protected] 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants