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/ 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/ 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/ 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
