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.

Sign up for GitHub

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

Jump to bottom

Fix and extend Geotiff IFD handling to allow all integer types #1341

Merged
merged 4 commits into from
May 7, 2024
Merged

Fix and extend Geotiff IFD handling to allow all integer types #1341

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bbdf615
Fix and extend Geotiff IFD handling to allow all integer types
WeatherGod May 6, 2024
07e9ec3
style check and some other cleanups
WeatherGod May 6, 2024
610b1fb
Apply suggestions from code review
WeatherGod May 7, 2024
ec4f996
Change some of the GeoTiff methods to package-private static
WeatherGod May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 27 additions & 1 deletion cdm/misc/src/main/java/ucar/nc2/geotiff/GeoTiff.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,13 +357,23 @@ private int writeValues(ByteBuffer buffer, IFDEntry ifd) {
private int writeIntValue(ByteBuffer buffer, IFDEntry ifd, int v) {
switch (ifd.type.code) {
case 1:
case 2:
case 6:
case 7:
// unsigned byte and ascii
// signed byte and undefined (usually treated as raw binary)
buffer.put((byte) v);
return 1;
case 3:
case 8:
// unsigned and signed short
buffer.putShort((short) v);
return 2;
case 4:
case 5:
case 9:
case 10:
// unsigned and signed rational and 32-bit integer
buffer.putInt(v);
return 4;
}
Expand Down Expand Up @@ -549,11 +559,27 @@ private int readIntValue(ByteBuffer buffer, IFDEntry ifd) {
switch (ifd.type.code) {
case 1:
case 2:
return (int) buffer.get();
// unsigned byte and unsigned ascii
return (int) buffer.get() & 0xff;
case 3:
// unsigned short
return readUShortValue(buffer);
case 4:
case 5:
// unsigned rational and unsigned 32-bit integer
// Yes, this can lead to truncation. This is a bug
// in the design of the IFDEntry API
return (int) (buffer.getInt() & 0xffffffffL);
case 6:
case 7:
// signed byte and "undefined" (usually treated as binary data)
return (int) buffer.get();
case 8:
// signed short
return (int) buffer.getShort();
case 9:
case 10:
// signed rational and signed 32-bit integer
return buffer.getInt();
}
return 0;
Expand Down
104 changes: 104 additions & 0 deletions cdm/misc/src/test/java/ucar/nc2/geotiff/TestIFDEntry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
/*
* Copyright (c) 1998-2024 University Corporation for Atmospheric Research/Unidata
* See LICENSE for license information.
*/

package ucar.nc2.geotiff;

import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.io.*;
WeatherGod marked this conversation as resolved.
Show resolved Hide resolved
import java.lang.ReflectiveOperationException;
import java.lang.invoke.MethodHandles;
import java.lang.reflect.Method;
import java.nio.ByteBuffer;
import java.nio.ByteOrder;
import java.util.ArrayList;
import java.util.Collections;
WeatherGod marked this conversation as resolved.
Show resolved Hide resolved
import java.util.List;

/**
* IFDEntry read/write
*
* @author Ben Root
* @since 5/6/2024
*/
@RunWith(Parameterized.class)
public class TestIFDEntry {
private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public IFDEntry ifd;
// Because we are dealing with reflection
public int testValue;
WeatherGod marked this conversation as resolved.
Show resolved Hide resolved

@Parameterized.Parameters(name = "{0}_{1}")
public static List<Object[]> getTestParameters() {
List<Object[]> result = new ArrayList<>();
// Unsigned
result.add(new Object[] {new IFDEntry(null, FieldType.BYTE, 1), 0});
result.add(new Object[] {new IFDEntry(null, FieldType.BYTE, 1), Byte.MAX_VALUE});
result.add(new Object[] {new IFDEntry(null, FieldType.BYTE, 1), 255});
result.add(new Object[] {new IFDEntry(null, FieldType.ASCII, 1), 0});
result.add(new Object[] {new IFDEntry(null, FieldType.ASCII, 1), Byte.MAX_VALUE});
result.add(new Object[] {new IFDEntry(null, FieldType.ASCII, 1), 255});
result.add(new Object[] {new IFDEntry(null, FieldType.SHORT, 1), 0});
result.add(new Object[] {new IFDEntry(null, FieldType.SHORT, 1), Byte.MAX_VALUE});
result.add(new Object[] {new IFDEntry(null, FieldType.SHORT, 1), Short.MAX_VALUE});
result.add(new Object[] {new IFDEntry(null, FieldType.SHORT, 1), 65535});
result.add(new Object[] {new IFDEntry(null, FieldType.LONG, 1), 0});
result.add(new Object[] {new IFDEntry(null, FieldType.LONG, 1), Byte.MAX_VALUE});
result.add(new Object[] {new IFDEntry(null, FieldType.LONG, 1), Short.MAX_VALUE});
// NOTE: because of the API design, unsigned longs can't be properly read or written
// for all possible values because Java's integer is signed.
result.add(new Object[] {new IFDEntry(null, FieldType.LONG, 1), Integer.MAX_VALUE});

// Signed
result.add(new Object[] {new IFDEntry(null, FieldType.SBYTE, 1), 0});
result.add(new Object[] {new IFDEntry(null, FieldType.SBYTE, 1), Byte.MIN_VALUE});
result.add(new Object[] {new IFDEntry(null, FieldType.SBYTE, 1), Byte.MAX_VALUE});
result.add(new Object[] {new IFDEntry(null, FieldType.SSHORT, 1), 0});
result.add(new Object[] {new IFDEntry(null, FieldType.SSHORT, 1), Byte.MIN_VALUE});
result.add(new Object[] {new IFDEntry(null, FieldType.SSHORT, 1), -Byte.MAX_VALUE});
result.add(new Object[] {new IFDEntry(null, FieldType.SSHORT, 1), Byte.MAX_VALUE});
result.add(new Object[] {new IFDEntry(null, FieldType.SSHORT, 1), Short.MIN_VALUE});
result.add(new Object[] {new IFDEntry(null, FieldType.SSHORT, 1), Short.MAX_VALUE});
result.add(new Object[] {new IFDEntry(null, FieldType.SLONG, 1), 0});
result.add(new Object[] {new IFDEntry(null, FieldType.SLONG, 1), -Byte.MAX_VALUE});
result.add(new Object[] {new IFDEntry(null, FieldType.SLONG, 1), Byte.MAX_VALUE});
result.add(new Object[] {new IFDEntry(null, FieldType.SLONG, 1), -Short.MAX_VALUE});
result.add(new Object[] {new IFDEntry(null, FieldType.SLONG, 1), Short.MAX_VALUE});
result.add(new Object[] {new IFDEntry(null, FieldType.SLONG, 1), Integer.MIN_VALUE});
result.add(new Object[] {new IFDEntry(null, FieldType.SLONG, 1), Integer.MAX_VALUE});

return result;
}

public TestIFDEntry(IFDEntry ifd, int testValue) {
this.ifd = ifd;
this.testValue = testValue;
}

@Test
public void testRoundtrip() throws ReflectiveOperationException {
GeoTiff geotiff = new GeoTiff("foobar");
// 16 bytes should be more than enough
ByteBuffer buffer = ByteBuffer.allocate(16);
ByteOrder byteOrder = ByteOrder.BIG_ENDIAN;
buffer.order(byteOrder);

Method writeMethod =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally don't like this reflection solution for testing private methods. First, in general, private methods should not be tested, though I understand that in practice some classes would need big refactors to make the public API unit testable. Second, I think it makes the test harder to read. And finally, if someone were to rename writeIntValue, the compiler would not catch an issue here, you would just get NoSuchMethodException.

Is there any reasonable way to test the public API of GeoTiff instead? I am not as familiar with the code, so I am not sure how feasible this is. If not, I propose to make the method writeIntValue package private so you can test it without reflection. Also not a perfect solution, but I think it is slightly nicer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coming from python, the idea that one shouldn't test private methods is completely foreign to me. Unfortunately, with the way the GeoTiff class is designed, it would be difficult to not only construct the many geotiffs instances with the appropriate data types and ranges, but to then subsequently test that it was encoded correctly. The test would have a huge amount of unrelated setup that would make the test difficult to read and understand.

I am totally fine with changing it to be package private. We could even go a step further and make them static as they do not interact with any class members.

geotiff.getClass().getDeclaredMethod("writeIntValue", ByteBuffer.class, ifd.getClass(), int.class);
Method readMethod = geotiff.getClass().getDeclaredMethod("readIntValue", ByteBuffer.class, ifd.getClass());
readMethod.setAccessible(true);
writeMethod.setAccessible(true);
int writeSize = (int) writeMethod.invoke(geotiff, buffer, ifd, testValue);
buffer.position(0);

int readValue = (int) readMethod.invoke(geotiff, buffer, ifd);

Assert.assertEquals(testValue, readValue);
}
}