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

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.

+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).

Re-add `MipFilter::kBase`, but keep `kNearest` as the default. (#52779)

@jonahwilliams

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

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.

@yjbanov

@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?

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.

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?

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.

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

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).

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

@jonahwilliams

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.

@goderbauer

Sounds good, thanks @goderbauer !

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

[Impeller] correctly map DL non-mipmap sample mode to non-mipmap samp…

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

[Impeller] correctly map DL non-mipmap sample mode to non-mipmap samp…

Navigation Menu

Repository navigation

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

6 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Activity

matanlurey commented on May 13, 2024

goderbauer commented on May 21, 2024

goderbauer commented on May 21, 2024

goderbauer commented on May 22, 2024

eyebrowsoffire commented on May 23, 2024

goderbauer commented on May 23, 2024

eyebrowsoffire commented on May 23, 2024

jonahwilliams commented on May 23, 2024

6 remaining items

matanlurey commented on Jun 3, 2024

jonahwilliams commented on Jun 3, 2024

goderbauer commented on Jun 3, 2024

jonahwilliams commented on Jun 3, 2024

flutter-triage-bot commented on Jun 3, 2024

github-actions commented on Jun 19, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions