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

Further filter/clear <SkPaint>.setDither(true), this time in DlSkPaintDispatchHelper #44912

Merged

Conversation

matanlurey
Copy link
Contributor

@matanlurey matanlurey commented Aug 21, 2023

Closes flutter/flutter#132860.

  • If setDither(true) is called, and an existing setColorSource is a gradient, it is ignored.
  • If setColorSource(...) is called, and it is a gradient, and dithering was previously set, it is cleared.

I'm not sure this is fool proof.

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM

@matanlurey
Copy link
Contributor Author

This doesn't actually work as written :(

It's possible for code to run the following (otherwise valid) branch:

paint.setDither(true);
...
paint.setColorSource(aGradient);

With this PR, setDither is discarded (the last known colorSource was not a gradient).

Let me try to understand the lifecycle of this class better, maybe there is a better place to do this kind of check?

@jonahwilliams
Copy link
Member

Ahh that makes sense. You'd probably need to intercept the dithering call when it is being converted into an SkPaint.

@matanlurey
Copy link
Contributor Author

I think <DlSkPaintDispatchHelper>.paint is the whole integration point that makes sense:

const SkPaint& paint() { return paint_; }

... and could be used to clear dithering if a color source of a gradient was not provided.

@matanlurey matanlurey force-pushed the engine-dither-skia-dispatch-ignore branch from 8a5b0cb to e6a5b7b Compare August 21, 2023 23:52
@matanlurey
Copy link
Contributor Author

Appears to be all good now! Yay!

display_list/skia/dl_sk_paint_dispatcher.h Outdated Show resolved Hide resolved
@matanlurey matanlurey requested a review from flar August 22, 2023 16:22
display_list/skia/dl_sk_paint_dispatcher.h Outdated Show resolved Hide resolved
@matanlurey matanlurey requested a review from flar August 22, 2023 21:22
Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@matanlurey matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 22, 2023
@auto-submit auto-submit bot merged commit 0a3cb0c into flutter:main Aug 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023
zanderso pushed a commit to flutter/flutter that referenced this pull request Aug 23, 2023
…sions) (#133106)

Manual roll requested by [email protected]

flutter/engine@b190f90...7d56840

2023-08-23 [email protected] Revert "Roll Dart SDK from
ab417bc74bb1 to c162b4979562 (1 revision)" (flutter/engine#44989)
2023-08-23 [email protected] Roll Fuchsia Mac SDK from
G25oJMO5jbUi-UN4F... to DoQ8KUxSk-5EU6VQ1... (flutter/engine#44988)
2023-08-23 [email protected] Revert "Make `FontWeight`
an enum, Remove unused text classes" (flutter/engine#44987)
2023-08-23 [email protected] Roll Dart SDK from
ab417bc74bb1 to c162b4979562 (1 revision) (flutter/engine#44986)
2023-08-23 [email protected] Roll Fuchsia Linux SDK from
kKI09su99b0AKs8b3... to VSvpNFoFjqXIQTcs6... (flutter/engine#44984)
2023-08-23 [email protected] Enable clang-tidy for
pre-push (opt-out), exclude `performance-unnecessary-value-param`
(flutter/engine#44936)
2023-08-22 [email protected] Restore old SurfaceTextureExternal
drawing code (flutter/engine#44979)
2023-08-22 [email protected] Roll Skia from d0918de21c1a to
aa208c8a2d60 (2 revisions) (flutter/engine#44981)
2023-08-22 [email protected] Initialize the texture
destruction callback in the Metal embedder test harness
(flutter/engine#44973)
2023-08-22 [email protected] Further filter/clear
`<SkPaint>.setDither(true)`, this time in `DlSkPaintDispatchHelper`
(flutter/engine#44912)
2023-08-22 [email protected] Roll Dart SDK from
3ebf0fedfceb to ab417bc74bb1 (1 revision) (flutter/engine#44977)
2023-08-22 [email protected] Roll Skia from bf6019be75ef to
d0918de21c1a (3 revisions) (flutter/engine#44975)
2023-08-22 [email protected] Roll Skia from c675298ddeda to
bf6019be75ef (3 revisions) (flutter/engine#44974)
2023-08-22 [email protected] Make
`FontWeight` an enum, Remove unused text classes (flutter/engine#44960)
2023-08-22 [email protected] Roll Skia from 9f4b81aac175 to
c675298ddeda (2 revisions) (flutter/engine#44971)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from kKI09su99b0A to VSvpNFoFjqXI
  fuchsia/sdk/core/mac-amd64 from G25oJMO5jbUi to DoQ8KUxSk-5E

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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023
gaaclarke pushed a commit to gaaclarke/engine that referenced this pull request Aug 30, 2023
…aintDispatchHelper` (flutter#44912)

Closes flutter/flutter#132860.

- If `setDither(true)` is called, and an existing `setColorSource` is a gradient, it is ignored.
- If `setColorSource(...)` is called, and it is a gradient, and dithering was previously set, it is cleared.

I'm not sure this is fool proof.
matanlurey added a commit that referenced this pull request Sep 1, 2023
…enableDithering. (#44705)

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](https://github.com/search?q=%22Paint.enableDithering%22+language%3ADart&type=code&l=Dart),
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Despite #44730, Skia is still dithering non-gradients
3 participants