Picture/Image Optimisations - The DPR Problem (Part 1/?) #3641
Conversation
…/picture-optimisations
…for getSourcesFromSrcSets
…/picture-optimisations
|
Size Change: +793 B (0%) Total Size: 3.12 MB
ℹ️ View Unchanged
|
| desiredWidth: number, | ||
| inlineSrcSets: SrcSetItem[], | ||
| ): SrcSetItem => { | ||
| // For a desired width, find the SrcSetItem which is the closest match | ||
| const sorted = inlineSrcSets.sort((a, b) => b.width - a.width); | ||
| // A match greated than the desired width will always be picked when one is available | ||
| const sorted = inlineSrcSets.slice().sort((a, b) => b.width - a.width); |
There was a problem hiding this comment.
Adding .slice() to avoid sideeffects as .sort modifies the original array
There was a problem hiding this comment.
such awareness of the evil of mutation...
| if ( | ||
| breakpoint >= breakpoints.tablet && | ||
| breakpoint < breakpoints.desktop | ||
| ) | ||
| return 680; |
There was a problem hiding this comment.
Looking at DCR, there is a breakpoint where the image width increases to aprx. 680px. This is brings DCR in parity with Frontend, which honoured this.
| return `(min-width: ${breakpoints.wide}px) 380px, 300px`; | ||
| if (breakpoint >= breakpoints.wide) return 380; | ||
| if (breakpoint >= breakpoints.tablet) return 300; | ||
| return breakpoint; // 100vw |
There was a problem hiding this comment.
Once under the tablet breakpoint, this is 100vw instead of 300px as it was before
OllysCoding
requested review from
DavidLawes,
jlieb10,
lucymonie,
nicl and
nitro-marky
as code owners
OllysCoding
changed the title
| desiredWidth: number, | ||
| inlineSrcSets: SrcSetItem[], | ||
| ): SrcSetItem => { | ||
| // For a desired width, find the SrcSetItem which is the closest match | ||
| const sorted = inlineSrcSets.sort((a, b) => b.width - a.width); | ||
| // A match greated than the desired width will always be picked when one is available | ||
| const sorted = inlineSrcSets.slice().sort((a, b) => b.width - a.width); |
There was a problem hiding this comment.
such awareness of the evil of mutation...
|
Excellent work writing that PR @OllysCoding 💯 🏅
Unless I am wrong that person is @paperboyo and when he is back may be able to provide some meaningful comments. |
The code use 'number' to describe both 'pixel' and 'viewport widths'. This add `Pixel` type alias to make the return type explicit and ease code reading.
|
@OllysCoding I have appended a commit adding a type alias for |
…c & functions for readability
|
Thanks for your feedback @mchv! Myself and @oliverlloyd went back over the code to work on readability & maintainability, which I hope is now much better (there were far too many sizes, widths, sources and srcsets all over the place!). |
|
Excellent job in the PR description, very clear summary of a complex problem 😬! Brief question - I don't claim to know the answer but I'm curious what you think. Currently I believe you're proposing to have a In our case we have 8 breakpoints and 2 supported DPRs, so number of What do you think of the second option proposed in guardian/image-rendering#239? In that scenario we provide a Again, I don't know if that's better, as you can see in that issue it's only one of two possible solutions (the other being the approach taken in this PR). But given that you've done a lot of research on this recently I'd be curious what you think 🙂. Also I'm looking forward to getting all the excellent work in this PR available in common-rendering so AR can benefit too! 🎉 |
Can’t agree enough! Great job, @OllysCoding! I’m very interested in opinions on different approaches, as Jamie says, but don’t have an opinion myself. I didn’t know our frontend logic uses media queries not supported by Safari! I would only say that I think it’s beneficial for the URLs to explicitly spell out |
|
Thanks for the feedback @JamieB-gu!
I was aware while writing this code that it would result in a lot more Worth noting that
I think ultimately the differences between the two options are very slight. The key tradeoff between the two is really, how declarative do you want to be at the cost of a larger DOM? The former (as implemented in this PR) gives us the choice of when to switch between high and low DPR variants, which could be beneficial if we want to look into if our choice of 1.25 is the most optimal. The second option entrusts the browser with this responsibility, and if there is room for optimisation there, we lose it - but we do get a slightly smaller DOM structure. For me it makes sense to stick with the implementation which we use on Frontend, since it's known to work well, and I don't think there's a huge cost to the safari workaround we use. I do also want to add further optimisations to this solution in future PRs, so I'd be interested to revisit the two solutions once the full context of the work is complete, and see if there's a more obvious choice! |
Oh, I do have an opinion now, then :-). I’m in favour of switching at 1.25, as this means my usually-zoomed-in-to-133% desktop browser gets HiDPI images as is always have, haha (in all seriousness, whether what I like is most optimal indeed, I don’t know). We also have slightly different cut-off point for cutouts, but this was purely because PNGs were too weighty which is now moot point as everyone gets WebP (and, hopefully, soonish – AVIF/JXL). |
There was a problem hiding this comment.
It’s great to see people caring about images 🖼️ 🎉
What do you think of the second option proposed in guardian/image-rendering#239? In that scenario we provide a
<source>tag per breakpoint, and usesrcsetto manage the DPR variations. That would reduce the number of<source>tags and drop the need to worry about the webkit (Safari) version ofmin-resolutionas @mxdvl mentioned.
I think, like @JamieB-gu mentioned, that we should investigate Option two, which is the only real reason you’d want to use the size attribute. This final output would be something like:
<picture>
<source
media="(min-width: 375px)">
sizes="(-webkit-min-device-pixel-ratio: 1.25),(min-resolution: 120dpi) 640px, 320px"
srcset="https://xxx.jpg?width=320px 320w, https://xxx.jpg?width=320px&dpr=2 640w"
/>
<source
media="(min-width: 740px)">
sizes="(-webkit-min-device-pixel-ratio: 1.25),(min-resolution: 120dpi) 1200px, 600px"
srcset="https://xxx.jpg?width=600px 600w, https://xxx.jpg?width=600px&dpr=2 1200w"
/>
<source
media="(min-width: 1200px)">
sizes="(-webkit-min-device-pixel-ratio: 1.25),(min-resolution: 120dpi) 1800px, 900px"
srcset="https://xxx.jpg?width=600px 900w, https://xxx.jpg?width=900px&dpr=2 1800w"
/>
</picture>mediarelates to the size of the viewport and select the rightsourcesizesrelates to the pixel density and picks the rightsrcsetsrcsetshows the possible versions of this image for this specific viewport/mediaattribute.
| The sizes html attribute acts as the translation layer between the size of the viewport and the size of the image source you'd like to pick. A good way to think about this is that, beyond a certain width, main media (inline) images never go beyond `620px` wide, and this html attribute gives us a way to communicate that to the browser. | ||
|
|
||
| For example `<source sizes="(min-width: 660px) 620px, 100vw">` tells the browser, "Hey, if your viewport is 660px or wider, always look for an image source which is 620px wide. If not, default to an image which is the same width as 100% of the viewport width". We can provide as many sizes & queries as we want, with the `(query) px, (query) px, ... , px)` syntax, where the last argument without a query is the default if none others match. |
There was a problem hiding this comment.
Worth noting: it only makes sense to have multiple sizes if you have multiple srcset.
There was a problem hiding this comment.
Thinking about images again this morning, so wrote down a few ideas and comments.
I wanted to point out that the 85 and 45 quality values for images have not been tested extensively enough, and a associated piece of work would be to review these values and A/B test the performance with our Core Web Vitals metrics.
| } | ||
| ): Pixel => { | ||
| if (format.display === ArticleDisplay.Immersive && isMainMedia) | ||
| return breakpoint; |
There was a problem hiding this comment.
Is this right? Isn’t the desired size the whole width of the viewport?
| // Immersive MainMedia elements fill the height of the viewport, meaning | ||
| // on mobile devices even though the viewport width is small, we'll need | ||
| // a larger image to maintain quality. To solve this problem we're using | ||
| // the viewport height (vh) to calculate width. The value of 167vh | ||
| // relates to an assumed image ratio of 5:3 which is equal to | ||
| // 167 (viewport height) : 100 (viewport width). | ||
| sizes="167vh" |
There was a problem hiding this comment.
This is clever, but ideally we would have a portrait source. Otherwise the majority of the image downloaded isn’t even used.
To investigate in another PR, though.
There was a problem hiding this comment.
Indeed we could use resizer to cut off sides in this scenario. It could mean, browsers will have to download another image when orientation or window ratio changes, though. Worth investigating!
Also worth noting, the assumption of 5:3 crop, while practically often true at the Guardian, isn’t necessary as we carry real ratio in CAPI.
There was a problem hiding this comment.
as we carry real ratio in CAPI
Oh, so we could replace 167 with a value calculated based on ratio? That seems like an easy win.
There was a problem hiding this comment.
Waaaait… Why I can see aspectRatio only for the trail (always 5:3) image, but not for Main media which eg. here is a different ratio?! We could calculate from width and height, but I will ask CAPI why aspectRatio isn’t offered for all imagery.
There was a problem hiding this comment.
Boo! It’s not in CAPI, coz it’s not in FlexibleAPI, coz it’s not in Media API (Grid). Quite a change, I suppose, but I will ask around. 🤦♂️
Thanks for having a look, Max! ❤️ Now, doing it before we will use AVIF makes little sense in my mind. Changing those values would decache all our imagery (even if we would earlier devise a way to test them less globally). I would argue, let’s have a look at quality values, dpr steps and the rest when we will be working on AVIF. Sadly, our image resizer is the slowest with offering support compared to (all?) competitors… |
|
@OllysCoding wins Jimmy’s Award for Best Described and Most Informative PRs 2021! Congratulations on behalf of The Committee 🥇 |
Summary
On DCR, there is a bug with how images are rendered which means high resolution displays can often fetch excessively-large variants of images. This means increased cost in bandwidth for both our users and our fastly bill.
This PR replicates the functionality of this Frontend PR to make use per-breakpoint source elements with media attributes to prevent the browser from selecting larger image sources than we intend.
Some related & interesting background PR's & Articles:
Background Info
Documentation on how picture tags work & more - A great place to start if this is new to you! (Added in this PR)
What does DCR currently do for images?
DCR renders images in
<picture>tags, with each picture element containing two<source>elements. These source elements differentiate between high & low DPR/DPI (device pixel ratios).We use the following media query on the first of our two
<source>elements to tell the browser, "Hey, if you're a 'high DPR device', use this source element":<source media"(-webkit-min-device-pixel-ratio: 1.25), (min-resolution: 120dpi)" >This lets us do something different for these "High DPR" devices, often on these super high resolution screens, getting an equivalently high resolution image is unnecessary - the difference is barely perceptible to the human eye.
For non-high-dpr image sources we provide the quality attribute to fastly as
?quality=85, however for these high-dpr image sources we have, we tell fastly?quality=45&dpr=2.These high-dpr image sources have, as we learned earlier, double the resolution of the regular image sources, however by providing a lower quality number, they're more compressed. At some point in time someone at The Guardian sat and looked at images side-by-side and decided this config was the best for high resolution devices.
All in all this leads to a DCR picture element looking something like:
The DPR Problem
Everything up until now is context & background information to make sure anyone who reads this PR can understand what the changes made actually do. Now let's talk about the problem, why it happens & how we have solved it!
This problem comes from how the browser tries to compensate for high DPR displays when choosing an image source. As described above, we provide a set of sources for high DPR displays, and target them with a media query to ensure it's picked.
Unfortunately the browser itself tries to compensate for high DPR as well, but in a less efficient way. Once the browser has figured out what size source it wants from the
sizesattribute on our source element, it's then multiplied by the DPR of the display to get the desired width it will look for in oursrcset:This poses a problem for us, because if we tell the browser "Hey choose a 620px image", and the browser has a DPR of say 3, it will actually look for an image source for 1860px, far higher resolution than is needed for the user to have a good experience.
How do we solve this?
Lucky for us, this problem has already been solved on Frontend, and its existence is currently a regression on DCR.
So what does Frontend do? As shown in this PR, rather than having just 2 source elements, we instead have 2 source elements per breakpoint (one for high DPR, one for regular displays), and provide just one size & source set for that source element.
Lets look at a simplified example (with only 1 source per breakpoint):
In this example, we have the logic that usually would have been in our sizes attribute (
sizes="(min-width: 660px) 620px, 100vw") extrapolated into individual source elements. All our breakpoint which are 660px or larger offer only 1 choice, the 620px source. The lower breakpoints have looked for sources which are closest to their own size, as a replacement for100vw.This solves our DPR problem because, the
mediaattribute uses CSS pixels, and because we only offer 1 image source for each of these elements, once the browser has picked its source element, we basically strong arm it into which source to use. Even if it multiplies the size it gets fromsizes, it still has to use the source we give as it's the only available option.Implementation details
The primary change for this PR is the (previously called)
getSizesfunction. This function takes various information about the image we're trying to render (role, layout, etc), and returns the contents for thesizesattribute. By refactoring this code, we can instead have it also accept a breakpoint, and have it perform the same logic as the queries it was producing to instead return an expected image width for that breakpoint:I've also introduced an optimisation function called
optimiseBreakpointSizeswhich will look for redundant source's and remove them (for example if all your sources for breakpoint 660px and above return 620px, you only need 1). This should help avoid us overfilling the DOM with source elements.Final before and after
Before:
After:
Future Improvements
There are a few outstanding issues that can be solved with this solution,
Once these PR's are raised they'll be linked to here.