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

Added support for Uint8Array, Uint8ClampedArray #2511

Merged
merged 7 commits into from
Jan 6, 2021

Conversation

leon
Copy link
Contributor

@leon leon commented Jan 3, 2021

This allows us to work with raw pixels data easier.

It is a continuation of: #2256

Regarding memory efficiency. I've done research and have come up with

inputDescriptor.buffer = Buffer.from(uint8array.buffer);

This will create a Buffer which points to the underlying Uint8Arrays buffer.
They will share the same memory.

And for the other way around its:

new Uint8Array(data.buffer)

Example

So the full around circle would be:

const input = Uint8Array.from([1, 1, 1, 2, 2, 2]); // or Uint8ClampedArray
const image = sharp(input, {
  raw: {
    width: 2,
    height: 1,
    channels: 3
  }
});
image.toUint8Array().then((output) => {
  assert.deepStrictEqual(input, output);
});

I have added both unit tests and documentation for the new feature.
All tests are passing.

Performance wise, the only changed code path is another couple of instanceof checks in the input handler, which should not be a problem.

Let me know if I need to do any changes 👍🏻

const input = Uint8Array.from([1, 1, 1, 2, 2, 2]); // or Uint8ClampedArray
const image = sharp(input, {
  raw: {
    width: 2,
    height: 1,
    channels: 3
  }
});
image.toUint8Array().then((output) => {
  assert.deepStrictEqual(input, output);
});
@coveralls
Copy link

coveralls commented Jan 3, 2021

Coverage Status

Coverage remained the same at 100.0% when pulling f45f993 on leon:feature/to-uint8array into bf1b326 on lovell:master.

Copy link
Owner

@lovell lovell left a comment

Choose a reason for hiding this comment

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

Hi Leon, thank you for this PR, I've left a few questions/comments inline.

test/unit/toUint8Array.js Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
test/unit/toUint8Array.js Outdated Show resolved Hide resolved
lib/is.js Show resolved Hide resolved
lib/output.js Outdated Show resolved Hide resolved
@lovell lovell mentioned this pull request Jan 4, 2021
This fixed the hidden problem of the callback version returning the unmodified version.
Removed nested then calls and used async await instead.

Also fixed bug where Buffer objects will always satisfy the instanceof Uint8Array checks.
@leon
Copy link
Contributor Author

leon commented Jan 4, 2021

Question, advice?

Should we discard the toUint8Array() function?

Having looked at it for a bit, maybe it's not needed.
At first I thought it would be a good idea to match the input and output apis.
But since we won't be able to output to both Uint8Array and Uint8ClampedArray, maybe it's better if I just update the documentation for toBuffer() to show how to easily create a Uint8Array or Uint8ClampedArray from the returned Buffer?

What do you think @lovell ?

@leon leon requested a review from lovell January 4, 2021 20:08
@lovell
Copy link
Owner

lovell commented Jan 5, 2021

Great, happy to keep it simple; the addition of real world examples to the documentation is always a good thing, thank you.

There's value in supporting Uint8Array (and Uint8ClampedArray) on the input side so please can we keep this bit.

@leon
Copy link
Contributor Author

leon commented Jan 5, 2021

Did the changes we discussed.
It was the right call.
It is better to inform the users of how to work with Buffers and Uint8Arrays.

Copy link
Owner

@lovell lovell left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, there are a couple of lines failing the linter checks. Please can you fix these.

lib/output.js Outdated
* // output the raw pixels
* .raw()
* .toBuffer();
*
Copy link
Owner

Choose a reason for hiding this comment

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

/__w/sharp/sharp/lib/output.js:112:3: Trailing spaces not allowed.

lib/output.js Outdated
* // this will not copy the data, instead it will change `data`s underlying ArrayBuffer
* // so `data` and `pixelArray` point to the same memory location
* const pixelArray = new Uint8ClampedArray(data.buffer);
*
Copy link
Owner

Choose a reason for hiding this comment

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

/__w/sharp/sharp/lib/output.js:117:3: Trailing spaces not allowed.

@leon
Copy link
Contributor Author

leon commented Jan 6, 2021

Sorry, fixed it now

Copy link
Owner

@lovell lovell left a comment

Choose a reason for hiding this comment

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

Brilliant, thank you.

@lovell lovell merged commit 4821a11 into lovell:master Jan 6, 2021
@lovell lovell added this to the v0.27.1 milestone Jan 6, 2021
@leon leon deleted the feature/to-uint8array branch January 6, 2021 12:13
lovell added a commit that referenced this pull request Jan 6, 2021
@nnmrts
Copy link

nnmrts commented Jan 6, 2021

Wow @leon, I'm actually really happy to see this, thank you and @lovell. Feels great to see something I struggled with and couldn't finish, actually being finished. Awesome. This was the last piece for my project to go completely node.js-buffer-free (on my end at least). Really hyped to try this out when the new release is here. ❤️

@leon
Copy link
Contributor Author

leon commented Jan 7, 2021

@nnmrts I'm happy you liked it 😃

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