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

2/4 Support BigTIFF encoded TIFF data #584

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

don-vip
Copy link

@don-vip don-vip commented Jun 18, 2022

@don-vip don-vip marked this pull request as ready for review June 18, 2022 21:13
@don-vip don-vip changed the title Support BigTIFF encoded TIFF data 2/4 Support BigTIFF encoded TIFF data Jun 19, 2022
Copy link
Owner

@drewnoakes drewnoakes left a comment

Choose a reason for hiding this comment

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

Looks good. As with your other PRs, I'm dropping my thoughts here as a note to myself for when I pull this down to play with it, hopefully later today.

final int tiffMarker = reader.getUInt16(2);
final short tiffMarker = (short) reader.getUInt16(2);
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not totally sure about this change, as Java doesn't have an unsigned short type (unlike .NET where this change is being ported from). Generally throughout the Java library we widen the type to allow the full unsigned range to remain unsigned. In this case though it's not really a numeric value used for any kind of size or comparison, but just an identifier. That said, it does change the public API, which is a breaking change. There are other breaking changes in these PRs though.

Just dumping out my thoughts here. I'm going to pull down your work here and take it for a spin before merging.

* @return the 64 bit int value, between 0x0000000000000000 and 0xFFFFFFFFFFFFFFFF
* @throws IOException the buffer does not contain enough bytes to service the request, or index is negative
*/
public long getUInt64(int index) throws IOException
Copy link
Owner

Choose a reason for hiding this comment

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

Java doesn't have an unsigned long, so this method is redundant as far as I can tell. Keeping it does at least allow calling code to express the intent, despite the limitation of the language.

I notice though that, comparing to getInt64 above, there are a bunch of && masks in that method that may be redundant (and possibly others too).

Copy link
Author

Choose a reason for hiding this comment

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

you're right, I wasn't sure what to do. It mainly helps to compare the .NET and Java codebases.

Source/com/drew/imaging/tiff/TiffReader.java Outdated Show resolved Hide resolved
? reader.getUInt64(finalTagOffset)
: reader.getUInt32(finalTagOffset);

if (nextIfdOffsetLong != 0 && nextIfdOffsetLong <= Integer.MAX_VALUE) {
Copy link
Owner

Choose a reason for hiding this comment

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

The fact that getUInt64 might return a negative value is a problem here. I'm wondering if that method should be changed to throw if the value is actually negative. Otherwise we have to do things like check nextIfdOffsetLong < 0 here (and possibly elsewhere).

On the other hand, throwing might lead to failing to read data that would otherwise be safe to read.

I really wish Java had unsigned numeric types!

Note that while BigTIFF supports files greater than 2 GiB in size, our current implementation does not due to the pervasive use of Int32 throughout the code to represent offsets into the data.
This will only ever be a 16-bit value.
This allows combining the add and test operations into a single lookup.
@don-vip
Copy link
Author

don-vip commented Jul 29, 2024

This one looks good too, I see the Java <> .NET differences disappear for BigTIFF files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants