StefanOltmann commented on code in PR #366:
URL: https://github.com/apache/commons-imaging/pull/366#discussion_r1501383880
##########
src/test/java/org/apache/commons/imaging/formats/jpeg/exif/ExifRewriterRoundtripTest.java:
##########
@@ -56,124 +61,160 @@ public static Stream<File> data() throws Exception {
private File duplicateFile;
- private void assertTiffEquals(final TiffOutputSet tiffOutputSet, final
TiffOutputSet tiffOutputSet1) {
- final List<TiffOutputDirectory> directories =
tiffOutputSet.getDirectories();
- final List<TiffOutputDirectory> directories1 =
tiffOutputSet1.getDirectories();
- assertEquals(directories.size(), directories1.size(), "The
TiffOutputSets have different numbers of directories.");
-
- for (int i = 0; i < directories.size(); i++) {
- final List<TiffOutputField> fields =
directories.get(i).getFields();
- final List<TiffOutputField> fields1 =
directories1.get(i).getFields();
- assertEquals(fields.size(), fields1.size(), "The
TiffOutputDirectories have different numbers of fields.");
-
- for (int j = 0; j < fields.size(); j++) {
- final TiffOutputField field = fields.get(j);
- final TiffOutputField field1 = fields1.get(j);
- assertEquals(field.tag, field1.tag, "TiffOutputField tag
mismatch.");
- assertEquals(field.tagInfo, field1.tagInfo, "TiffOutputField
tagInfo mismatch.");
- assertEquals(field.abstractFieldType,
field1.abstractFieldType, "TiffOutputField fieldType mismatch.");
- assertEquals(field.count, field1.count, "TiffOutputField count
mismatch.");
+ private void assertTiffEquals(final TiffOutputSet sourceTiffOutputSet,
final TiffOutputSet duplicateTiffOutputSet) {
+
+ final List<TiffOutputDirectory> sourceDirectories =
sourceTiffOutputSet.getDirectories();
+ final List<TiffOutputDirectory> duplicatedDirectories =
duplicateTiffOutputSet.getDirectories();
+
+ assertEquals(sourceDirectories.size(), duplicatedDirectories.size(),
"The TiffOutputSets have different numbers of directories.");
+
+ for (int i = 0; i < sourceDirectories.size(); i++) {
+
+ final TiffOutputDirectory sourceDirectory =
sourceDirectories.get(i);
+ final TiffOutputDirectory duplicateDirectory =
duplicatedDirectories.get(i);
+
+ assertEquals(sourceDirectory.getType(),
duplicateDirectory.getType(), "Directory type mismatch.");
+
+ final List<TiffOutputField> sourceFields =
sourceDirectory.getFields();
+ final List<TiffOutputField> duplicateFields =
duplicateDirectory.getFields();
+
+ final boolean fieldCountMatches = sourceFields.size() ==
duplicateFields.size();
+
+ if (!fieldCountMatches) {
+
+ /*
+ * Note that offset fields are not written again if a re-order
makes
+ * them unnecessary. The TiffWriter does not write in the same
order,
+ * but tries to optimize it.
+ */
+
+ final List<Integer> sourceTags = new ArrayList<>();
+ final List<Integer> duplicatedTags = new ArrayList<>();
+
+ for (TiffOutputField field : sourceFields)
+ sourceTags.add(field.tag);
+
+ for (TiffOutputField field : duplicateFields)
+ duplicatedTags.add(field.tag);
+
+ final List<Integer> missingTags = new ArrayList<>(sourceTags);
+ missingTags.removeAll(duplicatedTags);
+ missingTags.remove((Integer)
ExifTagConstants.EXIF_TAG_EXIF_OFFSET.tag);
+ missingTags.remove((Integer)
ExifTagConstants.EXIF_TAG_GPSINFO.tag);
+ missingTags.remove((Integer)
ExifTagConstants.EXIF_TAG_INTEROP_OFFSET.tag);
+ missingTags.remove((Integer)
TiffTagConstants.TIFF_TAG_JPEG_INTERCHANGE_FORMAT.tag);
+
+ assertTrue(missingTags.isEmpty(), "Missing tags: " +
missingTags);
+ }
+
+ for (TiffOutputField sourceField : sourceFields) {
+
+ final boolean isOffsetField =
+ sourceField.tag ==
ExifTagConstants.EXIF_TAG_EXIF_OFFSET.tag ||
+ sourceField.tag ==
ExifTagConstants.EXIF_TAG_GPSINFO.tag ||
+ sourceField.tag ==
ExifTagConstants.EXIF_TAG_INTEROP_OFFSET.tag ||
+ sourceField.tag ==
TiffTagConstants.TIFF_TAG_JPEG_INTERCHANGE_FORMAT.tag;
+
+ /*
+ * Ignore offset fields. They may not be needed after rewrite
+ * and their value changes anyway.
+ */
+ if (isOffsetField)
+ continue;
+
+ TiffOutputField duplicateField =
duplicateDirectory.findField(sourceField.tag);
+
+ assertNotNull(duplicateField, "Field is missing: " +
sourceField.tagInfo);
+
+ assertEquals(sourceField.tag, duplicateField.tag,
"TiffOutputField tag mismatch.");
+ assertEquals(sourceField.abstractFieldType,
duplicateField.abstractFieldType, "TiffOutputField fieldType mismatch.");
+ assertEquals(sourceField.count, duplicateField.count,
"TiffOutputField count mismatch.");
+
+ boolean sameValue = sourceField.bytesEqual(duplicateField);
+
+ assertTrue(sameValue, "Bytes are different for field: " +
sourceField);
}
}
}
private void copyToDuplicateFile(final File sourceFile, final
TiffOutputSet duplicateTiffOutputSet) throws IOException, ImagingException,
ImagingException {
+
final ExifRewriter exifRewriter = new ExifRewriter();
+
duplicateFile = createTempFile();
+
try (OutputStream duplicateOutputStream = new BufferedOutputStream(new
FileOutputStream(duplicateFile))) {
exifRewriter.updateExifMetadataLossless(sourceFile,
duplicateOutputStream, duplicateTiffOutputSet);
}
}
private File createTempFile() {
+
final String tempDir = FileUtils.getTempDirectoryPath();
final String tempFileName = this.getClass().getName() + "-" +
random.nextLong() + ".tmp";
+
return new File(tempDir, tempFileName);
}
private TiffOutputSet duplicateTiffOutputSet(final TiffOutputSet
sourceTiffOutputSet) throws ImagingException {
- final TiffOutputSet duplicateTiffOutputSet = new TiffOutputSet();
+
+ final TiffOutputSet duplicateTiffOutputSet = new TiffOutputSet(
+ sourceTiffOutputSet.byteOrder
+ );
+
for (final TiffOutputDirectory tiffOutputDirectory :
sourceTiffOutputSet) {
duplicateTiffOutputSet.addDirectory(tiffOutputDirectory);
}
+
return duplicateTiffOutputSet;
}
- private JpegImageMetadata getJpegImageMetadata(final File sourceFile)
throws ImagingException, IOException {
+ private JpegImageMetadata getJpegImageMetadata(final File sourceFile)
throws IOException {
+
final JpegImageMetadata jpegImageMetadata = (JpegImageMetadata)
Imaging.getMetadata(sourceFile);
- if (null == jpegImageMetadata) {
+
+ if (jpegImageMetadata == null) {
throw new TestSkippedException();
}
+
return jpegImageMetadata;
}
private TiffImageMetadata getTiffImageMetadata(final JpegImageMetadata
sourceJpegImageMetadata) {
+
final TiffImageMetadata tiffImageMetadata =
sourceJpegImageMetadata.getExif();
- if (null == tiffImageMetadata) {
+
+ if (tiffImageMetadata == null) {
throw new TestSkippedException();
}
+
return tiffImageMetadata;
}
private TiffOutputSet getTiffOutputSet(final TiffImageMetadata
sourceTiffImageMetadata) throws ImagingException {
+
final TiffOutputSet tiffOutputSet =
sourceTiffImageMetadata.getOutputSet();
+
if (tiffOutputSet == null) {
throw new TestSkippedException();
}
+
return tiffOutputSet;
}
@AfterEach
void tearDown() {
+
if (duplicateFile != null && duplicateFile.exists()) {
duplicateFile.delete();
duplicateFile.deleteOnExit();
}
}
- @ParameterizedTest
- @MethodSource("data")
- public void
updateExifMetadataLossless_copyWithoutChanges_TiffImageMetadataIsIdentical(final
File sourceFile) throws Exception {
- /*
- * Load EXIF data from source file, skipping over any test images
without any EXIF data
- */
- final JpegImageMetadata sourceJpegImageMetadata =
getJpegImageMetadata(sourceFile);
- final TiffImageMetadata sourceTiffImageMetadata =
getTiffImageMetadata(sourceJpegImageMetadata);
- final TiffOutputSet sourceTiffOutputSet =
getTiffOutputSet(sourceTiffImageMetadata);
-
- /*
- * Copy the TiffOutputSet to a duplicate TiffOutputSet
- */
- final TiffOutputSet duplicateTiffOutputSet =
duplicateTiffOutputSet(sourceTiffOutputSet);
-
- /*
- * Copy the file to a duplicate file, using updateExifMetadataLossless
and the duplicate TiffOutputSet
- */
- copyToDuplicateFile(sourceFile, duplicateTiffOutputSet);
-
- /*
- * Load EXIF data from duplicate file
- */
- final JpegImageMetadata duplicateJpegImageMetadata =
getJpegImageMetadata(duplicateFile);
- final TiffImageMetadata duplicateTiffImageMetadata =
getTiffImageMetadata(duplicateJpegImageMetadata);
-
- /*
- * Compare the source TiffImageMetadata to the one loaded from the
duplicate file. This fails!
- */
- final List<? extends ImageMetadata.ImageMetadataItem>
imageMetadataItems = sourceTiffImageMetadata.getItems();
- final List<? extends ImageMetadata.ImageMetadataItem>
imageMetadataItems1 = duplicateTiffImageMetadata.getItems();
- assertEquals(imageMetadataItems.size(), imageMetadataItems1.size(),
"The TiffImageMetadata have different numbers of imageMetadataItems.");
-
- for (int i = 0; i < imageMetadataItems.size(); i++) {
- final ImageMetadata.ImageMetadataItem imageMetadataItem =
imageMetadataItems.get(i);
- final ImageMetadata.ImageMetadataItem imageMetadataItem1 =
imageMetadataItems1.get(i);
- assertEquals(imageMetadataItem.toString(),
imageMetadataItem1.toString(), "ImageMetadataItem toString mismatch.");
- }
- }
-
Review Comment:
This toString() comparison test is useless.
The TiffWriter does change the order of fields in an attempt to optimize for
space. This results in fields being reordered and unnecessary offset fields to
be dropped.
You only can compare the TiffOutputSet.
I assume the author did this to compare values. This is why I introduced the
value comparison API to TiffOutputField.
##########
src/test/java/org/apache/commons/imaging/formats/jpeg/exif/ExifRewriterRoundtripTest.java:
##########
@@ -56,124 +61,160 @@ public static Stream<File> data() throws Exception {
private File duplicateFile;
- private void assertTiffEquals(final TiffOutputSet tiffOutputSet, final
TiffOutputSet tiffOutputSet1) {
- final List<TiffOutputDirectory> directories =
tiffOutputSet.getDirectories();
- final List<TiffOutputDirectory> directories1 =
tiffOutputSet1.getDirectories();
- assertEquals(directories.size(), directories1.size(), "The
TiffOutputSets have different numbers of directories.");
-
- for (int i = 0; i < directories.size(); i++) {
- final List<TiffOutputField> fields =
directories.get(i).getFields();
- final List<TiffOutputField> fields1 =
directories1.get(i).getFields();
- assertEquals(fields.size(), fields1.size(), "The
TiffOutputDirectories have different numbers of fields.");
-
- for (int j = 0; j < fields.size(); j++) {
- final TiffOutputField field = fields.get(j);
- final TiffOutputField field1 = fields1.get(j);
- assertEquals(field.tag, field1.tag, "TiffOutputField tag
mismatch.");
- assertEquals(field.tagInfo, field1.tagInfo, "TiffOutputField
tagInfo mismatch.");
- assertEquals(field.abstractFieldType,
field1.abstractFieldType, "TiffOutputField fieldType mismatch.");
- assertEquals(field.count, field1.count, "TiffOutputField count
mismatch.");
+ private void assertTiffEquals(final TiffOutputSet sourceTiffOutputSet,
final TiffOutputSet duplicatedTiffOutputSet) {
+
+ final List<TiffOutputDirectory> sourceDirectories =
sourceTiffOutputSet.getDirectories();
+ final List<TiffOutputDirectory> duplicatedDirectories =
duplicatedTiffOutputSet.getDirectories();
+
+ assertEquals(sourceDirectories.size(), duplicatedDirectories.size(),
"The TiffOutputSets have different numbers of directories.");
+
+ for (int i = 0; i < sourceDirectories.size(); i++) {
+
+ final TiffOutputDirectory sourceDirectory =
sourceDirectories.get(i);
+ final TiffOutputDirectory duplicatedDirectory =
duplicatedDirectories.get(i);
+
+ assertEquals(sourceDirectory.getType(),
duplicatedDirectory.getType(), "Directory type mismatch.");
+
+ final List<TiffOutputField> sourceFields =
sourceDirectory.getFields();
+ final List<TiffOutputField> duplicatedFields =
duplicatedDirectory.getFields();
+
+ final boolean fieldCountMatches = sourceFields.size() ==
duplicatedFields.size();
+
+ if (!fieldCountMatches) {
+
+ /*
+ * Note that offset fields are not written again if a re-order
makes
+ * them unnecessary. The TiffWriter does not write in the same
order,
+ * but tries to optimize it.
+ */
+
+ final List<Integer> sourceTags = new ArrayList<>();
+ final List<Integer> duplicatedTags = new ArrayList<>();
+
+ for (TiffOutputField field : sourceFields)
+ sourceTags.add(field.tag);
+
+ for (TiffOutputField field : duplicatedFields)
+ duplicatedTags.add(field.tag);
+
+ final List<Integer> missingTags = new ArrayList<>(sourceTags);
+ missingTags.removeAll(duplicatedTags);
+ missingTags.remove((Integer)
ExifTagConstants.EXIF_TAG_EXIF_OFFSET.tag);
+ missingTags.remove((Integer)
ExifTagConstants.EXIF_TAG_GPSINFO.tag);
+ missingTags.remove((Integer)
ExifTagConstants.EXIF_TAG_INTEROP_OFFSET.tag);
+ missingTags.remove((Integer)
TiffTagConstants.TIFF_TAG_JPEG_INTERCHANGE_FORMAT.tag);
+
+ assertTrue(missingTags.isEmpty(), "Missing tags: " +
missingTags);
+ }
+
+ for (TiffOutputField sourceField : sourceFields) {
+
+ final boolean isOffsetField =
+ sourceField.tag ==
ExifTagConstants.EXIF_TAG_EXIF_OFFSET.tag ||
+ sourceField.tag ==
ExifTagConstants.EXIF_TAG_GPSINFO.tag ||
+ sourceField.tag ==
ExifTagConstants.EXIF_TAG_INTEROP_OFFSET.tag ||
+ sourceField.tag ==
TiffTagConstants.TIFF_TAG_JPEG_INTERCHANGE_FORMAT.tag;
+
+ /*
+ * Ignore offset fields. They may not be needed after rewrite
+ * and their value changes anyway.
+ */
+ if (isOffsetField)
+ continue;
+
+ TiffOutputField duplicatedField =
duplicatedDirectory.findField(sourceField.tag);
+
+ assertNotNull(duplicatedField, "Field is missing: " +
sourceField.tagInfo);
+
+ assertEquals(sourceField.tag, duplicatedField.tag,
"TiffOutputField tag mismatch.");
+ assertEquals(sourceField.abstractFieldType,
duplicatedField.abstractFieldType, "TiffOutputField fieldType mismatch.");
+ assertEquals(sourceField.count, duplicatedField.count,
"TiffOutputField count mismatch.");
+
+ boolean sameValue = sourceField.bytesEqual(duplicatedField);
+
+ assertTrue(sameValue, "Bytes are different for field: " +
sourceField);
}
}
}
private void copyToDuplicateFile(final File sourceFile, final
TiffOutputSet duplicateTiffOutputSet) throws IOException, ImagingException,
ImagingException {
+
final ExifRewriter exifRewriter = new ExifRewriter();
+
duplicateFile = createTempFile();
+
try (OutputStream duplicateOutputStream = new BufferedOutputStream(new
FileOutputStream(duplicateFile))) {
exifRewriter.updateExifMetadataLossless(sourceFile,
duplicateOutputStream, duplicateTiffOutputSet);
}
}
private File createTempFile() {
+
final String tempDir = FileUtils.getTempDirectoryPath();
final String tempFileName = this.getClass().getName() + "-" +
random.nextLong() + ".tmp";
+
return new File(tempDir, tempFileName);
}
private TiffOutputSet duplicateTiffOutputSet(final TiffOutputSet
sourceTiffOutputSet) throws ImagingException {
- final TiffOutputSet duplicateTiffOutputSet = new TiffOutputSet();
+
+ final TiffOutputSet duplicateTiffOutputSet = new TiffOutputSet(
+ sourceTiffOutputSet.byteOrder
+ );
+
for (final TiffOutputDirectory tiffOutputDirectory :
sourceTiffOutputSet) {
duplicateTiffOutputSet.addDirectory(tiffOutputDirectory);
}
+
return duplicateTiffOutputSet;
}
- private JpegImageMetadata getJpegImageMetadata(final File sourceFile)
throws ImagingException, IOException {
+ private JpegImageMetadata getJpegImageMetadata(final File sourceFile)
throws IOException {
+
final JpegImageMetadata jpegImageMetadata = (JpegImageMetadata)
Imaging.getMetadata(sourceFile);
- if (null == jpegImageMetadata) {
+
+ if (jpegImageMetadata == null) {
throw new TestSkippedException();
}
+
return jpegImageMetadata;
}
private TiffImageMetadata getTiffImageMetadata(final JpegImageMetadata
sourceJpegImageMetadata) {
+
final TiffImageMetadata tiffImageMetadata =
sourceJpegImageMetadata.getExif();
- if (null == tiffImageMetadata) {
+
+ if (tiffImageMetadata == null) {
throw new TestSkippedException();
}
+
return tiffImageMetadata;
}
private TiffOutputSet getTiffOutputSet(final TiffImageMetadata
sourceTiffImageMetadata) throws ImagingException {
+
final TiffOutputSet tiffOutputSet =
sourceTiffImageMetadata.getOutputSet();
+
if (tiffOutputSet == null) {
throw new TestSkippedException();
}
+
return tiffOutputSet;
}
@AfterEach
void tearDown() {
+
if (duplicateFile != null && duplicateFile.exists()) {
duplicateFile.delete();
duplicateFile.deleteOnExit();
}
}
- @ParameterizedTest
Review Comment:
This `toString()` comparison test is useless.
The TiffWriter does change the order of fields in an attempt to optimize for
space. This results in fields being reordered and unnecessary offset fields to
be dropped.
You only can compare the `TiffOutputSet`.
I assume the author did this to compare values. This is why I introduced the
value comparison API to TiffOutputField.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
