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

limitInputPixels should probably be 53-bit integer #3238

Closed
AndrewJDR opened this issue May 25, 2022 · 7 comments
Closed

limitInputPixels should probably be 53-bit integer #3238

AndrewJDR opened this issue May 25, 2022 · 7 comments

Comments

@AndrewJDR
Copy link

src/common.h and src/common.cc show that limitInputPixels is an int, but pixel counts can certainly exceed this (e.g. inputs for tile()) while still perhaps being worthy of some limiting, so ideally you could still set limits above 2^31 or 2^32.

@lovell
Copy link
Owner

lovell commented May 25, 2022

This would require support for the BigInt data type, which was added to Node-API v6.

This means we'll need to wait until we drop support for Node.js 12, probably the forthcoming v0.31.0 release, then we can bump the minimum Node-API version from v5 to v6 (or v7) and consider adding this feature.

https://nodejs.org/dist/latest/docs/api/n-api.html#node-api-version-matrix

@AndrewJDR
Copy link
Author

@lovell Isn't napi_create_int64() (since node v8.4.0) sufficient?

@lovell
Copy link
Owner

lovell commented May 25, 2022

That would provide support for 53-bit integers, which is a wider range than 32-bit, but to support your request for 64-bit integers we'll need BigInt.

@AndrewJDR
Copy link
Author

Ah, sure... 53-bit is more than sufficient for this purpose, in my opinion -- I can revise it.

@AndrewJDR AndrewJDR changed the title limitInputPixels should probably be 64-bit integer limitInputPixels should probably be 53-bit integer May 26, 2022
@lovell
Copy link
Owner

lovell commented May 26, 2022

Great, thanks for the update, this might be a good time to roll out the :why_not_both: meme! 😄

@AndrewJDR
Copy link
Author

AndrewJDR commented May 26, 2022

Oh, I should also mention -- if you pass a number >= 2^32 (or likely even >= 2^31, since it's signed under the hood) you'll get the Input image exceeds pixel limit error on images sized smaller than the number you passed. This was initially mystifying until I had a look at the code. And I didn't test for this, but I presume anything >= 2^31 overflows and you end up with some arbitrary number between -(2^31) ... 2^31. Anyhow, this is a long way of saying some type of overflow warning/error or documentation might be good, though it's less necessary once there is 53/64 bit support... I don't think many will run into it after that.

@lovell
Copy link
Owner

lovell commented May 30, 2022

v0.30.6 now available with support for 53-bit integer values for limitInputPixels, up from 32-bit, thanks for the suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants