gatsbyjs / gatsby

The best React-based framework with performance, scalability and security built in.
https://www.gatsbyjs.com
MIT License
55.26k stars 10.3k forks source link

BUG: MozJPEG with PNG and `toFormat: JPG` outputs PNG with jpg extension #10882

Closed polarathene closed 3 years ago

polarathene commented 5 years ago

Description

When enabling mozjpeg in my gatsby-config.js, jpg images do see better compression, but any png source images if using toFormat: JPG in their graphql query, will not actually format the image into jpeg, but instead output a png image data with a jpg file extension.

Steps to reproduce

Enable mozjpeg in gatsby-config.js for gatsby-plugin-sharp:

//   plugins: [
    {
      resolve: `gatsby-plugin-sharp`,
      options: {
        useMozJpeg: true,
      },
    },

Use the default starter project with gatsby new and edit the image.js component to include toFormat: JPG:

const Image = () => (
  <StaticQuery
    query={graphql`
      query {
        placeholderImage: file(relativePath: { eq: "gatsby-astronaut.png" }) {
          childImageSharp {
            fluid(maxWidth: 300 toFormat: JPG) {
              ...GatsbyImageSharpFluid
            }
          }
        }
      }
    `}
    render={data => <Img fluid={data.placeholderImage.childImageSharp.fluid} />}
  />
)

Build the site, notice no error, check the image assets, eg the same dimension will be output 800x800(despite the maxWidth: 300?) it is roughly 150KiB, vs about 34KiB when mozjpeg is not enabled. For the default displayed image of 300x300, it's 9KiB vs 40KiB(mozjpeg). It displays fine in the browser so can go unnoticed, if you use any image tools or for some file browsers(Dolphin for me) the image preview may fail to generate because it's not actually a jpeg image when inspected.

If unable to reproduce, this could be due to the build of mozjpeg installed to your system/OS. Some mozjpeg will support PNG input if they've been built with that support, mine does not. I can provide a reproducible project with an Alpine docker container if interested.

Expected result

Either actually convert into a JPG image format, or don't use jpg extension(and probably throw an error/warning instead of failing silently).

Actual result

PNG image format but jpg file extension. PNG did have slightly reduced filesize for the same dimensions but was still PNG data.

Environment

Gatsby v2.4.8, Custom Alpine Linux 3.8 Docker container, NodeJS v10, custom built mozjpeg(v3.4.0 via CMake not autotools, functions correctly for JPG, only static and win32 optionally support PNG via CMake builds). Project is the default starter from gatsby new.

polarathene commented 5 years ago

@Fetten seems to be the one who added the mozjpeg support, so might be able to chime in about this?

I'm not sure if there is an easy way to check for cjpeg(mozjpeg) having png support or not, but catching an error from mozjpeg or checking if the output is actually png data still could raise an exception to let the user know?

gurpreet-hanjra commented 5 years ago

@polarathene you might need to quality param when using toFormat. Something like this - fluid(maxWidth: 100, toFormat: JPG, quality: 100) Image created is JPG, without transparency. (Although docs doesn't mention about passing quality param as required)

Fetten commented 5 years ago

@polarathene Thanks for your report! I cannot see how this could be related to the MozJPEG implementation but would be glad to dig into it. Please let me know if the suggestion from @gurpreet-hanjra helps.

polarathene commented 5 years ago

@gurpreet-hanjra @Fetten Setting quality to 100 does regular PNG to JPG conversion for 800x800 size output of 245KiB, when enabling mozjpeg, same result, PNG image at 150KiB with jpg extension.

I don't think a quality param is required? As stated, PNG to JPG without enabling mozjpeg works fine and as expected. As does plain JPG with mozjpeg enabled, the issue is specific to using unsupported input format(I haven't tried other formats like webp, but expect similar behaviour).

If it is working for you(as in file contents are JPG not PNG regardless of the fact that the browser displays it), you should notice a filesize difference between regular compression with cjpeg vs mozjpeg cjpeg.

I cannot see how this could be related to the MozJPEG implementation but would be glad to dig into it.

I don't think it's anything wrong with your implementation per se, just that it's allowing unsupported formats to use mozjpeg due to toFormat being used. If the input image has not been converted to jpg already, then mozjpeg needs to support that input format, png is only supported optionally not all mozjpeg builds will include it.

Do you have any tests in place? Should an image format be converted to regular JPG at 100% lossless quality first before being run through mozjpeg? Or should mozjpeg support only allow JPG input? (Not sure how many are using PNG or other formats as input, or if they're aware this issue can happen where they think their images are being compressed and actually JPG format)

Fetten commented 5 years ago

Thanks for the feedback, @polarathene!

You are absolutely right, after a quick look I am also sure this is due to the toFormat here: https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-plugin-sharp/src/index.js#L264

Should an image format be converted to regular JPG at 100% lossless quality first before being run through mozjpeg? Or should mozjpeg support only allow JPG input?

Converting non-jpgs first to jpg when mozjpeg is enabled should cover all cases in my opinion, although I'm not sure how the image results of conversion + mozjpeg will look like.

I won't be able to have a deeper look into and fix this before the end of next week. If you would like to submit a PR by yourself before then - feel free :)

polarathene commented 5 years ago

I'm not sure if you should go ahead with this, at least not specifically for mozjpeg. I'll need to confirm against the default mozjpeg binary that is provided via the npm package, for majority of users it might support PNG by default.

It's not clear if the upcoming release will still support PNG with CMake, still waiting on input on how to build with that, and assume the npm package won't release a newer binary without PNG support. When the npm package is unable to use an existing binary, it does attempt to build from source, which may/may not build with PNG support if libpng isn't available on the system?

I think this can be also said for the sharp package or gatsby-plugin-sharp(which also uses imagemin packages that attempt to download a pre-built binary or compile from source). When sorting out my Alpine image, these binaries did not support musl, and it's not practical to build node_modules into the Docker image, or include the build tools(bloat), so I provided my own binaries. During that process, I remember gatsby-plugin-sharp working fine unless I used withWebp which called cwebp that couldn't find cjpeg or libpng iirc and threw an error/exception preventing the build. (Note: at node_modules install time, npm install would fail and that failure would be visible, but yarn appears successful without verbose output).


TL;DR: It's probably better to detect the problem and throw an error/exception instead if possible rather than silently fix it. Besides, what about those that do have mozjpeg with PNG support? They would suffer possible quality loss and extra processing time regardless?

Another option might be to restrict to JPG input only(reject toFormat: JPG inputs) with a config option to release the constraint(I'm not sure if more than PNG is supported, if not then it could be specific to that), another option could force the extra conversion to regular JPG first, but I know maintainers are not fond of piling on config options.. in which case, detecting the output isn't the expected image format and throwing an exception is probably best(and generic for all not just mozjpeg), not sure if that incurs much overhead.

polarathene commented 5 years ago

Going with last suggestion for handling this type of problem beyond just mozjpeg, image-type could work.

gatsbot[bot] commented 5 years ago

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!

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

gatsbot[bot] commented 5 years ago

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.

Thanks again for being part of the Gatsby community!

polarathene commented 5 years ago

Still a valid bug.

jeanregisser commented 5 years ago

Got bit by this too. Can we reopen?

jeliasson commented 5 years ago

Also got this problem using gatsby version 2.1.31 and the starter template (gatsby new <name>) together with a Dockerfile based of node:alpine. Thoughts?

polarathene commented 5 years ago

Suggestion for fix is to have a method that can verify the image's mime type matches the extension, and if it doesn't, throw an Error so the developer is aware there was an issue during Sharp processing for the file.


In this case, if you want to fix the problem causing it, don't use MozJPEG with PNG images if your version of MozJPEG doesn't include support for PNG input. You can convert the PNG to JPG first(but that should be done by you prior to processing, users with PNG support shouldn't have to incur an additional conversion).

jeliasson commented 5 years ago

In my case, I'm not even trying to convert it to JPEG from what I can tell.

const Image = () => (
  <StaticQuery
    query={graphql`
      query {
        placeholderImage: file(relativePath: { eq: "gatsby-astronaut.png" }) {
          childImageSharp {
            fluid(maxWidth: 300) {
              ...GatsbyImageSharpFluid
            }
          }
        }
      }
    `}
    render={data => <Img fluid={data.placeholderImage.childImageSharp.fluid} />}
  />
)

Also, how can I include support for MozJPEG and/or MozPNG on a alpine-image. Whhat apk packages may that me? Thanks!

polarathene commented 5 years ago

@jeliasson Then your issue is unrelated to this bug, MozJPEG is only for JPEG outputs, there are versions that can take PNG images as inputs(but it's unclear that will be supported in newer versions built with CMake).

I haven't touched my Docker alpine gatsby image for a few months now, it wasn't very reliable(npm packages installed binaries for png/webp and maybe jpg that were not compatible for alpine, sharp needed libvips if it's binary wasn't compatible at the time, when pre-built binaries did work, it wasn't updated to the latest sharp like other binaries even though a specific bug only affecting alpine was patched).

I have a multi-stage docker image that builds sharp binary for alpine, mozjpeg also required being built. for the imagemin packages that bring in cwebp(webp support) and pngquant(png support), I used alpine local packages and copied over the binaries(their packages don't support using local versions). It still had some issues though, so I never got around to publishing the images.

I'd advise using a different base image and accepting the extra weight, you can give alpine build dependencies but that will add 200MB roughly to size.

Another bit of advice if you're not aware, you can't run npm/yarn locally for packages and have the binaries work in your alpine image, they're not likely compatible, so you must install node packages inside the container(which may lose cache whenever you kill the container).

jeliasson commented 5 years ago

@polarathene Thanks for your extended answer! I don't mind having larger images, but are more concerned about build times. I'll see what I can come up with. Thanks again!

polarathene commented 5 years ago

While my suggestion for checking the image output data is what it should be(and not just a different extension) is still worthwhile to consider it appears that the package being used for this doesn't actually support PNG input at all.

Gatsby support should then raise an error or convert to JPG(at 100% quality?) before passing over to mozjpeg? @Fetten

wardpeet commented 5 years ago

we could convert it to a jpg buffer before passing it to mozjpg if it's not a jpg

gatsbot[bot] commented 5 years ago

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!

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

LekoArts commented 3 years ago

Closing this as stale since in the meantime Gatsby v3 and updated related packages were released. Please try with the latest versions and if you still see this problem open a new bug report (it must include a minimal reproduction).