[framework] FilterQualiy.low is poor default for the Image widget.

[jonahwilliams]

FilterQuality.low corresponds to linear sampling with the base mip level only (with Skia). Flutter's image widget uses this as the default filter quality for rendering. Unfortunately this has major fidelity/performance problems if the image is substantially larger than the destination rectangle (a common issue).

Avoiding mipmaps will impact image quality, as entire rows/columns of pixels will be entirely dropped.
Avoiding mipmaps will slow down rendering. While a small image will sample fewer pixels from the larger image, these pixels will necessarily be more spread out causing increased L1/L2 cache misses.

Mips solve both of these problems.

Impeller initially implemented FilterQuality.low incorrectly and gave it mip levels. We do not plan to fix this until the framework is fixed, as it will have a negative impact on all Flutter applications.

[matanlurey]

+1, the naming is very wrong right now - misleading at best.

Not only should we change the default, we ideally should have names that actually coincide with what the engine is doing (ideally using industry standard terminology).

[goderbauer]

Some notes from talking to @jonahwilliams:

We should make medium the default. Need to check how breaking this is in terms of golden tests. This switch will unblock fixing low in the engine.

Long-term: The fact that the naming suggests an order here is misleading. We could turn FilterQuality into an enhanced enum, add new values to it with better names, and deprecate the old ones (need to check how commonly these are used to determine how breaking this would be). Potential new names:

none -> pixelated (nearest neighbor, no mips)
low -> (linear, no mips)
medium -> normal (linear, mips)
high -> perspective

[goderbauer]

Or some potentially more descriptive names:

enum class DlImageSampling {
  kNearestNeighbor,
  kLinear,
  kMipmapLinear,
  kCubic,
};

My preference would be to use these descriptive names that also do not imply any order. They'd have to go with some detailed docs describing the pros/cons of each.

[goderbauer]

@yjbanov @eyebrowsoffire Are there any potential concerns on the web (e.g. w.r.t. performance) with switching the default to FilterQuality.medium for images as suggested in this issue?

[eyebrowsoffire]

I think the only concern here is that both CanvasKit and Skwasm don't produce mipmaps for certain images. For those images, the request for mipmapped sampling may be ignored.

I think one of the things that is sort of wrong with our API is that you need to make a decision about whether you're going to use mipmapping at image creation time, but the filter quality is specified at draw time. The way dart:ui If we want to consistently get the benefits of mipmaps, we'd need to always generate mipmaps, regardless if those mipmaps are ever used (the user only uses low or none quality). Or perhaps generate mipmaps for the image just in time, but that would require some machinery at the skia level which I believe doesn't currently exist.

[goderbauer]

I think the only concern here is that both CanvasKit and Skwasm don't produce mipmaps for certain images. For those images, the request for mipmapped sampling may be ignored.

Thank you! Could you qualify what the "certain" images are for which mipmaps are not produced? Is this more of an edge case or something that's gonna be fairly common?

[eyebrowsoffire]

I would say it's the common case on the web that doesn't populate mipmaps. The MakeLazyImageFromTextureSource API in CanvasKit doesn't populate it, and the analogous codepath in Skwasm also doesn't. We can change that if needed, but it does mean we are paying the perf cost of generating mipmaps whether we are using them or not.

[jonahwilliams]

I'd be surprised if generating mips was a substantial % of the cost of decoding or allocating large images. Whereas the performance hit you can take from not having them will be felt on every subsequent frame.

[jonahwilliams]

Unless canvaskit is generating mips on the CPU?

[eyebrowsoffire]

I'd be surprised if generating mips was a substantial % of the cost of decoding or allocating large images. Whereas the performance hit you can take from not having them will be felt on every subsequent frame.

I think that's a reasonable hypothesis. I am not in principle against generating mipmaps for all images.

Unless canvaskit is generating mips on the CPU?

For the images in question, no mipmaps are being generated at all right now, and it is more-or-less up to the bindings layer to generate them at the moment of texture creation. It might be as simple as calling generateMipmap on the GL context right after we create the texture.

[jonahwilliams]

You could also choose to not honor the filter quality setting if you don't have mips. Also, if the backend knows that the texture would be sampled from the 1st mip level anyway, you could defer generating them.

[matanlurey]

Thanks for this change @jonahwilliams and @goderbauer. I think we missed part of the engine that is intended to stay in sync, though perhaps it's not important and the default actually doesn't come up (if so, we should delete the TODO).

[jonahwilliams]

we didn't miss it, its just not done yet: flutter/engine#53161 ;)

[goderbauer]

The changes to switch the defaults to medium for images have landed in the framework. I am going to kick this over to you @jonahwilliams to finish the engine side.

[jonahwilliams]

Sounds good, thanks @goderbauer !

[flutter-triage-bot]

The triaged-framework label is irrelevant if there is no team-framework label or fyi-framework label.