Add ability to control dithering on Paint

[ds84182]

Closes flutter/flutter#44134

web_ui does not implement dithering, since it's more of a hint to the underlying renderer.

Can test with the following code snippet:

import 'dart:typed_data';
import 'dart:ui';

void onFrame(_) {
  final pictureRecorder = PictureRecorder();
  final canvas = Canvas(pictureRecorder);
  canvas.drawPaint(Paint()
    ..dither = true
    ..shader = Gradient.linear(Offset.zero,
        Offset(window.physicalSize.width, window.physicalSize.height), [
          Color(0xFF4C4D52),
          Color(0xFF202124),
        ]));
  final picture = pictureRecorder.endRecording();
  final sceneBuilder = SceneBuilder()
    ..pushTransform(Float64List(16)
      ..[0] = 1.0
      ..[5] = 1.0
      ..[10] = 1.0
      ..[15] = 1.0)
    ..addPicture(Offset.zero, picture)
    ..pop();
  final scene = sceneBuilder.build();

  window.render(scene);

  window.scheduleFrame();
}

void main() {
  window.onBeginFrame = onFrame;

  window.scheduleFrame();
}

Without dithering, the banding is very obvious on all devices. With dithering, the banding disappears completely.

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[Hixie]

I'd rather we didn't expose this. We should just make the gradients look good regardless. We shouldn't need the developer to control this.

[Hixie]

if we do expose this, we should implement it on web.
if we do expose this and don't implement it on web, we should still make the property mutable, so that code that relies on it mutating doesn't randomly break on web.

[ds84182]

@Hixie

I'd rather we didn't expose this. We should just make the gradients look good regardless. We shouldn't need the developer to control this.

I agree, however it might be a large change to do. It doesn't seem to break hard-stop gradients (like those used for the overflow indicator).

if we do expose this, we should implement it on web

I somewhat agree, but maybe the name of the property should be changed to ditherHint. The main issue is that the dithering flag is just a hint, and Skia will choose if it should dither or not. This also extends to web browsers, although I'm not 100% sure because I don't think Chrome will like being run on an RGB565 display but I'm willing to bet dithering gets enabled then.

If not supporting it on web is fine, I can add the backing field to web_ui Paint. The documentation already explains that the hint may be ignored.

[ds84182]

After some discussion I've moved over to a static field so it can be enabled and disabled globally. The dither property has been made private for now, and it defaults to the value of Paint.enableDithering at construction time. Apps that want to opt into dithered gradients should set Paint.enabledDithering before they do any drawing.

Still isn't enabled by default, but enabling it by default shouldn't have noticeable performance impacts. Most of the concern comes from whether having non-dithered gradients is a valid usecase.

[Hixie]

This will need tests.

I assume the new global is temporary? Can we mark it as deprecated? What's the timeframe by which we plan to flip the default and remove it?

[ds84182]

@Hixie

This will need tests.

Haven't added tests to the engine repository before. Where should the test live? I was browsing around for paint related tests, but I didn't see any.

I assume the new global is temporary? Can we mark it as deprecated? What's the timeframe by which we plan to flip the default and remove it?

The global value is temporary, since you would either want to opt in entirely or opt out entirely. However, we don't know if this is the right default behavior to have. Initially, it would be great to initialize this to true in PaintingBinding.initInstances on the master branch to estimate impact, and if apps need to opt-out they can set the value to false after their call to runApp.

But as of right now, the whole issue is finding out if apps rely on non-dithered gradients, and giving their developers an avenue to reach out if a default behavior change (which is a breaking change) needs a better, more permanent, escape hatch.

This may also have a very very slight performance impact for gradients heavy apps on less-powerful devices, if used by default. It isn't a big deal for low-end Android devices, but there might be smaller near-embedded devices (like Raspberry Pi) that might be affected by the change.

[Hixie]

@liyuqian @cbracken can you help @ds84182 with writing tests for this? Thanks!

But as of right now, the whole issue is finding out if apps rely on non-dithered gradients, and giving their developers an avenue to reach out if a default behavior change (which is a breaking change) needs a better, more permanent, escape hatch.

Seems reasonable.

This may also have a very very slight performance impact for gradients heavy apps on less-powerful devices, if used by default. It isn't a big deal for low-end Android devices, but there might be smaller near-embedded devices (like Raspberry Pi) that might be affected by the change.

Presumably gradient-heavy apps are the ones most likely to want dithering...

I wouldn't worry about the lower-end devices, we don't support those anyway. The Nest Home Hub is basically the lowest-end device I would worry about, as far as this feature goes.

[liyuqian]

@ds84182 : maybe put your test in https://github.com/flutter/engine/blob/master/testing/dart/canvas_test.dart ? There's already some Dart code that exercises Paint creation, Canvas drawing, and even image comparison.

[ds84182]

Added two image comparison tests, PTAL

[liyuqian]

LGTM! The dithered gradient golden image looks great to me. Can't wait to actually see them in the Flutter app on my phone. Currently the circular gradient animation of loading images does look quite banded. Any ETA on when I can see them in your app? 😃

[Hixie]

this should be unnecessary, FWIW. The constructor is normally implied.

[ds84182]

When it was omitted one of the web UI lints complained

[liyuqian]

Remove the one in lib/ui/painting.dart too and the lint should go away.

[ds84182]

That constructor can't be removed because its used to set _dither to true if enableDithering is true. I could use cascade syntax on _data's initializer, but readability suffers a little.

[liyuqian]

I see. Ping @jonahwilliams and @yjbanov to see if the web tools (the lint) can be relaxed in this case. If not, I'm Ok with just having this dummy constructor. My experience is that there's already a lot of dummy/duplicate code in lib/web_ui. The ideal future would be that we don't need any duplicate code in lib/web_ui, and this issue would go away naturally.

[yjbanov]

Let's keep this one. We are splitting the implementation between HTML and CanvasKit (#14320), and this constructor is becoming a factory, which we cannot omit.

[lukepighetti]

What’s the status of this PR? I’m having banding issues on an app I’m working on. It’s very noticeable when you start dealing with gradients based on blacks. I agree that gradients should just look good from the start but they clearly don’t and so dithering would be an ideal shim especially since it appears to be low hanging fruit.

[liyuqian]

@lukepighetti : this has been rolled into Flutter framework on flutter/flutter@bcacd06. It has not made to the last stable release, but it's in our beta (v1.14.6) and master branches.