[skwasm] Fix sampling options for low filter quality to match CanvasKit. #52331
Conversation
eyebrowsoffire
changed the title
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. |
| return SkSamplingOptions(SkFilterMode::kNearest, SkMipmapMode::kNone); | ||
| case FilterQuality::low: | ||
| return SkSamplingOptions(SkFilterMode::kNearest, SkMipmapMode::kNearest); | ||
| return SkSamplingOptions(SkFilterMode::kLinear, SkMipmapMode::kNone); |
There was a problem hiding this comment.
You definitely want nearest for mipmap mode low. The framework draws all images with filter quality low and mipmapping is required for correct rendering if the images are shrunk from their original size substantially.
There was a problem hiding this comment.
Although looking at the skia options, maybe we're handling this wrong:
enum SkLegacyFQ {
kNone_SkLegacyFQ = 0, //!< nearest-neighbor; fastest but lowest quality
kLow_SkLegacyFQ = 1, //!< bilerp
kMedium_SkLegacyFQ = 2, //!< bilerp + mipmaps; good for down-scaling
kHigh_SkLegacyFQ = 3, //!< bicubic resampling; slowest but good quality
kLast_SkLegacyFQ = kHigh_SkLegacyFQ,
};
Ignore me!
There was a problem hiding this comment.
I changed these to match with CanvasKit's behavior, which you can see here:
I also checked the native implementation. FilterQuality.low maps to DlImageSampling::kLinear, which is converted to an SkSamplingOptions object here:
When the mipmap mode is not specified, it defaults to none: https://github.com/google/skia/blob/cdede8e2e18b68d4412ac09d7d583034d22f3162/include/core/SkSamplingOptions.h#L79
So at least for skia rendering on native, I don't think what you're saying is true. It might be different for impeller though.
eyebrowsoffire
added
the
autosubmit
…147262) flutter/engine@b686508...303e718 2024-04-23 [email protected] Roll Skia from f9d6a2cf4179 to f09c6745031c (1 revision) (flutter/engine#52334) 2024-04-23 [email protected] [skwasm] Fix sampling options for low filter quality to match CanvasKit. (flutter/engine#52331) 2024-04-23 [email protected] [Impeller] Fix Vulkan validation error on latest MoltenVK. (flutter/engine#52332) 2024-04-23 [email protected] Roll Skia from 21563606e32d to f9d6a2cf4179 (1 revision) (flutter/engine#52330) 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
3 participants
These sampling options didn't match up with what CanvasKit was using. I also changed the tests to render images in all four image qualities on a golden.