Add EXIF write support to PNG output. #3736
Conversation
Stores any Exif data from the given ImageSpec into the PNG eXIf chunk.
|
If you think this contribution is valuable, could you give some guidance on:
|
| @@ -684,6 +684,13 @@ write_info(png_structp& sp, png_infop& ip, int& color_type, ImageSpec& spec, | |||
| (png_uint_32)(yres * scale), unittype); | |||
| } | |||
|
|
|||
| #ifdef PNG_eXIf_SUPPORTED | |||
There was a problem hiding this comment.
What determines if PNG_eXIf_SUPPORTED is found? Is it always present in sufficiently new libpng? (If so, how new? Is it always present based on a minimum version that we should try enforcing?)
There was a problem hiding this comment.
Indeed libpng >= 1.6.32 added support for the eXIf chunk in PNG and defines PNG_EXIf_SUPPORTED. To be honest, I'm not entirely sure if there is a way to build libpng >= 1.6.32 without this support. I took the same approach as is used elsewhere in this file for reading eXIf data.
| #ifdef PNG_eXIf_SUPPORTED | ||
| std::vector<char> exifBlob; | ||
| encode_exif(spec, exifBlob, endian::big); | ||
| png_set_eXIf_1(sp, ip, static_cast<png_uint_32>(exifBlob.size()), |
There was a problem hiding this comment.
Should we also be using png_get_eXIf functions for the reading, or is what we have sufficient?
There was a problem hiding this comment.
Looks like this is already the case: https://github.com/OpenImageIO/oiio/blob/master/src/png.imageio/png_pvt.h#L326-L336
There was a problem hiding this comment.
Indeed, OIIO seems to read Exif from PNG from the eXIf chunk and from embedded text (unofficial convention from before the eXIf chunk existed).
|
whether a test should be added somewhere -- yes, it would be helpful to test this. I think the best approach would be to augment testsuite/png/run.py to just write a simple small png with some Exif metadata, then print the metadata for the file you wrote and make sure it matches what you think you set? I can help you with this if you aren't quite sure what to do in those scripts. whether a CLA is required for a change this small (I did do this on company time) -- that mostly depends on the arrangement you have with your company. For a change this small, the OIIO project doesn't insist on a CLA. But you know more about the relationship with your employers. If you think there could be any dispute and you want to get it in writing that it was ok for you to contribute this code, then by all means have them fill out a CLA (more for your safety than ours). But if you are confident that company policy is that you are free to make contributions to open source projects like this one, then we'll take it at face value and won't require a CLA. |
|
I added a test to the png test suite as you suggested. Please let me know if you want this to be extended more. |
|
It turns out that for many of our CI tests (which purposely span many versions of various dependencies), the ones running in CentOS7-based containers are using an older version of libpng that doesn't support this feature! So the new testsuite/png/ref/out.txt now no longer matches the output of those tests. To fix it, restore the original out.txt and name it out-libpng15.txt (leaving out.txt being the correct output for the newer libpng varieties). The testsuite scripts will know that a test can pass if it matches any of the reference outputs. |
|
CI is still failing, but I know why, it's simple. I'm going to merge this and then just immediately amend the fix myself. Thanks for the patch! |
* Add Exif write support to PNG output. Stores any Exif data from the given ImageSpec into the PNG eXIf chunk. * add test for writing Exif data in png * add test reference output for libpng versions not supporting the eXIf chunk. * Update testsuite/png/ref/out-libpng15.txt Co-authored-by: Joris Nijs <joris.nijs@esko.com>
Description
Stores any EXIF data from the given ImageSpec into the PNG eXIf chunk.
Tests
Checklist:
If this is more extensive than a small change to existing code, IChange is small enough to not require a CLAhave previously submitted a Contributor License Agreement
(individual, and if there is any way my
employers might think my programming belongs to them, then also
corporate).
I have updated the documentation, if applicable.Not applicable I think(adding new test cases if necessary).