Hello Everyone Thanks to Phil for his time in reviewing the fix.
I 've updated the code with suggested changes & modified code can be reviewed at http://cr.openjdk.java.net/~pnarayanan/8164971/webrev.02/ Changes since previous revision are: . Comment statement at line: 2038 has been removed . Spelling mistake at line: 2278 has been corrected. The changes are minimal but felt it would be good to raise the review. Kindly provide your review feedback at your convenience. Thank you for your time Have a good day Prahalad N. -----Original Message----- From: Phil Race Sent: Saturday, August 12, 2017 2:38 AM To: Jayathirth D V; Prahalad Kumar Narayanan Cc: 2d-dev@openjdk.java.net Subject: Re: [OpenJDK 2D-Dev] [10] RFR: JDK-8164971: PNG metadata does not handle ImageCreationTime 2038 // } else if (childName.equals("ImageCreationTime")) { 2039 } else if (childName.equals("ImageCreationTime")) { 2038 can go 2278 // tIME chunk with Image modificaiton time spelling mistake .. other than that looks good to me. -phil. On 07/31/2017 10:23 PM, Jayathirth D V wrote: Hello Prahalad, The decoding and encoding of creation time is happening from last text chunk and they are in sync. Also PNGMetadata code is very well structured for decodeImageCreationTimeFromTextChunk() and encodeImageCreationTimeToTextChunk() code flow. Changes are fine. Thanks, Jay -----Original Message----- From: Prahalad Kumar Narayanan Sent: Thursday, July 20, 2017 7:57 PM To: Brian Burkhalter; Jayathirth D V; 2d-dev@openjdk.java.net Subject: RE: [OpenJDK 2D-Dev] [10] RFR: JDK-8164971: PNG metadata does not handle ImageCreationTime Hello Everyone Good day to you. This is a follow-up to the bug fix for [JDK-8164971] PNG Metadata does not handle image creation time. First, Thanks to Brian and Jay for their time in review & feedback. . I 've addressed the review suggestions and the updated code is available for review. . Webrev Link: http://cr.openjdk.java.net/~pnarayanan/8164971/webrev.01/ Description on changes are as follows: Brian's Suggestions: Specifically the date format is *recommended* as opposed to be *required* to conform to RFC 1123. What happens if for example there is only one tEXt chunk which has a Creation Time in ISO 8601 format [1], e.g., "2017-06-07 15:50"? . Update: . The logic supports decoding combined Date Time based on RFC1123 and ISO Formats. . This has been realized with JDK's inbuilt DateTimeFormatter objects. If multiple Creation Time keywords are present the algorithm to decide which one to use as the ImageCreationTime in standard image metadata is somewhat arbitrary. One could for example use the value of the first one, the value of the last one, the oldest one, etc. . Update: . The logic uses the time retrieved from the last decoded text chunk (tEXt, iTXt, or zTXt) with Creation Time to initialize Standard/Document/ImageCreationTime. . Similarly, when metadata is updated with Standard/Document/ImageCreationTime, the same is encoded within the last decoded text chunk. What examples of actual PNG "real world" files have been used for testing? For example ones from PngSuite [2] . Update: . I checked the PNG files of the test suite. Unfortunately none contain ancillary text chunk with 'Creation Time' . Hence, I manually created a PNG file with tEXt chunk containing Creation Time to test the changes. Jay's Suggestions: In PNGMetadata. extractCreationTimeFromText() you are updating the "creation_time_present" to false when it doesn't follow RFC1123 . Update: . This has been corrected. . Please Note: The method name in the internal PNGMetadata class has been changed to decodedImageCreationTimeFromTextChunk I see that, if there are multiple text chunks following RFC1123 text we update our standard metadata node with last parsed chunk. This information should be covered properly in comments as it is specific to our implementation and it is not part of PNG specification. . Update: . This has been addressed. The logic is substantiated with comments. In the present iteration of code in PNGMetadata.encodeCreationTimeToText() we are updating the first available text chunk with the new information provided in stanrdardmetadata node. . Update: . Yes. This is a very good find. Thank you. . The logic has been corrected to update the encoded time in the last decoded text chunk. It's better to maintain /* */ format for multiple line comments then using multiple //. . Update: . I had initially used // to keep my changes in synch with the existing code that uses // for multiline comments. . I 've reverted to /* */ to comply with guidelines. Other Information: . The code changes have been run through Jtreg and JCK test suites. No new regressions were seen. . Besides, the test case available with the webrev is comprehensive and tests the following scenarios- . Proper decoding of text chunks with 'Creation Time' from the test image. . Proper decoding of time in RFC1123 and ISO format. (Test provides incorrect values as well and code doesn't crash) . Proper values in Native tree's text chunks after updating metadata with Standard/Document/ImageCreationTime. . Proper values in Standard/Document/ImageCreationTime after updating metadata with text chunks using Native tree. Kindly review the changes when your time permits and provide your feedback. Thank you Have a good day Prahalad N. --------------------------------------- From: Brian Burkhalter Sent: Thursday, June 08, 2017 4:20 AM To: Jayathirth D V Cc: Prahalad Kumar Narayanan; 2d-dev@openjdk.java.net Subject: Re: [OpenJDK 2D-Dev] [10] RFR: JDK-8164971: PNG metadata does not handle ImageCreationTime Hi Jay, On Jun 7, 2017, at 3:42 AM, Jayathirth D V <jayathirth....@oracle.com> wrote: As per PNG Specification for text chunks http://libpng.org/pub/png/spec/1.2/PNG-Chunks.html#C.Anc-text . There can be multiple chunks with same keyword. Yep: "Any number of text chunks can appear, and more than one with the same keyword is permissible." 1) In PNGMetadata. extractCreationTimeFromText() you are updating the "creation_time_present" to false when it doesn't follow RFC1123. So if there are multiple text chunks and if the latest chunk doesn't follow RFC1123 while decoding it will result in "creation_time_present" to be false. In case where we are not able to decode the provided text, we should not update "creation_time_present" to false. I have not looked at the code but what I am wondering about is how it handles this part of the specification from the same section: "For the Creation Time keyword, the date format defined in section 5.2.14 of RFC 1123 is suggested, but not required [RFC-1123]. Decoders should allow for free-format text associated with this or any other keyword." Specifically the date format is *recommended* as opposed to be *required* to conform to RFC 1123. What happens if for example there is only one tEXt chunk which has a Creation Time in ISO 8601 format [1], e.g., "2017-06-07 15:50"? If multiple Creation Time keywords are present the algorithm to decide which one to use as the ImageCreationTime in standard image metadata is somewhat arbitrary. One could for example use the value of the first one, the value of the last one, the oldest one, etc. What examples of actual PNG "real world" files have been used for testing? For example ones from PngSuite [2] or those produced by typical user applications such as Apple's "Preview" or the Windows program "IrfanView" [3] or other common image viewers? Thanks, Brian [1] https://en.wikipedia.org/wiki/ISO_8601#Combined_date_and_time_representations [2] http://www.schaik.com/pngsuite/pngsuite.html [3] https://en.wikipedia.org/wiki/IrfanView
