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

[Alexey Ivanov]

On Tue, 4 Mar 2025 06:51:54 GMT, Jeremy Wood <duke at openjdk.org> wrote:

> Previously ImageTypeSpecifier treated all TYPE_BYTE_INDEXED as if they were opaque.
> 
> In this ticket's case: an ImageWriter received the wrong ImageTypeSpecifier and mistakenly indicated it *could* support a BufferedImage. But when the actual BufferedImage arrived (which was translucent), the ImageWriter threw an exception.
> 
> Instead:
> Now the ImageTypeSpecifier accurately describes the given BufferedImage.

src/java.desktop/share/classes/javax/imageio/ImageTypeSpecifier.java line 1:

> 1: /*

Could you bump the copyright year?

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.

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?

test/jdk/javax/imageio/plugins/jpeg/JpegWriterWriteNonOpaqueIndexColorModelTest.java line 63:

> 61:                 }
> 62:                 if (imageType == Transparency.BITMASK)
> 63:                     alpha[0] = 0;

Suggestion:

                if (imageType == Transparency.BITMASK) {
                    alpha[0] = 0;
                }

It's recommended to always use braces even when there's only one statement in the block.

test/jdk/javax/imageio/plugins/jpeg/JpegWriterWriteNonOpaqueIndexColorModelTest.java line 74:

> 72:             boolean result = ImageIO.write(bi, "jpg", new ByteArrayOutputStream());
> 73:             if (result != expectedWriteReturnValue)
> 74:                 throw new Error("ImageIO.write(..) returned " + result + " but we expected " + expectedWriteReturnValue);

Suggestion:

            if (result != expectedWriteReturnValue) {
                throw new Error("ImageIO.write(..) returned " + result + " but we expected " + expectedWriteReturnValue);
            }

test/jdk/javax/imageio/plugins/jpeg/JpegWriterWriteNonOpaqueIndexColorModelTest.java line 77:

> 75:             System.out.println("Tested passed");
> 76:             return true;
> 77:         } catch(Exception e) {

Suggestion:

        } catch (Exception e) {

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.

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.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987265582
PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987265134
PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987268179
PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987271648
PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987275369
PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987276141
PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987278113
PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987278977