Set AA flag for painting images - #51656

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@Piinks : I'm expecting to see some golden image differences for this change. If somehow there's no golden test that covers a rotated image, can you please create a PR that adds such a golden test? #30710 (comment) has a sample code.

First glance: I don't think we have any golden file tests that render actual images. Is that pertinent to the test you are looking for?

Are there any negative implications from always setting this to true? Should it be configurable?

@Piinks : an dummy image (e.g., all blue pixels) would be sufficient to test the AA behavior.

@goderbauer : I'd expect the performance impact to be similar to drawing a rect with or without AA. Most of our widgets (e.g., Container) would draw something (paths, rects, rrects...) in AA by default without any configurable switches. So I don't think adding an AA switch here is critical. Of course, making it configurable would be better if we ignore the API maintenance cost (which I don't think there's much). CC @Hixie to see if we'd like to expose the AA switch on an Image widget.

@Piinks : an dummy image (e.g., all blue pixels) would be sufficient to test the AA behavior.

Ok, are you wanting to have a test land on master first so you can verify there is a change? Otherwise, you can add the test as part of this change.

you can add the test as part of this change.

Actually that may be best to include in this change, rather than checking in a test that may or may not be affected by this.

@Piinks : yes, it would be nice if you can land a PR before this PR gets merged so we'll see the difference!

@Piinks : yes, it would be nice if you can land a PR before this PR gets merged so we'll see the difference!

And if it doesn't make a difference? I would recommend adding the test as part of this change. You can test locally that the bug is reproduced and see the visual diff with your patch applied to confirm. That seems inline with write test fix bug ?

@Piinks : sure, I'll do that. That probably would take a few days as I also need to clear some other tasks in the queue first. Meanwhile, I think a separate PR may also make sense as the test should reveal an aliased rectangle (or it shouldn't be merged), and this PR should fix that aliasing in presubmits (or this PR shouldn't be merged).

It would probably be worth checking if this impacts some golden test in Google.

If so, this is a breaking change, and I believe we'd have to expose a flag for it so it can be rolled into Google without manual intervention.

Thank you @dnfield for the remind! I'm curious what the current procedure is for those AA changes. For example, if a Skia roll changes AA and require golden image updates from Google, does that constitute a breaking change, and how do we roll it automatically? My old experience (which might be obsolete now) is to just approve those golden changes along the Google roll, and not file any breaking change announcements...

@Hixie would know but I believe we treat skia breakages differently.

I think this change is definitely worth making to be clear

For some reason it looks like the public copy of the breaking change policy (https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes) doesn't actually mention this, but the intent is that upstream dependencies (and PRs that roll those dependencies) aren't beholden to our breaking change policy currently.

@liyuqian I have an idea. This #30710 problem only occurs when we rotate (or skew) the image， not translate or scale. We can check the matrix of the canvas first, and turn on anti-aliasing only when there is a rotation (or skew) operation. The same can be applied to rectangle decoration, also can fix #14288.

I find code in SkMatrix.h , as follows

enum TypeMask {
        kIdentity_Mask    = 0,    //!< identity SkMatrix; all bits clear
        kTranslate_Mask   = 0x01, //!< translation SkMatrix
        kScale_Mask       = 0x02, //!< scale SkMatrix
        kAffine_Mask      = 0x04, //!< skew or rotate SkMatrix
        kPerspective_Mask = 0x08, //!< perspective SkMatrix
    };
// ...
 bool isTranslate() const { return !(this->getType() & ~(kTranslate_Mask)); }

We can add a function as follows:

bool isSkewOrRotate() const {return this->getType() & kAffine_Mask;}

@linxuebin1990 : thank you for the suggestion! Unfortunately, even if we only enable AA with rotation, it's might still be a breaking change according to our current policy.

Nevertheless, I think the SkMatrix::isSkewOrRotate might be helpful regardless of how we handle the PR here. Please feel free to propose that to https://groups.google.com/g/skia-discuss.

@liyuqian Did you have a chance to add a golden test for this?

@goderbauer @Piinks : I'm having some troubles with Image golden test. Please see the newest commit. I can see a nonempty golden image if ui.Image is used, but not when Image.memory is used, although they use the same bytes.

Gold has detected one or more untriaged digests on patchset 2.
Please triage them at https://flutter-gold.skia.org/search?issue=51656.

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

Changes to golden files are considered breaking changes, so consult Handling Breaking Changes to proceed. While there are exceptions to this rule, if this patch modifies an existing golden file, it is probably not an exception. Only new golden file tests, or downstream changes like those from skia updates are considered non-breaking.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #51656 at sha c509f684408e55f0b5dafa453562dc7ddb6673bf

@goderbauer @Piinks : I'm having some troubles with Image golden test. Please see the newest commit. I can see a nonempty golden image if ui.Image is used, but not when Image.memory is used, although they use the same bytes.

IIRC I've seen people design golden file tests with images by precaching them, let me find an example.

Golden file changes remain available for triage from new commit, Click here to view.

Changes reported for pull request #51656 at sha a35584d

@hterkelsen : this test always times out on Flutter web. My guess is that Flutter web probably skipped ui.decodeImageFromPixels call so the completer inside is never completed. What's your thought and how should I fix it?

Gold has detected one or more untriaged digests on patchset 4.
Please triage them at https://flutter-gold.skia.org/search?issue=51656.

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #51656 at sha ea2fbe5

@hterkelsen : I've directly written the PNG bytes into a constant int list as you suggested. However, the web test would still timeout if await precacheImage is called. Is that expected?

Discussed with @hterkelsen offline. We agreed that we shall proceed with merging this PR, and maybe come back later once he figured out why precacheImage failed on web.

Gold has detected one or more untriaged digests on patchset 6.
Please triage them at https://flutter-gold.skia.org/search?issue=51656.

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #51656 at sha bb108d0

It seems that the golden test on web is flaky as sometimes it generated empty images. Hence I'm going to disable the test on web instead of just skipping precacheImage.