lovell / sharp

High performance Node.js image processing, the fastest module to resize JPEG, PNG, WebP, AVIF and TIFF images. Uses the libvips library.
https://sharp.pixelplumbing.com
Apache License 2.0
29.17k stars 1.3k forks source link

Possible regression issue with `extract()` since v0.31 #3768

Closed mpaperno closed 1 year ago

mpaperno commented 1 year ago

Possible bug

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

Are you using the latest version of sharp?

The issue also affects versions >= 0.31.3 (prior 0.31.x releases not tested). The test script below runs fine with v0.30.7.

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

  System:
    OS: Windows 10 10.0.19045
    CPU: (16) x64 Intel(R) Core(TM) i7-7820X CPU @ 3.60GHz
    Memory: 35.97 GB / 63.70 GB
  Binaries:
    Node: 18.16.0 - nodejs\node.EXE
    npm: 9.5.1 - nodejs\npm.CMD
  npmPackages:
    sharp: 0.32.5 => 0.32.5

What are the steps to reproduce?

Since sharp v0.31, trying to repeatedly extract() parts of a loaded sharp() image instance fails after the first extract() call.

This worked up through v0.30.7.

See example below. If the image is reloaded before each extract() call then it works (see commented line). Seems like now the original image instance is modified or locked or something.

Perhaps I was doing it "wrong" before, but it sure is faster w/out reloading the image for each tile.

Anecdotally, at this point, in my actual program which uses sharp the tile extraction still works when it's only vertical (each tile is the same width as the original image), but fails when it tries to tile the image horizontally. However, I cannot recreate that with my example, which always fails. I haven't tracked down the difference, which is almost literally the same code but the sharp() image is constructed from PNG data in a Buffer instead of a file. 🤷🏼

What is the expected behaviour?

All parts of the image are extracted.

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

const sharp = require('sharp');

const imageFile = './octocat-200x200.png'
const tileSize = { w: 50, h: 50 };

async function process() {

    let img = sharp(imageFile);
    const meta = await img.metadata();

    console.log(
        `Extracting ${Math.floor(meta.width / tileSize.w)}x${Math.floor(meta.height / tileSize.h)} ${tileSize.w}x${tileSize.h} tiles for ${imageFile} from (${meta.width}x${meta.height})`
    );

    for (let x=0; x < meta.width; x += tileSize.w) {
        for (let y=0; y < meta.height; y += tileSize.h) {
            img.extract({ left: x, top: y, width: tileSize.w, height: tileSize.h })
            .png()
            // .toFile(`./image_tile_${x}_${y}.png`)
            .toBuffer()
            .then(() => {
                console.log(`Extracted tile at (${x}, ${y})`);
            })
            .catch((e) => {
                console.error(`Exception while extracting tile at (${x}, ${y}):\n`, e.stack);
            });
            // img = sharp(imageFile);  // fix for sharp > 0.30.7
        }
    }
}

process()
  .then(() => console.log('ok'))
  .catch((err) => console.error(err));

With sharp 0.30.7:

Extracted tile at (0, 0)
Extracted tile at (0, 100)
Extracted tile at (0, 150)
Extracted tile at (0, 50)
Extracted tile at (50, 0)
Extracted tile at (50, 50)
Extracted tile at (50, 100)
Extracted tile at (50, 150)
Extracted tile at (100, 0)
Extracted tile at (100, 50)
Extracted tile at (100, 100)
Extracted tile at (100, 150)
Extracted tile at (150, 0)
Extracted tile at (150, 50)
Extracted tile at (150, 100)
Extracted tile at (150, 150)

With later versions:

Exception while extracting tile at (0, 50):
 Error: extract_area: bad extract area
Exception while extracting tile at (0, 150):
 Error: extract_area: bad extract area
extract_area: bad extract area
extract_area: bad extract area
Exception while extracting tile at (0, 100):
 Error: extract_area: bad extract area
extract_area: bad extract area
extract_area: bad extract area
Extracted tile at (0, 0)
Exception while extracting tile at (50, 50):
 Error: extract_area: bad extract area
Exception while extracting tile at (50, 100):
 Error: extract_area: bad extract area
... more of the same

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

octocat-200x200


Please let me know if I can provide any more details.

Thanks! -Max

lovell commented 1 year ago

Please create a new instance with its own state per extraction - it's cleaner and almost zero-cost.

- img.extract({ left: x, top: y, width: tileSize.w, height: tileSize.h })
+ sharp(imageFile).extract({ left: x, top: y, width: tileSize.w, height: tileSize.h })

The sample code you've provided appears to be creating a single-depth image pyramid, so please see https://sharp.pixelplumbing.com/api-output#tile which, if it does what you need, will be faster and mean less code for you to maintain.

mpaperno commented 1 year ago

Hi, thanks very much for your reply.

Please create a new instance with its own state per extraction - it's cleaner and almost zero-cost.

I benchmarked the difference (with v0.30.7), and while it's not as much slower as I thought (in the benchmark at least), loading the data each time takes a bit longer and uses a few more resources (CPU/RAM). The speed seems a little improved in v0.32.5 though of course I can only test the "cleaner" way. I'm not clear why it's cleaner to load the same data multiple times.

FWIW, in our actual program the input data is being read out of a Buffer (not a file), and we're doing the tiling in "real-time" and potentially at high frequency with multiple near-simultaneous requests (yea, I know, "why are you using JS?" but that's a different topic). Anyway, it works fine, but every bit of savings helps.

The sample code you've provided appears to be creating a single-depth image pyramid, so please see https://sharp.pixelplumbing.com/api-output#tile which, if it does what you need, will be faster and mean less code for you to maintain.

I've seen that but, pardon my ignorance, didn't know how I could use it. What we need is each tile individually as a PNG Buffer (or some way to get each tile efficiently into base64) to be sent over a network. The image pyramid feature sounds like it outputs some kinda archive/collection which I'd then have to unpack.

The current code to maintain is literally that one loop like in my demo... simple and effective. But if there's a faster/more efficient way to do it, I'd love to know more.

Thanks, -Max

lovell commented 1 year ago

Thanks for the extra info, it sounds like tile() is not quite what you need.

libvips tries to be as efficient as possible and will attempt to decode as few input pixel values as it can in order to populate the region to be extracted. It also overlaps decode and encode. The bottleneck in the scenario you describe will probably be compression of the PNG output.

I highly recommend using the latest version of sharp as it provides the latest zlib-ng and therefore much faster PNG deflate compression on most CPUs.

mpaperno commented 1 year ago

libvips tries to be as efficient as possible and will attempt to decode as few input pixel values as it can in order to populate the region to be extracted. It also overlaps decode and encode.

Ah, that's clever, and does explain the "less than I expected" hit of loading the data each time.

And yes, in fact the compression is what we're really after here... I can just generate tiles from the original image w/out the PNG format intermediary (which is more efficient of course) but unfortunately the compression level on that output is low and the data is significantly larger than what Sharp/vips can do. So it's a trade-off, CPU vs. data transfer size.

Anyhow, the newer version does seem more performant indeed, so I'll give that another shot in real-world use with the method you suggested.

Thank you for your input, and all your work on Sharp! -Max

lovell commented 1 year ago

I hope this information helped. Please feel free to re-open with more details if further assistance is required.

mpaperno commented 1 year ago

Well my point of filing this was to report what seemed like a regression... it used to work, now it doesn't, and nothing in release notes about the function changing. Do with that as you will (or did, rather :) ).

I do appreciate that the workaround (which I'd already discovered, sorry for not making that clear) is still performant enough, and that sharp/vips makes the most efficient use of the data. There are clearly more allocations happening this way, but I'll take it.

Thanks!