Conversation
None of the EXIF handling had a single test. Add fixtures for all eight orientations, each with a big- and a little-endian TIFF header, that all display as the same 64x32 image with one colour per quadrant. They are generated by test/fixtures/exif/generate.py and checked there with ImageMagick's -auto-orient, so they stand apart from both the browser and Dropzone's own EXIF code. The tests record what the browser produces today, for every fixture: an upright thumbnail, `file.width` and `file.height` in displayed axes, resizing and cropping along the displayed axes, a custom `resize`'s source rectangle taken from the displayed image, and images from the server and PNGs left to the browser. The next changes must not move any of that. They also record the bug: the resized image carries the original orientation, although its pixels are already upright.
ExifRestore carried a hand-written base64 codec that decoded the whole original image in JavaScript, one character at a time, just to find a segment near its start. src/exif.ts does the same job with atob and btoa, and decodes only the head of the original -- 192 KiB, falling back to the whole file when EXIF sits further in. The faster codec is @kaymes' idea from #2001. It picks the same segment as before -- the first APP1 -- and was checked byte for byte against ExifRestore on every fixture, over five canvas encodings each and with originals that have no EXIF at all: 160 comparisons, all identical. Where to insert it is now read from the segment structure rather than taken as the first 0xFF after byte 3. That is the same place for a JPEG that opens with a JFIF header, which is what canvases write; without one, the old rule landed inside the next segment. restoreExif never throws. It runs inside `transformFile`, where an exception would leave the upload waiting forever; it hands the resized image back without the metadata instead.
Whether a JPEG's EXIF orientation got applied depended on the browser. Current ones do it while decoding; older ones do not, and there Dropzone only rotated the image if the page had loaded exif.js. To know what the pixels look like after drawing -- which the resized image's EXIF has to match -- you would have to know which of those happened. So Dropzone applies the orientation itself. extractOrientation reads it from the data URL and hands the browser a copy whose value is set to 1, changing those two bytes and re-encoding only the head of the file. The drawing then turns the pixels upright on the canvas. Every browser runs the same code and produces the same image, and exif.js is no longer consulted. The rotation code that was already there could not simply be switched on: it measured the image as stored, so `resize` got its dimensions in the wrong axes. For a portrait photo, `resizeWidth` would have limited the displayed height and a crop would have come out the wrong shape -- the same thing that makes #2001's strip-and-restore approach a behaviour change. Everything is now worked out in displayed axes, as the browser did it: `file.width` and `file.height` are the displayed size, `resize` sees that, and its source rectangle is mapped back into the stored pixels before drawing. Every test from the first commit passes unchanged. They were checked against four deliberate mistakes -- resizing in stored axes, a wrong rotation, an unmapped source rectangle, the browser also rotating -- and each one fails between 6 and 56 of them. Images passed to `displayExistingFile` are still left to the browser: they need not be data URLs, and nothing is resized from them.
The resized image is drawn upright, but it got the original's EXIF copied onto it as it was, orientation included. Anything that honours EXIF -- image viewers, and servers that auto-orient with sharp, Pillow or ImageMagick -- then turned the already upright pixels a second time: portrait phone photos came out on their side, and upside-down ones the other way up. restoreExif now sets the orientation to 1 in the segment it copies, and leaves the rest of the EXIF alone. Viewers that honour it and viewers that do not now show the same, correct, image. The fixture tests assert exactly that for all eight orientations in both byte orders; without this change the 14 that are not already upright fail. Fixes #1967, #2081 and #2194.
Contributor
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
The orientation fix it described is done, as a patch rather than in 7.0, and the EXIF code it listed among the things to split out of dropzone.ts has already moved to src/exif.ts.
Nothing told users that a resized JPEG keeps the camera's EXIF, which includes the location it was taken at, or that other formats drop it -- and it is the natural place to say that the result is upright. The row in the options table now runs past the width the table is padded to. Re-padding every other row to match would be a diff of pure whitespace, so it is left as it is.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1967
Fixes #2081
Fixes #2194
Supersedes #2001 — thank you @kaymes. Your diagnosis in #1967 is exactly what was going on, and the faster
atob/btoasegment handling in this PR is your idea. The reason it doesn't use #2001's strip-and-restore approach is below; it comes down to backwards compatibility, not correctness.The bug
Browsers apply a JPEG's EXIF orientation when they decode it, so the canvas
resizeImagedraws on already holds upright pixels. The original EXIF was then copied onto the result unchanged, orientation included, so anything that honours EXIF — image viewers, and servers that auto-orient with sharp, Pillow or ImageMagick — turned the image a second time. Portrait phone photos came out on their side and upside-down ones flipped.The approach
Dropzone now applies the orientation itself instead of leaving it to the browser, and writes the orientation back as
1:extractOrientationreads the orientation from the data URL and hands the browser a copy whose value says1. Only those two bytes change, and only the head of the file gets decoded and re-encoded.restoreExifcopies the EXIF across as before, with the orientation set to1. Everything else in it — date, camera, location — is kept.Why not strip and restore
Stripping EXIF before decoding also gives correct output in EXIF-aware viewers, but it's a behaviour change in three ways:
resizeWidthwould limit the displayed height, and a non-square crop would come out the wrong shape.file.width/file.heightwould change meaning between the thumbnail pass and the resize pass.Here, everything stays in displayed axes, exactly as the browser did it:
resizereceives the displayed size, and its source rectangle is mapped back into the stored pixels before drawing. The existing manual-rotation code (the exif.js path) couldn't simply be switched on because it computedresizein stored axes — which is the same regression.Commits
Worth reviewing one at a time; each one passes the full suite on its own.
test/fixtures/exif/generate.pyand checked with ImageMagick's-auto-orient, so they don't depend on the browser or on Dropzone's code. The tests record today's output: thumbnails,file.width/height, resize and crop geometry, a customresizewith an off-centre source rectangle, server images and PNGs. They also record the bug.ExifRestoreon every fixture across five canvas encodings each: 160 comparisons, all identical.Compatibility
The only intended visible change is the orientation value in resized JPEGs, and only where it was wrong. Thumbnails, pixels, resize geometry,
file.width/heightand the public API are unchanged, and images passed todisplayExistingFileare still left to the browser. Anyone who compensated for this on the server has to remove that, which the changeset says.Two details:
restoreExifandextractOrientationnever throw. They run inside the thumbnail queue andtransformFile, where an exception would hang things (see FileReaderFailure Causes Permanent Deadlock in Thumbnail Queue (_processThumbnailQueue` Stalls Indefinitely) #2365); on anything unexpected they return their input.Tested
Chromium, which is the suite's engine. The new code keeps to ES2017, like the rest of the source, so the build still needs no down-levelling.