Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS chunk while reading non-indexed RGB images
+1
-phil.
On 04/13/2018 01:31 AM, Jayathirth D V wrote:
Hi Phil,
Thanks for your inputs.
I have updated the code to reflect the changes mentioned by you and changed
test case to use ByteArrayInput/OutputStream.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/6788458/webrev.08/
Regards,
Jay
-Original Message-
From: Phil Race
Sent: Wednesday, April 11, 2018 10:23 PM
To: Jayathirth D V; Prahalad Kumar Narayanan; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
http://cr.openjdk.java.net/~jdv/6788458/webrev.07/src/java.desktop/share/classes/com/sun/imageio/plugins/png/PNGMetadata.java.udiff.html
There's an extra line added at the end which you can remove.
Why do the tests needs to create temp disk files ?
Can't we do this all with ByteArrayInput/OutputStream ?
Less worry about clean up there.
I note that this
286 boolean tRNSTransparentPixelPresent =
1287 theImage.getSampleModel().getNumBands() == inputBands + 1
&&
1288 metadata.hasTransparentColor();
is evaluated for each row when I expect it only needs to be evaluated once for
the whole decode pass.
But the overhed is presumably low and it is next to the use so I think it
should be OK as is.
However the code that does the transparent per-pixel settings does seem
needlessly wasteful. These can be moved outside at least the entire row decode :
final int[] temp = new int[inputBands + 1];
final int opaque = (bitDepth < 16) ? 255 : 65535;
Note that I added final to them ..
Also array accesses are (relatively) slow but since you need an array to pass
to setPixel and there's only two accesses I don't see how we can improve on
that here by assigning the values from the ps array to local variables.
-phil.
On 04/09/2018 11:19 PM, Jayathirth D V wrote:
HI Prahalad,
Thanks for your inputs.
Regarding-
Few minor corrections to PNGImageReader- . Line: 1310
. This missed my eye in the last review.
. We could create int[] temp outside the for loop. I tried this with your
changes & it works.
. It's an optimization so that we don't end up creating int[] array for
every pixel in the row.
Before sending webrev.06, I actually made similar changes of moving temp[] creation
and/or calculating "opaque" to outside of for loop.
But this will cause creation of temp[] and/ or calculation of "opaque" for each row
in all the cases where there is no RGB/Gray & tRNS combination. So I reverted those
change before sending webrev.06.
I would like to keep all the calculation specific to RGB/Gray & tRNS
combination in its own code flow.
Other modifications are made.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/6788458/webrev.07/
Thanks,
Jay
-Original Message-
From: Prahalad Kumar Narayanan
Sent: Monday, April 09, 2018 2:23 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
I looked into the latest code changes.
The code changes are good.
Few minor corrections to PNGImageReader- . Line: 1310
. This missed my eye in the last review.
. We could create int[] temp outside the for loop. I tried this with your
changes & it works.
. It's an optimization so that we don't end up creating int[] array for
every pixel in the row.
. Line: 1320, 1329
. Align the opening { on the same line as the if statement.
. Line: 1479
. Correct the indentation of the expression within if condition.
. A suggestion that could help:
if ((theImage.getSampleModel().getNumBands()
== inputBandsForColorType[colorType] + 1)
&& metadata.hasTransparentColor()) {
// code block
checkReadParamBandSettings(param,
...
}
Thank you
Have a good day
Prahalad N.
-Original Message-
From: Jayathirth D V
Sent: Friday, April 06, 2018 5:19 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
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 value
Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS chunk while reading non-indexed RGB images
Hi Phil,
Thanks for your inputs.
I have updated the code to reflect the changes mentioned by you and changed
test case to use ByteArrayInput/OutputStream.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/6788458/webrev.08/
Regards,
Jay
-Original Message-
From: Phil Race
Sent: Wednesday, April 11, 2018 10:23 PM
To: Jayathirth D V; Prahalad Kumar Narayanan; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
http://cr.openjdk.java.net/~jdv/6788458/webrev.07/src/java.desktop/share/classes/com/sun/imageio/plugins/png/PNGMetadata.java.udiff.html
There's an extra line added at the end which you can remove.
Why do the tests needs to create temp disk files ?
Can't we do this all with ByteArrayInput/OutputStream ?
Less worry about clean up there.
I note that this
286 boolean tRNSTransparentPixelPresent =
1287 theImage.getSampleModel().getNumBands() == inputBands
+ 1 &&
1288 metadata.hasTransparentColor();
is evaluated for each row when I expect it only needs to be evaluated once for
the whole decode pass.
But the overhed is presumably low and it is next to the use so I think it
should be OK as is.
However the code that does the transparent per-pixel settings does seem
needlessly wasteful. These can be moved outside at least the entire row decode :
final int[] temp = new int[inputBands + 1];
final int opaque = (bitDepth < 16) ? 255 : 65535;
Note that I added final to them ..
Also array accesses are (relatively) slow but since you need an array to pass
to setPixel and there's only two accesses I don't see how we can improve on
that here by assigning the values from the ps array to local variables.
-phil.
On 04/09/2018 11:19 PM, Jayathirth D V wrote:
> HI Prahalad,
>
> Thanks for your inputs.
>
> Regarding-
> Few minor corrections to PNGImageReader- . Line: 1310
> . This missed my eye in the last review.
> . We could create int[] temp outside the for loop. I tried this with
> your changes & it works.
> . It's an optimization so that we don't end up creating int[] array for
> every pixel in the row.
>
> Before sending webrev.06, I actually made similar changes of moving temp[]
> creation and/or calculating "opaque" to outside of for loop.
> But this will cause creation of temp[] and/ or calculation of "opaque" for
> each row in all the cases where there is no RGB/Gray & tRNS combination. So I
> reverted those change before sending webrev.06.
> I would like to keep all the calculation specific to RGB/Gray & tRNS
> combination in its own code flow.
>
> Other modifications are made.
> Please find updated webrev for review:
> http://cr.openjdk.java.net/~jdv/6788458/webrev.07/
>
> Thanks,
> Jay
>
> -Original Message-----
> From: Prahalad Kumar Narayanan
> Sent: Monday, April 09, 2018 2:23 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
>
> I looked into the latest code changes.
> The code changes are good.
>
> Few minor corrections to PNGImageReader- . Line: 1310
> . This missed my eye in the last review.
> . We could create int[] temp outside the for loop. I tried this with
> your changes & it works.
> . It's an optimization so that we don't end up creating int[] array for
> every pixel in the row.
>
> . Line: 1320, 1329
> . Align the opening { on the same line as the if statement.
>
> . Line: 1479
> . Correct the indentation of the expression within if condition.
> . A suggestion that could help:
> if ((theImage.getSampleModel().getNumBands()
> == inputBandsForColorType[colorType] + 1)
> && metadata.hasTransparentColor()) {
> // code block
> checkReadParamBandSettings(param,
> ...
> }
>
> Thank you
> Have a good day
>
> Prahalad N.
>
> -----Original Message-
> From: Jayathirth D V
> Sent: Friday, April 06, 2018 5:19 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
>
> 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.
>
Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS chunk while reading non-indexed RGB images
http://cr.openjdk.java.net/~jdv/6788458/webrev.07/src/java.desktop/share/classes/com/sun/imageio/plugins/png/PNGMetadata.java.udiff.html
There's an extra line added at the end which you can remove.
Why do the tests needs to create temp disk files ?
Can't we do this all with ByteArrayInput/OutputStream ?
Less worry about clean up there.
I note that this
286 boolean tRNSTransparentPixelPresent =
1287 theImage.getSampleModel().getNumBands() == inputBands + 1
&&
1288 metadata.hasTransparentColor();
is evaluated for each row when I expect it only needs to be
evaluated once for the whole decode pass.
But the overhed is presumably low and it is next to the use so
I think it should be OK as is.
However the code that does the transparent per-pixel settings does seem
needlessly wasteful. These can be moved outside at least the entire
row decode :
final int[] temp = new int[inputBands + 1];
final int opaque = (bitDepth < 16) ? 255 : 65535;
Note that I added final to them ..
Also array accesses are (relatively) slow but since you need
an array to pass to setPixel and there's only two accesses I
don't see how we can improve on that here by assigning
the values from the ps array to local variables.
-phil.
On 04/09/2018 11:19 PM, Jayathirth D V wrote:
HI Prahalad,
Thanks for your inputs.
Regarding-
Few minor corrections to PNGImageReader- . Line: 1310
. This missed my eye in the last review.
. We could create int[] temp outside the for loop. I tried this with your
changes & it works.
. It's an optimization so that we don't end up creating int[] array for
every pixel in the row.
Before sending webrev.06, I actually made similar changes of moving temp[] creation
and/or calculating "opaque" to outside of for loop.
But this will cause creation of temp[] and/ or calculation of "opaque" for each row
in all the cases where there is no RGB/Gray & tRNS combination. So I reverted those
change before sending webrev.06.
I would like to keep all the calculation specific to RGB/Gray & tRNS
combination in its own code flow.
Other modifications are made.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/6788458/webrev.07/
Thanks,
Jay
-Original Message-
From: Prahalad Kumar Narayanan
Sent: Monday, April 09, 2018 2:23 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
I looked into the latest code changes.
The code changes are good.
Few minor corrections to PNGImageReader- . Line: 1310
. This missed my eye in the last review.
. We could create int[] temp outside the for loop. I tried this with your
changes & it works.
. It's an optimization so that we don't end up creating int[] array for
every pixel in the row.
. Line: 1320, 1329
. Align the opening { on the same line as the if statement.
. Line: 1479
. Correct the indentation of the expression within if condition.
. A suggestion that could help:
if ((theImage.getSampleModel().getNumBands()
== inputBandsForColorType[colorType] + 1)
&& metadata.hasTransparentColor()) {
// code block
checkReadParamBandSettings(param,
...
}
Thank you
Have a good day
Prahalad N.
-Original Message-
From: Jayathirth D V
Sent: Friday, April 06, 2018 5:19 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
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 valu
Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS chunk while reading non-indexed RGB images
Hello Jay
Changes look good.
Thanks
Have a good day
Prahalad N.
-Original Message-
From: Jayathirth D V
Sent: Tuesday, April 10, 2018 11:49 AM
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
HI Prahalad,
Thanks for your inputs.
Regarding-
Few minor corrections to PNGImageReader- . Line: 1310
. This missed my eye in the last review.
. We could create int[] temp outside the for loop. I tried this with your
changes & it works.
. It's an optimization so that we don't end up creating int[] array for
every pixel in the row.
Before sending webrev.06, I actually made similar changes of moving temp[]
creation and/or calculating "opaque" to outside of for loop.
But this will cause creation of temp[] and/ or calculation of "opaque" for each
row in all the cases where there is no RGB/Gray & tRNS combination. So I
reverted those change before sending webrev.06.
I would like to keep all the calculation specific to RGB/Gray & tRNS
combination in its own code flow.
Other modifications are made.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/6788458/webrev.07/
Thanks,
Jay
-Original Message-
From: Prahalad Kumar Narayanan
Sent: Monday, April 09, 2018 2:23 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
I looked into the latest code changes.
The code changes are good.
Few minor corrections to PNGImageReader- . Line: 1310
. This missed my eye in the last review.
. We could create int[] temp outside the for loop. I tried this with your
changes & it works.
. It's an optimization so that we don't end up creating int[] array for
every pixel in the row.
. Line: 1320, 1329
. Align the opening { on the same line as the if statement.
. Line: 1479
. Correct the indentation of the expression within if condition.
. A suggestion that could help:
if ((theImage.getSampleModel().getNumBands()
== inputBandsForColorType[colorType] + 1)
&& metadata.hasTransparentColor()) {
// code block
checkReadParamBandSettings(param,
...
}
Thank you
Have a good day
Prahalad N.
-Original Message-
From: Jayathirth D V
Sent: Friday, April 06, 2018 5:19 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
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
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-
Few minor corrections to PNGImageReader- . Line: 1310
. This missed my eye in the last review.
. We could create int[] temp outside the for loop. I tried this with your
changes & it works.
. It's an optimization so that we don't end up creating int[] array for
every pixel in the row.
Before sending webrev.06, I actually made similar changes of moving temp[]
creation and/or calculating "opaque" to outside of for loop.
But this will cause creation of temp[] and/ or calculation of "opaque" for each
row in all the cases where there is no RGB/Gray & tRNS combination. So I
reverted those change before sending webrev.06.
I would like to keep all the calculation specific to RGB/Gray & tRNS
combination in its own code flow.
Other modifications are made.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/6788458/webrev.07/
Thanks,
Jay
-Original Message-
From: Prahalad Kumar Narayanan
Sent: Monday, April 09, 2018 2:23 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
I looked into the latest code changes.
The code changes are good.
Few minor corrections to PNGImageReader- . Line: 1310
. This missed my eye in the last review.
. We could create int[] temp outside the for loop. I tried this with your
changes & it works.
. It's an optimization so that we don't end up creating int[] array for
every pixel in the row.
. Line: 1320, 1329
. Align the opening { on the same line as the if statement.
. Line: 1479
. Correct the indentation of the expression within if condition.
. A suggestion that could help:
if ((theImage.getSampleModel().getNumBands()
== inputBandsForColorType[colorType] + 1)
&& metadata.hasTransparentColor()) {
// code block
checkReadParamBandSettings(param,
...
}
Thank you
Have a good day
Prahalad N.
-Original Message-
From: Jayathirth D V
Sent: Friday, April 06, 2018 5:19 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
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 c
Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS chunk while reading non-indexed RGB images
Hello Jay
I looked into the latest code changes.
The code changes are good.
Few minor corrections to PNGImageReader-
. Line: 1310
. This missed my eye in the last review.
. We could create int[] temp outside the for loop. I tried this with your
changes & it works.
. It's an optimization so that we don't end up creating int[] array for
every pixel in the row.
. Line: 1320, 1329
. Align the opening { on the same line as the if statement.
. Line: 1479
. Correct the indentation of the expression within if condition.
. A suggestion that could help:
if ((theImage.getSampleModel().getNumBands()
== inputBandsForColorType[colorType] + 1)
&& metadata.hasTransparentColor()) {
// code block
checkReadParamBandSettings(param,
...
}
Thank you
Have a good day
Prahalad N.
-Original Message-
From: Jayathirth D V
Sent: Friday, April 06, 2018 5:19 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
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 PNG
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
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
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. I have merged previous test cases. Now we have 2 test cases each verifying the code changes for RGB & Gray image for 8 & 16 bit depth values. Please find updated webrev for review: http://cr.openjdk.java.net/~jdv/6788458/webrev.05/ Thanks, Jay -Original Message- From: Prahalad Kumar Narayanan Sent: Monday, April 02, 2018 12:03 PM To: Krishna Addepalli; 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 code changes. Please find my review observation & suggestions herewith. My understanding of problem statement: . As I understand, our PNGImageReader parses tRNS chunk information from metadata whenever ignoreMetadata is false or paletted image is decoded. . But doesn't use the transparency information contained in the chunk to return BufferedImages with alpha/ transparency. Appropriate Changes: File: PNGImageReader.java, Method: Iterator getImageTypes Line: 1633 . This method is internally invoked while creating BufferedImage for destination at Line 1409: theImage = getDestination(param, getImageTypes(0), width, height); . The move to add ImageTypeSpecifier based on BufferedImage.TYPE_4BYTE_ABGR as the first element (index: 0) of the list is good. File: PNGImageReader.java Method: readMetadata Line: 731 . The if check for parsing tRNS chunk even when ignoreMetadata is set to true is good. . The chunk's information will be needed. Other Observation & Suggestions: File: PNGImageReader.java, Method: decodePass Line: (1088 - 1112), (1277-1327) . In the code chunks of listed line numbers, you have modified passRow's internal type- ImageTypeSpecifier, and thus its manipulation logic as well. . The changes are not required in my view. Reasons are- . passRow corresponds to the data from input image source. . imgRas corresponds to the destination buffered image. . To return a BufferedImage with alpha/ transparency, it would suffice to update imgRas with alpha component at Line 1371. imRas.setPixel(dstX, dstY, ps); // if ps contains alpha, it would suffice the need. . However, the proposed changes modify how we interpret the source through passRow. . To set alpha component on imgRas, we would require comparison of color components present in passRow with that of tRNS chunk. . But, passRow 's internal type- ImageTypeSpecifier need not be changed. This way most of the complex code changes can be avoided. File: PNGImageReader.java, Method: isTransparentPixelPresent Line: 1545 . The logic of this method considers both input image source and destination bands to decide whether transparency is available. . In my view, The method should consider only the alpha in input image source and not the destination. . Here are code points where the method is invoked a) 1089 : To create a writable raster- passRow. passRow corresponds to the data of image source. b) 1279 : To update the passRow's databuffer. passRow corresponds to the data of image source. c) 1512 : To pass appropriate band length to checkParamBandSettings. (Hardcoded value: 4) d) 1632 & 1648 : To update ArrayList l based on presence of tRNS in image source. . Each of the locations except (c) pertain to image source and not the destination. . One possible solution would be to discard this method and use the existing flag tRNS_present at (c). . Besides, The proposed changes don't address images with gray scale with alpha in tRNS chunk. . Your first email reads- "PNG_COLOR_GRAY image with tRNS chunk(which is very rare)" . Just curious to know if there 's any basis for this observation ? . If we consider suggestions on decodePass method, I believe, we could support setting alpha values for grayscale images as well. Line: 1555 . Please use tRNS_colorType together with tRNS_present flag. File: PNGImageReader.java, Method: readMetadata Line: 701 Rephrase From: * Optimization: We can skip reading metadata if ignoreMetadata * flag is set and colorType is
Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS chunk while reading non-indexed RGB images
Hi Krishna,
I verified usage of tRNS_colorType values in PNGImageRader, PNGImageWriter &
PNGMetadata. We are using tRNS_coloType only when they are of type RGB, PALETTE
or GRAY only in conjunction with tRNS_present variable. There are no places
where undefined tRNS_colorType is used.
Thanks for your suggestion to use constant variable for undefined colorType. I
have made appropriate changes for the same.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/6788458/webrev.04/
I also see that Prahalad has given suggestions over webrev.03, I will go
through them and update accordingly later.
Thanks,
Jay
From: Krishna Addepalli
Sent: Monday, April 02, 2018 11:40 AM
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
Hmmm, thanks for the clarification, but this raises one more question: Now that
you are initializing the colorType to an invalid value, do you need to check
appropriately in all the places where the color is being used? Otherwise, it
might lead to undefined behavior.
Also, I would like you to either add a comment at the point of initialization
or better yet, define another static constant of "UNDEFINED", and then set the
tRNS_colorType to this value, so that the code reads correct naturally without
any comments.
Thanks,
Krishna
From: Jayathirth D V
Sent: Monday, April 2, 2018 11:33 AM
To: Krishna Addepalli mailto:krishna.addepa...@oracle.com"krishna.addepa...@oracle.com>; 2d-dev
mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net>
Subject: RE: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
Hi Krishna,
The constant values for different color types is :
static final int PNG_COLOR_GRAY = 0;
static final int PNG_COLOR_RGB = 2;
static final int PNG_COLOR_PALETTE = 3;
static final int PNG_COLOR_GRAY_ALPHA = 4;
static final int PNG_COLOR_RGB_ALPHA = 6;
Since tRNS_colorType is used to hold one of the above mentioned values if we
don't initialize it to some unused value(here we are using -1) we will be
representing PNG_COLOR_GRAY by default.
By default the value will be -1 after the change and after we parse tRNS chunk
it will contain appropriate value. The initialization of tRNS_colorType change
can be considered as newly added check.
Thanks,
Jay
From: Krishna Addepalli
Sent: Monday, April 02, 2018 9:56 AM
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
Hi Jay,
The changes look fine. However, I have one more question: Why did you have to
explicitly specify the initial value to "tRNS_colorType" in PNGMetadata.java?
How is it affecting your implementation?
Thanks,
Krishna
From: Jayathirth D V
Sent: Wednesday, March 28, 2018 1:43 PM
To: Krishna Addepalli mailto:krishna.addepa...@oracle.com"krishna.addepa...@oracle.com>; 2d-dev
mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net>
Subject: RE: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
Hi Krishna,
Thanks for providing the inputs.
1. I suggest to rename considerTransparentPixel as isAlphaPresent.
As discussed I have added new name as isTransparentPixelPresent()
2. You can refactor the function body as below:
Return ((destinationBands == null ||
destinationBands.length == 4) &&
metadata.tRNS_colorType == PNG_COLOR_RGB);
Changes are made.
3. From line 1088 through 1113, I see some repeated calculations like
bytesPerRow etc. Why can't we reuse the previously defined variables? Apart
from destBands, all the other calculations look similar and repeated.
Previous to this change all the values like inputsBands, bytesPerRow,
eltsPerRow were same between the decoded output and destination image.
Now because we are changing only the destination image due to the presence of
transparent pixel, we need both these set of values. inputsBands, bytesPerRow,
eltsPerRow will be used for decoded output and destBands, destEltsPerRow for
destination image. Its better if don't mingle code flow or variables between
these two code paths.
4. When you are copying the pixels to a writable raster, at line 1273,
you could reduce the repetition of the lines, by just having one statement
inside if as below:
for (int i = 0; i < passWidth; i++) {
byteData[destidx] = curr[srcidx];
byteData[destidx + 1] = curr[srcidx + 1];
byteData[destidx + 2] = curr[srcidx + 2];
if (curr[srcidx] == (byte)metadata.tRNS_red &&
Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS chunk while reading non-indexed RGB images
a good day
Prahalad N.
- Original Message -
From: Krishna Addepalli
Sent: Monday, April 02, 2018 11:40 AM
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
Hmmm, thanks for the clarification, but this raises one more question: Now that
you are initializing the colorType to an invalid value, do you need to check
appropriately in all the places where the color is being used? Otherwise, it
might lead to undefined behavior.
Also, I would like you to either add a comment at the point of initialization
or better yet, define another static constant of "UNDEFINED", and then set the
tRNS_colorType to this value, so that the code reads correct naturally without
any comments.
Thanks,
Krishna
From: Jayathirth D V
Sent: Monday, April 2, 2018 11:33 AM
To: Krishna Addepalli <krishna.addepa...@oracle.com>; 2d-dev
<2d-dev@openjdk.java.net>
Subject: RE: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
Hi Krishna,
The constant values for different color types is :
static final int PNG_COLOR_GRAY = 0;
static final int PNG_COLOR_RGB = 2;
static final int PNG_COLOR_PALETTE = 3;
static final int PNG_COLOR_GRAY_ALPHA = 4;
static final int PNG_COLOR_RGB_ALPHA = 6;
Since tRNS_colorType is used to hold one of the above mentioned values if we
don't initialize it to some unused value(here we are using -1) we will be
representing PNG_COLOR_GRAY by default.
By default the value will be -1 after the change and after we parse tRNS chunk
it will contain appropriate value. The initialization of tRNS_colorType change
can be considered as newly added check.
Thanks,
Jay
From: Krishna Addepalli
Sent: Monday, April 02, 2018 9:56 AM
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
Hi Jay,
The changes look fine. However, I have one more question: Why did you have to
explicitly specify the initial value to "tRNS_colorType" in PNGMetadata.java?
How is it affecting your implementation?
Thanks,
Krishna
From: Jayathirth D V
Sent: Wednesday, March 28, 2018 1:43 PM
To: Krishna Addepalli <krishna.addepa...@oracle.com>; 2d-dev
<2d-dev@openjdk.java.net>
Subject: RE: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
Hi Krishna,
Thanks for providing the inputs.
1. I suggest to rename considerTransparentPixel as isAlphaPresent.
As discussed I have added new name as isTransparentPixelPresent()
2. You can refactor the function body as below:
Return ((destinationBands == null ||
destinationBands.length == 4) &&
metadata.tRNS_colorType == PNG_COLOR_RGB);
Changes are made.
3. From line 1088 through 1113, I see some repeated calculations like
bytesPerRow etc. Why can't we reuse the previously defined variables? Apart
from destBands, all the other calculations look similar and repeated.
Previous to this change all the values like inputsBands, bytesPerRow,
eltsPerRow were same between the decoded output and destination image.
Now because we are changing only the destination image due to the presence of
transparent pixel, we need both these set of values. inputsBands, bytesPerRow,
eltsPerRow will be used for decoded output and destBands, destEltsPerRow for
destination image. Its better if don't mingle code flow or variables between
these two code paths.
4. When you are copying the pixels to a writable raster, at line 1273, you
could reduce the repetition of the lines, by just having one statement inside
if as below:
for (int i = 0; i < passWidth; i++) {
byteData[destidx] = curr[srcidx];
byteData[destidx + 1] = curr[srcidx + 1];
byteData[destidx + 2] = curr[srcidx + 2];
if (curr[srcidx] == (byte)metadata.tRNS_red &&
curr[srcidx + 1] == (byte)metadata.tRNS_green &&
curr[srcidx + 2] == (byte)metadata.tRNS_blue)
{
byteData[destidx + 3] = (byte)0;
} else {
byteData[destidx + 3] = (byte)255;
}
srcidx += 3;
destidx += 4;
}
Same thing can be done for the loop that executes for 16bit pixel data.
Changes are made.
Please find updated webrev for review :
http://cr.openjdk.java.net/~jdv/6788458/webrev.03/
Thanks,
Jay
From: Krishna Addepalli
Sent: Wednesday, March 28, 2018 11:52 AM
To: Jayathirth D V; 2d-dev
Subject: RE: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while
Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS chunk while reading non-indexed RGB images
Hmmm, thanks for the clarification, but this raises one more question: Now that
you are initializing the colorType to an invalid value, do you need to check
appropriately in all the places where the color is being used? Otherwise, it
might lead to undefined behavior.
Also, I would like you to either add a comment at the point of initialization
or better yet, define another static constant of "UNDEFINED", and then set the
tRNS_colorType to this value, so that the code reads correct naturally without
any comments.
Thanks,
Krishna
From: Jayathirth D V
Sent: Monday, April 2, 2018 11:33 AM
To: Krishna Addepalli <krishna.addepa...@oracle.com>; 2d-dev
<2d-dev@openjdk.java.net>
Subject: RE: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
Hi Krishna,
The constant values for different color types is :
static final int PNG_COLOR_GRAY = 0;
static final int PNG_COLOR_RGB = 2;
static final int PNG_COLOR_PALETTE = 3;
static final int PNG_COLOR_GRAY_ALPHA = 4;
static final int PNG_COLOR_RGB_ALPHA = 6;
Since tRNS_colorType is used to hold one of the above mentioned values if we
don't initialize it to some unused value(here we are using -1) we will be
representing PNG_COLOR_GRAY by default.
By default the value will be -1 after the change and after we parse tRNS chunk
it will contain appropriate value. The initialization of tRNS_colorType change
can be considered as newly added check.
Thanks,
Jay
From: Krishna Addepalli
Sent: Monday, April 02, 2018 9:56 AM
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
Hi Jay,
The changes look fine. However, I have one more question: Why did you have to
explicitly specify the initial value to "tRNS_colorType" in PNGMetadata.java?
How is it affecting your implementation?
Thanks,
Krishna
From: Jayathirth D V
Sent: Wednesday, March 28, 2018 1:43 PM
To: Krishna Addepalli mailto:krishna.addepa...@oracle.com"krishna.addepa...@oracle.com>; 2d-dev
mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net>
Subject: RE: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
Hi Krishna,
Thanks for providing the inputs.
1. I suggest to rename considerTransparentPixel as isAlphaPresent.
As discussed I have added new name as isTransparentPixelPresent()
2. You can refactor the function body as below:
Return ((destinationBands == null ||
destinationBands.length == 4) &&
metadata.tRNS_colorType == PNG_COLOR_RGB);
Changes are made.
3. From line 1088 through 1113, I see some repeated calculations like
bytesPerRow etc. Why can't we reuse the previously defined variables? Apart
from destBands, all the other calculations look similar and repeated.
Previous to this change all the values like inputsBands, bytesPerRow,
eltsPerRow were same between the decoded output and destination image.
Now because we are changing only the destination image due to the presence of
transparent pixel, we need both these set of values. inputsBands, bytesPerRow,
eltsPerRow will be used for decoded output and destBands, destEltsPerRow for
destination image. Its better if don't mingle code flow or variables between
these two code paths.
4. When you are copying the pixels to a writable raster, at line 1273,
you could reduce the repetition of the lines, by just having one statement
inside if as below:
for (int i = 0; i < passWidth; i++) {
byteData[destidx] = curr[srcidx];
byteData[destidx + 1] = curr[srcidx + 1];
byteData[destidx + 2] = curr[srcidx + 2];
if (curr[srcidx] == (byte)metadata.tRNS_red &&
curr[srcidx + 1] == (byte)metadata.tRNS_green &&
curr[srcidx + 2] == (byte)metadata.tRNS_blue)
{
byteData[destidx + 3] = (byte)0;
} else {
byteData[destidx + 3] = (byte)255;
}
srcidx += 3;
destidx += 4;
}
Same thing can be done for the loop that executes for 16bit pixel data.
Changes are made.
Please find updated webrev for review :
http://cr.openjdk.java.net/~jdv/6788458/webrev.03/
Thanks,
Jay
From: Krishna Addepalli
Sent: Wednesday, March 28, 2018 11:52 AM
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
Hi Jay,
I have s
Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS chunk while reading non-indexed RGB images
Hi Krishna,
The constant values for different color types is :
static final int PNG_COLOR_GRAY = 0;
static final int PNG_COLOR_RGB = 2;
static final int PNG_COLOR_PALETTE = 3;
static final int PNG_COLOR_GRAY_ALPHA = 4;
static final int PNG_COLOR_RGB_ALPHA = 6;
Since tRNS_colorType is used to hold one of the above mentioned values if we
don't initialize it to some unused value(here we are using -1) we will be
representing PNG_COLOR_GRAY by default.
By default the value will be -1 after the change and after we parse tRNS chunk
it will contain appropriate value. The initialization of tRNS_colorType change
can be considered as newly added check.
Thanks,
Jay
From: Krishna Addepalli
Sent: Monday, April 02, 2018 9:56 AM
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
Hi Jay,
The changes look fine. However, I have one more question: Why did you have to
explicitly specify the initial value to "tRNS_colorType" in PNGMetadata.java?
How is it affecting your implementation?
Thanks,
Krishna
From: Jayathirth D V
Sent: Wednesday, March 28, 2018 1:43 PM
To: Krishna Addepalli mailto:krishna.addepa...@oracle.com"krishna.addepa...@oracle.com>; 2d-dev
mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net>
Subject: RE: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
Hi Krishna,
Thanks for providing the inputs.
1. I suggest to rename considerTransparentPixel as isAlphaPresent.
As discussed I have added new name as isTransparentPixelPresent()
2. You can refactor the function body as below:
Return ((destinationBands == null ||
destinationBands.length == 4) &&
metadata.tRNS_colorType == PNG_COLOR_RGB);
Changes are made.
3. From line 1088 through 1113, I see some repeated calculations like
bytesPerRow etc. Why can't we reuse the previously defined variables? Apart
from destBands, all the other calculations look similar and repeated.
Previous to this change all the values like inputsBands, bytesPerRow,
eltsPerRow were same between the decoded output and destination image.
Now because we are changing only the destination image due to the presence of
transparent pixel, we need both these set of values. inputsBands, bytesPerRow,
eltsPerRow will be used for decoded output and destBands, destEltsPerRow for
destination image. Its better if don't mingle code flow or variables between
these two code paths.
4. When you are copying the pixels to a writable raster, at line 1273,
you could reduce the repetition of the lines, by just having one statement
inside if as below:
for (int i = 0; i < passWidth; i++) {
byteData[destidx] = curr[srcidx];
byteData[destidx + 1] = curr[srcidx + 1];
byteData[destidx + 2] = curr[srcidx + 2];
if (curr[srcidx] == (byte)metadata.tRNS_red &&
curr[srcidx + 1] == (byte)metadata.tRNS_green &&
curr[srcidx + 2] == (byte)metadata.tRNS_blue)
{
byteData[destidx + 3] = (byte)0;
} else {
byteData[destidx + 3] = (byte)255;
}
srcidx += 3;
destidx += 4;
}
Same thing can be done for the loop that executes for 16bit pixel data.
Changes are made.
Please find updated webrev for review :
http://cr.openjdk.java.net/~jdv/6788458/webrev.03/
Thanks,
Jay
From: Krishna Addepalli
Sent: Wednesday, March 28, 2018 11:52 AM
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
Hi Jay,
I have some points as below:
1. I suggest to rename considerTransparentPixel as isAlphaPresent.
2. You can refactor the function body as below:
Return ((destinationBands == null ||
destinationBands.length == 4) &&
metadata.tRNS_colorType == PNG_COLOR_RGB);
3. From line 1088 through 1113, I see some repeated calculations like
bytesPerRow etc. Why can't we reuse the previously defined variables? Apart
from destBands, all the other calculations look similar and repeated.
4. When you are copying the pixels to a writable raster, at line 1273,
you could reduce the repetition of the lines, by just having one statement
inside if as below:
for (int i = 0; i < passWidth; i++) {
byteData[destidx] = curr[srcidx];
byteData[destidx
Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS chunk while reading non-indexed RGB images
Hi Jay,
The changes look fine. However, I have one more question: Why did you have to
explicitly specify the initial value to "tRNS_colorType" in PNGMetadata.java?
How is it affecting your implementation?
Thanks,
Krishna
From: Jayathirth D V
Sent: Wednesday, March 28, 2018 1:43 PM
To: Krishna Addepalli <krishna.addepa...@oracle.com>; 2d-dev
<2d-dev@openjdk.java.net>
Subject: RE: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
Hi Krishna,
Thanks for providing the inputs.
1. I suggest to rename considerTransparentPixel as isAlphaPresent.
As discussed I have added new name as isTransparentPixelPresent()
2. You can refactor the function body as below:
Return ((destinationBands == null ||
destinationBands.length == 4) &&
metadata.tRNS_colorType == PNG_COLOR_RGB);
Changes are made.
3. From line 1088 through 1113, I see some repeated calculations like
bytesPerRow etc. Why can't we reuse the previously defined variables? Apart
from destBands, all the other calculations look similar and repeated.
Previous to this change all the values like inputsBands, bytesPerRow,
eltsPerRow were same between the decoded output and destination image.
Now because we are changing only the destination image due to the presence of
transparent pixel, we need both these set of values. inputsBands, bytesPerRow,
eltsPerRow will be used for decoded output and destBands, destEltsPerRow for
destination image. Its better if don't mingle code flow or variables between
these two code paths.
4. When you are copying the pixels to a writable raster, at line 1273,
you could reduce the repetition of the lines, by just having one statement
inside if as below:
for (int i = 0; i < passWidth; i++) {
byteData[destidx] = curr[srcidx];
byteData[destidx + 1] = curr[srcidx + 1];
byteData[destidx + 2] = curr[srcidx + 2];
if (curr[srcidx] == (byte)metadata.tRNS_red &&
curr[srcidx + 1] == (byte)metadata.tRNS_green &&
curr[srcidx + 2] == (byte)metadata.tRNS_blue)
{
byteData[destidx + 3] = (byte)0;
} else {
byteData[destidx + 3] = (byte)255;
}
srcidx += 3;
destidx += 4;
}
Same thing can be done for the loop that executes for 16bit pixel data.
Changes are made.
Please find updated webrev for review :
http://cr.openjdk.java.net/~jdv/6788458/webrev.03/
Thanks,
Jay
From: Krishna Addepalli
Sent: Wednesday, March 28, 2018 11:52 AM
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
Hi Jay,
I have some points as below:
1. I suggest to rename considerTransparentPixel as isAlphaPresent.
2. You can refactor the function body as below:
Return ((destinationBands == null ||
destinationBands.length == 4) &&
metadata.tRNS_colorType == PNG_COLOR_RGB);
3. From line 1088 through 1113, I see some repeated calculations like
bytesPerRow etc. Why can't we reuse the previously defined variables? Apart
from destBands, all the other calculations look similar and repeated.
4. When you are copying the pixels to a writable raster, at line 1273,
you could reduce the repetition of the lines, by just having one statement
inside if as below:
for (int i = 0; i < passWidth; i++) {
byteData[destidx] = curr[srcidx];
byteData[destidx + 1] = curr[srcidx + 1];
byteData[destidx + 2] = curr[srcidx + 2];
if (curr[srcidx] == (byte)metadata.tRNS_red &&
curr[srcidx + 1] == (byte)metadata.tRNS_green &&
curr[srcidx + 2] == (byte)metadata.tRNS_blue)
{
byteData[destidx + 3] = (byte)0;
} else {
byteData[destidx + 3] = (byte)255;
}
srcidx += 3;
destidx += 4;
}
Same thing can be done for the loop that executes for 16bit pixel data.
I haven't yet checked the test cases.
Thanks,
Krishna
From: Jayathirth D V
Sent: Wednesday, March 28, 2018 9:52 AM
To: 2d-dev mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net>
Subject: Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: P
Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS chunk while reading non-indexed RGB images
Hi Krishna,
Thanks for providing the inputs.
1. I suggest to rename considerTransparentPixel as isAlphaPresent.
As discussed I have added new name as isTransparentPixelPresent()
2. You can refactor the function body as below:
Return ((destinationBands == null ||
destinationBands.length == 4) &&
metadata.tRNS_colorType == PNG_COLOR_RGB);
Changes are made.
3. From line 1088 through 1113, I see some repeated calculations like
bytesPerRow etc. Why can't we reuse the previously defined variables? Apart
from destBands, all the other calculations look similar and repeated.
Previous to this change all the values like inputsBands, bytesPerRow,
eltsPerRow were same between the decoded output and destination image.
Now because we are changing only the destination image due to the presence of
transparent pixel, we need both these set of values. inputsBands, bytesPerRow,
eltsPerRow will be used for decoded output and destBands, destEltsPerRow for
destination image. Its better if don't mingle code flow or variables between
these two code paths.
4. When you are copying the pixels to a writable raster, at line 1273,
you could reduce the repetition of the lines, by just having one statement
inside if as below:
for (int i = 0; i < passWidth; i++) {
byteData[destidx] = curr[srcidx];
byteData[destidx + 1] = curr[srcidx + 1];
byteData[destidx + 2] = curr[srcidx + 2];
if (curr[srcidx] == (byte)metadata.tRNS_red &&
curr[srcidx + 1] == (byte)metadata.tRNS_green &&
curr[srcidx + 2] == (byte)metadata.tRNS_blue)
{
byteData[destidx + 3] = (byte)0;
} else {
byteData[destidx + 3] = (byte)255;
}
srcidx += 3;
destidx += 4;
}
Same thing can be done for the loop that executes for 16bit pixel data.
Changes are made.
Please find updated webrev for review :
http://cr.openjdk.java.net/~jdv/6788458/webrev.03/
Thanks,
Jay
From: Krishna Addepalli
Sent: Wednesday, March 28, 2018 11:52 AM
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
Hi Jay,
I have some points as below:
1. I suggest to rename considerTransparentPixel as isAlphaPresent.
2. You can refactor the function body as below:
Return ((destinationBands == null ||
destinationBands.length == 4) &&
metadata.tRNS_colorType == PNG_COLOR_RGB);
3. From line 1088 through 1113, I see some repeated calculations like
bytesPerRow etc. Why can't we reuse the previously defined variables? Apart
from destBands, all the other calculations look similar and repeated.
4. When you are copying the pixels to a writable raster, at line 1273,
you could reduce the repetition of the lines, by just having one statement
inside if as below:
for (int i = 0; i < passWidth; i++) {
byteData[destidx] = curr[srcidx];
byteData[destidx + 1] = curr[srcidx + 1];
byteData[destidx + 2] = curr[srcidx + 2];
if (curr[srcidx] == (byte)metadata.tRNS_red &&
curr[srcidx + 1] == (byte)metadata.tRNS_green &&
curr[srcidx + 2] == (byte)metadata.tRNS_blue)
{
byteData[destidx + 3] = (byte)0;
} else {
byteData[destidx + 3] = (byte)255;
}
srcidx += 3;
destidx += 4;
}
Same thing can be done for the loop that executes for 16bit pixel data.
I haven't yet checked the test cases.
Thanks,
Krishna
From: Jayathirth D V
Sent: Wednesday, March 28, 2018 9:52 AM
To: 2d-dev mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net>
Subject: Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
Hello All,
I have enhanced both the test case to verify pixels which not only match tRNS
transparent pixel data but also for them which doesn't match tRNS transparent
pixel data.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/6788458/webrev.02/
Thanks,
Jay
From: Jayathirth D V
Sent: Wednesday, March 28, 2018 8:28 AM
To: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk
Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS chunk while reading non-indexed RGB images
Hi Jay,
I have some points as below:
1. I suggest to rename considerTransparentPixel as isAlphaPresent.
2. You can refactor the function body as below:
Return ((destinationBands == null ||
destinationBands.length == 4) &&
metadata.tRNS_colorType == PNG_COLOR_RGB);
3. From line 1088 through 1113, I see some repeated calculations like
bytesPerRow etc. Why can't we reuse the previously defined variables? Apart
from destBands, all the other calculations look similar and repeated.
4. When you are copying the pixels to a writable raster, at line 1273,
you could reduce the repetition of the lines, by just having one statement
inside if as below:
for (int i = 0; i < passWidth; i++) {
byteData[destidx] = curr[srcidx];
byteData[destidx + 1] = curr[srcidx + 1];
byteData[destidx + 2] = curr[srcidx + 2];
if (curr[srcidx] == (byte)metadata.tRNS_red &&
curr[srcidx + 1] == (byte)metadata.tRNS_green &&
curr[srcidx + 2] == (byte)metadata.tRNS_blue)
{
byteData[destidx + 3] = (byte)0;
} else {
byteData[destidx + 3] = (byte)255;
}
srcidx += 3;
destidx += 4;
}
Same thing can be done for the loop that executes for 16bit pixel data.
I haven't yet checked the test cases.
Thanks,
Krishna
From: Jayathirth D V
Sent: Wednesday, March 28, 2018 9:52 AM
To: 2d-dev <2d-dev@openjdk.java.net>
Subject: Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
Hello All,
I have enhanced both the test case to verify pixels which not only match tRNS
transparent pixel data but also for them which doesn't match tRNS transparent
pixel data.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/6788458/webrev.02/
Thanks,
Jay
From: Jayathirth D V
Sent: Wednesday, March 28, 2018 8:28 AM
To: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
Hello All,
I just realized that the test case Read16BitPNGWithTRNSChunk.java is creating
(50, 50) image(which was done while debugging the issue), which is not needed
as we need single pixel to verify the fix as it is done in
Read8BitPNGWithTRNSChunk.java. I have updated Read16BitPNGWithTRNSChunk.java to
reflect this small change.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/6788458/webrev.01/
Thanks,
Jay
From: Jayathirth D V
Sent: Tuesday, March 27, 2018 6:51 PM
To: 2d-dev
Subject: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
Hello All,
Please review the following solution in JDK11 :
Bug : https://bugs.openjdk.java.net/browse/JDK-6788458
Webrev : http://cr.openjdk.java.net/~jdv/6788458/webrev.00/
Issue: When we try to read non-indexed RGB PNG image having transparent pixel
information in tRNS chunk, ImageIO.PNGImageReader ignores the transparent pixel
information. If we use Toolkit.getDefaultToolkit().createImage() it doesn't
ignore the transparent pixel information.
Root cause: In ImageIO.PNGImageReader() we store tRNS chunk values in
readMetadata() -> parse_tRNS_chunk (), but we never use the tRNS R, G, B value
to compare with decoded image information. Also in ImageIO.PNGImageReader() for
IHDR colortype RGB we use only 3 channel destination BufferedImage. So even if
we use the tRNS chunk value we need destination BufferedImage of 4 channels to
represent transparency.
Solution: While assigning destination image in PNGImageReader.getImageTypes()
if we know that PNG image is of type RGB and has tRNS chunk then we need to
assign a destination image having 4 channels. After that in decodePass() we
need to create 4 channel destination raster and while we store decoded image
information into the destination BufferedImage we need to compare decoded R, G,
B values with tRNS R, G, B values and store appropriate alpha channel value.
Also we use 4 channel destination image in case of RGB image only when tRNS
chunk is present and ImageReadParam.destinationBands is null or
ImageReadParam.destinationBands is equal to 4.
One more change is that we have optimization in PNGImageReader.readMetadata()
where if ignoreMetadata is true and IHDR colortype is not of type
PNG_COLOR_PALETTE, we ignore all the chunk data and just try to find first IDAT
chunk. But if we need tRNS chunk values in case of IHDR colortype PNG_COLOR_RGB
we need to parse tNRS chunk also. So in readMetadata(
Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS chunk while reading non-indexed RGB images
Hello All, I have enhanced both the test case to verify pixels which not only match tRNS transparent pixel data but also for them which doesn't match tRNS transparent pixel data. Please find updated webrev for review: http://cr.openjdk.java.net/~jdv/6788458/webrev.02/ Thanks, Jay From: Jayathirth D V Sent: Wednesday, March 28, 2018 8:28 AM To: 2d-dev Subject: Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS chunk while reading non-indexed RGB images Hello All, I just realized that the test case Read16BitPNGWithTRNSChunk.java is creating (50, 50) image(which was done while debugging the issue), which is not needed as we need single pixel to verify the fix as it is done in Read8BitPNGWithTRNSChunk.java. I have updated Read16BitPNGWithTRNSChunk.java to reflect this small change. Please find updated webrev for review: http://cr.openjdk.java.net/~jdv/6788458/webrev.01/ Thanks, Jay From: Jayathirth D V Sent: Tuesday, March 27, 2018 6:51 PM To: 2d-dev Subject: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS chunk while reading non-indexed RGB images Hello All, Please review the following solution in JDK11 : Bug : https://bugs.openjdk.java.net/browse/JDK-6788458 Webrev : http://cr.openjdk.java.net/~jdv/6788458/webrev.00/ Issue: When we try to read non-indexed RGB PNG image having transparent pixel information in tRNS chunk, ImageIO.PNGImageReader ignores the transparent pixel information. If we use Toolkit.getDefaultToolkit().createImage() it doesn't ignore the transparent pixel information. Root cause: In ImageIO.PNGImageReader() we store tRNS chunk values in readMetadata() -> parse_tRNS_chunk (), but we never use the tRNS R, G, B value to compare with decoded image information. Also in ImageIO.PNGImageReader() for IHDR colortype RGB we use only 3 channel destination BufferedImage. So even if we use the tRNS chunk value we need destination BufferedImage of 4 channels to represent transparency. Solution: While assigning destination image in PNGImageReader.getImageTypes() if we know that PNG image is of type RGB and has tRNS chunk then we need to assign a destination image having 4 channels. After that in decodePass() we need to create 4 channel destination raster and while we store decoded image information into the destination BufferedImage we need to compare decoded R, G, B values with tRNS R, G, B values and store appropriate alpha channel value. Also we use 4 channel destination image in case of RGB image only when tRNS chunk is present and ImageReadParam.destinationBands is null or ImageReadParam.destinationBands is equal to 4. One more change is that we have optimization in PNGImageReader.readMetadata() where if ignoreMetadata is true and IHDR colortype is not of type PNG_COLOR_PALETTE, we ignore all the chunk data and just try to find first IDAT chunk. But if we need tRNS chunk values in case of IHDR colortype PNG_COLOR_RGB we need to parse tNRS chunk also. So in readMetadata() also appropriate changes are made. Note : Initially the enhancement request was present only for 8 bit RGB PNG images but after making more modifications realized that we can add support for 16 bit RGB image also by making similar incremental changes. The tRNS chunk value is still ignored for Gray PNG image. If we need to support PNG_COLOR_GRAY image with tRNS chunk(which is very rare) we can take that up in a separate bug as it needs different set of changes. Thanks, Jay
Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS chunk while reading non-indexed RGB images
Hello All, I just realized that the test case Read16BitPNGWithTRNSChunk.java is creating (50, 50) image(which was done while debugging the issue), which is not needed as we need single pixel to verify the fix as it is done in Read8BitPNGWithTRNSChunk.java. I have updated Read16BitPNGWithTRNSChunk.java to reflect this small change. Please find updated webrev for review: http://cr.openjdk.java.net/~jdv/6788458/webrev.01/ Thanks, Jay From: Jayathirth D V Sent: Tuesday, March 27, 2018 6:51 PM To: 2d-dev Subject: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS chunk while reading non-indexed RGB images Hello All, Please review the following solution in JDK11 : Bug : https://bugs.openjdk.java.net/browse/JDK-6788458 Webrev : http://cr.openjdk.java.net/~jdv/6788458/webrev.00/ Issue: When we try to read non-indexed RGB PNG image having transparent pixel information in tRNS chunk, ImageIO.PNGImageReader ignores the transparent pixel information. If we use Toolkit.getDefaultToolkit().createImage() it doesn't ignore the transparent pixel information. Root cause: In ImageIO.PNGImageReader() we store tRNS chunk values in readMetadata() -> parse_tRNS_chunk (), but we never use the tRNS R, G, B value to compare with decoded image information. Also in ImageIO.PNGImageReader() for IHDR colortype RGB we use only 3 channel destination BufferedImage. So even if we use the tRNS chunk value we need destination BufferedImage of 4 channels to represent transparency. Solution: While assigning destination image in PNGImageReader.getImageTypes() if we know that PNG image is of type RGB and has tRNS chunk then we need to assign a destination image having 4 channels. After that in decodePass() we need to create 4 channel destination raster and while we store decoded image information into the destination BufferedImage we need to compare decoded R, G, B values with tRNS R, G, B values and store appropriate alpha channel value. Also we use 4 channel destination image in case of RGB image only when tRNS chunk is present and ImageReadParam.destinationBands is null or ImageReadParam.destinationBands is equal to 4. One more change is that we have optimization in PNGImageReader.readMetadata() where if ignoreMetadata is true and IHDR colortype is not of type PNG_COLOR_PALETTE, we ignore all the chunk data and just try to find first IDAT chunk. But if we need tRNS chunk values in case of IHDR colortype PNG_COLOR_RGB we need to parse tNRS chunk also. So in readMetadata() also appropriate changes are made. Note : Initially the enhancement request was present only for 8 bit RGB PNG images but after making more modifications realized that we can add support for 16 bit RGB image also by making similar incremental changes. The tRNS chunk value is still ignored for Gray PNG image. If we need to support PNG_COLOR_GRAY image with tRNS chunk(which is very rare) we can take that up in a separate bug as it needs different set of changes. Thanks, Jay
