Hello Jay The fix looks good.
Thanks Have a good day Prahalad N. -----Original Message----- From: Jayathirth D V Sent: Thursday, December 14, 2017 9:47 AM To: Sergey Bylokhov; Prahalad Kumar Narayanan; 2d-dev Subject: RE: [OpenJDK 2D-Dev] [10] RFR JDK-8190997: PNGImageReader throws NullPointerException when PLTE section is missing Hello Sergey, Thanks for your inputs. Comparison using == just comes out of me because of my coding style. Since the variable name PLTE_present is self-explanatory that it is of type boolean I also feel from verbose perspective we should use ! operator. I have updated the code to reflect the same. Apart from verbose perspective is there anything different in Java between using == & ! in this case. Please provide your inputs as it would be helpful in future changes. Please find updated webrev for review: http://cr.openjdk.java.net/~jdv/8190997/webrev.02/ Thanks, Jay -----Original Message----- From: Sergey Bylokhov Sent: Wednesday, December 13, 2017 9:35 PM To: Jayathirth D V; Prahalad Kumar Narayanan; 2d-dev Subject: Re: [OpenJDK 2D-Dev] [10] RFR JDK-8190997: PNGImageReader throws NullPointerException when PLTE section is missing It looks fine, but I wonder why metadata.PLTE_present == false is used instead of !metadata.PLTE_present On 13/12/2017 02:50, Jayathirth D V wrote: > Hello Prahalad, > > Thanks for your inputs. > I have updated the code based on your inputs. > > Please find updated webrev for review: > http://cr.openjdk.java.net/~jdv/8190997/webrev.01/ > > Thanks, > Jay > > -----Original Message----- > From: Prahalad Kumar Narayanan > Sent: Wednesday, December 13, 2017 3:29 PM > To: Jayathirth D V; 2d-dev > Subject: RE: [OpenJDK 2D-Dev] [10] RFR JDK-8190997: PNGImageReader throws > NullPointerException when PLTE section is missing > > Hello Jay > > I looked into the changes. > The logic to check the presence of PLTE chunk is correct. > > Few minor changes > . The if condition to check whether PLTE chunk exists would get executed > every time an IDAT chunk is encountered. > . For png images with multiple IDAT chunks, this is a repeated check > and can be avoided. > . You can move the if condition within the if block that stores > location of 1st IDAT chunk. > > . Secondly, the wording within the IIOException can be changed. > . PNG specification mentions IHDR chunk as Image Header and this is > separate from PLTE chunk. > . So you could rephrase "PNG Header doesn't contain required PLTE > chunk" as "PNG image doesn't contain required PLTE chunk" > > Thank you > Have a good day > > Prahalad N. > > ----- Original Message ----- > From: Jayathirth D V > Sent: Wednesday, December 13, 2017 3:02 PM > To: 2d-dev > Subject: [OpenJDK 2D-Dev] [10] RFR JDK-8190997: PNGImageReader throws > NullPointerException when PLTE section is missing > > Hello All, > > Please review the following fix in JDK10 : > > Bug : https://bugs.openjdk.java.net/browse/JDK-8190997 > Webrev : http://cr.openjdk.java.net/~jdv/8190997/webrev.00/ > > Issue : When we try to read PNG image with color type as PNG_COLOR_PALETTE in > IHDR header but missing the required PLTE chunk we throw NullPointerException. > > Root cause : We finish reading metadata but when we try to read image > information and access palette related data it throws NPE as palette related > data is not initialized/not present. > > Solution : PNG specification mandates that if color type present in IHDR is > PNG_COLOR_PALETTE(type 3), PLTE chunk should appear before the first IDAT > chunk. So while reading metadata itself we should verify the same and if > required PLTE chunk is absent we should throw proper IIOException instead of > allowing the code to continue further and throw NPE while trying to access > palette information. > > Thanks, > Jay > -- Best regards, Sergey.
