[OpenJDK 2D-Dev]  RFR: JDK-8164971: PNG metadata does not handle ImageCreationTime
Prahalad Kumar Narayanan
prahalad.kumar.narayanan at oracle.com
Thu Jun 8 03:10:13 UTC 2017
Hello Brian and Jay
Good day to you.
Thank you for your time and review suggestions.
Let me answer to each of your review suggestions here:
> 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 , e.g., "2017-06-07 15:50"?
. I understand the suggestion that ISO 8601 is also a valid representation for the 'time' data.
. Since there is no mention of using the ISO standard for time in the PNG Specification, I 'm not checking for this format in the encoded time data.
. If required, we can attempt to check if our DateTimeFormatter.ISO_* decode time from the String.
. But this only adds more complexity to the logic because tEXt chunks could contain date without the zone information Or date without time information etc.,
. I 'm not sure how our JDK's DateTimeFormatter.ISO_* work. I will check and get back.
> 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.
. Yes. There is no mention about the logic to deploy in the PNG specification.
. In the code changes, I 've used the last successfully decoded text chunk's information to set the value of Standard/ Document/ ImageCreationTime.
. However, the reverse isn't true. Once decoding of image is complete, any update to metadata using Standard/ Document/ ImageCreationTime node updates the first available text chunk and not the last successfully decoded text chunk. (A bug in code change that Jay has pointed out.)
> What examples of actual PNG "real world" files have been used for testing? For example ones from PngSuite 
. I manually created a PNG file with our Java logo in it and ImageCreationTime in the tEXt chunk.
. But this is a good suggestion to use a test suite. I will check with images in the test suite and update.
> In PNGMetadata. extractCreationTimeFromText() you are updating the "creation_time_present" to false when it doesn't follow RFC1123
. Yes. This will be corrected.
> From the PNGMetadata. extractCreationTimeFromText() implementation 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.
. I generally substantiate the logic with enough comments. Looks like I 've missed this one.
. Since API documentations are implementation-independent, I will not update comments as part of APIs but only internal implementation within the PNGMetadata.
. When I post the new webrev, this will be corrected.
> When an user adds new "ImageCreationTime" standard node which follows RFC1123 text to already present metadata, we should update the last text chunk from which we have captured creation time information while decoding
. A correction. The Standard/ Document/ ImageCreationTime does not need to follow RFC1123.
. The attributes are integers with year, month, day, hour, minutes, seconds.
. It's the text chunks in the native metadata tree that 'may' conform to RFC1123 standard.
> 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.
. Yes. This is a very good find. Thank you.
. I will be correct this in the next webrev.
> One more question is about how are we planning to maintain IIOMetadata consistency between multiple decoding & encoding sequences.
. Well, We discussed on this in-detail. For the benefit of others in community, let me put the findings here-
. Our PNG image writer writes all similar text chunks together. For Example: all tEXt chunks together, all iTXt chunks together etc.,
. So if user adds multiple text chunks with Creation Time, the ordering is not ensured at the time of encoding.
. Thus, we cannot guarantee consistency across the lifetime of decoder. (between multiple decode & encode sequences)
. Any attempt to provide consistency will resemble a HACK and a messy code, which I don't wish to do now.
. Besides, the PNG spec does not lay any logic to choose the creation time in these circumstances. Hence, I would prefer to leave the code as is.
> It's better to maintain /* */ format for multiple line comments then using multiple //.
. In this regard, I 've used the file's existing code-comment style so that changes are consistent and ensures code readability.
. Refer to Line 254 (existing code)
254 // tRNS chunk
255 // If external (non-PNG sourced) data has red = green = blue,
256 // always store it as gray and promote when writing
. In my view, code readability gets affected if the existing code // multi-line comments were preceded by comments like
238 * creation_time_present- Indicates that the native metadata contains
239 * the image creation time initialized in its variables. The flag is set
240 * true after successfully decoding the time represented as string in the
241 * text chunks or when user explicitly provides the creation time by
242 * manipulating the node tree.
. I would like to receive the suggestion from others as well on this before I continue to modify the comment style in my code changes.
Thank you once again for your time in review and detailed suggestions.
I will get back with changes soon.
Have a good day
From: Brian Burkhalter
Sent: Thursday, June 08, 2017 4:20 AM
To: Jayathirth D V
Cc: Prahalad Kumar Narayanan; 2d-dev at openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev]  RFR: JDK-8164971: PNG metadata does not handle ImageCreationTime
On Jun 7, 2017, at 3:42 AM, Jayathirth D V <jayathirth.d.v at 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 , 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  or those produced by typical user applications such as Apple's "Preview" or the Windows program "IrfanView"  or other common image viewers?
More information about the 2d-dev