Avoid amp-img srcset selecting a smaller image if it will look unacceptably distorted#8055
Merged
Merged
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
a43613d to
cc73287
Compare
Contributor
|
LGTM, but deferring to @dvoytenko. |
Contributor
Author
|
Note that I have now signed the CLA |
|
CLAs look good, thanks! |
dvoytenko
approved these changes
Mar 13, 2017
Contributor
dvoytenko
left a comment
There was a problem hiding this comment.
Thanks a lot! Looks great!
mrjoro
pushed a commit
to mrjoro/amphtml
that referenced
this pull request
Apr 28, 2017
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Account for proportionality ratio to
srcsetselect calculation when choosing a smaller image over a larger oneCurrently the
srcsetselection chooses the closest size, favoring the larger size by 10%. So if you have a 400px wide and 800px wide version of an image in the srcset, and the current<amp-img>element width is 590px (590 - 400 = 190 * 1.1 = 209;800 - 590 = 210), amp selects the 400px version and you wind up getting a highly distorted image rendering. However, if you had 1400px and 1800px wide versions of an image, and the current element width is 1590px, choosing the 1400px version is a good call. The selection algorithm needs to account for proportionality to predict how distorted the image will wind up looking.In this PR, I chose to make 1.2 the maximum acceptable ratio of element width to image width for choosing a smaller image. In specific terms, my first example:
590 / 400 = 1.475, which would be above the max ratio, sosrcsetselects the larger image. In the 2nd example:1590 / 1400 = 1.136, ratio is below 1.2, so<amp-img>goes with the smaller image.This also scales down well to smaller image sizes. I recently had an instance where we had a
30pxand120pxwide image version in thesrcset. The element was being rendered at 70px, and<amp-img>was choosing the 30px version, which of course looks unacceptable. The ratio makes it obvious:70 / 30 = 2.33.Related-to #1280