Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS chunk while reading non-indexed RGB images
Hi Prahalad,
Thanks for your inputs.
Regarding :
File: PNGImageReader.java
Line: 1329, 1342
. The else block for if (check for transparent pixel) is redundant across
both PNG_COLOR_RGB and PNG_COLOR_GRAY.
. We could use Arrays.fill with opaque value and set the alpha to 0 if
pixel is transparent.
int[] temp = new int[inputBands + 1];
int opaque = (bitDepth < 16) ? 255 : 65535;
Arrays.fill(temp, opaque);
. All subsequent operations checking for transparent color value and
setting alpha to 0 would be required.
I think we should not use Arrays.fill() at this place and assign values to all
the channels. It is a per pixel operation and we would be writing data twice.
Instead of using Arrays.fill(), I thought of just assigning value to alpha
channel:
int[] temp = new int[inputBands + 1];
temp[inputBands] = (bitDepth < 16) ? 255 : 65535;
if (metadata.tRNS_colorType == PNG_COLOR_RGB) {
I think even doing this is not ideal because anyway for all the pixels we will
be checking pixel data with tRNS data, and assign value in alpha channel. There
is no need for us to assign some value and override it again later if a
condition satisfies.
So I am assigning opaque value to alpha channel at same place. But I have
reduced the LOC by using ternary operator for determining the opaque value for
alpha channel.
All other changes are updated.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/6788458/webrev.06/
Thanks,
Jay
-Original Message-
From: Prahalad Kumar Narayanan
Sent: Friday, April 06, 2018 1:42 PM
To: Jayathirth D V; 2d-dev
Subject: RE: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
Hello Jay
Good day to you.
I looked into the latest changes.
The code changes are nearly complete. Just a few tweaks.
File: PNGImageReader.java
Line: 1280
Rephrase from:
/*
* In case of colortype PNG_COLOR_RGB or PNG_COLOR_GRAY
* if we have transparent pixel information from tRNS chunk
* we need to consider that also and store proper information
* in alpha channel.
*
* Also we create destination image with extra alpha channel
* in getImageTypes() when we have tRNS chunk for colorType
* PNG_COLOR_RGB or PNG_COLOR_GRAY.
*/
Rephrase to:
/*
* For PNG images of color type PNG_COLOR_RGB or PNG_COLOR_GRAY
* that contain a specific transparent color (given by tRNS
* chunk), we compare the decoded pixel color with the color
* given by tRNS chunk to set the alpha on the destination.
*/
File: PNGImageReader.java
Line: 1588
Rephrase from:
/*
* In case of PNG_COLOR_RGB or PNG_COLOR_GRAY, if we
* have transparent pixel information in tRNS chunk
* we create destination image having alpha channel.
*/
Rephrase to:
/*
* For PNG images of color type PNG_COLOR_RGB or PNG_COLOR_GRAY that
* contain a specific transparent color (given by tRNS chunk), we add
* ImageTypeSpecifier(s) that support transparency to the list of
* supported image types.
*/
File: PNGImageReader.java
Line(s): 1290, 1493, 1596, 1619
. The lines mentioned above check whether metadata has specific transparent
color using-
metadata.tRNS_present &&
(metadata.tRNS_colorType == PNG_COLOR_RGB ||
metadata.tRNS_colorType == PNG_COLOR_GRAY)
. The above check is not only redundant but also metadata specific.
. We could move the code to a common method in PNGMetadata-
boolean hasTransparentColor() {
return tRNS_present
&& (tRNS_colorType == PNG_COLOR_RGB
|| tRNS_colorType == PNG_COLOR_GRAY);
}
. I understand 1596 and 1619 check for specific color type but they can be
avoided with this method as well.
. As per the specification, tRNS values depend on the image's color type.
. This will reduce the complex check from-
if (theImage.getSampleModel().getNumBands() ==
inputBandsForColorType[colorType] + 1 &&
metadata.tRNS_present &&
(metadata.tRNS_colorType == PNG_COLOR_RGB ||
metadata.tRNS_colorType == PNG_COLOR_GRAY))
{
to-
if (metadata.hasTransparentColor()
&& (theImage.getSampleModel().getNumBands() ==
inputBandsForColorType[colorType] + 1)) {
File: PNGImageReader.java
Line: 1329, 1342
. The else block for if (check for transparent pixel) is redundant across
both PNG_COLOR_RGB and PNG_COLOR_GRAY.
. We could use Arrays.fill with opaque value and set the alpha to 0 if
pixel is transparent.
Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS chunk while reading non-indexed RGB images
Hello Jay
Good day to you.
I looked into the latest changes.
The code changes are nearly complete. Just a few tweaks.
File: PNGImageReader.java
Line: 1280
Rephrase from:
/*
* In case of colortype PNG_COLOR_RGB or PNG_COLOR_GRAY
* if we have transparent pixel information from tRNS chunk
* we need to consider that also and store proper information
* in alpha channel.
*
* Also we create destination image with extra alpha channel
* in getImageTypes() when we have tRNS chunk for colorType
* PNG_COLOR_RGB or PNG_COLOR_GRAY.
*/
Rephrase to:
/*
* For PNG images of color type PNG_COLOR_RGB or PNG_COLOR_GRAY
* that contain a specific transparent color (given by tRNS
* chunk), we compare the decoded pixel color with the color
* given by tRNS chunk to set the alpha on the destination.
*/
File: PNGImageReader.java
Line: 1588
Rephrase from:
/*
* In case of PNG_COLOR_RGB or PNG_COLOR_GRAY, if we
* have transparent pixel information in tRNS chunk
* we create destination image having alpha channel.
*/
Rephrase to:
/*
* For PNG images of color type PNG_COLOR_RGB or PNG_COLOR_GRAY that
* contain a specific transparent color (given by tRNS chunk), we add
* ImageTypeSpecifier(s) that support transparency to the list of
* supported image types.
*/
File: PNGImageReader.java
Line(s): 1290, 1493, 1596, 1619
. The lines mentioned above check whether metadata has specific transparent
color using-
metadata.tRNS_present &&
(metadata.tRNS_colorType == PNG_COLOR_RGB ||
metadata.tRNS_colorType == PNG_COLOR_GRAY)
. The above check is not only redundant but also metadata specific.
. We could move the code to a common method in PNGMetadata-
boolean hasTransparentColor() {
return tRNS_present
&& (tRNS_colorType == PNG_COLOR_RGB
|| tRNS_colorType == PNG_COLOR_GRAY);
}
. I understand 1596 and 1619 check for specific color type but they can be
avoided with this method as well.
. As per the specification, tRNS values depend on the image's color type.
. This will reduce the complex check from-
if (theImage.getSampleModel().getNumBands() ==
inputBandsForColorType[colorType] + 1 &&
metadata.tRNS_present &&
(metadata.tRNS_colorType == PNG_COLOR_RGB ||
metadata.tRNS_colorType == PNG_COLOR_GRAY))
{
to-
if (metadata.hasTransparentColor()
&& (theImage.getSampleModel().getNumBands() ==
inputBandsForColorType[colorType] + 1)) {
File: PNGImageReader.java
Line: 1329, 1342
. The else block for if (check for transparent pixel) is redundant across
both PNG_COLOR_RGB and PNG_COLOR_GRAY.
. We could use Arrays.fill with opaque value and set the alpha to 0 if
pixel is transparent.
int[] temp = new int[inputBands + 1];
int opaque = (bitDepth < 16) ? 255 : 65535;
Arrays.fill(temp, opaque);
. All subsequent operations checking for transparent color value and
setting alpha to 0 would be required.
File: PNGImageReader.java
All modified Lines:
. The opening braces '{' can appear in the new line. Some engineers do
follow this.
. Since the rest of the code aligns opening braces in the same line as the
' if ' statement we could follow the same for code readability.
Test File: ReadPngGrayImageWithTRNSChunk.java and
ReadPngRGBImageWithTRNSChunk.java
. The finally blocks should check whether the objects- ImageOutputStream
and File, are not null.
. The call to directory(while is a File).delete() is not required in my
view. Verify this once.
. Dispose the writer after the write operation is complete.
Thank you
Have a good day
Prahalad N.
-Original Message-
From: Jayathirth D V
Sent: Thursday, April 05, 2018 3:26 PM
To: Prahalad Kumar Narayanan; 2d-dev
Subject: RE: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
Hello Prahalad,
Thank you for the inputs.
I gave updated the code to not change ImageTypeSpecifier of passRow. Now while
storing the pixel values into imRas we comparing the pixel RGB/Gray values to
tRNS RGB/Gray values and storing appropriate value for alpha channel.
I have added support to use tRNS_Gray value when IHDR color type is
PNG_COLOR_GRAY - We are now creating 2 channel destination image whenever we
see that tRNS chunk is present for PNG_COLOR_GRAY in getImageTypes().
isTransparentPixelPresent() code is removed and we are using available tRNS
properties.
