The .02 version looks fine.
On 18/12/2017 20:55, Jayathirth D V wrote:
Hi Sergey,
Could you please provide your inputs and review the fix.
Thanks,
Jay
-----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.
