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.

Sign up for GitHub

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

Jump to bottom

Control of trns chunk #1629

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Control of trns chunk #1629

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
664d5e4
Added possibility to skip tRNS chunk and set palette dividable by 3
mastrodaro Jul 24, 2020
7d82162
updated CHANGELOG.md
mastrodaro Jul 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ project adheres to [Semantic Versioning](http://semver.org/).
* Speed up `fillStyle=` and `strokeStyle=`
### Added
* Export `rsvgVersion`.
* Control over tRNS chunk generation.
### Fixed
* Fix BMP issues. (#1497)
* Update typings to support jpg and addPage on NodeCanvasRenderingContext2D (#1509)
Expand Down
11 changes: 7 additions & 4 deletions lib/pngstream.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,15 @@ var PNGStream = module.exports = function PNGStream(canvas, options) {
}

Readable.call(this);
this.alpha = options && options.alpha !== undefined ? options.alpha : true;

if (options &&
options.palette instanceof Uint8ClampedArray &&
options.palette.length % 4 !== 0) {
throw new Error("Palette length must be a multiple of 4.");
if (options && options.palette instanceof Uint8ClampedArray) {
var divider = this.alpha ? 4 : 3;
if (options.palette.length % divider !== 0) {
throw new Error(`Palette length must be a multiple of ${divider}.`);
}
}

this.canvas = canvas;
this.options = options || {};
};
Expand Down
16 changes: 12 additions & 4 deletions src/Canvas.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,14 +263,17 @@ static void parsePNGArgs(Local<Value> arg, PngClosure& pngargs) {
Local<Value> filters = Nan::Get(obj, Nan::New("filters").ToLocalChecked()).ToLocalChecked();
if (filters->IsUint32()) pngargs.filters = Nan::To<uint32_t>(filters).FromMaybe(0);

Local<Value> alpha = Nan::Get(obj, Nan::New("alpha").ToLocalChecked()).ToLocalChecked();
pngargs.alpha = Nan::To<bool>(alpha).FromMaybe(true);
Local<Value> palette = Nan::Get(obj, Nan::New("palette").ToLocalChecked()).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.";
uint8_t divider = pngargs.alpha ? 4 : 3;
if (pngargs.nPaletteColors % divider != 0) {
throw "Palette length must be a multiple of 4 (with alpha) or 3 (without alpha).";
}
pngargs.nPaletteColors /= 4;
pngargs.nPaletteColors /= divider;
Nan::TypedArrayContents<uint8_t> _paletteColors(palette_ta);
pngargs.palette = *_paletteColors;
// Optional background color index:
Expand Down Expand Up @@ -424,10 +427,15 @@ NAN_METHOD(Canvas::ToBuffer) {
try {
PngClosure closure(canvas);
parsePNGArgs(info[1], closure);
if (closure.nPaletteColors == 0xFFFFFFFF) {

if (closure.alpha && closure.nPaletteColors == 0xFFFFFFFF) {
Nan::ThrowError("Palette length must be a multiple of 4.");
return;
}
if (!closure.alpha && closure.nPaletteColors == 0xFFFFFF) {
Nan::ThrowError("Palette length must be a multiple of 3.");
return;
}

Nan::TryCatch try_catch;
status = canvas_write_to_png_stream(canvas->surface(), PngClosure::writeVec, &closure);
Expand Down
16 changes: 10 additions & 6 deletions src/PNG.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,20 +213,24 @@ static cairo_status_t canvas_write_png(cairo_surface_t *surface, png_rw_ptr writ

png_set_IHDR(png, info, width, height, bpc, png_color_type, PNG_INTERLACE_NONE, PNG_COMPRESSION_TYPE_DEFAULT, PNG_FILTER_TYPE_DEFAULT);

uint8_t backgroundIndex = closure->closure->backgroundIndex;
if (png_color_type == PNG_COLOR_TYPE_PALETTE) {
size_t nColors = closure->closure->nPaletteColors;
bool alpha = closure->closure->alpha;
uint8_t* colors = closure->closure->palette;
uint8_t backgroundIndex = closure->closure->backgroundIndex;
png_colorp pngPalette = (png_colorp)png_malloc(png, nColors * sizeof(png_colorp));
png_bytep transparency = (png_bytep)png_malloc(png, nColors * sizeof(png_bytep));
uint8_t divider = alpha ? 4 : 3;
for (i = 0; i < nColors; i++) {
pngPalette[i].red = colors[4 * i];
pngPalette[i].green = colors[4 * i + 1];
pngPalette[i].blue = colors[4 * i + 2];
transparency[i] = colors[4 * i + 3];
pngPalette[i].red = colors[divider * i];
pngPalette[i].green = colors[divider * i + 1];
pngPalette[i].blue = colors[divider * i + 2];
if (alpha) transparency[i] = colors[4 * i + 3];
}
png_set_PLTE(png, info, pngPalette, nColors);
png_set_tRNS(png, info, transparency, nColors, NULL);
if (alpha) {
png_set_tRNS(png, info, transparency, nColors, NULL);
}
png_set_packing(png); // pack pixels
// have libpng free palette and trans:
png_data_freer(png, info, PNG_DESTROY_WILL_FREE_DATA, PNG_FREE_PLTE | PNG_FREE_TRNS);
Expand Down
1 change: 1 addition & 0 deletions src/closure.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ struct PngClosure : Closure {
uint32_t resolution = 0; // 0 = unspecified
// Indexed PNGs:
uint32_t nPaletteColors = 0;
bool alpha = true;
uint8_t* palette = nullptr;
uint8_t backgroundIndex = 0;

Expand Down
27 changes: 24 additions & 3 deletions test/image.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,27 @@ describe('Image', function () {
assert.ok(!keys.includes('setSource'));
});

it('does not create chunks tRNS and bKGD if they are irrelevant', function() {
var bKGD = "624b4744";
var PLTE = "504c5445";
var tRNS = "74524e53";
var palette = new Uint8ClampedArray([0, 0, 0, 0, 0, 100, 0, 0, 200]);

var canvas = createCanvas(10, 10);
var ctx = canvas.getContext('2d', { pixelFormat: 'A8' })
var idata = ctx.getImageData(0, 0, 10, 10);
idata[0] = 0;
idata[1] = 1;
idata[2] = 2;
ctx.putImageData(idata, 0, 0);

// getting backgroundIndex out of palette to make bKGD to not appear
var bufferRGBNoBg = canvas.toBuffer('image/png', {compressionLevel: 0, filters: undefined, palette: palette, backgroundIndex: 3});
assert.strictEqual(bufferRGBNoBg.includes(PLTE, 0, "hex"), true);
assert.strictEqual(bufferRGBNoBg.includes(tRNS, 0, "hex"), false);
assert.strictEqual(bufferRGBNoBg.includes(bKGD, 0, "hex"), false);
});

describe('supports BMP', function () {
it('parses 1-bit image', function (done) {
let img = new Image();
Expand Down Expand Up @@ -386,7 +407,7 @@ describe('Image', function () {
255, 0, 0, 127,
255, 255, 255, 127,
]);

done();
};

Expand All @@ -404,7 +425,7 @@ describe('Image', function () {
testImgd(img, [
255, 0, 0, 255,
]);

done();
};

Expand All @@ -423,7 +444,7 @@ describe('Image', function () {
255, 0, 0, 255,
0, 255, 0, 255,
]);

done();
};

Expand Down
6 changes: 5 additions & 1 deletion types/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,13 @@ export interface PngConfig {
* filters. Defaults to `canvas.PNG_ALL_FITLERS`.
*/
filters?: number
/**
* _For creating indexed PNGs._ Decide to include alpha channel. Defaults to true.
*/
alpha?: boolean
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.)

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

@mastrodaro mastrodaro Aug 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

/**
* _For creating indexed PNGs._ The palette of colors. Entries should be in
* RGBA order.
* RGBA order (with alpha) or RGB (without alpha).
*/
palette?: Uint8ClampedArray
/**
Expand Down