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
