Correctly crop embedded thumbs for Canon JPEGs
ClosedPublic

Authored by tommo on May 22 2019, 3:49 AM.

Details

Summary

JPEGs created by Canon Cameras have an embedded thumbnail. However only a certain area of the thumb is "valid", i.e. there are black bars on top and bottom. This change moves the thumbnail extraction logic to JpegContent and correctly crops the thumb by reading the Canon specific exif tag. The same black bars can be observed in thumbnails by Sony and Nikon. They however don't seem to provide such an exif tag, AFAIK.

Also suggests to let the thumbnailgenerator use the embedded thumbnail even if applyExifOrientation() is disabled. I don't see how the original image and its embedded thumb can be rotated differently, i.e. effectively being two different images. The only purpose of applyExifOrientation() should be to apply any rotation/transformation consistently to both, the original and its thumb, and not preventing to use the embedded thumb at all.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
tommo requested review of this revision.May 22 2019, 3:49 AM
tommo created this revision.

Example Canon thumbnail attached:

tommo edited the summary of this revision. (Show Details)May 22 2019, 3:56 AM
ngraham added inline comments.
lib/jpegcontent.cpp
580

What does 4 mean here?

585

In general we prefer to use descriptive variable names

tommo added inline comments.May 22 2019, 5:49 AM
lib/jpegcontent.cpp
580

ThumbnailImageValidArea specifies a rectangle: Top left, bottom right. That makes four coordinates.

585

Sure. Do you insist in this case?

ngraham added inline comments.May 22 2019, 3:26 PM
lib/jpegcontent.cpp
580

Gotcha, thanks! Might make sense to put that in a comment so it's more obvious.

585

Yes please. :)

tommo added inline comments.May 22 2019, 4:36 PM
lib/jpegcontent.cpp
585

Pls., which name would you consider to be descriptive in this particular case? I could have also used the result of orientation() as a temporary, IMO this would be descriptive enough. I just wanted to bind it to a variable to avoid calling it twice.

ngraham added inline comments.May 22 2019, 5:10 PM
lib/jpegcontent.cpp
585

Orientation orientation = orientation();

Which looks somewhat ridiculous in the initialization, but it's much more readable later:
&& orientation != NORMAL && orientation != NOT_AVAILABLE

tommo added inline comments.May 23 2019, 4:49 AM
lib/jpegcontent.cpp
585

This won't compile. You can't use orientation as function in this context, since it's a variable.

tommo updated this revision to Diff 58547.May 23 2019, 2:45 PM
ngraham accepted this revision.May 23 2019, 7:02 PM

Ahh whatever, this is fine as-is then.

This revision is now accepted and ready to land.May 23 2019, 7:02 PM
This revision was automatically updated to reflect the committed changes.