DBW: Responsive Images Audit

[patrickhulce]

R: all

addresses #978 #876

[patrickhulce]

Waiting to add tests for feedback on the audit mechanism. Right now as described in the fileoverview...

  *   Checks to see if the images used on the page are larger than
  *   their display sizes. The audit will list all images that are larger than
  *   their display size regardless of DPR (a 1000px wide image displayed as a
  *   500px high-res image on a Retina display will show up as 75% unused);
  *   however, the audit will only fail pages that use images that have waste
  *   when computed with DPR taken into account.

This gives the somewhat confusing result though that a page can easily pass with mobile emulation on and fail on desktop even though the mobile violations are more impactful to the user.

Example Mobile Result

Example Desktop Result of Same Site

[brendankenny]

This gives the somewhat confusing result though that a page can easily pass with mobile emulation on and fail on desktop even though the mobile violations are more impactful to the user.

I know there's also been advice that you don't necessarily want a 1:1 image pixel to display pixel ratio on mobile devices as it's just a waste, and really you want some notion of angular resolution of those pixels, but I have no idea of what the state of the art is on that.

[brendankenny]

you don't necessarily want a 1:1 image pixel to display pixel ratio on mobile devices as it's just a waste

e.g. ratio of 1 may be too low, 2 could work well, but 2.6 or whatever would be wasted pixels as the average visitor won't hold the phone up close to look at most pictures (however you want to measure that)

[patrickhulce]

e.g. ratio of 1 may be too low, 2 could work well, but 2.6 or whatever would be wasted pixels as the average visitor won't hold the phone up close to look at most pictures (however you want to measure that)

Yeah I'd just generally prefer Lighthouse to stay out of these judgement calls when possible. IMO if there's a legitimate reason for acting a particular way that isn't very unpredictable LH should try not to fail you for it but just make sure you're aware of it. This does get into the area of something I'd really like to see across the board though: severity of audits rather than pass/fail. Feels like another conversation though :)

[brendankenny]

severity of audits rather than pass/fail

well we do have support for 0-100 scores, we just haven't used them in a while :) Defining how severe is severe is hard, of course

[brendankenny]

Looking good. While we figure out scoring and what constitutes waste, some initial comments, mostly around style

[brendankenny]

what is !usedRatio protecting against here? NaN? May want to use !isFinite(usedRatio) instead so it handles Infinity as well. Both seem like an error condition, though (as opposed to usedRatio >= 1), so it may warrant some kind of error string instead?

[patrickhulce]

Yeah will better handle the edge cases, originally this was for an image that was never shown (0 used pixels) but that is now handled by checking for the network record

[brendankenny]

better to use responseReceivedTime instead of startTime since connection overhead won't change much compared to transmission time alone

[brendankenny]

this was actually meant to be a question; I don't actually know that it's better to do this :)

[patrickhulce]

yeah that's a good point! also should decide what to do about handling data URI images since the "response time" will be nothing but they just impacted another assets response time

[brendankenny]

maybe introduce a unusedRatio or wastedRatio intermediate variable for 1 - usedRatio

[patrickhulce]

done

[brendankenny]

this is a clever solution, though it's obviously an estimate. I would imagine it's a better estimate for a block-based image format vs something like PNG? Have you considered your canvas solution (from the image optimization audit) instead of/in addition to this?

[patrickhulce]

Yeah just an estimate, but the impact of the differences is likely to be much more pronounced at the smaller size end of the spectrum where it's less likely to matter anyway. That seemed like overkill for the first pass, but I'm open to sharing that little gem of logic :)

[brendankenny]

maybe @param {number} DPR devicePixelRatio to make this param clearer on first read?

[patrickhulce]

done

[brendankenny]

need ! before Promise

[patrickhulce]

done

[brendankenny]

element

[patrickhulce]

done

[brendankenny]

stringify for escaped encoding or...? better to parse with new URL() and reject bad ones?

[patrickhulce]

just as replacement for '${element.src}', but will change. they all should be valid urls since they came from img.currentSrc

[brendankenny]

can just return tag (without the promise wrapper) if using Promise.all

[patrickhulce]

yeah will do

[brendankenny]

Promise.all makes me nervous when interacting with the debugger protocol. Right now driver.evaluateAsync is just the one command internally, but if we change that at some point, Promise.all could end up interleaving commands. Usually better to go serial. OTOH, that sets the network requests off one at a time, much slower than necessary. Maybe ship the array of images to investigate off to the browser in one evaluateAsync call and put the loop over them in there?

[patrickhulce]

will serialize, there shouldn't be many network requests happening here and if there end up being many we should revisit

[brendankenny]

there shouldn't be many network requests happening here and if there end up being many we should revisit

yeah, for the requests I'm mostly worried about the cache behavior on devices with less storage. It's possible that for an image on a page the decoded pixel data is still present but the image file has been evicted from the network cache, so img.src = url may force another download. We'll have to investigate to find out if that is possible.

[ebidel]

I'm wrestling with display size. "viewport size"?

[patrickhulce]

Well not just viewport size but how large the image is when displayed after taking into account DPR. i.e. if the viewport is huge but the image fills 1/4 of the width of the viewport and DPR is 2, target 1/2 of the viewport size.

[ebidel]

"device display size"?

[patrickhulce]

sgtm

[ebidel]

TIL about naturalWidth and naturalHeight :)

[ebidel]

how confident are we that this relationship of wastedBytes -> wastedTime is correct?

[patrickhulce]

More confident than the wastedPixels -> wastedBytes relationship! Do you have anything in particular you're thinking of? This somewhat assumes keep-alive and the congestion window is already flooded by the time we hit these images.

[ebidel]

Nothing in particular. Just thinking out loud. Maybe the final label should be clear that this is an estimate? https://github.com/GoogleChrome/lighthouse/pull/1497/files/b4fe416f62b776844a9a743864921256f01dc86a#diff-31529c3403e44b3ff053b53ab95cca7dR118

[patrickhulce]

done!

[ebidel]

Nice trick to get full urls. Any value in usingnew URL() with the 2nd base param instead of creating dom? I guess what you have also works if there's <base> tag on the page.

[patrickhulce]

Didn't know about the 2nd base param! I'll switch to that.

[ebidel]

doc element as a HTMLImageElement

[patrickhulce]

will doc

[ebidel]

can you add comment for later that currentSrc is used b/c it gives you the chosen media src that the browser selected based on the display?

[patrickhulce]

done

[ebidel]

you could move imgElementInfo above and reuse it in the first if

[patrickhulce]

done

[ebidel]

I found x% savings kinda confusing in the screenshot. Maybe % waste would be a better thing to report to the user?

[ebidel]

Or somehow communicating that this is the % savings if the correct size was used...?

[patrickhulce]

How about "potential savings" as in the previous audits? I was trying to find ways to make these seem more encouraging to users rather than scolding since they can be a bit harsh. If you've done everything imaginable in the realm of responsive images and you're over by like 10%, it felt mean to say 10% waste.

[ebidel]

yea, potential savings SGTM

[ebidel]

Complex stuff. Seems like a nice approach.

#978 also mention css image-set() and background images. I feel that adding CSS stuff to the audit might be a bit much. WDYT?

[patrickhulce]

#978 also mention css image-set() and background images. I feel that adding CSS stuff to the audit might be a bit much. WDYT?

@ebidel yeah I struggled more with those since it's much less obvious how to get their actual size used and they're much more likely to be used as sprites which will intentionally be too big for their display size. I agree on postponing and we can just leave that issue open for now.

[ebidel]

Since you're running this on the page, if the site uses a <base href="http://somethingelse.com/">, new URL(url, window.location.href).href; might not be correct. e.g. http://jsbin.com/yilakeqeyi/edit?html,output

return new URL(url, document.baseURI || window.location.href).href; should do it.

[patrickhulce]

done

[brendankenny]

document.baseURI || window.location.href

drive-by nit: if there isn't a <base> element then these should be equal, and document.baseURI will always be defined anyways, so return new URL(url, document.baseURI).href should be sufficient

[brendankenny]

looking good. A few last things.

Should also have a smoke test for this. If you don't want to add it to the DBW test, the offline-ready test runs the full default config so it runs all the audits. The offline-ready page has an image in it, so it'll be able to assert something about it (could also test the unoptimized images audit output in there as well).

[brendankenny]

issue for this?

[patrickhulce]

#1517

[brendankenny]

add the issue number to the TODO so we'll have the cross reference

[patrickhulce]

done

[brendankenny]

purposefully only using the last error (overwriting each time)?

[patrickhulce]

Yeah that seemed to be the pattern followed a few other places

[brendankenny]

worth including size info for help debugging? could only be naturalWidth, naturalHeight, clientWidth, or clientHeight as the culprits.

OTOH this may never really happen

[patrickhulce]

Yeah this theoretically should never happen and was just for catching a bug I had lower down.

[brendankenny]

no, sounds good for now. If we add another measure here we can change it :)

[brendankenny]

yeah, maybe remove if we don't use it

[patrickhulce]

done

[brendankenny]

I agree on postponing and we can just leave that issue open for now.

Would you mind adding a checklist or whatever of possible improvements (and the TODO you added in this PR and anything else you've thought about while writing this) at the top of that issue so we have a central place for tracking?

[patrickhulce]

Would you mind adding a checklist or whatever of possible improvements (and the TODO you added in this PR and anything else you've thought about while writing this) at the top of that issue so we have a central place for tracking?

Done but split out the TODO into a new issue since it isn't really responsive image specific (applies to all byte efficiency audits)

[patrickhulce]

@brendankenny PTAL :)

[brendankenny]

two last things :)

[brendankenny]

does this warning show up in the extendedInfo and so can be checked?

[patrickhulce]

Yes it's the 2nd in length: 2

[brendankenny]

add the issue number to the TODO so we'll have the cross reference

[brendankenny]

🖼↕️ LGTM!
↔️