Add support for png_text metadata, allow to customize metadata for other backends.

[Xarthisius]

PNG Specification allows for custom png_text structures as an image attributes. The keywords that are given in the PNG specs are:

    Title            Short (one line) title or
                     caption for image
    Author           Name of image's creator
    Description      Description of image (possibly long)
    Copyright        Copyright notice
    Creation Time    Time of original image creation
                     (usually RFC 1123 format, see below)
    Software         Software used to create the image
    Disclaimer       Legal disclaimer
    Warning          Warning of nature of content
    Source           Device used to create the image
    Comment          Miscellaneous comment; conversion
                     from other image format

This PR allows to pass a dictionary with a metadata that can be used to fill those key/value pairs in png writer. Structure of the data and its type is not validated.

Additionally this PR allows to customize "software/creator" values in other backends such as PDF and PS.

[QuLogic]

Instead of replacing the entire metadata, I'd have the argument update the existing dictionary that was originally generated in the code. That way, the user doesn't need to figure out the default strings (version info and such), but can still provide other metadata or override it if really wanted.

[tacaswell]

I agree with @QuLogic that user supplied data should update the default data, not completely replace it.

@mdboom should check the c, but this looks reasonable to me otherwise.

This will need and entry in whats_new.

[tacaswell]

Please update the existing meta-data rather than completely replacing it with the user supplied metadata.

[QuLogic]

A few small issues.

[QuLogic]

This would be confusing for users, as they would probably provide regular strings (as that's what all the other ones accept.) If bytestrings are really required here, it should do the encoding after obtaining the user's metadata.

[QuLogic]

You also don't need six.b because we don't need to support Python 2.5.

[QuLogic]

Unnecessary change, looks like.

[QuLogic]

Brace is on the wrong line (comparatively speaking.)

[QuLogic]

Should be 4-space indent.

[tacaswell]

Concern has been addressed.

[QuLogic]

I believe it's actually (some small subset of) latin1/iso-8859-1.

[NelleV]

I don't find this documentation particularly useful. You could you add at least some examples of metadata ?
Also, numpydoc format requires a space before the column : metadata : dictionary, …

[QuLogic]

The original parameters all don't have a space before the colon, and rendered fine, so I don't think that's a requirement.

[NelleV]

It is. The current documentation is not rendered properly.

[QuLogic]

Oh yes, I missed that the bolding is wrong.

[tacaswell]

Mild preference for adding metadata=None to the signature.

[tacaswell]

This pop needs to be removed?

[tacaswell]

err, sorry, there were two of these and I grew confused reading the diffs.

[tacaswell]

Can you add metadata to https://github.com/matplotlib/matplotlib/blob/master/src/_png.cpp#L86 ?

[tacaswell]

Sorry in advance if I am asking obvious questions, just trying to understand the libpng API.

From http://www.libpng.org/pub/png/libpng-manual.txt

    png_set_text(png_ptr, info_ptr, text_ptr, num_text);

    text_ptr       - array of png_text holding image
                     comments

    text_ptr[i].compression - type of compression used
                 on "text" PNG_TEXT_COMPRESSION_NONE
                           PNG_TEXT_COMPRESSION_zTXt
                           PNG_ITXT_COMPRESSION_NONE
                           PNG_ITXT_COMPRESSION_zTXt
    text_ptr[i].key   - keyword for comment.  Must contain
                 1-79 characters.
    text_ptr[i].text  - text comments for current
                         keyword.  Can be NULL or empty.
    text_ptr[i].text_length - length of text string,
                 after decompression, 0 for iTXt
    text_ptr[i].itxt_length - length of itxt string,
                 after decompression, 0 for tEXt/zTXt
    text_ptr[i].lang  - language of comment (NULL or
                         empty for unknown).
    text_ptr[i].translated_keyword  - keyword in UTF-8 (NULL
                         or empty for unknown).

    Note that the itxt_length, lang, and lang_key
    members of the text_ptr structure only exist when the
    library is built with iTXt chunk support.  Prior to
    libpng-1.4.0 the library was built by default without
    iTXt support. Also note that when iTXt is supported,
    they contain NULL pointers when the "compression"
    field contains PNG_TEXT_COMPRESSION_NONE or
    PNG_TEXT_COMPRESSION_zTXt.

• Do we also need to set text_length?
• From these docs it is not clear to me that setting the key to NULL is allowed.
• Do we need to check the length of the key?

I can not quite sort out the difference between tEXt, TEXT, and iTXt in the docs. It seems we are using tEXt (due to the compression flag set) which seems like a good idea as the iTXt has a bunch of comments about bugs that result in non-readable pngs with some versions of libpng.

This is valuable work thank you again for taking this on!

[Xarthisius]

Do we also need to set text_length?

AFAICT, it's totally ignored. There's an explicit strlen in png_set_text that populates text_length in the struct.

From these docs it is not clear to me that setting the key to NULL is allowed.

I'm not sure how to interpret the standard either. In libpng there's explicit:

if (text_ptr[i].key == NULL)
    continue;

in the loop over text_ptr.

Do we need to check the length of the key?

It's checked internally. If the key exceeds 79 it is truncated and warning is issued:

libpng warning: keyword truncated

[tacaswell]

It makes sense that it takes care of the length it's self (so we can not lie to it, that seems like an obvious attack vector).

I am a tad concerned about the NULL and that behavior may be a libpng implementation detail. Might be safer to go with 'BAD KEY N' or something like that? Keys can be repeated, but seems safer to make them unique (as coming from a python dictionary they will be unique on the way in).

Might be worth turning that warning into a python warning? I am 👍 with punting on that for this PR.

[Xarthisius]

ping, is there anything else left to do before this PR is merged?

[efiring]

This looks like a good contribution, but it would be nice to have @mdboom look at the agg part and @jkseppan look at the pdf and pd parts.

As @tacaswell noted, it will need a whats-new entry.

I have a question about the API and behavior: it looks like the present scheme, in which metadata comes in as a dictionary, has the undesirable side effect of making the order in which the metadata appear in the file non-deterministic (and at the very least, not under the control of the user). This could be avoided by using an OrderedDict, or by using a sequence of (key, value) tuples. Correct?

[Xarthisius]

As @tacaswell noted, it will need a whats-new entry.

Sorry, it wasn't clear who's supposed to add it. I'll work on it.

I have a question about the API and behavior: it looks like the present scheme, in which metadata comes in as a dictionary, has the undesirable side effect of making the order in which the metadata appear in the file non-deterministic (and at the very least, not under the control of the user). This could be avoided by using an OrderedDict, or by using a sequence of (key, value) tuples. Correct?

In principle you are right. However, since there are no methods to read text value by its index why would it matter? I would say that random order is desirable side effect. It will prevent people from writing software that makes ill assumptions about metadata.

[tacaswell]

On the bright side, with python 3.6 dictionary order is deterministic again!

[efiring]

I know zilch about metadata in image files, but I am guessing that utilities dump the metadata in the order in which it is found, e.g. for display to the screen, in which case it would be nice to control the order--wouldn't it?

Regarding deterministic vs non-deterministic, see #6317 for a discussion.

[codecov-io]

Current coverage is 62.07% (diff: 81.81%)

Merging #7349 into master will decrease coverage by <.01%

@@             master      #7349   diff @@
==========================================
  Files           174        174          
  Lines         56007      56021    +14   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          34765      34773     +8   
- Misses        21242      21248     +6   
  Partials          0          0          

Powered by Codecov. Last update e73625e...935e02f

[tacaswell]

'power cycled' to restart against current master

[NelleV]

Hi @Xarthisius
Do you mind rebasing? I think this is ready to be merged. Sorry it took so long.
Thanks,
N

[tacaswell]

@Xarthisius Thanks!

Sorry this took so long to get merged.