Skip to content

Choose higher-res image variants on low-DPR screens #69799

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 5, 2020

Conversation

yjbanov
Copy link
Contributor

@yjbanov yjbanov commented Nov 4, 2020

Description

Choose higher-res image variants on low device-pixel ratio screens.

Entry-level and mid-range laptops, desktop monitors, and some entry-level tablets, have screens with low device-pixel ratios (<2.0). Choosing a lower resolution image results in visible scaling artifacts (#63122). This PR changes the logic such that we choose higher-res image variants on low device-pixel ratio screens.

Related Issues

Fixes #63122
Fixes #44108

Tests

Updated image_resolution_test.dart.

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Nov 4, 2020
@google-cla google-cla bot added the cla: yes label Nov 4, 2020
Comment on lines +66 to +74
/// For example, for a screen with device pixel ratio 1.25 the image chosen
/// would be 2.0x/heart.png, even though heart.png (i.e. 1.0) is closer. This
/// is because the screen is considered low-resolution. For a screen with
/// device pixel ratio of 2.25 the image chosen would also be 2.0x/heart.png.
/// This is because the screen is considered to be a high-resolution screen,
/// and therefore upscaling a 2.0x image to 2.25 won't result in visible
/// upscaling artifacts. However, for a screen with device-pixel ratio 3.25 the
/// image chosen would be 4.0x/heart.png because it's closer to 4.0 than it is
/// to 2.0.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this depend on how much upscaling is happening? Like upscaling from 1.0 to 1.25 is a 25% increase, but 2.0 to 2.25 is only a 12.5% increase.

I'm just throwing an idea out there in case it makes sense to you: what if, instead of having a 2.0 DPR limit, we check the upscaling factor and decide based on that. i.e. set a 20% upscale limit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not very predictable. Even a 5% upscale can visibly degrade the image depending on the upscale algorithm, because no matter how big or small the scale all pixels are affected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay then this looks good to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would kind of prefer not documenting the vagaries of how we determine this in this way - maybe just as a // comment instead of a doc comment.

The concern being that I really think this method is not going to stand up to the test of time, and it'll be "more" breaking since we documented all these implementation details.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more observable by the developer and their users than a typical implementation detail. Developers are the ones supplying the asset variants and without knowing how we choose them won't know which ones to provide (e.g. #44108 (comment)). I'd prefer not hiding these docs.

this method is not going to stand up to the test of time

That's fine, we'll just update the docs. I expect we will expand the functionality (e.g. improve physical pixel density detection, support scaled images), but I don't expect much will change otherwise as far as the developer is concerned. Provided there is no extra image scaling, this method should be pretty close to ideal (just like, provided that speeds are small enough and objects big enough, Newtonian physics is close to ideal).

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - this is the heuristic we decided to go with for now. but we also need an issue tracking improvements to this solution.

@yjbanov
Copy link
Contributor Author

yjbanov commented Nov 5, 2020

I don't have the exact device to test it, but I'm assuming this is also fixing #44108.

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with concern about being overly-specific in documentation.

As we discussed offline - this seems like an improvement for some cases that are really bad right now, but I still am not 100% convinced this is going to be a good long term solution.

Comment on lines +66 to +74
/// For example, for a screen with device pixel ratio 1.25 the image chosen
/// would be 2.0x/heart.png, even though heart.png (i.e. 1.0) is closer. This
/// is because the screen is considered low-resolution. For a screen with
/// device pixel ratio of 2.25 the image chosen would also be 2.0x/heart.png.
/// This is because the screen is considered to be a high-resolution screen,
/// and therefore upscaling a 2.0x image to 2.25 won't result in visible
/// upscaling artifacts. However, for a screen with device-pixel ratio 3.25 the
/// image chosen would be 4.0x/heart.png because it's closer to 4.0 than it is
/// to 2.0.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would kind of prefer not documenting the vagaries of how we determine this in this way - maybe just as a // comment instead of a doc comment.

The concern being that I really think this method is not going to stand up to the test of time, and it'll be "more" breaking since we documented all these implementation details.

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yjbanov yjbanov merged commit 496efca into flutter:master Nov 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
5 participants