-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Handle Exif orientations for JPEG images (fixes #1670) #2296
Conversation
64ed3d8
to
ff0f2ab
Compare
Would it be possible to get a yes/no on this? Thanks! |
This looks really good, I definitely think we want this! Review time has been scarce for a while, but I'm carving out more time and I should be able to do a review soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. I don't understand why libjpeg doesn't read out exif info but I'm glad this doesn't introduce a dependency.
I tested the images you linked to and they all worked. I'll open a separate PR that adds those.
Some of of this is a bit @zbjornson's wheelhouse (pixel copying) so I might wait a bit for other comments before merging.
CHANGELOG.md
Outdated
@@ -10,6 +10,7 @@ project adheres to [Semantic Versioning](http://semver.org/). | |||
### Changed | |||
### Added | |||
### Fixed | |||
* Fixed Exif orientation in JPEG files being ignored (#1670) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this to 3.0.0 below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/Image.cc
Outdated
|
||
bool hasBytes(unsigned n) const override { return (_idx + n - 1 < _len); } | ||
|
||
uint8_t getNext() override { return _buf[_idx++]; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be doing a bounds check? I see that you're always calling hasBytes
before getNext
, but wonder if it's better for future changes to guard this. getc
below essentially does bounds checking, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I think there's no point doing unnecessary bounds checking, but I've added it.
Can you link to any issues that describe the 3.0/fabricjs problem? I'm not sure how easy backporting will be since we've changed prebuilds drastically for 3.0.0. |
incorrec now, correct after we merge #2296
incorrect now, correct after we merge #2296
incorrect now, correct after we merge #2296
I am not aware of any issues with Fabric and node-canvas 3.0. It would just be nice to have this in (our) production, and as I understand that 3.0 is a bit of an API change, perhaps Fabric is in no hurry to update. |
(Regarding why libjpeg does not perform EXIF rotation, I think it's because the library is so old, but since it is used in so many locations they don't want to change anything. libjpeg-turbo, which I think is more commonly used (due to better performance), has rejected implementing EXIF rotation, for among other reasons, compatibility with libjpeg, since it aims to be a drop-in replacement, just faster. libjpeg-turbo is also an ISO reference implementation for the JPEG spec, so in some sense, it is libjpeg these days. Anyway, you can read their justification for refusing to implement EXIF rotation at libjpeg-turbo/libjpeg-turbo#774, and the issue that references. But it is a sorry state of affairs.) |
Great. Thanks again, and sorry for the wait. We could look at backporting to 2.0 without prebuilds, but with is unlikely since prebuilds are a major headache. |
broke in #2296 because we don't run tests on PRs for some reason - getNext didn't return in all paths. Check wasn't necessary. - Windows doesn't have fseeko or ftello. I highly doubt we'll ever need 64 bits to address images. - Lambda functions get their own anonymous types and C++ standards don't require them to be interchangeable.
broke in #2296 because we don't run tests on PRs for some reason - getNext didn't return in all paths. Check wasn't necessary. - Windows doesn't have fseeko or ftello. I highly doubt we'll ever need 64 bits to address images. - Lambda functions get their own anonymous types and C++ standards don't require them to be interchangeable.
Implements simple Exif parsing for JPEG files, enough to determine if there is an Exif tag, and if that tag has an orientation, rotates the image. This makes node-canvas compatible with browsers, which also rotate the image based on the Exif orientation tag. This fixes #1670.
A simple test program for 2 of the 3 paths follows. This loads the JPEG, draws it to the canvas, and saves the result to
./test.png
. Test images are available (among other places) from https://github.com/noell/jpg-exif-test-images:`
const fs = require('fs')
const { createCanvas, loadImage } = require('canvas')
// One source of sample images with EXIF rotations is at
// https://github.com/noell/jpg-exif-test-images
const imagePath = process.argv[2];
loadImage(imagePath).then(image => {
const w = image.naturalWidth
const h = image.naturalHeight
const canvas = createCanvas(w, h/, 'PDF'/) // uncomment to test the mime device path
const context = canvas.getContext('2d')
context.fillStyle = '#f0f';
context.fillRect(0, 0, w, h);
context.drawImage(image, 0, 0, w, h)
}).catch(e => {
console.log("[error]", e);
})
`
Since one major use of
node-canvas
is in thefabric
project, which has not updated to the newly release 3.0 version yet, I think it would be helpful to backport to 2.11 (they use 2.11.2 at the moment), since it seems that 3.0 makes breaking changes for fabric. I initially made these changes on a branch from the v2.11.2 tag, and they merged with no problems, so I expect this changeset can be merged into a 2.11 branch. I can also create a separate pull request to whatever branch would be appropriate, if that is helpful.