Skip to content
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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 24 additions & 6 deletions src/main/java/edu/illinois/library/cantaloupe/image/Metadata.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonProperty;
import edu.illinois.library.cantaloupe.image.exif.Directory;
import edu.illinois.library.cantaloupe.image.exif.Field;
import edu.illinois.library.cantaloupe.image.exif.Tag;
import edu.illinois.library.cantaloupe.image.iptc.DataSet;
import edu.illinois.library.cantaloupe.image.xmp.MapReader;
Expand Down Expand Up @@ -120,10 +121,7 @@ public Orientation getOrientation() {
if (orientation == null) {
orientation = Orientation.ROTATE_0;
}
} catch (IllegalArgumentException e) {
LOGGER.info("readOrientation(): {}", e.getMessage());
orientation = Orientation.ROTATE_0;
} catch (RiotException e) {
} catch (IllegalArgumentException | RiotException e) {
LOGGER.info("readOrientation(): {}", e.getMessage());
orientation = Orientation.ROTATE_0;
}
Expand All @@ -132,9 +130,29 @@ public Orientation getOrientation() {
}

private void readOrientationFromEXIF() {
Field field = exif.getField(Tag.ORIENTATION);
Object value = exif.getValue(Tag.ORIENTATION);
if (value != null) {
orientation = Orientation.forEXIFOrientation((int) value);
if (field != null && value != null) {
switch (field.getDataType()) {
case LONG:
case SLONG:
case SHORT:
case SSHORT:
// 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)
// 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.
if (value instanceof Long) {
orientation = Orientation.forEXIFOrientation(Math.toIntExact((long) value));
} else if (value instanceof Integer) {
Copy link
Contributor

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!

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor

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?

Copy link
Contributor Author

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)

Copy link
Contributor Author

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!

orientation = Orientation.forEXIFOrientation((int) value);
}
break;
default:
LOGGER.warn("readOrientationFromEXIF(): Unsupported Orientation data type: {}",
field.getDataType());
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,14 @@ public Object getValue(Tag tag) {
.orElse(null);
}

public Field getField(Tag tag) {
return fields.keySet()
.stream()
.filter(f -> f.getTag().equals(tag))
.findFirst()
.orElse(null);
}

@Override
public int hashCode() {
final Map<Integer,Integer> codes = new HashMap<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,15 @@
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import java.nio.file.FileVisitOption;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;

import static org.junit.jupiter.api.Assertions.*;

Expand Down Expand Up @@ -232,6 +235,20 @@ void testGetOrientationWithOnlyIllegalEXIFOrientation() throws Exception {
}
}

@Test
void testGetOrientationWithOnlyLONGEXIFOrientation() throws Exception {
// This image has exif Orientation stored as SLONG, causing a failure (github issue #548)
Path fixture = TestUtil.getImage("jpg-exif-long-orientation.jpg");
ImageReader reader = new ImageReaderFactory()
.newImageReader(Format.get("jpg"), fixture);
try {
Metadata metadata = reader.getMetadata(0);
assertEquals(Orientation.ROTATE_0, metadata.getOrientation());
} finally {
reader.dispose();
}
}

@Test
void testGetOrientationWithOnlyXMPOrientation() throws Exception {
Path fixture = TestUtil.getImage("jpg-xmp-orientation-90.jpg");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,14 @@ void testGetValueWithoutMatchingValue() {
assertNull(ifd.getValue(Tag.MAKE));
}

@Test
void testGetField() {
final Directory ifd = new Directory(TagSet.BASELINE_TIFF);
ifd.put(Tag.MAKE, DataType.ASCII, "Cats");
Field field = ifd.getField(Tag.MAKE);
assertEquals(DataType.ASCII, field.getDataType());
}

@Test
void testHashCodeWithEqualInstances() {
final Directory subIFD1 = new Directory(TagSet.EXIF);
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.