-
Notifications
You must be signed in to change notification settings - Fork 111
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
Fix exception when parsing EXIF data with LONG Orientation #684
Fix exception when parsing EXIF data with LONG Orientation #684
Conversation
@jcoyne please consider this for next release |
src/main/java/edu/illinois/library/cantaloupe/image/Metadata.java
Outdated
Show resolved
Hide resolved
@janhoy Can you re-target this PR to |
4d50703
to
c6e9d67
Compare
@jcoyne I added the tree commits on top of develop branch, force-pushed and re-targeted the PR to |
Thank you @janhoy |
// Thus to be lenient we accept either and convert to int (github issue #548) | ||
if (value instanceof Long) { | ||
orientation = Orientation.forEXIFOrientation(Math.toIntExact((long) value)); | ||
} else if (value instanceof Integer) { |
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.
hi @janhoy question about the else if
. How would be the value instanceof Integer
if the field is already instance of Long? Is that a Java typing issue when the Value is read from EXIF? This is just me not knowing enough probably. Thanks!
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.
I was also surprised by this. Looks like some bug somewhere, that the EXIF typing is long but it gets stuffed into an integer, thus causing ClassCastException. That's the kind of stuff you see in the wild even if it is not as per spec.
So we could really skip the reading of EXIF type and just check the Java type of the value, since we need to fall back to that anyway. The only value in the four switch-case clauses is to trigger the warn log msg, but it could also have been in an else
clause after instanceof
checks of Long and Integer fails.
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.
@janhoy it would be helpful to put those assumptions in a comment
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.
@janhoy We are quite happy with this contribution. We'd like to get this merged for an upcoming release. Are you able to add these code comments?
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.
I added three lines of java comment already about this, what execlty do you feel is unclear and could you propose additional comment text to fix it?
// According to spec the orientation must be an unsigned short (16 bit)
// However, we have seen exif data in the wild with LONG and SLONG types
// Thus to be lenient we accept either and convert to int (github issue #548)
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.
Added these two lines:
// It could also happen that 'value' is in fact a Java Integer even if the
// exif data type is LONG or SLONG, so we need to check for that as well.
Hope this is mergeable and will be included in next release!
Thank you @janhoy for your work on this. |
Fixes #548
Based on PR #549 and adds tests as well as a more lenient parsing.
The test jpg image is a real image from the
/var/cache/cantaloupe/image/
folder of our Cantaloupe server. Somehow the generated thumbnail stored in the cache by Cantaloupe itself ended up with aSLONG
datatype forOrientation
field. But it turned out that the value itself wasInteger
, so a cast to(long)
did not work. With this patch we acceptLONG
,SHORT
,SLONG
andSSHORT
and then usesinstanceof
to do any casting to int.I tried to shrink the size of the image from 36kb, but doing so with Apple Preview also rewrote the EXIF data with correct datatype :(
For some reason we see this bug popping up all the time with 5.0.6, so we currently run a patched version of 5.0.5 in production.