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 <HYPERLINK
"mailto:krishna.addepa...@oracle.com"krishna.addepa...@oracle.com>; 2d-dev
<HYPERLINK "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 + 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 <HYPERLINK "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 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
