Support toBuffer("image/jpeg") and unify export configs

[zbjornson]

Docs showing the new API: https://github.com/zbjornson/node-canvas/tree/tobufferjpeg#canvastobuffer

• Added: support for canvas.toBuffer("image/jpeg"). This unblocks Missing sync canvas.toDataURL("image/jpeg") API #1146 and helps make canvas.toBuffer() the universal method for getting encoded data out of a canvas. Unblocks adding toBlob if we want it -- a later topic.

• Changed (breaking): PNG filter and ZLIB compression options are now named instead of positional for canvas.toBuffer and (undocumented) canvas.pngStream. Less awkward API, easier support for more parameters in the future (including Output high resolution PNG #716/PNG files written with wrong resolution in header #766).

• Improved: canvas.toBuffer, canvas.jpegStream and canvas.pngStream now all accept the same config objects. Previously some options were only available when streaming.

• Removed (breaking): The weird argument handling that allowed the compression level "0" (string). The logic in that section was all sort of weird, not sure if I'm misreading it.

• Changed (breaking): The quality argument for jpegStream now goes from 0 to 1 to match the quality argument in canvas.toBlob.

• Question: Should there be a canvas.toBuffer(callback, "raw")? NO per below. There's no async work to be done here (just a memcpy). Adding this would make the API consist, but would encourage a bad usage pattern (callback-all-the-things!). There's also no canvas.toBuffer(callback) support for SVG or PDF canvases.

• image/jpeg uses the threadpool. Question: image/jpeg runs in the main thread currently, but that could reasonably be offloaded to the threadpool later. It's easy enough to invoke the callback in the next tick now so that if it moves to the threadpool in the future it'll be async now and in the future -- should it? One less semver major change in the future.

• Have you updated CHANGELOG.md?

Fixes #982

[LinusU]

🙌

Haven't looked too closely yet, but looks super good!

[zbjornson]

Alright, finally ready for review! Original post edited (two remaining questions).

[LinusU]

Should there be a canvas.toBuffer(callback, "raw")? Adding this would make the API consist, but would encourage a bad usage pattern (callback-all-the-things!).

My vote goes to not adding it in.

image/jpeg runs in the main thread currently, but that could reasonably be offloaded to the threadpool later. It's easy enough to invoke the callback in the next tick now so that if it moves to the threadpool in the future it'll be async now and in the future -- should it?

Sounds good 👍

[chearon]

Phew, this is gonna make things a lot less confusing. But why would someone use toBuffer(callback, ...) instead of .pngStream() or .jpegStream()? Is this to essentially match HTMLCanvasElement.toBlob()?

Since .toBlob() only supports JPEG/PNG, is there any reason we should have .toBuffer("raw")? It seems what ImageData is for, but maybe if people like it/are already using it we should leave it, but then there's the lack of toBuffer(callback, "raw" which might be confusing.

I don't know this part of the code that well, but am I right that this is also changing the behavior of toBuffer() without any arguments? It's a good change since it matches .toBlob() but I think this means people can't do image.src = canvas.toBuffer() like we say in the readme.

[zbjornson]

My vote goes to not adding [toBuffer(callback, "raw")] in.

Mine as well. We can add it later if someone has a compelling reason.

why would someone use toBuffer(callback, ...) instead of .pngStream() or .jpegStream()

You get the whole buffer at once instead of dealing with a stream. (Merits of each discussed starting here: #743 (comment).)

any reason we should have .toBuffer("raw")

You mean vs. canvas.getImageData()? See @adamhooper's nice justification here: #819 (comment) and #306 (comment)

am I right that this is also changing the behavior of toBuffer() without any arguments

No change there! There's still the synchronous version that returns a buffer if you don't provide a callback for the first arg.

---

I'll work on making JPEG pseudo-async shortly -- I have that in some branch from a year ago.

[zbjornson]

Okay, ready to go! I'm sorry this got so big, but I decided to clean up closure_t now instead of making it a bigger Frankenstein's monster.

toBuffer(callback, "image/jpeg") is truly async, using the libuv threadpool, not just nextTick.

Removed a redundant memcpy that was called in a loop.

Can someone with MacOS check that this builds please? (I enabled CPP exceptions in the gyp file and can only test on Win and Linux.)

[chearon]

Builds on macOS for me! Will try to provide bigger feedback soon.

[jfizz]

Hi @zbjornson, I tried your branch on my macbook. The sync version canvas.toBuffer('image/jpeg', {quality: 0.5}) worked but the async version canvas.toBuffer((err, buf) => {}, 'image/jpeg', {quality: 0.95}) failed with the error Error: <unknown error status>. Any thoughts? Below are my versions of the required dependencies.

node 8.9.4
pkg-config 0.29.2
cairo 1.14.12
pango 1.42.1
libpng 1.6.34
jpeg 9b
giflib 5.1.4

Also, after reading your comments above about toBuffer(callback, "image/jpeg") being truly async, the following line in the docs should probably be updated:

canvas.toBuffer((err, buf) => {
  if (err) throw err; // encoding failed
  // buf is JPEG-encoded image at 95% quality
  // Note that this callback is currently called synchronously.
}, 'image/jpeg', {quality: 0.95})

[zbjornson]

@jfizz thanks for trying it out and for catching the doc error.

That message sounds like it comes from libjpeg... Could you post a repro case for it please, or does it happen with any canvas?

[jfizz]

Here is the repo case.

const {createCanvas} = require('canvas');
const canvas = createCanvas(200, 200);
canvas.toBuffer((err, buf) => {
  console.log(err);
  console.log(buf);
}, 'image/jpeg', {quality: 0.95});

However, the plot thickens. I ran the above script 10 times in a row and it worked 7 / 10 times. For the 3 times it failed the error was Error: <unknown error status>.

[chearon]

Can confirm same behavior on macOS

$ node test.js
null
<Buffer ff d8 ff e0 00 10 4a 46 49 46 00 01 01 00 00 01 00 01 00 00 ff db 00 43 00 08 06 06 07 06 05 08 07 07 07 09 09 08 0a 0c 14 0d 0c 0b 0b 0c 19 12 13 0f ... >
$ node test.js
null
<Buffer ff d8 ff e0 00 10 4a 46 49 46 00 01 01 00 00 01 00 01 00 00 ff db 00 43 00 08 06 06 07 06 05 08 07 07 07 09 09 08 0a 0c 14 0d 0c 0b 0b 0c 19 12 13 0f ... >
$ node test.js
Error: <unknown error status>
undefined

[zbjornson]

Fixed docs and crash, thanks for finding. Want to give it another try please?

[jfizz]

Works for me!

[chearon]

Not sure if it belongs in this PR or not but createPNGStream and createJPEGStream aren't documented, but are used in the examples instead of pngStream and jpegStream. Choosing one or the other might further reduce confusion.

[zbjornson]

I guess create is idiomatic (following fs.createReadStreaam for example)...

[zbjornson]

Added a third commit to standardize on create*. (Didn't remove jpegStream/pngStream.)

(GitHub is showing the commits out of order now (why, issue).)

[chearon]

Sorry this PR is getting old @zbjornson. I don't know this part of the code well, but it looks great.

@LinusU ok to merge?

[LinusU]

Looks awesome!

[KFoxder]

@chearon @LinusU @zbjornson So I was trying to use the resolution option like so:

canvas.toBuffer('image/png', { compressionLevel: 0, resolution: 200 });

And I noticed no difference in the actual PPI of the image. I went digging through this code and I found the parsePNGArgs method. Nowhere in this code does it ever parse the resolution option. Can you confirm that this is a bug and not intended or that I am missing something?

static void parsePNGArgs(Local<Value> arg, PngClosure& pngargs) {
  if (arg->IsObject()) {
    Local<Object> obj = Nan::To<Object>(arg).ToLocalChecked();

    Local<Value> cLevel = obj->Get(Nan::New("compressionLevel").ToLocalChecked());
    if (cLevel->IsUint32()) {
      uint32_t val = Nan::To<uint32_t>(cLevel).FromMaybe(0);
      // See quote below from spec section 4.12.5.5.
      if (val <= 9) pngargs.compressionLevel = val;
    }

    Local<Value> filters = obj->Get(Nan::New("filters").ToLocalChecked());
    if (filters->IsUint32()) pngargs.filters = Nan::To<uint32_t>(filters).FromMaybe(0);

    Local<Value> palette = obj->Get(Nan::New("palette").ToLocalChecked());
    if (palette->IsUint8ClampedArray()) {
      Local<Uint8ClampedArray> palette_ta = palette.As<Uint8ClampedArray>();
      pngargs.nPaletteColors = palette_ta->Length();
      if (pngargs.nPaletteColors % 4 != 0) {
        throw "Palette length must be a multiple of 4.";
      }
      pngargs.nPaletteColors /= 4;
      Nan::TypedArrayContents<uint8_t> _paletteColors(palette_ta);
      pngargs.palette = *_paletteColors;
      // Optional background color index:
      Local<Value> backgroundIndexVal = obj->Get(Nan::New("backgroundIndex").ToLocalChecked());
      if (backgroundIndexVal->IsUint32()) {
        pngargs.backgroundIndex = static_cast<uint8_t>(Nan::To<uint32_t>(backgroundIndexVal).FromMaybe(0));
      }
    }
  }
}