From 4ee02e03bf7a12ef16c941cc4d755e90c5563091 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Tue, 21 Nov 2023 00:43:30 +1100 Subject: [PATCH] Updates from code review and regression testing --- Source/com/drew/lang/SequentialReader.java | 18 ------ Source/com/drew/lang/StreamReader.java | 3 - .../drew/metadata/exif/ExifTiffHandler.java | 2 +- .../NikonPictureControl1Descriptor.java | 60 ++++++++++--------- .../NikonPictureControl1Directory.java | 54 ++++++++++------- .../NikonPictureControl2Descriptor.java | 18 ++++-- .../NikonPictureControl2Directory.java | 37 +++++++++--- .../drew/lang/SequentialAccessTestBase.java | 11 ---- 8 files changed, 107 insertions(+), 96 deletions(-) diff --git a/Source/com/drew/lang/SequentialReader.java b/Source/com/drew/lang/SequentialReader.java index a5edc084e..f9a3f5a15 100644 --- a/Source/com/drew/lang/SequentialReader.java +++ b/Source/com/drew/lang/SequentialReader.java @@ -362,9 +362,6 @@ public StringValue getNullTerminatedStringValue(int maxLengthBytes, Charset char /** * Returns the sequence of bytes punctuated by a \0 value. - * It will place the cursor after the first occurrence of \0. - *
- * Use getNullTerminatedStringAndSkipToNextPosition if you want the cursor to move moved at the end of maxLengthBytes. * * @param maxLengthBytes The maximum number of bytes to read. If a \0 byte is not reached within this limit, * the returned array will be maxLengthBytes long. @@ -389,19 +386,4 @@ public byte[] getNullTerminatedBytes(int maxLengthBytes) throws IOException System.arraycopy(buffer, 0, bytes, 0, length); return bytes; } - - /** - * Read until the null terminated byte and automatically move the end of the requested position. - * @param maxLengthBytes - * @param charset - * @return - * @throws IOException - */ - public StringValue getNullTerminatedStringAndSkipToNextPosition(int maxLengthBytes, Charset charset) throws IOException { - byte[] bytes = this.getNullTerminatedBytes(maxLengthBytes); - if (bytes.length < maxLengthBytes - 1) { - this.trySkip(maxLengthBytes - bytes.length - 1); - } - return new StringValue(bytes, charset); - } } diff --git a/Source/com/drew/lang/StreamReader.java b/Source/com/drew/lang/StreamReader.java index 3b98beae8..78761d866 100644 --- a/Source/com/drew/lang/StreamReader.java +++ b/Source/com/drew/lang/StreamReader.java @@ -22,12 +22,10 @@ package com.drew.lang; import com.drew.lang.annotations.NotNull; -import com.drew.metadata.StringValue; import java.io.EOFException; import java.io.IOException; import java.io.InputStream; -import java.nio.charset.Charset; /** * @@ -143,5 +141,4 @@ private long skipInternal(long n) throws IOException _pos += skippedTotal; return skippedTotal; } - } diff --git a/Source/com/drew/metadata/exif/ExifTiffHandler.java b/Source/com/drew/metadata/exif/ExifTiffHandler.java index 64a8316b7..d7d5f94e7 100644 --- a/Source/com/drew/metadata/exif/ExifTiffHandler.java +++ b/Source/com/drew/metadata/exif/ExifTiffHandler.java @@ -360,7 +360,7 @@ public boolean customProcessTag(final int tagOffset, _metadata.addDirectory(directory); return true; } else if (byteCount == 74) { - //TODO: + //TODO NikonPictureControl3Directory } } } diff --git a/Source/com/drew/metadata/exif/makernotes/NikonPictureControl1Descriptor.java b/Source/com/drew/metadata/exif/makernotes/NikonPictureControl1Descriptor.java index 7a890a7b3..b7afb6975 100644 --- a/Source/com/drew/metadata/exif/makernotes/NikonPictureControl1Descriptor.java +++ b/Source/com/drew/metadata/exif/makernotes/NikonPictureControl1Descriptor.java @@ -6,13 +6,16 @@ import static com.drew.metadata.exif.makernotes.NikonPictureControl1Directory.TAG_PICTURE_CONTROL_ADJUST; import static com.drew.metadata.exif.makernotes.NikonPictureControl1Directory.TAG_TONING_EFFECT; -public final class NikonPictureControl1Descriptor extends TagDescriptor { - public NikonPictureControl1Descriptor(NikonPictureControl1Directory directory) { +public final class NikonPictureControl1Descriptor extends TagDescriptor +{ + public NikonPictureControl1Descriptor(NikonPictureControl1Directory directory) + { super(directory); } @Override - public String getDescription(int tagType) { + public String getDescription(int tagType) + { switch (tagType) { case TAG_PICTURE_CONTROL_ADJUST: return getPictureControlAdjustDescription(); @@ -25,7 +28,8 @@ public String getDescription(int tagType) { } } - public String getPictureControlAdjustDescription() { + public String getPictureControlAdjustDescription() + { return getIndexedDescription( TAG_PICTURE_CONTROL_ADJUST, "Default Settings", @@ -34,58 +38,60 @@ public String getPictureControlAdjustDescription() { ); } - public String getFilterEffectDescription() { - byte[] value = _directory.getByteArray(TAG_FILTER_EFFECT); + public String getFilterEffectDescription() + { + Integer value = _directory.getInteger(TAG_FILTER_EFFECT); if (value == null) { return null; } - switch (value[0]) { - case (byte) 0x80: + switch (value) { + case 0x80: return "Off"; - case (byte) 0x81: + case 0x81: return "Yellow"; - case (byte) 0x82: + case 0x82: return "Orange"; - case (byte) 0x83: + case 0x83: return "Red"; - case (byte) 0x84: + case 0x84: return "Green"; - case (byte) 0xFF: + case 0xFF: return "N/A"; default: return super.getDescription(TAG_FILTER_EFFECT); } } - public String getToningEffectDescription() { - byte[] value = _directory.getByteArray(TAG_TONING_EFFECT); + public String getToningEffectDescription() + { + Integer value = _directory.getInteger(TAG_TONING_EFFECT); if (value == null) { return null; } - switch (value[0]) { - case (byte) 0x80: + switch (value) { + case 0x80: return "B&W"; - case (byte) 0x81: + case 0x81: return "Sepia"; - case (byte) 0x82: + case 0x82: return "Cyanotype"; - case (byte) 0x83: + case 0x83: return "Red"; - case (byte) 0x84: + case 0x84: return "Yellow"; - case (byte) 0x85: + case 0x85: return "Green"; - case (byte) 0x86: + case 0x86: return "Blue-green"; - case (byte) 0x87: + case 0x87: return "Blue"; - case (byte) 0x88: + case 0x88: return "Purple-blue"; - case (byte) 0x89: + case 0x89: return "Red-purple"; - case (byte) 0xFF: + case 0xFF: return "N/A"; default: return super.getDescription(TAG_TONING_EFFECT); diff --git a/Source/com/drew/metadata/exif/makernotes/NikonPictureControl1Directory.java b/Source/com/drew/metadata/exif/makernotes/NikonPictureControl1Directory.java index 8151cd590..9cf94bfb6 100644 --- a/Source/com/drew/metadata/exif/makernotes/NikonPictureControl1Directory.java +++ b/Source/com/drew/metadata/exif/makernotes/NikonPictureControl1Directory.java @@ -2,15 +2,21 @@ import com.drew.lang.Charsets; import com.drew.lang.SequentialByteArrayReader; +import com.drew.lang.annotations.NotNull; import com.drew.metadata.Directory; import java.io.IOException; import java.util.HashMap; -public final class NikonPictureControl1Directory extends Directory { +public final class NikonPictureControl1Directory extends Directory +{ + // Tag values are offsets into the underlying data. + // Data from https://exiftool.org/TagNames/Nikon.html#PictureControl + public static final int TAG_PICTURE_CONTROL_VERSION = 0; public static final int TAG_PICTURE_CONTROL_NAME = 4; public static final int TAG_PICTURE_CONTROL_BASE = 24; + // skip 4 public static final int TAG_PICTURE_CONTROL_ADJUST = 48; public static final int TAG_PICTURE_CONTROL_QUICK_ADJUST = 49; public static final int TAG_SHARPNESS = 50; @@ -24,7 +30,8 @@ public final class NikonPictureControl1Directory extends Directory { private static final HashMap TAG_NAME_MAP = new HashMap<>(); - static { + static + { TAG_NAME_MAP.put(TAG_PICTURE_CONTROL_VERSION, "Picture Control Version"); TAG_NAME_MAP.put(TAG_PICTURE_CONTROL_NAME, "Picture Control Name"); TAG_NAME_MAP.put(TAG_PICTURE_CONTROL_BASE, "Picture Control Base"); @@ -40,22 +47,28 @@ public final class NikonPictureControl1Directory extends Directory { TAG_NAME_MAP.put(TAG_TONING_SATURATION, "Toning Saturation"); } - public NikonPictureControl1Directory() { + public NikonPictureControl1Directory() + { setDescriptor(new NikonPictureControl1Descriptor(this)); } + @NotNull @Override - public String getName() { + public String getName() + { return "Nikon PictureControl 1"; } + @NotNull @Override - protected HashMap getTagNameMap() { + protected HashMap getTagNameMap() + { return TAG_NAME_MAP; } - public static NikonPictureControl1Directory read(byte[] bytes) throws IOException { - int EXPECTED_LENGTH = 58; + public static NikonPictureControl1Directory read(byte[] bytes) throws IOException + { + final int EXPECTED_LENGTH = 58; if (bytes.length != EXPECTED_LENGTH) { throw new IllegalArgumentException("Must have " + EXPECTED_LENGTH + " bytes."); @@ -65,21 +78,20 @@ public static NikonPictureControl1Directory read(byte[] bytes) throws IOExceptio NikonPictureControl1Directory directory = new NikonPictureControl1Directory(); - directory.setString(TAG_PICTURE_CONTROL_VERSION, reader.getNullTerminatedStringAndSkipToNextPosition(4, Charsets.UTF_8).toString()); - directory.setString(TAG_PICTURE_CONTROL_NAME, reader.getNullTerminatedStringAndSkipToNextPosition(20, Charsets.UTF_8).toString()); - directory.setString(TAG_PICTURE_CONTROL_BASE, reader.getNullTerminatedStringAndSkipToNextPosition(20, Charsets.UTF_8).toString()); - + directory.setObject(TAG_PICTURE_CONTROL_VERSION, reader.getStringValue(4, Charsets.UTF_8)); + directory.setObject(TAG_PICTURE_CONTROL_NAME, reader.getStringValue(20, Charsets.UTF_8)); + directory.setObject(TAG_PICTURE_CONTROL_BASE, reader.getStringValue(20, Charsets.UTF_8)); reader.skip(4); - directory.setObject(TAG_PICTURE_CONTROL_ADJUST, reader.getByte()); - directory.setObject(TAG_PICTURE_CONTROL_QUICK_ADJUST, reader.getByte()); - directory.setObject(TAG_SHARPNESS, reader.getByte()); - directory.setObject(TAG_CONTRAST, reader.getByte()); - directory.setObject(TAG_BRIGHTNESS, reader.getByte()); - directory.setObject(TAG_SATURATION, reader.getByte()); - directory.setObject(TAG_HUE_ADJUSTMENT, reader.getByte()); - directory.setObject(TAG_FILTER_EFFECT, reader.getByte()); - directory.setObject(TAG_TONING_EFFECT, reader.getByte()); - directory.setObject(TAG_TONING_SATURATION, reader.getByte()); + directory.setObject(TAG_PICTURE_CONTROL_ADJUST, reader.getUInt8()); + directory.setObject(TAG_PICTURE_CONTROL_QUICK_ADJUST, reader.getUInt8()); + directory.setObject(TAG_SHARPNESS, reader.getUInt8()); + directory.setObject(TAG_CONTRAST, reader.getUInt8()); + directory.setObject(TAG_BRIGHTNESS, reader.getUInt8()); + directory.setObject(TAG_SATURATION, reader.getUInt8()); + directory.setObject(TAG_HUE_ADJUSTMENT, reader.getUInt8()); + directory.setObject(TAG_FILTER_EFFECT, reader.getUInt8()); + directory.setObject(TAG_TONING_EFFECT, reader.getUInt8()); + directory.setObject(TAG_TONING_SATURATION, reader.getUInt8()); assert (reader.getPosition() == EXPECTED_LENGTH); diff --git a/Source/com/drew/metadata/exif/makernotes/NikonPictureControl2Descriptor.java b/Source/com/drew/metadata/exif/makernotes/NikonPictureControl2Descriptor.java index 37d498b0d..50742f3e8 100644 --- a/Source/com/drew/metadata/exif/makernotes/NikonPictureControl2Descriptor.java +++ b/Source/com/drew/metadata/exif/makernotes/NikonPictureControl2Descriptor.java @@ -6,13 +6,16 @@ import static com.drew.metadata.exif.makernotes.NikonPictureControl2Directory.TAG_PICTURE_CONTROL_ADJUST; import static com.drew.metadata.exif.makernotes.NikonPictureControl2Directory.TAG_TONING_EFFECT; -public final class NikonPictureControl2Descriptor extends TagDescriptor { - public NikonPictureControl2Descriptor(NikonPictureControl2Directory directory) { +public final class NikonPictureControl2Descriptor extends TagDescriptor +{ + public NikonPictureControl2Descriptor(NikonPictureControl2Directory directory) + { super(directory); } @Override - public String getDescription(int tagType) { + public String getDescription(int tagType) + { switch (tagType) { case TAG_PICTURE_CONTROL_ADJUST: return getPictureControlAdjustDescription(); @@ -25,7 +28,8 @@ public String getDescription(int tagType) { } } - public String getPictureControlAdjustDescription() { + public String getPictureControlAdjustDescription() + { return getIndexedDescription( TAG_PICTURE_CONTROL_ADJUST, "Default Settings", @@ -34,7 +38,8 @@ public String getPictureControlAdjustDescription() { ); } - public String getFilterEffectDescription() { + public String getFilterEffectDescription() + { byte[] value = _directory.getByteArray(TAG_FILTER_EFFECT); if (value == null) { return null; @@ -58,7 +63,8 @@ public String getFilterEffectDescription() { } } - public String getToningEffectDescription() { + public String getToningEffectDescription() + { byte[] value = _directory.getByteArray(TAG_TONING_EFFECT); if (value == null) { return null; diff --git a/Source/com/drew/metadata/exif/makernotes/NikonPictureControl2Directory.java b/Source/com/drew/metadata/exif/makernotes/NikonPictureControl2Directory.java index 65bd89890..74cb0a2cd 100644 --- a/Source/com/drew/metadata/exif/makernotes/NikonPictureControl2Directory.java +++ b/Source/com/drew/metadata/exif/makernotes/NikonPictureControl2Directory.java @@ -2,30 +2,43 @@ import com.drew.lang.Charsets; import com.drew.lang.SequentialByteArrayReader; +import com.drew.lang.annotations.NotNull; import com.drew.metadata.Directory; import java.io.IOException; import java.util.HashMap; -public final class NikonPictureControl2Directory extends Directory { +public final class NikonPictureControl2Directory extends Directory +{ + // Tag values are offsets into the underlying data. + // Data from https://exiftool.org/TagNames/Nikon.html#PictureControl + public static final int TAG_PICTURE_CONTROL_VERSION = 0; public static final int TAG_PICTURE_CONTROL_NAME = 4; public static final int TAG_PICTURE_CONTROL_BASE = 24; public static final int TAG_PICTURE_CONTROL_ADJUST = 48; public static final int TAG_PICTURE_CONTROL_QUICK_ADJUST = 49; + // skip 1 public static final int TAG_SHARPNESS = 51; + // skip 1 public static final int TAG_CLARITY = 53; + // skip 1 public static final int TAG_CONTRAST = 55; + // skip 1 public static final int TAG_BRIGHTNESS = 57; + // skip 1 public static final int TAG_SATURATION = 59; + // skip 1 public static final int TAG_HUE = 61; + // skip 1 public static final int TAG_FILTER_EFFECT = 63; public static final int TAG_TONING_EFFECT = 64; public static final int TAG_TONING_SATURATION = 65; private static final HashMap TAG_NAME_MAP = new HashMap<>(); - static { + static + { TAG_NAME_MAP.put(TAG_PICTURE_CONTROL_VERSION, "Picture Control Version"); TAG_NAME_MAP.put(TAG_PICTURE_CONTROL_NAME, "Picture Control Name"); TAG_NAME_MAP.put(TAG_PICTURE_CONTROL_BASE, "Picture Control Base"); @@ -42,21 +55,27 @@ public final class NikonPictureControl2Directory extends Directory { TAG_NAME_MAP.put(TAG_TONING_SATURATION, "Toning Saturation"); } - public NikonPictureControl2Directory() { + public NikonPictureControl2Directory() + { setDescriptor(new NikonPictureControl2Descriptor(this)); } + @NotNull @Override - public String getName() { + public String getName() + { return "Nikon PictureControl 2"; } + @NotNull @Override - protected HashMap getTagNameMap() { + protected HashMap getTagNameMap() + { return TAG_NAME_MAP; } - public static NikonPictureControl2Directory read(byte[] bytes) throws IOException { + public static NikonPictureControl2Directory read(byte[] bytes) throws IOException + { int EXPECTED_LENGTH = 68; if (bytes.length != EXPECTED_LENGTH) { @@ -67,9 +86,9 @@ public static NikonPictureControl2Directory read(byte[] bytes) throws IOExceptio NikonPictureControl2Directory directory = new NikonPictureControl2Directory(); - directory.setString(TAG_PICTURE_CONTROL_VERSION, reader.getNullTerminatedStringAndSkipToNextPosition(4, Charsets.UTF_8).toString()); - directory.setString(TAG_PICTURE_CONTROL_NAME, reader.getNullTerminatedStringAndSkipToNextPosition(20, Charsets.UTF_8).toString()); - directory.setString(TAG_PICTURE_CONTROL_BASE, reader.getNullTerminatedStringAndSkipToNextPosition(20, Charsets.UTF_8).toString()); + directory.setString(TAG_PICTURE_CONTROL_VERSION, reader.getStringValue(4, Charsets.UTF_8).toString()); + directory.setString(TAG_PICTURE_CONTROL_NAME, reader.getStringValue(20, Charsets.UTF_8).toString()); + directory.setString(TAG_PICTURE_CONTROL_BASE, reader.getStringValue(20, Charsets.UTF_8).toString()); reader.skip(4); directory.setObject(TAG_PICTURE_CONTROL_ADJUST, reader.getByte()); diff --git a/Tests/com/drew/lang/SequentialAccessTestBase.java b/Tests/com/drew/lang/SequentialAccessTestBase.java index c1dae75ab..8c2008e54 100644 --- a/Tests/com/drew/lang/SequentialAccessTestBase.java +++ b/Tests/com/drew/lang/SequentialAccessTestBase.java @@ -265,17 +265,6 @@ the cursor is after B (third) position assertEquals("ABCDEF", reader.getNullTerminatedString(6, Charsets.UTF_8).toString()); } - @Test - public void testGetNullTerminatedStringAndSkipToNextPosition() throws IOException { - byte NULL = 0x00; - byte[] bytes = new byte[]{0x41, 0x42, NULL, NULL, NULL, 0x43, 0x44, NULL, 0x41, 0x42, 0x43, 0x44, 0x45, 0x46}; - SequentialReader reader = createReader(bytes); - - assertEquals("AB", reader.getNullTerminatedStringAndSkipToNextPosition(5, Charsets.UTF_8).toString()); - assertEquals("CD", reader.getNullTerminatedStringAndSkipToNextPosition(3, Charsets.UTF_8).toString()); - assertEquals("ABCDEF", reader.getNullTerminatedStringAndSkipToNextPosition(6, Charsets.UTF_8).toString()); - } - @Test public void testGetString() throws IOException {