RFR: 8160327: Support for thumbnails present in APP1 marker for JPEG [v2]

Phil Race prr at openjdk.org
Thu Feb 13 21:07:12 UTC 2025


On Thu, 2 Jan 2025 06:49:19 GMT, Jeremy <duke at openjdk.org> wrote:

>> This adds support for parsing thumbnails in an APP1 Exif marker.
>> 
>> This builds on an unfinished proposal by Brian Burkhalter (around 2016). In that previous work the only additional meta info he parsed was the image creation time; this PR similarly includes the same property. (I can't speak to why he included that property, but it looks like he has a lot of experience with ImageIO so I trust his judgment.)
>> 
>> The test addresses the original images attached to the ticket plus a few extra images I found on my computer that include unusual properties. (Possibly those images are malformed, but if they exist in the wild and other platforms support them then I'd prefer to support them too.)
>
> Jeremy has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8160327: fixing typo so `thumbnailPos` can be zero
>   
>   The `thumbnailLength` cannot be zero, but the position can be.

This is a desirable and over-due fix but some small things to handle first.
Whilst this passed testing, I have some comments, one in-line, but testing related ones here
There are 6 images attached, but the test uses only 5.
The images were all attached to the bug, but I don't think that means we automatically have the rights to push them into JDK.
Brian Burkhalter attached at least 3 of them, so I need him to speak up about where they came from. I'd also like @bplb to review this change

src/java.desktop/share/classes/com/sun/imageio/plugins/jpeg/ExifMarkerSegment.java line 165:

> 163:                 ByteOrder.BIG_ENDIAN : ByteOrder.LITTLE_ENDIAN);
> 164:         if (input.readUnsignedShort() != TIFF_MAGIC) {
> 165:             throw new IllegalArgumentException("Bad magic number");

Where does this exception end up ? I would have supposed that if there's an Exif segment we don't like it would be best to just act like the segment isn't there.

-------------

PR Review: https://git.openjdk.org/jdk/pull/22898#pullrequestreview-2616189089
PR Review Comment: https://git.openjdk.org/jdk/pull/22898#discussion_r1955194737


More information about the client-libs-dev mailing list