[OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS chunk while reading non-indexed RGB images
Jayathirth D V
jayathirth.d.v at oracle.com
Mon Apr 2 07:58:43 UTC 2018
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 <HYPERLINK "mailto:krishna.addepalli at oracle.com"krishna.addepalli at oracle.com>; 2d-dev <HYPERLINK "mailto:2d-dev at openjdk.java.net"2d-dev at 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 <HYPERLINK "mailto:krishna.addepalli at oracle.com"krishna.addepalli at oracle.com>; 2d-dev <HYPERLINK "mailto:2d-dev at openjdk.java.net"2d-dev at 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 at openjdk.java.net"2d-dev at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20180402/9d380c16/attachment-0001.html>
More information about the 2d-dev
mailing list