garydgregory commented on code in PR #366:
URL: https://github.com/apache/commons-imaging/pull/366#discussion_r1537442107
##########
src/test/java/org/apache/commons/imaging/formats/jpeg/exif/ExifRewriterRoundtripTest.java:
##########
@@ -56,124 +62,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 =
TiffOutputFieldTestUtil.bytesEqual(sourceField, 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) {
Review Comment:
Don't clutter a PR changes in style, please, it clutters the PR with useless
changes and takes longer to review.
##########
src/main/java/org/apache/commons/imaging/formats/tiff/write/TiffOutputField.java:
##########
@@ -66,6 +66,10 @@ public boolean bytesEqual(final byte[] data) {
return Arrays.equals(bytes, data);
}
+ public boolean bytesEqual(final TiffOutputField other) {
+ return Arrays.equals(bytes, other.bytes);
+ }
Review Comment:
The method is still not package-private.
##########
src/test/java/org/apache/commons/imaging/formats/jpeg/exif/ExifRewriterRoundtripTest.java:
##########
@@ -56,124 +62,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 =
TiffOutputFieldTestUtil.bytesEqual(sourceField, duplicateField);
+
+ assertTrue(sameValue, "Bytes are different for field: " +
sourceField);
}
}
}
private void copyToDuplicateFile(final File sourceFile, final
TiffOutputSet duplicateTiffOutputSet) throws IOException, ImagingException,
ImagingException {
+
Review Comment:
Don't change whitespace, please. It clutters up the PR with noise and makes
it harder to review.
--
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: issues-unsubscr...@commons.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
