On Wed, 12 Mar 2025 18:23:12 GMT, Jeremy Wood <d...@openjdk.org> wrote:
>> JPEG segments can only be 65535-bytes long. (The marker length is expressed
>> as 2 bytes.) The problem in this ticket is that we were writing more than
>> 65535 bytes to a segment, which later caused parsing errors when we tried to
>> read the file back.
>>
>> This includes 2 changes:
>>
>> 1. We now clip the thumbnail width/height to fit within the available
>> marker. We were already constraining the width & height to a max of 255.
>> (Because the width & height are expressed as 1 byte.) So this new clipping
>> is similar to something we were already doing, and we issue the same
>> WARNING_THUMB_CLIPPED warning to the writer. (Does this require a CSR?)
>>
>> 2. This adds a bounds check to `write2bytes`. IMO the bigger problem in this
>> ticket was the `ImageIO.write(..)` caller thought they ended up with a valid
>> JPEG. (We were violating the "do no harm / lose no data" doctrine.) Now if
>> something like this ever comes up again: writing will fail with an
>> IIOException. Technically you can argue this change is unnecessary because
>> the new clipping avoids this error condition, but IMO including this safety
>> net is an overall good thing to keep. If anyone disagrees I'm OK with
>> removing it.
>>
>> Separately:
>> I included (and reversed) a commit in this branch that provides an alternate
>> solution. That commit anticipates the overflow problem and switches to a
>> JPEG-encoded thumbnail. From an end user perspective this "just works": they
>> get a (slightly lossly) thumbnail of the original dimensions. But I assume
>> it would be more controversial compared to the clipping approach, since the
>> clipping approach has a strong precedent in this codebase.
>
> Jeremy Wood has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - 8351110: adding a few clarifying comments
> - 8351110: changing `catch` statement to support assertEquals's Error
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/com/sun/imageio/plugins/jpeg/JFIFMarkerSegment.java
line 383:
> 381: if (thumbWidth * thumbHeight > maxArea) {
> 382:
> writer.warningOccurred(JPEGImageWriter.WARNING_THUMB_CLIPPED);
> 383: double scale = Math.sqrt( ((double)maxArea) /
> (double)(thumbWidth * thumbHeight));
Does it make sense to add a space before the closing parenthesis? For
consistency with the opening one; or remove these spaces just inside the method
call parentheses.
Suggestion:
double scale = Math.sqrt( ((double)maxArea) /
(double)(thumbWidth * thumbHeight) );
test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java line 66:
> 64: boolean b2 = new WriteJPEGThumbnailTest(100, 219).run();
> 65: if (!(b1 && b2))
> 66: System.err.println("Test failed");
Suggestion:
if (!(b1 && b2)) {
System.err.println("Test failed");
throw new Error("Test failed");
}
Always use braces, even for one-line statement.
You must throw an exception, otherwise the test will never fail from jtreg
point of view.
jdk> ./build/windows-x86_64-server-release/jdk/bin/java -jar
../jtreg/lib/jtreg.jar \
-w:./build/jtwork -nr -v \
test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java
runner starting test: javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java
runner finished test: javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java
Passed. Execution successful
Test results: passed: 1
And I don't have your fix.
test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java line 86:
> 84: } finally {
> 85: jpegData = byteOut.toByteArray();
> 86: }
Does it have to be in the `finally` block?
You can assign `jpegData` outside of the `try` block. If an exception is thrown
in `try`, the method stops executing and assigning to `jpegData` makes no sense.
test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java line 104:
> 102: }
> 103: System.out.println("\tTest passed");
> 104: } catch(Throwable e) {
Suggestion:
} catch (Throwable e) {
test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java line 142:
> 140: g.fillRect(0,900,100,100);
> 141: g.setColor(Color.magenta);
> 142: g.fillRect(900,900,100,100);
Spaces after commas, upper-case color constants:
Suggestion:
g.setColor(Color.RED);
g.fillRect(0, 0, 100, 100);
g.setColor(Color.GREEN);
g.fillRect(900, 0, 900, 100);
g.setColor(Color.ORANGE);
g.fillRect(0, 900, 100, 100);
g.setColor(Color.MAGENTA);
g.fillRect(900, 900, 100, 100);
test/jdk/javax/imageio/plugins/jpeg/WriteJPEGThumbnailTest.java line 152:
> 150: while(readers.hasNext()) {
> 151: reader = readers.next();
> 152: if(reader.canReadRaster()) {
Suggestion:
while (readers.hasNext()) {
reader = readers.next();
if (reader.canReadRaster()) {
-------------
PR Review: https://git.openjdk.org/jdk/pull/23920#pullrequestreview-2743856146
PR Review Comment: https://git.openjdk.org/jdk/pull/23920#discussion_r2029289420
PR Review Comment: https://git.openjdk.org/jdk/pull/23920#discussion_r2029310112
PR Review Comment: https://git.openjdk.org/jdk/pull/23920#discussion_r2029297962
PR Review Comment: https://git.openjdk.org/jdk/pull/23920#discussion_r2029303202
PR Review Comment: https://git.openjdk.org/jdk/pull/23920#discussion_r2029301961
PR Review Comment: https://git.openjdk.org/jdk/pull/23920#discussion_r2029302608
