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

[Skia] Only respect ui.Paint.dither when the colorSource is a gradient #44730

Merged
merged 14 commits into from
Aug 16, 2023

Conversation

matanlurey
Copy link
Contributor

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, i.e.

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.

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.

LGTM

@jonahwilliams jonahwilliams requested a review from flar August 15, 2023 18:21
@matanlurey matanlurey force-pushed the engine-sk-dither-guard branch from 8f8e630 to a1e1fe7 Compare August 15, 2023 19:25
@matanlurey matanlurey self-assigned this Aug 15, 2023
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.

Suggested changes.

(Though I really dislike the concept of "ForSuchAndSuchBackend" when describing a basic property of an object. Really, it's just identifying that a color source is some kind of gradient...)

display_list/skia/dl_sk_canvas.cc Outdated Show resolved Hide resolved
display_list/dl_paint.h Outdated Show resolved Hide resolved
@matanlurey
Copy link
Contributor Author

Though I really dislike the concept of "ForSuchAndSuchBackend" when describing a basic property of an object. Really, it's just identifying that a color source is some kind of gradient...

Fair enough! I wanted to make sure I (and nobody else) felt like it was anything more than a hack for a specific backend, but I don't have a strong feeling about the name specifically.

@matanlurey matanlurey force-pushed the engine-sk-dither-guard branch from d0d85ef to af290e9 Compare August 15, 2023 19:55
@matanlurey matanlurey requested a review from flar August 15, 2023 20:26
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.

Should remove dead code.

display_list/skia/dl_sk_canvas.cc Outdated Show resolved Hide resolved
display_list/dl_paint.h Outdated Show resolved Hide resolved
@matanlurey matanlurey requested a review from flar August 15, 2023 21:23
ci/licenses_golden/excluded_files Outdated Show resolved Hide resolved
display_list/skia/dl_sk_canvas.cc Outdated Show resolved Hide resolved
@matanlurey matanlurey requested a review from flar August 15, 2023 22:28
display_list/skia/dl_sk_canvas.cc Outdated Show resolved Hide resolved
display_list/skia/dl_sk_conversions_unittests.cc Outdated Show resolved Hide resolved
@matanlurey matanlurey force-pushed the engine-sk-dither-guard branch from f504834 to 2ccbf58 Compare August 15, 2023 23:11
@matanlurey matanlurey requested a review from flar August 15, 2023 23:11
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

(technically, ToSk(paint) could be moved to the conversions file. I'm not sure why that appears on its own (and I wrote this))

@matanlurey matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 15, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Aug 16, 2023

auto label is removed for flutter/engine/44730, due to - The status or check suite Linux mac_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 16, 2023
@matanlurey matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 16, 2023
@auto-submit auto-submit bot merged commit 0922302 into flutter:main Aug 16, 2023
@matanlurey matanlurey deleted the engine-sk-dither-guard branch August 16, 2023 01:18
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 16, 2023
fluttermirroringbot pushed a commit to flutter/flutter that referenced this pull request Aug 16, 2023
…132618)

flutter/engine@659cdfc...decaccf

2023-08-16 [email protected] Roll Fuchsia Mac SDK from 7iuIq3PsSkuCmuEMr... to Zp9or9YwxZHHPeQbA... (flutter/engine#44747)
2023-08-16 [email protected] Move cache builders to prod. (flutter/engine#44739)
2023-08-16 [email protected] Enabling the host application to control the timing of attaching the |FlutterView| to the engine (flutter/engine#43595)
2023-08-16 [email protected] Roll Skia from 9fc1c628456a to 3d2b84e28e79 (1 revision) (flutter/engine#44746)
2023-08-16 [email protected] Roll Dart SDK from b36934aae968 to e6bf503b36fe (1 revision) (flutter/engine#44745)
2023-08-16 [email protected] [Skia] Only respect ui.Paint.dither when the colorSource is a gradient (flutter/engine#44730)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from 7iuIq3PsSkuC to Zp9or9YwxZHH

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
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.
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.

3 participants