feat: support reading QR codes from ImageData #15
Conversation
|
Sorry I haven't been able to take a close look at this PR yet. The feature seems very useful, thank you! I'll try my best to review this week. |
index.js
Outdated
| }); | ||
| } | ||
| } else if (img && typeof img === "object") { | ||
| const channels = img.data.length / img.width / img.height; |
There was a problem hiding this comment.
We should avoid rounding error here, and probably check for negative and zero img.width and img.height so that the addon code may rely on the fact that theses values are sane.
There was a problem hiding this comment.
I'll sanity check img.width and img.height. As for avoiding rounding, it does check that channels is exactly 1, 3, or 4. Is that good enough or are you imagining a scenario where it's not one of those values but is extremely close and somehow still passes for equal?
Rather than requiring that the data be in PNG or JPEG format, allow it to be an ImageData object with raw pixel data. This is useful if, for example, you are looking for a QR code in a portion of an image you've already loaded and cropped.
There is actually one small difference in this version: when previously a validation error would throw synchronously even when a promise was expected, now it returns a rejected promise. The `throw` behavior is still the same when a callback is provided, which is probably still not great as it means you need two handlers: one for anything thrown by the call to `decode` and another for the `err` in the callback.
Updates based on PR feedback.
eventualbuddha
force-pushed
the
feat/raw
branch
from
d066972
to
6b6e55a
Compare
|
@eventualbuddha thanks for addressing my concerns, I'll do a second round of review this week. |
| return ( | ||
| typeof number === 'number' && | ||
| number > 0 && | ||
| (number | 0) === number && |
There was a problem hiding this comment.
Are we trying to floor here? If yes, why the bitwise OR trick rather than Math.floor?
There was a problem hiding this comment.
Yeah, Math.floor works fine. I think editing C files put me in a C mood 😛
| expect(function () { | ||
| quirc.decode(); | ||
| }).to.throw(Error, "img must be a Buffer"); | ||
| it("should return a rejected Promise when no arguments are given", function (done) { |
|
Thanks, @kaworu! Looking forward to pulling this into https://github.com/votingworks/vxsuite. |
|
Of course it's time for a release! I just want to take the time to setup GitHub action to build & push to npm. At that point, quirc might have bumped to 1.1 (see this PR) and we can sync right before we release 2.3. |
My PR was merged (kaworu/node-quirc#15), so now we can pass an `ImageData` directly to be decoded without encoding as a PNG first.
My PR was merged (kaworu/node-quirc#15), so now we can pass an `ImageData` directly to be decoded without encoding as a PNG first.
Rather than requiring that the data be in PNG or JPEG format, allow it to be an ImageData object with raw pixel data. This is useful if, for example, you are looking for a QR code in a portion of an image you've already loaded and cropped.