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 values to 
> all the channels. It is a per pixel operation and we would be writing data 
> twice.
>
> Instead of using Arrays.fill(),  I thought of just assigning value to alpha 
> channel:
>   int[] temp = new int[inputBands + 1];
>   temp[inputBands] = (bitDepth < 16) ? 255 : 65535;  if 
> (metadata.tRNS_colorType == PNG_COLOR_RGB) {
>
> I think even doing this is not ideal because anyway for all the pixels we 
> will be checking pixel data with tRNS data, and assign value in alpha 
> channel. There is no need for us to assign some value and override it again 
> later if a condition satisfies.
> So I am assigning opaque value to alpha channel at same place. But I have 
> reduced the LOC by using ternary operator for determining the opaque value 
> for alpha channel.
>
> All other changes are updated.
> Please find updated webrev for review:
> http://cr.openjdk.java.net/~jdv/6788458/webrev.06/
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Prahalad Kumar Narayanan
> Sent: Friday, April 06, 2018 1:42 PM
> To: Jayathirth D V; 2d-dev
> Subject: RE: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader 
> ignores tRNS chunk while reading non-indexed RGB images
>
> Hello Jay
>
> Good day to you.
>
> I looked into the latest changes.
> The code changes are nearly complete. Just a few tweaks.
>
> File: PNGImageReader.java
> Line: 1280
> Rephrase from:
>                 /*
>                  * In case of colortype PNG_COLOR_RGB or PNG_COLOR_GRAY
>                  * if we have transparent pixel information from tRNS chunk
>                  * we need to consider that also and store proper information
>                  * in alpha channel.
>                  *
>                  * Also we create destination image with extra alpha channel
>                  * in getImageTypes() when we have tRNS chunk for colorType
>                  * PNG_COLOR_RGB or PNG_COLOR_GRAY.
>                  */
> Rephrase to:
>                 /*
>                  * For PNG images of color type PNG_COLOR_RGB or 
> PNG_COLOR_GRAY
>                  * that contain a specific transparent color (given by tRNS
>                  * chunk), we compare the decoded pixel color with the color
>                  * given by tRNS chunk to set the alpha on the destination.
>                  */
>
> File: PNGImageReader.java
> Line: 1588
> Rephrase from:
>          /*
>           * In case of PNG_COLOR_RGB or PNG_COLOR_GRAY, if we
>           * have transparent pixel information in tRNS chunk
>           * we create destination image having alpha channel.
>           */
>
> Rephrase to:
>          /*
>           * For PNG images of color type PNG_COLOR_RGB or PNG_COLOR_GRAY that
>           * contain a specific transparent color (given by tRNS chunk), we add
>           * ImageTypeSpecifier(s) that support transparency to the list of
>           * supported image types.
>           */
>
> File: PNGImageReader.java
> Line(s): 1290, 1493, 1596, 1619
>      . The lines mentioned above check whether metadata has specific 
> transparent color using-
>              metadata.tRNS_present &&
>              (metadata.tRNS_colorType == PNG_COLOR_RGB ||
>               metadata.tRNS_colorType == PNG_COLOR_GRAY)
>
>      . The above check is not only redundant but also metadata specific.
>      . We could move the code to a common method in PNGMetadata-
>          boolean hasTransparentColor() {
>              return tRNS_present
>                     && (tRNS_colorType == PNG_COLOR_RGB
>                         || tRNS_colorType == PNG_COLOR_GRAY);
>          }
>      . I understand 1596 and 1619 check for specific color type but they can 
> be avoided with this method as well.
>      . As per the specification, tRNS values depend on the image's color type.
>      . This will reduce the complex check from-
>              if (theImage.getSampleModel().getNumBands() ==
>                      inputBandsForColorType[colorType] + 1 &&
>                  metadata.tRNS_present &&
>                  (metadata.tRNS_colorType == PNG_COLOR_RGB ||
>                   metadata.tRNS_colorType == PNG_COLOR_GRAY))
>              {
>        to-
>              if (metadata.hasTransparentColor()
>                  && (theImage.getSampleModel().getNumBands() ==
>                      inputBandsForColorType[colorType] + 1)) {
>
> File: PNGImageReader.java
> Line: 1329, 1342
>      . The else block for if (check for transparent pixel) is redundant 
> across both PNG_COLOR_RGB and PNG_COLOR_GRAY.
>      . We could use Arrays.fill with opaque value and set the alpha to 0 if 
> pixel is transparent.
>              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

Reply via email to