On Thu, 2 Jan 2025 06:49:19 GMT, Jeremy <d...@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.
src/java.desktop/share/classes/com/sun/imageio/plugins/jpeg/JPEGImageReader.java
line 1653:
> 1651: if (exifMarkerSegment != null
> 1652: && exifMarkerSegment.getNumThumbnails() == 1) {
> 1653: return 1;
I know that my original code was also like this, but I think the eventual
intent was to read both JFIF (APP0) and Exif (APP1) thumbnails if both are
present. In such a case, the thumbnail count would be 2, the JFIF thumbnail
would be at index 0, and the Exif thumbnail at index 1.
In general I would expect that if both of these thumbnails were present, then
they would be identical. If this were not the case, however, then preferring
the Exif thumbnail would be a behavioral change. This is not necessarily a
blocker for the current PR, but it might need to be addressed later.
src/java.desktop/share/classes/com/sun/imageio/plugins/jpeg/JPEGImageReader.java
line 1758:
> 1756: }
> 1757:
> 1758: // Now we know that there is a jfif segment and that iis is
> good
I think `iis` should be `it` in this comment.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22898#discussion_r1956635292
PR Review Comment: https://git.openjdk.org/jdk/pull/22898#discussion_r1956634835
