[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] [10] RFR: JDK-8164971: PNG metadata does not handle ImageCreationTime
From: Prahalad Kumar Narayanan <prahalad.kumar.narayanan () oracle ! com>
Date: 2017-06-08 3:10:13
Message-ID: 7db9e300-ee3c-4ff3-8794-13f4e36eab16 () default
[Download RAW message or body]
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.d.v@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
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic