Skip to content

Restrict selection of srcset to the two closest values#6741

Merged
dvoytenko merged 5 commits intoampproject:masterfrom
dvoytenko:srcset4
Dec 20, 2016
Merged

Restrict selection of srcset to the two closest values#6741
dvoytenko merged 5 commits intoampproject:masterfrom
dvoytenko:srcset4

Conversation

@dvoytenko
Copy link
Copy Markdown
Contributor

Closes #6734.

/cc @sebastianbenz

src/srcset.js Outdated
let minScore = 1000000;
for (let i = 0; i < this.sources_.length; i++) {
const length = this.sources_.length;
let minWidth = this.sources_[length - 1].width / dpr;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we get a test case for no width specified? I don't understand why we divide it by 2.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Technically, if you specify w for srcset - they are required for all srcset. In other words, having one absent is an error. However, these cases are seen commonly in practice and in fact all of the browsers tolerate them. Since we have no idea what width it is, the only assumption that it's smaller than the rest and we assume it's half the minimum width.

src/srcset.js Outdated
// is the stop point.
if (sourceWidth >= width) {
// Bull's eye or at the minimum.
if (sourceWidth == width || i == length - 1) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can fix this oddness by iterating in order, right?

let prevWidth = Infinity;
for (let i = 0; i < length; i++) {
  if (sourceWidth == width) {
    return i;
  }

  if (sourceWidth < width) {
    const delta = width - sourceWidth;
    const lastDelta = (prevWidth - width) * 0.9;
    return (delta < lastDelta) ? i : i - 1;
  }
  prevWidth = sourceWidth;
}

// Return closest width
return i;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Which "oddness"?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That you have to guard against the end of the array.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got it. Done.

Dima Voytenko added 2 commits December 19, 2016 17:01
src/srcset.js Outdated
let minScore = 1000000;
for (let i = 0; i < this.sources_.length; i++) {
const length = this.sources_.length;
const minWidth = this.sources_[length - 1].width / dpr;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With our sorting algorithm, we can't guarantee that this value isn't the undefined:

[{ width: 640 }, { width: 320 }, { width: undefined}].sort(sortByWidth);
// => [{ width: 640 }, { width: 320 }, { width: undefined}]

Actually, the undefined width doesn't move at all when sorting, so we can't guarantee it's in any position.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually, just realized that width: undefined and width: 0 cases are already disallowed. So, that branch is meaningless. Removed. Thanks for the catch!

src/srcset.js Outdated
// The right value is now between `i` and `i + 1` - select the one
// that is closer with a slight preference toward higher numbers.
const delta = sourceWidth - width;
const prevDelta = width - prevWidth * 0.9;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are we multiplying the prevWidth by 0.9, and not the delta itself? That would mean we could get bad results when the prevWidth was very similar to width but we have very large widths.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The comment above explains it: we give a 10% preferences for a bigger width. This is a compromise between Chrome (that always selects the closest source) and Safari (that always selects the bigger source).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I understand the comment, but the implementation doesn't follow that:

const prevWidth = 1900;
const width = 1920;
const sourceWidth = 2000;

//...
const delta = 2000 - 1920; // => 80
const prevDelta = 1920 - 1900 * .9; // => 210

That prevDelta is drastic, even though it's the better choice. Now take:

const prevWidth = 1919;
const width = 1920;
const sourceWidth = 2000;

//...
const delta = 2000 - 1920; // => 80
const prevDelta = 1920 - 1919 * .9; // => 192.9

Even 1px difference makes us swing to the larger value every time. Instead, we could do:

const prevDelta = (width - prevWidth) * 1.1;
(1920 - 1900) * 1.1; // => 22
(1920 - 1919) * 1.1; // => 1.1

// The "turning" point
(1920 - 1847) * 1.1; // => 80.3

Now we get proper 10% penalties.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got it. Misunderstood. Done.

@dvoytenko dvoytenko merged commit 6bf2aa7 into ampproject:master Dec 20, 2016
@dvoytenko dvoytenko deleted the srcset4 branch December 20, 2016 21:30
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
* Restrict selection of srcset to the two closest values

* review fixes

* lints

* removed width=undefined special case

* review fixes
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
* Restrict selection of srcset to the two closest values

* review fixes

* lints

* removed width=undefined special case

* review fixes
torch2424 pushed a commit to torch2424/amphtml that referenced this pull request Jan 3, 2017
* Restrict selection of srcset to the two closest values

* review fixes

* lints

* removed width=undefined special case

* review fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants