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 on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

I feel like these clarifications are more of a report card for the implementation rather than documentation of the intention. The way they are phrased could end up cementing the expectation of the results in lieu of fixing the underlying problems.

@flar

I feel like these clarifications are more of a report card for the implementation rather than documentation of the intention.

I think it's still better to have them than not, but...

in lieu of fixing the underlying problems.

...I agree that we should fix it eventually. Based on the cases I've seen so far, I would just remove low altogether and remap all options as follows:

Implicitly, the current algorithm used by high is gone too. I haven't seen any examples where it would produce a better result compared to medium.

For advanced use-cases the Skia team tells me we can provide an explicit sampler. This way users can have a high degree of control. That would require an extension in our dart:ui API.

It would be a breaking change, so I wouldn't rush just yet.

We have the following possible techniques that can be applied at a quality vs performance tradeoff:

nearest neighbor (none)
bilinear (low)
bicubic ()
mipmap ()

The last 2 aren't mutually exclusive since you could apply nn/bilinear/bicubic even while doing mipmapping. There are also other filter algorighms, but we don't really embrace those. Also, mipmap is targeted at improving quality while reducing and just looks like bilinear (or bicubic potentially) while enlarging.

If I'd get rid of any of the names I'd probably get rid of "none" as it is just such a bad name. It implies that nothing will happen if taken out of context and it falls out of the spectrum of "low/med/hi". I might have named it "fastest" or "lowest" originally, ignoring whether 3 or 4 constants are useful for Flutter apps.

Yeah, so a more advanced API for picking a sampling matrix and/or mipmapping can be provided separately from the FilterQuality enum. FilterQuality could be the "non-advanced" API, with simple explanations and unsurprising behavior. Currently, it requires elaborate explanations (as you can see from this PR) and surprising behavior (e.g. medium is better quality than high, what?!). I think none (or low)/medium/high is good enough for the simple case. The sampling algorithm behind today's high does not qualify as "simple" and should be moved to the advanced API (which I wouldn't rush to add until users ask for it).

To that end, Skia is moving to SkSamplingOptions, of which SkFilterQuality is just a legacy value with which to construct the sampling options.

https://api.skia.org/SkSamplingOptions_8h.html

Cubic is its own sampling method which does not appear to use mipmaps.
Within a texture you can have nearest neighbor or bilinear blending
On top of that you can separately control mipmap level selection to any of None, Nearest, or Linear

The SkFilterQuality options now map to:

• none - Nearest/None
• low - Linear/None
• medium - Linear/Linear
• high - Cubic (does not mipmap)

which is essentially as it was before

I turned the discussion about actually changing the API into a separate feature request: flutter/flutter#76737

Ping @Hixie for followup.

See also #24797

Can't we just make the implementation of "high" act like "medium" when shrinking, if that's really better?

This may be doable in Skia, but it's probably quite difficult to do on the Flutter side (engine or framework). The issue is that we don't have a way to tell if we're shrinking the image or magnifying it. It depends on the aggregate transform matrix that includes all layers as well as transforms inside the picture. Pictures are opaque and as of today backed by SkPicture directly. There's no way for us to intercept the rendering process and switch the sampling algorithm. I'm guessing Skia can probably do this, or perhaps provide an API for us to customize image rendering. If we go the custom recording format route we may not need anything extra from Skia (Android and Chrome went this route). However, if we want to keep the direct link to SkPicture we need more features from Skia.

OpenGL and Metal have concepts of min and mag filters, but I don't see them exposed in Skia unless I'm missing something.

I meant in Skia, yes.