Skip to content

Add raw image support - #1256

Open
basvanmeurs wants to merge 8 commits into
Automattic:masterfrom
basvanmeurs:master
Open

Add raw image support#1256
basvanmeurs wants to merge 8 commits into
Automattic:masterfrom
basvanmeurs:master

Conversation

@basvanmeurs

Copy link
Copy Markdown

When you need to read parsed raw ARGB image data, you need to load the image, then draw it on a canvas and convert it to a buffer:

    var img = new Canvas.Image();
    img.src = data;
    var canvas = new Canvas(img.width, img.height);
    var ctx = canvas.getContext('2d');
    ctx.drawImage(img, 0, 0, img.width, img.height);
    var buffer = canvas.toBuffer('raw');

This is an unnecessary double copy of the data. This pull request contains a new method: Image.getRawData, which returns the buffer directly:

    var img = new Canvas.Image();
    img.src = data;
    var buffer = img.getRawData();

This allows for a more efficient usage of Canvas.Image.

We'd appreciate it if you'd merge this into the node-canvas module!

basvanmeurs added a commit to rdkcentral/Lightning that referenced this pull request Sep 25, 2018

@zbjornson zbjornson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good. Needs a conflict fixed again (sorry), and ideally one trivial change.

I can make the changes real quick if you want.

Comment thread src/Image.cc Outdated
Comment thread src/Image.cc
*/
NAN_GETTER(Image::GetRawData) {
Image *img = Nan::ObjectWrap::Unwrap<Image>(info.This());
if (img->_surface) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Would prefer to have this as a guard instead, e.g.

if (!img->_surface) {
  info.GetReturnValue().Set(Nan::Undefined());
  return;
}

// ...

Comment thread src/Image.cc
} else {
info.GetReturnValue().Set(Nan::Undefined());
}
return;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No need to return at the end of the function ☺️

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Also don't need the else { set undefined } block -- that's implicit if you didn't set another return value.

Comment thread src/Image.cc Outdated
Comment thread test/image.test.js Outdated
var buf = img.rawData;
assert.ok(buf instanceof Buffer);
assert.strictEqual(409600, img.rawData.length);
});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please follow standard style here (2 spaces indentation, space between function and (), no semicolons)

Comment thread test/image.test.js Outdated
Comment thread test/image.test.js Outdated
Comment thread test/image.test.js Outdated
@LinusU

LinusU commented Oct 11, 2018

Copy link
Copy Markdown
Collaborator

Personally not a fan of tacking more non-standard things on to the prototype 🤔 I do see the use case though.

Generally, what do we think about having these as standalone exported functions? Not sure that this is a good idea, but it could be something like this:

const { getRawImageBuffer } = require('canvas')

// img = ...
const buffer = getRawImageBuffer(img)

Just a thought 🤔 💭

@basvanmeurs

Copy link
Copy Markdown
Author

Can you please re-consider bringing this in to node-canvas, or explain what needs to be done exactly to get it in there?

@basvanmeurs

Copy link
Copy Markdown
Author

Hi guys, can you spare some time to review and consider merge please?

@zbjornson

zbjornson commented Feb 26, 2019

Copy link
Copy Markdown
Collaborator

I don't mind if this goes on the prototype or is an accessory function. It's slightly more convenient on the prototype (one less function to import), but agree with @LinusU that we should avoid adding more non-standard methods. I defer to others' decisions.

A few of Linus's comments are still pending though, and the last commit should be removed (CI forces a build from source).

@LinusU

LinusU commented Feb 28, 2019

Copy link
Copy Markdown
Collaborator

I defer to others' decisions.

I feel the same 😁

The comments need to be addressed before we merge though...

@NightScript370

Copy link
Copy Markdown

I just sent a Pull Request addressing the comments

@zbjornson
zbjornson force-pushed the master branch 2 times, most recently from 64ed3d8 to ff0f2ab Compare December 28, 2023 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants