+1 Thanks, Krishna
-----Original Message----- From: Jayathirth D V Sent: Tuesday, November 20, 2018 1:59 PM To: 2d-dev <2d-dev@openjdk.java.net> Subject: Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211795: ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458 Thanks Sergey for the review. I need one more review. Please review the latest webrev: http://cr.openjdk.java.net/~jdv/8211795/webrev.01/ Thanks, Jay -----Original Message----- From: Sergey Bylokhov Sent: Saturday, November 17, 2018 3:35 AM To: Jayathirth D V; 2d-dev Subject: Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211795: ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458 Looks fine. On 16/11/2018 02:16, Jayathirth D V wrote: > Hi Sergey, > > As discussed offline I did more analysis on whether we can use common > variable to determine number of bands. Since we have "outputSampleSize.length > - 1" and "inputBands + 1" kind of things. > > Actually scale array will be used on input data(ps[]), so we should use input > bands value to create and use scale array. Before JDK-6788458 there was no > difference between input bands and output bands so we didn't see any issue. > But after JDK-6788458 number of bands data is different between input and > output data for PNG_COLOR_RGB/GRAY having tRNS chunk. > > So I have made change to use inputBands data for creation and use of scale > array. Ran complete imageio jtreg and JCK tests there are no failures. > Please find updated webrev for review: > http://cr.openjdk.java.net/~jdv/8211795/webrev.01/ > > Thanks, > Jay > > -----Original Message----- > From: Jayathirth D V > Sent: Wednesday, November 14, 2018 1:09 PM > To: Sergey Bylokhov; 2d-dev > Subject: RE: [OpenJDK 2D-Dev] [12] RFR JDK-8211795: > ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458 > > Hi Sergey, > > Thanks for the review. > > As you pointed out, yes the AIOOB is happening for ps[b]. I have updated the > analysis in JBS bug also. > > Basically the calculation of numBands is not proper because we take numBands > value from destination image. This destination image will have extra alpha > channel for Gray or RGB input data(ps[]) which will throw AIOOB. > > So we need to update the logic of how we calculate numBands different for PNG > Gray/RGB image havng tRNS chunk. Fortunately, webrev.00 is actually doing > this job. > > Regarding whether we need to change scale array logic : We expect first 3 > channel to be RGB and first channel to be Gray for PNG_COLOR_RGB and > PNG_COLOR_GRAY respectively. So just updating numBands information will > create proper scale array. So there is no need to change the scale array > logic. > > History JDK-6788458 : Toolkit was able to show transparent color information > for RGB/Gray PNG image when it has tRNS chunk, but ImageIO didn't support it. > To use tRNS data and show transparent color in output image we needed to add > extra alpha channel for PNG RGB/Gray image with tRNS chunk. But fix present > in JDK-6788458 didn't handle the case where bitDepth adjustment is needed and > we are using band information from output image(having extra alpha channel) > on input image which has no alpha channel. Change in numBands logic for this > bug fixes that issue. > > > Thanks, > Jay > > -----Original Message----- > From: Sergey Bylokhov > Sent: Wednesday, November 14, 2018 4:07 AM > To: Jayathirth D V; 2d-dev > Subject: Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211795: > ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458 > > Hi, Jay. > > Can you please provide some more detail about this bug. > >> Root cause : In JDK-6788458 we are adding extra alpha channel for >> destination whenever we have tRNS chunk. But the number of bands in bitDepth >> scale array was not changed when we have tRNS chunk. This is causing >> ArrayIndexOutOfBoundsException for scale array. > > As far as I understand the AIOOB is occurred when we access ps[b] at line > 1308 not when we access the scale array, because the scale array is created > as "scale = new int[numBands][]". So maybe numBands should depends on the > passRow? or the creation of scale[][xxx] should be updated? > BTW this code uses +1/-1 in a lot of places already, and it is not always > clear why. > > -- Best regards, Sergey.
