Skip to content
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

Bits per pixel calculation is not intrinsic-density-aware #12729

Closed
eeeps opened this issue Jul 1, 2021 · 3 comments
Closed

Bits per pixel calculation is not intrinsic-density-aware #12729

eeeps opened this issue Jul 1, 2021 · 3 comments

Comments

@eeeps
Copy link

eeeps commented Jul 1, 2021

The optimized images logic looks to see if images are less than two bits/pixel. But the pixels it's measuring (element.naturalWidth * element.naturalHeight) are the density-corrected intrinsic dimensions. So this image:

<img src="one-and-a-half-bits-per-pixel.jpg" />

would pass the audit. But this one, embedding the same image resource (and therefore having the same number of bytes), would have 1/4 the total "pixels" as measured by the audit:

<img srcset="one-and-a-half-bits-per-pixel.jpg 2x" />

..and it would fail.

@eeeps
Copy link
Author

eeeps commented Jul 1, 2021

Similarly, this image:

<img srcset="four-bits-per-pixel.jpg 100w" sizes="100vw" />

would pass when encountered in almost all viewports, because its intrinsic density will be 100w ÷ 100vw in CSS pixels (aka window.innerWidth). If my math is correct that means as long as the viewport is over 200px wide, this four-bits-per-pixel image will pass the audit.

@patrickhulce
Copy link
Collaborator

Thanks for filing @eeeps! Do you have a minimal example where Lighthouse is getting this wrong?

You are correct that ordinary natural dimensions would be incorrect, but Lighthouse does not trust the natural dimensions in picture/srcset situations and instead creates a new image to measure the natural dimensions of the source URL

const canTrustNaturalDimensions = !isPicture && !element.srcset;

If you're not seeing this work as expected, a live example with expected/actual would be great :)

@eeeps
Copy link
Author

eeeps commented Jul 16, 2021

Ah! I missed that code path. Should have tested before I filed, apologies.

I'll make a small note that, when EXIF-based intrinsic image sizing becomes more widespread, even image resources that aren't embedded within srcsets could have intrinsic densities != 1. That's not common, yet, though. I'll file a new issue (with a minimal test!) if I start noticing it in the wild.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants