sharp icon indicating copy to clipboard operation
sharp copied to clipboard

resize fastShrinkOnLoad not working

Open smremde opened this issue 1 year ago • 6 comments

Possible bug

Is this a possible bug in a feature of sharp, unrelated to installation?

  • [x ] Running npm install sharp completes without error.
  • [x ] Running node -e "require('sharp')" completes without error.

If you cannot confirm both of these, please open an installation issue instead.

Are you using the latest version of sharp?

  • [x ] I am using the latest version of sharp as reported by npm view sharp dist-tags.latest.

If you cannot confirm this, please upgrade to the latest version and try again before opening an issue.

If you are using another package which depends on a version of sharp that is not the latest, please open an issue against that package instead.

What is the output of running npx envinfo --binaries --system --npmPackages=sharp --npmGlobalPackages=sharp?

What are the steps to reproduce?

resize a large image that should take advantage of fastShrinkOnLoad

// "/tmp/largeimage.jpg" is 12288x6144

const sharp = require('sharp');

{
    const start = +new Date();
    sharp("/tmp/largeimage.jpg").resize(3072, 1536, { fastShrinkOnLoad: true }).toFile("/tmp/shrink.sharp.jpg", (err, output) => {
        console.log(+new Date() - start)
    })
}

{
    const start = +new Date();
    sharp("/tmp/largeimage.jpg").resize(3072, 1536, { fastShrinkOnLoad: false }).toFile("/tmp/shrink.sharp.jpg", (err, output) => {
        console.log(+new Date() - start)
    })
}
1338
1342

What is the expected behaviour?

vips is faster

$ time vips jpegload /tmp/largeimage.jpg --shrink 4  /tmp/shrink.vips.jpg

real    0m0.588s
user    0m0.547s
sys     0m0.095s

Please provide a minimal, standalone code sample, without other dependencies, that demonstrates this problem

See above

Please provide sample image(s) that help explain this problem

I generated a blank 12288x6144 image and also a pure noise 12288x6144 image

smremde avatar May 04 '23 11:05 smremde

Reducing the input dimensions by exactly 1/4 to reach the target dimensions, as in this example, is susceptible to the libjpeg-turbo shrink-on-load rounding error detailed at https://github.com/lovell/sharp/issues/3066. For this reason, and for these specific dimensions, the fastShrinkOnLoad setting is skipped and a shrink of 2 is applied in both cases and a final Lanczos-based resize used for the remainder.

lovell avatar May 04 '23 21:05 lovell

I had noticed that issue, but not looked at the pipeline code. I can see that yes you are likely correct, this is what is happening.

Obviously it's a fairly large performance hit (over twice the runtime vs vips cli) that I don't think is necessary for this particular use. I can't see any of these artefacts in the vips cli output (but not too sure what I am looking for)

Do you see a way around this bar a custom implementation (we do a lot of 1/2, 1/4, 1/8 resizing - and cropping too, but I see the shrink on load is disabled with extract)

smremde avatar May 04 '23 23:05 smremde

We can't predict when libjpeg-turbo will differ in its rounding calculations (e.g. differing SIMD intrinsics) so must halve its use when a 1/2, 1/4, 1/8 shrink produces one of the target dimensions to avoid the upscaling by 1 pixel problem.

I guess we could make a breaking change so this setting becomes an enum named something like "shrinkOnLoad" and accept one of: "fastest" (may be rounding errors, moire etc.), "fast" (equivalent to fastShrinkOnLoad=true, default), "slower" (equivalent to fastShrinkOnLoad=false), or "none".

lovell avatar May 05 '23 08:05 lovell

Thanks for helping me understand this better. I think you are saying that only certain architectures will experience the bug?

If so I can verify, this is not an issue on my system for /2, /4, /8 divisions and whole pixel outputs, with the following code:

(async () => {
    const input = `/run/shm/in.jpg`
    const output = `/run/shm/out.gif`
    for (let factor of [8, 4, 2]) {
        for (let width = factor; width * factor <= 65536; width += factor) {
            const info = await sharp({ create: { width: width, height: 8 * factor, channels: 3, background: 'red' } }).jpeg({ quality: 92 }).toFile(input);
            {
                const child = spawn('djpeg', ['-scale', `1/${factor}`, '-gif', '-outfile', output, input]);
                await new Promise((resolve, reject) => {
                    child.on('close', resolve);
                });
                const meta = await sharp(output).metadata();
                if (meta.width * factor != width || meta.height != 8)
                    console.log(`djpeg width ${width} and factor ${factor} FAILED ${meta.width} != ${width / factor} || ${meta.height} != 8`);
            }
            {
                const child = spawn('vips', ['jpegload', input, '--shrink', `${factor}`, output]);
                await new Promise((resolve, reject) => {
                    child.on('close', resolve);
                });
                const meta = await sharp(output).metadata();
                if (meta.width * factor != width || meta.height != 8)
                    console.log(`vips width ${width} and factor ${factor} FAILED ${meta.width} != ${width / factor} || ${meta.height} != 8`);
            }
        }
        console.log(`${factor} done`)
    }
})();

I can't find reference to the rounding error anywhere outside of this repo. Are there examples where shrinks on an input image which would divide exactly (i.e. width and height are integer multiples of jpegShrinkOnLoad) could cause this issue?

If this is the case, then yes, a third option use the fastest approach would help - I understand this is a breaking change and so potentially a custom native implentation would be beneficial. We do a lot of shrink (by powers of 2) and crops (DCT block aligned) and so this may be the best option.

Also, My C++ isn't strong, but from my understanding the mapping of shrink to jpegShrinkOnLoad looks like the second column of the table below. proposedQuality below is what I think it should equal (more speed same quality) and proposedFast is what we discussed.

If this is correct is there a bug for example, shrink of 2.1 could use jpegShrinkOnLoad 2, 4.9 could use 4 etc

shrink current proposedQuality proposedFast
0.1 1 1 1
0.9 1 1 1
1.0 1 1 1
1.1 1 1 1
1.9 1 1 1
2.0 1 1 2
2.1 1 2 2
2.9 1 2 2
3.0 2 2 2
3.1 2 2 2
3.9 2 2 2
4.0 2 2 4
4.1 2 4 4
4.9 2 4 4
5.0 4 4 4
5.1 4 4 4
5.9 4 4 4
6.0 4 4 4
6.1 4 4 4
6.9 4 4 4
7.0 4 4 4
7.1 4 4 4
7.9 4 4 4
8.0 4 4 8
8.1 4 8 8
8.9 4 8 8
9.0 8 8 8
9.1 8 8 8
9.9 8 8 8

smremde avatar May 05 '23 15:05 smremde

Are there examples where shrinks on an input image which would divide exactly (i.e. width and height are integer multiples of jpegShrinkOnLoad) could cause this issue?

#3526 was one recent example that provides useful dimensions to test with (500x399).

lovell avatar May 11 '23 16:05 lovell

Hi Lovell,

Yes all the examples I have seen require rounding. I'm looking for examples that do not require rounding.

My hypothesis is that if shrink is a power of two and orginalWidth % shrink == 0 and originalHeight % shrink == 0 and fastShrinkOnLoad==true, then then jpegShrinkOnLoad = shrink should be fine

Best

smremde avatar May 11 '23 17:05 smremde