[OpenJDK 2D-Dev] [11] RFR JDK-5109146: PNGMetadata Background color initialization from standard metadata is incomplete

Phil Race philip.race at oracle.com
Mon May 14 16:49:20 UTC 2018


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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20180514/e142f439/attachment.html>


More information about the 2d-dev mailing list