[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-07-20 14:27:05
Message-ID: 6775871e-e09c-4000-b82b-133d79751076 () default
[Download RAW message or body]

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.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]