[OpenJDK 2D-Dev] [10] RFR JDK-8190997: PNGImageReader throws NullPointerException when PLTE section is missing

[Jayathirth D V]

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.