Hello Jay
Good day to you.
I looked into the latest changes.
The code changes are nearly complete. Just a few tweaks.
File: PNGImageReader.java
Line: 1280
Rephrase from:
/*
* In case of colortype PNG_COLOR_RGB or PNG_COLOR_GRAY
* if we have transparent pixel information from tRNS chunk
* we need to consider that also and store proper information
* in alpha channel.
*
* Also we create destination image with extra alpha channel
* in getImageTypes() when we have tRNS chunk for colorType
* PNG_COLOR_RGB or PNG_COLOR_GRAY.
*/
Rephrase to:
/*
* For PNG images of color type PNG_COLOR_RGB or PNG_COLOR_GRAY
* that contain a specific transparent color (given by tRNS
* chunk), we compare the decoded pixel color with the color
* given by tRNS chunk to set the alpha on the destination.
*/
File: PNGImageReader.java
Line: 1588
Rephrase from:
/*
* In case of PNG_COLOR_RGB or PNG_COLOR_GRAY, if we
* have transparent pixel information in tRNS chunk
* we create destination image having alpha channel.
*/
Rephrase to:
/*
* For PNG images of color type PNG_COLOR_RGB or PNG_COLOR_GRAY that
* contain a specific transparent color (given by tRNS chunk), we add
* ImageTypeSpecifier(s) that support transparency to the list of
* supported image types.
*/
File: PNGImageReader.java
Line(s): 1290, 1493, 1596, 1619
. The lines mentioned above check whether metadata has specific transparent
color using-
metadata.tRNS_present &&
(metadata.tRNS_colorType == PNG_COLOR_RGB ||
metadata.tRNS_colorType == PNG_COLOR_GRAY)
. The above check is not only redundant but also metadata specific.
. We could move the code to a common method in PNGMetadata-
boolean hasTransparentColor() {
return tRNS_present
&& (tRNS_colorType == PNG_COLOR_RGB
|| tRNS_colorType == PNG_COLOR_GRAY);
}
. I understand 1596 and 1619 check for specific color type but they can be
avoided with this method as well.
. As per the specification, tRNS values depend on the image's color type.
. This will reduce the complex check from-
if (theImage.getSampleModel().getNumBands() ==
inputBandsForColorType[colorType] + 1 &&
metadata.tRNS_present &&
(metadata.tRNS_colorType == PNG_COLOR_RGB ||
metadata.tRNS_colorType == PNG_COLOR_GRAY))
{
to-
if (metadata.hasTransparentColor()
&& (theImage.getSampleModel().getNumBands() ==
inputBandsForColorType[colorType] + 1)) {
File: PNGImageReader.java
Line: 1329, 1342
. The else block for if (check for transparent pixel) is redundant across
both PNG_COLOR_RGB and PNG_COLOR_GRAY.
. We could use Arrays.fill with opaque value and set the alpha to 0 if
pixel is transparent.
int[] temp = new int[inputBands + 1];
int opaque = (bitDepth < 16) ? 255 : 65535;
Arrays.fill(temp, opaque);
. All subsequent operations checking for transparent color value and
setting alpha to 0 would be required.
File: PNGImageReader.java
All modified Lines:
. The opening braces '{' can appear in the new line. Some engineers do
follow this.
. Since the rest of the code aligns opening braces in the same line as the
' if ' statement we could follow the same for code readability.
Test File: ReadPngGrayImageWithTRNSChunk.java and
ReadPngRGBImageWithTRNSChunk.java
. The finally blocks should check whether the objects- ImageOutputStream
and File, are not null.
. The call to directory(while is a File).delete() is not required in my
view. Verify this once.
. Dispose the writer after the write operation is complete.
Thank you
Have a good day
Prahalad N.
-----Original Message-----
From: Jayathirth D V
Sent: Thursday, April 05, 2018 3:26 PM
To: Prahalad Kumar Narayanan; 2d-dev
Subject: RE: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
Hello Prahalad,
Thank you for the inputs.
I gave updated the code to not change ImageTypeSpecifier of passRow. Now while
storing the pixel values into imRas we comparing the pixel RGB/Gray values to
tRNS RGB/Gray values and storing appropriate value for alpha channel.
I have added support to use tRNS_Gray value when IHDR color type is
PNG_COLOR_GRAY - We are now creating 2 channel destination image whenever we
see that tRNS chunk is present for PNG_COLOR_GRAY in getImageTypes().
isTransparentPixelPresent() code is removed and we are using available tRNS
properties.
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<ImageTypeSpecifier> 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<ImageTypeSpecifier> 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 not PNG_COLOR_PALETTE. But we need
* to parse only the tRNS chunk even in the case where IHDR colortype
* is not PNG_COLOR_PALETTE, because we need tRNS chunk transparent
* pixel information for PNG_COLOR_RGB while storing the pixel data
* in decodePass().
To:
* Optimization: We can skip reading metadata if ignoreMetadata
* flag is set and colorType is not PNG_COLOR_PALETTE. However,
* we parse tRNS chunk to retrieve the transparent color from the
* metadata irrespective of the colorType. Doing so, helps
* PNGImageReader to appropriately identify and set transparent
* pixels in the decoded image.
File: PNGMetadata.java
Line: 271
. Reset the tRNS_colorType to -1 in the reset() method.
. This will not concern if tRNS_colorType is used in conjunction with
tRNS_present flag.
. However, the new method isTransparentPixelAvailable() uses tRNS_colorType
directly.
. When the same ImageReader is used to read multiple PNG images, this could
pose a problem.
Thank you
Have 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 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() 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
