sizeByPixelDensity was deprecated because of a bug

[bripkens]

Description

Gatsby v2 is reporting that the sizeByPixelDensity option is deprecated (spamming our gatsby develop mode quite heavily in fact). Specifically gatsby-plugin-sharp documents this as a deprecated config option, but gatsby-remark-images doesn't.

Also the deprecation reason in gatsby-plugin-sharp is wrong. This feature does work fine for JPEG and PNG files in Gatsby v1, but it is broken in Gatsby v2. We are in fact using this extensively for https://docs.instana.io. Looking through some tickets that were opened around this topic, it seems this is the result of a lot of confusion. Most specifically #12743 as a result of which 4131a09 was made. Especially the deprecation commit removes a test which proves that it can work.

As far as I can tell (as the original author of this feature as part of Gatsby v1):

a) The feature works as proven by the test and our own heavy usage in Gatsby v1.
b) The feature was broken as part of Gatsby v2.
b) There is demand for this feature as shown by at least one more bug ticket asking for such a capability.
c) Pixel density is not a simple topic and there is a lot of confusion around this. Possibly needing more examples? Additional test cases might have helped as well to avoid the breakage with Gatsby v2.

Steps to reproduce

Add the following plugin configuration:

{
  resolve: `gatsby-remark-images`,
  options: {
    maxWidth: 1000,
    quality: 80,
    jpegProgressive: true,
    pngCompressionLevel: 9,
    sizeByPixelDensity: true,
    linkImagesToOriginal: false,
    showCaptions: false,
  },
}

Use an image with resolution 144. Ensure that it renders at half the actual horizontal pixels.

Expected result

sizeByPixelDensity should not be deprecated. Instead the bug introduced with Gatsby v2 should be fixed.

The following screenshot shows what it looks like with Gatsby v1 (works as intended). Note the size of the original picture in MacOS' image viewer. Both browser and image viewer are showing the image at 100% size.

Actual result

A bug was introduced with Gatsby v2, breaking the feature, causing a lot of confusion which ended with the deprecation of sizeByPixelDensity. The following screenshot shows what it looks like with Gatsby v2 (sorry, cannot link to the deployed version because we aren't ready to ship this yet). Note the size of the original picture in MacOS' image viewer. Both browser and image viewer are showing the image at 100% size.

Environment

  System:
    OS: macOS 10.15.4
    CPU: (8) x64 Intel(R) Core(TM) i7-7700HQ CPU @ 2.80GHz
    Shell: 3.2.57 - /bin/bash
  Binaries:
    Node: 10.18.0 - ~/.nvm/versions/node/v10.18.0/bin/node
    Yarn: 1.21.1 - ~/.nvm/versions/node/v10.18.0/bin/yarn
    npm: 6.14.4 - ~/.nvm/versions/node/v10.18.0/bin/npm
  Languages:
    Python: 2.7.16 - /usr/local/bin/python
  Browsers:
    Chrome: 81.0.4044.113
    Firefox: 75.0
    Safari: 13.1
  npmPackages:
    gatsby: ^2.20.23 => 2.20.23
    gatsby-plugin-algolia: ^0.7.0 => 0.7.0
    gatsby-plugin-anchor-links: ^1.1.1 => 1.1.1
    gatsby-plugin-breadcrumb: ^9.0.1 => 9.0.1
    gatsby-plugin-catch-links: ^2.2.2 => 2.2.2
    gatsby-plugin-dark-mode: ^1.1.1 => 1.1.1
    gatsby-plugin-less: ^3.1.2 => 3.1.2
    gatsby-plugin-manifest: ^2.3.4 => 2.3.4
    gatsby-plugin-mdx: ^1.1.8 => 1.1.8
    gatsby-plugin-meta-redirect: ^1.1.1 => 1.1.1
    gatsby-plugin-react-helmet: ^3.2.3 => 3.2.3
    gatsby-plugin-react-svg: ^3.0.0 => 3.0.0
    gatsby-plugin-sharp: ^2.5.5 => 2.5.5
    gatsby-plugin-smoothscroll: ^1.1.0 => 1.1.0
    gatsby-redirect-from: ^0.2.1 => 0.2.1
    gatsby-remark-autolink-headers: ^2.2.2 => 2.2.2
    gatsby-remark-code-buttons: ^2.0.4 => 2.0.4
    gatsby-remark-copy-linked-files: ^2.2.3 => 2.2.3
    gatsby-remark-embed-markdown: 0.0.4 => 0.0.4
    gatsby-remark-embed-snippet: ^4.2.3 => 4.2.3
    gatsby-remark-external-links: ^0.0.4 => 0.0.4
    gatsby-remark-images: ^3.2.3 => 3.2.3
    gatsby-remark-images-medium-zoom: ^1.5.0 => 1.5.0
    gatsby-remark-prismjs: ^3.4.2 => 3.4.2
    gatsby-remark-relative-images: ^0.3.0 => 0.3.0
    gatsby-source-filesystem: ^2.2.3 => 2.2.3
    gatsby-transformer-remark: ^2.7.2 => 2.7.2
    gatsby-transformer-sharp: ^2.4.5 => 2.4.5
    gatsby-transformer-yaml-full: ^0.2.0 => 0.2.0
    gatsby-transformer-yaml-plus: ^0.2.2 => 0.2.2
    gatsby-yaml-full-file: ^0.2.0 => 0.2.0
    gatsby-yaml-full-markdown: ^0.2.1 => 0.2.1

[pieh]

Especially the deprecation commit removes a test which proves that it can work.

Yes, it can work, but there is more than few examples of it not working correctly.

sharp author/maintainer said that metadata.density is not really supported for jpeg/png:

• Expose setting xres/yres for JPEG/PNG output lovell/sharp#967 (comment)
• Manually set DPI TIFF image lovell/sharp#823 (comment)
  (this was in the issue you linked in your description).

We used density to make calculation for sizeByPixelDensity so density being unreliable, just made sizeByPixelDensity unreliable and "weirdness magnet".

That said - this might have changed in more recent sharp versions. I will try to grab same input images that caused problems and check with newer sharp version that we used at that time

[pieh]

Ok, so if you check https://github.com/pieh/sharp-density - it has one of the images that was causing problems referenced in the #12743 - running node index.js will show you density = 1829 - which ... is quite high? :) In any case this was causing problems like this https://twitter.com/jhooks/status/1108851826642706435

Just in case this is exactly this image https://github.com/eggheadio/how-to-egghead/blob/3a54c95c4fd1b9a779998732bac1f6713bf36d34/content/instructor/screencasting/audio-equipment/images/audio-setup/full-set-up.jpg and it wasn't tampered with by us ...

There might be nicer way to do this than just disabling entire option, but the problem with this was that it was very unreliable and people couldn't see those issues coming (there was no errors or anything like that), so potential for broken deploys was too great and we decided that this option is just not safe

[pieh]

Forgot to mention - above small test repository is using latest sharp so there wasn't changes there since removal of sizeByPixelDensity option. It's just likely that image metadata is either malformed or there is something wrong in sharp metadata handling

[bripkens]

Considering that you are carrying on with disabling this further: What is the Gatsby way of placing an image that is smaller than the content width while keeping it sharp for retina screens? So far it looks like we are only removing capabilities that at least work in some scenarios while not providing alternatives.

An alternative to disabling this completely could be to have some recommendations in this docs, accompanied by warnings/ignore rules for densities that do not seem to make sense. Possibly complemented by whitelisting of supported image resolutions.

[ichik]

Yeah, would've been nice for it to work if you know what you are doing and setting pixel density correctly.

[pieh]

Considering that you are carrying on with disabling this further

The docs PR was mostly to correctly reflect current state of things - please don't read too much into it "carrying on with disabling"

I found few original issues / pull requests about this:

• gatsby-plugin-sharp responsiveSizes maxWidth resizes to larger than the original #1509 (this one in particular have lots of discussion)
• remark-images blows up images to 100% width and ignores ppi #1982 (that was your original issue)
• remark-images blows up images to 100% width and ignores ppi #1987 (your PR adding density handling - no sizeByPixelDensity option initially)
• [gatsby-plugin-sharp] High DPI images are rendered tiny #2292 (issue about density support causing problems)
• Pixel density aware resizing should be optional #2713 (your PR making this optional and adding sizeByPixelDensity option)

The interesting findings in #1509 to me was that GitHub renderer only special cases 144DPI pretty much, and this (that + maybe few other whitelisted DPI) could be the solution here (so restore handling of the option - but only really do calculation if density is one of the whitelisted ones instead of supporting any value - as quite often there is "garbage" in the metadata that just cause more problems). So the way I could see this work is the option wouldn't be just true/false one. Instead it could be more nuanced:
just true could try to be safe and only support 144 dpi. Then you could say something like unsafeAll to have similar handling as previous true, and then potentially something like [144, 145] to allow arbitrary DPIs (as setting it this way require users to understand what this option does on deep level and hopefully not drop the support workload of the problematic stuff like this on maintainers)

The thing about documentation is that it's great when users learning how to do things or need a reference, but it's not so great when troubleshooting issues. What will happen if we would just restore sizeByPixelDensity option as it was, that will still get reports about issues that we then have to point to documentation - but also not everyone will have this in their mind always so likely we maintainers will spend lot of time re-debugging same problem over and over again.

Does above make sense?

[pieh]

Also the issue you created ( #1982 ) as "Summary of" #1509 explicetely say about 144DPI and nothing else, so it seems at least on this part we would be on same page?

Just additional thought - if we would be to re-add this - I would suggest changing the name of the option to signify a bit different behaviour (the old option deprecation message would be changed to lookup docs for the new one)

[bripkens]

Sounds like a fair suggestion @pieh! Thank you for your investigation! 👍

[github-actions]

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 30 days of inactivity. It’s been at least 20 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here. You can also add the label "not stale" to keep this issue open!
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

[github-actions]

Hey again!

It’s been 30 days since anything happened on this issue, so our friendly neighborhood robot (that’s me!) is going to close it.
Please keep in mind that I’m only a robot, so if I’ve closed this issue in error, I’m HUMAN_EMOTION_SORRY. Please feel free to reopen this issue or create a new one if you need anything else.
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks again for being part of the Gatsby community! 💪💜

[Zenkylo]

I understand we've had a regression with this issue. I'd like to offer my two cents as DPI is definitely a "real" aspect when it comes to some applications. In the web world this isn't much of an issue. We have native desktop applications that render images and when provided wrong DPI images they render improperly. Also for printing purposes there are also inconsistencies if a JPEG or PNG file is not provided proper DPI.

It's hard to get to the bottom of the property nomenclature as applications and APIs all seem to reference them differently. px/mm, dpi, resolution, ppi, ...densityetc.

Anyways, as far as Sharp, with the lack of support for density I've been using a base image with the DPI of my requirements and creating a composite with that image as the base. I have quite a few base images with varying DPI. I fetch the one I need and composite it with the transient image. The resulting output image appears to take the DPI of the base image in the composite.