Correctly crop embedded thumbs for Canon JPEGs

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



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

R260 Gwenview
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
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.

What does 4 mean here?


In general we prefer to use descriptive variable names

tommo added inline comments.May 22 2019, 5:49 AM

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


Sure. Do you insist in this case?

ngraham added inline comments.May 22 2019, 3:26 PM

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


Yes please. :)

tommo added inline comments.May 22 2019, 4:36 PM

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

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

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.