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:
Brian:
> 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"?
. 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 [2]
. 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.
Jay:
> 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
237 /*
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.
243 */
. 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.
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
