RFR: 8351108: ImageIO.write(..) fails with exception when writing JPEG with IndexColorModel [v3]

[Jeremy Wood]

On Mon, 10 Mar 2025 13:16:21 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

>> Jeremy Wood has updated the pull request incrementally with five additional commits since the last revision:
>> 
>>  - 8351108: adding empty line to bottom of file
>>    
>>    This is in response to:
>>    https://github.com/openjdk/jdk/pull/23884#discussion_r1987278977
>>  - Merge remote-tracking branch 'origin/JDK-8351108' into JDK-8351108
>>  - 8351108: replacing wildcard import
>>    
>>    This is in response to:
>>    https://github.com/openjdk/jdk/pull/23884#discussion_r1987268179
>>  - 8351108: updating copyright year
>>    
>>    This is in response to:
>>    https://github.com/openjdk/jdk/pull/23884#discussion_r1987265582
>>  - 8351108: changing indentation / line wrapping
>>    
>>    This is in response to:
>>    https://github.com/openjdk/jdk/pull/23884#discussion_r1987265134
>
> src/java.desktop/share/classes/javax/imageio/ImageTypeSpecifier.java line 1:
> 
>> 1: /*
> 
> Could you bump the copyright year?

Thanks, this is updated

> src/java.desktop/share/classes/javax/imageio/ImageTypeSpecifier.java line 923:
> 
>> 921:             int bufferedImageType = ((BufferedImage)image).getType();
>> 922:             if (bufferedImageType != BufferedImage.TYPE_CUSTOM &&
>> 923:                   bufferedImageType != BufferedImage.TYPE_BYTE_INDEXED) {
> 
> Suggestion:
> 
>             if (bufferedImageType != BufferedImage.TYPE_CUSTOM &&
>                     bufferedImageType != BufferedImage.TYPE_BYTE_INDEXED) {
> 
> Usually, 8-space indentation used for wrapped continuation lines, if the wrapped isn't aligned to a structure on the first line.
> 
> Personally, I'd move the `&&` operator to the wrapped line — it clearly indicates it's a continuation. Yet neither style has been agreed upon.

OK, this is updated.

> test/jdk/javax/imageio/plugins/jpeg/JpegWriterWriteNonOpaqueIndexColorModelTest.java line 33:
> 
>> 31: 
>> 32: import javax.imageio.ImageIO;
>> 33: import java.awt.*;
> 
> Could you expand the wildcard imports?

OK, this is updated.

> test/jdk/javax/imageio/plugins/jpeg/JpegWriterWriteNonOpaqueIndexColorModelTest.java line 81:
> 
>> 79:             e.printStackTrace();
>> 80:             return false;
>> 81:         }
> 
> Should an exception be allowed to escape? You throw an `Error` if the expected return value doesn't match.

This is by design.

The main method resembles:

    public static void main(String[] args) throws IOException {
        boolean b1 = testJpegWriter(Transparency.OPAQUE, "OPAQUE", true);
        boolean b2 = testJpegWriter(Transparency.BITMASK, "BITMASK", false);
        boolean b3 = testJpegWriter(Transparency.TRANSLUCENT, "TRANSLUCENT", false);
        if (!(b1 && b2 && b3))
            throw new Error("Test failed");
    }


The intention here is to help log multiple failures at once. (Otherwise: if there are two failures A and B, then you can only discover B after resolving A.)

> test/jdk/javax/imageio/plugins/jpeg/JpegWriterWriteNonOpaqueIndexColorModelTest.java line 83:
> 
>> 81:         }
>> 82:     }
>> 83: }
> 
> Having a blank line in the end of a file is preferred.

OK, this is updated.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987717503
PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987717590
PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987717421
PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987717299
PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987717159