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

feat(gatsby-plugin-sharp): Add support for pngquant speed option #9563

Merged
merged 5 commits into from
Jan 16, 2019
Merged

feat(gatsby-plugin-sharp): Add support for pngquant speed option #9563

merged 5 commits into from
Jan 16, 2019

Conversation

ascorbic
Copy link
Contributor

@ascorbic ascorbic commented Oct 30, 2018

This adds a pngCompressionSpeed option to image queries, which is passed through to pngquant as the speed option. In most cases this shouldn't be used, but in some cases with large numbers of PNGs this can make a significant difference in build times.

As explained in imagemin-pngquant; "Speed 10 has 5% lower quality, but is 8 times faster than the default."

Contrary to the docs in pngquant and imagemin-pngquant, the default seems to be 0, not 3. I initially defaulted to 3, but the snapshots broke, which is how I saw that the docs were wrong.

@ascorbic ascorbic changed the title Sharp: Add support for pngquant speed option feat(gatsby-plugin-sharp): Add support for pngquant speed option Oct 30, 2018
@pieh
Copy link
Contributor

pieh commented Nov 12, 2018

Was checking linked issue and seems like speed 0 is just using default (which now seems to be 4?) - might be worth to adjust docs before merging this?

@wardpeet
Copy link
Contributor

@ascorbic I wanted to update your PR so we can get this merged but I don't have access.

could you enable this feature https://blog.github.com/2016-09-07-improving-collaboration-with-forks/ or could you do the changes yourself.

Here's a patch file:

From 73f9fbd2698a86ae0eb08e958cd27c5535ccfef6 Mon Sep 17 00:00:00 2001
From: Ward Peeters <[email protected]>
Date: Fri, 11 Jan 2019 09:37:38 +0100
Subject: [PATCH] update comments

---
 packages/gatsby-plugin-sharp/README.md    | 4 ++--
 packages/gatsby-plugin-sharp/src/index.js | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/packages/gatsby-plugin-sharp/README.md b/packages/gatsby-plugin-sharp/README.md
index 418275e..e8cebf1 100644
--- a/packages/gatsby-plugin-sharp/README.md
+++ b/packages/gatsby-plugin-sharp/README.md
@@ -14,7 +14,7 @@ For JPEGs it generates progressive images with a default quality level of 50.
 For PNGs it uses [pngquant](https://github.com/pornel/pngquant) to compress
 images. By default it uses a quality setting of [50-75]. The `pngCompressionSpeed`
 value is a speed/quality trade-off from 1 (brute-force) to 10 (fastest). Speed
-10 has 5% lower quality, but is 8 times faster than the default (0). In most
+10 has 5% lower quality, but is 8 times faster than the default (4). In most
 cases you should stick with the default, but if you have very large numbers
 of PNGs then it can significantly reduce build times.

@@ -114,7 +114,7 @@ following:
 - `duotone` (bool|obj, default: false)
 - `toFormat` (string, default: '')
 - `cropFocus` (string, default: '[sharp.strategy.attention][6]')
-- `pngCompressionSpeed` (int, default: 0)
+- `pngCompressionSpeed` (int, default: 4)

 #### toFormat

diff --git a/packages/gatsby-plugin-sharp/src/index.js b/packages/gatsby-plugin-sharp/src/index.js
index 9ae79db..4062bdd 100644
--- a/packages/gatsby-plugin-sharp/src/index.js
+++ b/packages/gatsby-plugin-sharp/src/index.js
@@ -66,7 +66,8 @@ const generalArgs = {
   quality: 50,
   jpegProgressive: true,
   pngCompressionLevel: 9,
-  pngCompressionSpeed: 0,
+  // default is 4 (https://github.com/kornelski/pngquant/blob/4219956d5e080be7905b5581314d913d20896934/rust/bin.rs#L61)
+  pngCompressionSpeed: 4,
   base64: false,
   grayscale: false,
   duotone: false,
--
2.7.4

you can apply it with git am < file.patch

@wardpeet wardpeet self-assigned this Jan 11, 2019
@wardpeet wardpeet added the status: awaiting author response Additional information has been requested from the author label Jan 11, 2019
@ascorbic
Copy link
Contributor Author

@wardpeet I do have the "allow edits" option enabled, so I'm not sure why you can't edit. I'll apply the patch.

We need to be clear that this is a breaking change, but so will upgrading to the fixed version of pngquant as it no longer supports 0 as a speed. All images with no compression speed set (i.e. all of them currently) will be compressed at level 4 instead, which is quicker but with a larger file size. As @kornelski points out in the linked issue, we shouldn't be using snapshot tests on images anyway, as pngquant doesn't guarantee bit-exact reproducibility.

@wardpeet
Copy link
Contributor

wardpeet commented Jan 11, 2019

probably something wrong with my setup than. (thanks you!)

I'm not certain why this is a breaking change, we're not upgrading pngquant and by default this is 4, if you set it to 0.

@ascorbic
Copy link
Contributor Author

It was a bug in pngquant: it was supposed to default to 4, but until the change was made recently it was mistakenly defaulting to 0.

@kornelski
Copy link

kornelski commented Jan 11, 2019

Please don't try to use speed 0 for anything. I'm going to make it a hard error in the next version.

0 never existed as a speed setting in pngquant. It never did anything. It never was a default. The whole 0 rumor is a misinterpretation of pngquant's behavior when parsing command-line arguments, where invalid arguments (such as nonsense speed 0) were ignored and did nothing.

To get the default speed, don't set any (i.e don't set --speed, don't call liq_set_speed). If you set any number, that's your choice. Valid values start at 1.

@kornelski
Copy link

kornelski commented Jan 11, 2019

If you want to have an integration test that verifies that pngquant works well quality-wise, then I recommend using an image comparison tool.

I use dssim which is pretty robust and gives reliable results for images that pngquant outputs, including dithering.

Other tools may be fine too, but be careful if you compare with a tool that computes difference naively per pixel (like imagemagick's compare).

  • you can safely compare images without dithering (--nofs). Their overall quality will remain pretty stable, even if palettes are different.

  • if you test images with dithering: you must be careful, because dithering noise can change, and to naive tools this will look like a much bigger difference than it really is. One way around this is to compare images after blurring or scaling down by 50%, so that neighboring dithering pixels are mixed together, so you're checking whether overall color is OK.

@ascorbic
Copy link
Contributor Author

@kornelski What confused me was that as my file list here showed, the previous behaviour when it was passed no arguments was to generate files that were not the same as any other speed option - and were smaller than --speed 1.

@kornelski
Copy link

kornelski commented Jan 11, 2019

@ascorbic as I've said, it was a misinterpretation of what happened. The speed 0 argument was totally ignored because of invalid, unsupported value. It doesn't mean 0 is the default. It means default is the default, and was never changed from the default due to invalid arguments given.

pngquant # no setting, pngquant --speed 0 and pngquant --speed i-love-ponies all used to give the same result, but it doesn't mean that i-love-ponies is the default speed setting.

@ascorbic
Copy link
Contributor Author

@kornelski No, I realise that 0 wasn't the default and was just an invalid value - I just still don't understand what it was actually defaulting to, as there was no valid speed value that gave the same output as leaving off the --speed flag or passing an invalid value.

@kornelski
Copy link

kornelski commented Jan 11, 2019

Yes, that was a documentation being out of date. At some point I changed the default value without updating the documentation about what speed setting is closest to the internal default setting.

@ascorbic
Copy link
Contributor Author

OK, but is there a speed setting that is the same as the internal default? When I checked before, I couldn't find one that was.

@kornelski
Copy link

Before 2.12 it was 3, after 2.12 it's 4:

ImageOptim/libimagequant@e3e6a19

@ascorbic
Copy link
Contributor Author

OK, I think what threw me was that my test image was smaller when compressed with the default speed than it was with speed 3, so I assumed it must be a slower speed setting. Anyway, thanks for making pngquant: it's awesome, and thanks for clearing that up.

@kornelski
Copy link

kornelski commented Jan 11, 2019

The speed setting affects amount of effort put into the algorithm, which mostly but not always means higher visual quality.

However, higher-quality image may take less space because there's less dithering noise, OR it may take more space, because there's more detail. So there's no certain relationship between speed setting and file size.

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wardpeet wardpeet merged commit b789689 into gatsbyjs:master Jan 16, 2019
gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019
…sbyjs#9563)

This adds a `pngCompressionSpeed` option to image queries, which is passed through to pngquant as the `speed` option. In most cases this shouldn't be used, but in some cases with large numbers of PNGs this can make a significant difference in build times.

As explained in [imagemin-pngquant](https://github.com/imagemin/imagemin-pngquant#imageminpngquantoptionsinput); _"Speed 10 has 5% lower quality, but is 8 times faster than the default."_

[Contrary to the docs](kornelski/pngquant#313) in pngquant and imagemin-pngquant, the default seems to be 0, not 3. I initially defaulted to 3, but the snapshots broke, which is how I saw that the docs were wrong.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting author response Additional information has been requested from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants