Control of trns chunk

[mastrodaro]

Allowed to pass pallete w/o alpha, added alpha parameter to check this while creating png stream. No alpha will result in skiping trns to be generated.

Thanks for contributing!

• Have you updated CHANGELOG.md?

[zbjornson]

Looks good! Thanks for contributing this. I have a slightly different API proposal to consider, but am open to the current API also.

[zbjornson]

Instead of adding an export settings, what do you think of reusing the existing alpha setting from the canvas context attributes, e.g.:

const ctx = canvas.getContext("2d", {alpha: false}); // alpha: false
canvas.toBuffer("image/png", {palette: new Uint8Array([r1, g1, b1, r2, g2, b2])}); // so no alpha here

We currently automatically switch to RGB24 if alpha is false:

node-canvas/src/CanvasRenderingContext2d.cc

Lines 711 to 714 in bf5126b

	// alpha: false forces use of RGB24
	Local<Value> alpha = Nan::Get(ctxAttributes, Nan::New("alpha").ToLocalChecked()).ToLocalChecked();
	if (alpha->IsBoolean() && !Nan::To<bool>(alpha).FromMaybe(false)) {
	format = CAIRO_FORMAT_RGB24;

but we don't have to do that since RGB16_565 and RGB30 also have no alpha channel (i.e. {pixelFormat: "RGB30", alpha: false} is valid).

What do you think of that API? If you're okay with it, then the lines I quoted above just need to change to this:

      if (alpha->IsBoolean() && !Nan::To<bool>(alpha).FromMaybe(false) && format == CAIRO_FORMAT_ARGB32) {
        format = CAIRO_FORMAT_RGB24;
      }

and one or two places in this PR need to be updated to (1) store the alpha property value and (2) use it during encoding.

[mastrodaro]

This was my primary idea but after I saw this format selection algorithm I though it was too risky to modify the relation of RGB24 and alpha. I will try to reuse alpha from context creation.

[mastrodaro]

also I dont know how to read value that is set in backend (Imagebackend) [at Context2d CanvasRenderingContext2d.cc] to use used in PNG.h
there is somehow generated cairo_image_surface_get_format and I would like to achieve something like cairo_image_surface_get_alpha after I set alpha in backend

[zbjornson]

I think you need to add a new member to the Context2d or Canvas class that stores whether or not alpha is in use (add to CanvasRenderingContext2d.h or Canvas.h). If you don't do that, then I don't think there's a way for us to know if, say, an A8 image palette has transparency or not.

To use that value when encoding, you could add a property to the PngClosure struct (in closure.h) like bool alpha, and set it on the closure when setting up the PNG encoding task (everywhere you see parsePNGArgs used in Canvas.cc). That closure is passed in to the PNG encoder.

(Sorry for the slow reply, will try to respond faster so we can land this.)

[mastrodaro]

Ok I will try this hopefully tomorrow - last time I had problems to retrieve back data I set on backend.

[mastrodaro]

My changes so far:
same as with format I added alpha

static_cast<ImageBackend*>(canvas->backend())->setFormat(format);
static_cast<ImageBackend*>(canvas->backend())->setAlpha(alpha);

Not sure if needed but also added (similar to format) in header file:

static NAN_GETTER(GetFormat);
static NAN_GETTER(GetAlpha);

Not sure what should be default for alpha in construct - probably true to do not introduce breaking changes.
ImageBackend.h:

bool alpha = true;

So further I added (similar to format) in ImageBackend:

void ImageBackend::setAlpha(bool _alpha) {
	this->alpha = _alpha;
}

and also getter.

In result I was not able to fetch alpha in PNG.h similar as its done with format cairo_format_t format = cairo_image_surface_get_format(surface);

And I assume I should fetch it from surface somehow.

p.s. what exactly is closure, isnt it the params I pass to the canvas.createPNGStream? Then I am trying to move alpha I introduced in this object to the construct of attributes in call canvas.getContext.

[zbjornson]

static NAN_GETTER(GetFormat);
static NAN_GETTER(GetAlpha);

You can omit these. They're used for adding getters that are accessible from JS. Adding them would mean adding non-standard features, which we try to avoid.

Not sure what should be default for alpha in construct

This shouldn't matter as long as you're setting alpha in all code paths (i.e. for all pixelFormats) in GetContext.

In result I was not able to fetch alpha in PNG.h

I would:

1. Add bool alpha to PngClosure in closure.h
2. Set that property everywhere in Canvas.cc where you see parsePNGArgs in use currently, like closure.alpha = canvas->backend()->alpha.
3. Use it in PNG.h, like closure->closure->alpha.

(Untested, but roughly that...)

what exactly is closure

It's not a great name, especially since we have closure->closure in some places (and they're two different structs/classes!). It's just a struct of data that we define (in closure.h) that libpng passes around through all of its APIs for us, allowing us to have access to whatever data we need.