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.

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

pass in a raw pixelBuffer to the open command #86

Merged
merged 5 commits into from
Dec 11, 2014
Merged

pass in a raw pixelBuffer to the open command #86

merged 5 commits into from
Dec 11, 2014

Conversation

wouterverweirder
Copy link
Contributor

Referencing my previous pull request #84 - but now on the 0.0.6 branch

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.19%) when pulling 9de8fae on wouterverweirder:version/0.0.6 into 9a126b4 on EyalAr:version/0.0.6.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) when pulling 63060b5 on wouterverweirder:version/0.0.6 into 9a126b4 on EyalAr:version/0.0.6.

@wouterverweirder
Copy link
Contributor Author

motivation for using open: jpg and png buffers are already accepted as input. I think it might be confusing to use a separate function to create images based on raw buffers.

Possible code optimization might be creating a decoder in c which doesn't transform the raw input buffers, but only does the validation of the buffer size & parameters. Right now this happens in javascript. However, I don't think there's much of a speed benefit of doing that in c, it would rather be for code structuring reasons.

@wouterverweirder
Copy link
Contributor Author

I got rid of the transparency option, as this can be auto-detected based on the given width, height and buffer size.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) when pulling d457592 on wouterverweirder:version/0.0.6 into 9a126b4 on EyalAr:version/0.0.6.

callback(err, err ? null : new Image(pixelsBuf, width, height, trans));
});
if(typeof type === 'object') {
if(!type.width && !type.height) {
Copy link
Owner

Choose a reason for hiding this comment

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

This condition can be dropped. The next two ones are sufficient.

if(!type.height) {
throw Error("Missing height");
}
if(typeof type.width !== "number") {
Copy link
Owner

Choose a reason for hiding this comment

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

width and height need to be positive integers.

Can replace both of the following conditions

if(!type.width) {
    throw Error("Missing width");
}
if(typeof type.height !== "number") {
    throw Error("Height must be numeric");
}

With:

if (!(type.width === parseInt(type.width) && type.width > 0)){
    throw Error("Width must be a positive integer");
}

This will cover both cases where type.width is undefined, and not of the correct type.

@EyalAr
Copy link
Owner

EyalAr commented Dec 10, 2014

I'm wondering if the section for validating the type object of the raw buffer properties can be externalized as custom Decree type, such as in here

Then do something like:

// ...
{
    name: 'type',
    types: ['string', 'rawBufferProperties'],
    optional: true
}
// ...

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) when pulling 7c3f3c6 on wouterverweirder:version/0.0.6 into 9a126b4 on EyalAr:version/0.0.6.

@wouterverweirder
Copy link
Contributor Author

improved validation based on your feedback & created a custom decree type to validate the width and height (required positive integer).
Buffer size validation is still in open method, as this has to check 2 parameters (buffer.length and the width + height from the type object).

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) when pulling f13c07c on wouterverweirder:version/0.0.6 into 9a126b4 on EyalAr:version/0.0.6.

EyalAr added a commit that referenced this pull request Dec 11, 2014
pass in a raw pixelBuffer to the open command
@EyalAr EyalAr merged commit 4d9c498 into EyalAr:version/0.0.6 Dec 11, 2014
@EyalAr
Copy link
Owner

EyalAr commented Dec 11, 2014

Great! I've merged this, and am going to push a few more changes, in case you want to take a look.

Thanks a lot for this PR!

@wouterverweirder
Copy link
Contributor Author

cool :-)

@EyalAr EyalAr mentioned this pull request Dec 21, 2014
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.

3 participants