[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS chunk while reading non-index
From:       Phil Race <philip.race () oracle ! com>
Date:       2018-04-17 15:30:36
Message-ID: 5dfb9228-2308-5ccd-8dd2-2b626fc4740b () oracle ! com
[Download RAW message or body]

+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 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.addepalli@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.addepalli@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


[prev in list] [next in list] [prev in thread] [next in thread]