+1 from me.
-phil.
On 05/09/2018 05:43 AM, Jayathirth D V wrote:
Hi Krishna,
Thanks for the review.
1.I see that the fix is to update the bKGD_colorType for Background
color child. However, don’t you need to do the same thing for paletted
images as well?
/We are not updating bKGD_colorType, we are adding missing
bKGD_colorType when user wants to specify background color. In
standard metadata if user wants to provide color palette and with
corresponding background color index then he should specify palette
using Chroma->Palette node and background index using Chroma->
//BackgroundIndex node./
1.As for the test case, I’m not sure how ImageReaders and ImageWriters
work.
I see that you query them from ImageIO, so the question is, do you
need to get a new ImageReader/ImageWriter for each image read/write
operation?
If so, then, there is a potential problem when you call
VerifyNativeRGBValuesFromBKGDChunk() and
VerifyStandardRGBValuesFromBKGDChunk() in succession, since each in
turn calls writeAndReadMetaData() and the writer is disposed.
On the other hand, if querying for the reader/writer for once is
enough, then you can initialize the image reader/writer in the static
block itself, and then run the whole test.
/We don’t need multiple instances of writer/reader.I have changed the
code to create ImageReader & Writer instance only once. Also
PNGImageWriter/Reader doesn’t override dispose() method present in
super class which is just empty method, that is the reason why we
didn’t get NPE when writer was getting disposed and used again
previously. I have still kept writer & reader dispose() calls at the
end in test case which will be used if we plan to override these
methods in future./
Also I noticed that test case was not failing even without fix. This
was happening because of usage of common metadata between
VerifyNativeRGBValuesFromBKGDChunk() &
VerifyStandardRGBValuesFromBKGDChunk(). I have updated the test case
to create independent metadata variables for
VerifyNativeRGBValuesFromBKGDChunk() &
VerifyStandardRGBValuesFromBKGDChunk().
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/5109146/webrev.02/
<http://cr.openjdk.java.net/%7Ejdv/5109146/webrev.02/>
Thanks,
Jay
*From:* Krishna Addepalli
*Sent:* Wednesday, May 09, 2018 11:51 AM
*To:* 2d-dev
*Subject:* Re: [OpenJDK 2D-Dev] [11] RFR JDK-5109146: PNGMetadata
Background color initialization from standard metadata is incomplete
Hi Jay,
Here are my observations/questions:
1.I see that the fix is to update the bKGD_colorType for Background
color child. However, don’t you need to do the same thing for paletted
images as well?
2.As for the test case, I’m not sure how ImageReaders and ImageWriters
work.
I see that you query them from ImageIO, so the question is, do you
need to get a new ImageReader/ImageWriter for each image read/write
operation?
If so, then, there is a potential problem when you call
VerifyNativeRGBValuesFromBKGDChunk() and
VerifyStandardRGBValuesFromBKGDChunk() in succession, since each in
turn calls writeAndReadMetaData() and the writer is disposed.
On the other hand, if querying for the reader/writer for once is
enough, then you can initialize the image reader/writer in the static
block itself, and then run the whole test.
Thanks,
Krishna
*From:* Jayathirth D V
*Sent:* Wednesday, April 18, 2018 3:34 PM
*To:* 2d-dev
*Subject:* RE: [OpenJDK 2D-Dev] [11] RFR JDK-5109146: PNGMetadata
Background color initialization from standard metadata is incomplete
Hello All,
Since changes related to JDK-6574555 is pushed.
Please find new webrev which captures test case changes over JDK-6574555.
http://cr.openjdk.java.net/~jdv/5109146/webrev.01/
<http://cr.openjdk.java.net/%7Ejdv/5109146/webrev.01/>
Thanks,
Jay
*From:* Jayathirth D V
*Sent:* Tuesday, April 17, 2018 3:34 PM
*To:* 2d-dev
*Subject:* [OpenJDK 2D-Dev] [11] RFR JDK-5109146: PNGMetadata
Background color initialization from standard metadata is incomplete
Hello All,
Please review the following fix in JDK11 :
Bug : https://bugs.openjdk.java.net/browse/JDK-5109146
Webrev : http://cr.openjdk.java.net/~jdv/5109146/webrev.00/
<http://cr.openjdk.java.net/%7Ejdv/5109146/webrev.00/>
_Issue:_ PNGMetadata.mergeStandardTree() function doesn’t set proper
bKGD colortype.
_Solution:_ When bKGD R, G, B values are unique we need to store
appropriate bKGD colortype in metadata.
_Note_ : Test case which is present as part of review in JDK-6574555
is only updated to handle regression scenario for this bug also.
Regression scenario between this bug and JDK-6574555 differ only in
what type(native/standard) metadata we use to merge bKGD RGB values.
Thanks,
Jay
