Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Write out proper tiff header version in png EXIF blobs #3984

Merged
merged 2 commits into from Sep 13, 2023

Conversation

jessey-git
Copy link
Contributor

Description

When writing out EXIF headers in PNG files, the tiff_version field wasn't handling endianness properly.

This lead to some tools like exiv2 refusing to process the file at all[1]

[1] Exiv2/exiv2#2745

Checklist:

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have ensured that the change is tested somewhere in the testsuite
    (adding new test cases if necessary).
  • If I added or modified a C++ API call, I have also amended the
    corresponding Python bindings (and if altering ImageBufAlgo functions, also
    exposed the new functionality as oiiotool options).
  • My code follows the prevailing code style of this project. If I haven't
    already run clang-format before submitting, I definitely will look at the CI
    test that runs clang-format and fix anything that it highlights as being
    nonconforming.

Signed-off-by: Jesse Yurkovich <jesse.y@gmail.com>
@jessey-git
Copy link
Contributor Author

There is one additional quirk which I have not been able to find the cause of. exiftool -v3 will complain about what looks like a duplicate set of eXIf headers in the png:

From exiftool: Warning = IFD0 pointer references previous IFD0 directory

From exiv2:
STRUCTURE OF PNG FILE: T:\png_exif.png
 address | chunk |  length | data                           | checksum
       8 | IHDR  |      13 | ............                   | 0x3cafe9a7
      33 | eXIf  |      54 | MM.*.......,.......H.......... | 0x37db1a45    <-- XResolution\YResolution TIFF exif data
      99 | oFFs  |       9 | ........                       | 0xda2ab6ce
     120 | pHYs  |       9 | ...,...H                       | 0x73db2dc6
     141 | IDAT  |      29 | ..cdhhh`@..`.....4.......%...  | 0xd095bfc8
     182 | eXIf  |      54 | MM.*.......,.......H.......... | 0x37db1a45    <-- Why has it been duplicated??
     248 | IEND  |       0 |                                | 0xae426082

Signed-off-by: Jesse Yurkovich <jesse.y@gmail.com>
@jessey-git
Copy link
Contributor Author

I believe this is ready for review. The "duplication" problem noted is not critical and can be solved separately if no one happens to have a good solution to checkin along side the primary fix.

@brechtvl
Copy link
Contributor

I'm not sure that duplication is an actual problem. That header follows the calls to png_set_IHDR, png_set_oFFs, png_set_pHYs in png_pvt.h. It seems like the image resolution and physical resolution are equal which seems valid?

@jessey-git
Copy link
Contributor Author

Wow, I might be getting extremely unlucky. We're using libpng 1.6.39 (Nov 2022) and there's an interesting fix in 1.6.40 which might explain the duplication at least. https://sourceforge.net/p/libpng/code/ci/e6c5bf46c4fa3e3738a2ed0896ce33fc270e0cac/

Ensure that only one eXIf chunk is written in the entire datastream

EXIF data can be stored in an eXIf chunk before IDAT, or after IDAT,
but the entire PNG datastream may contain one eXIf chunk at most. ...

I haven't found good information about the IFD0 pointer references previous IFD0 directory warning though - just that the exiv2 authors said it's a real issue in the first report. I could see if bumping the libpng version helps, it'll just take a bit of time to rebuild the world.

@lgritz Does the primary fix here seem ok though?

Copy link
Collaborator

@lgritz lgritz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lgritz lgritz merged commit c1d8fe6 into AcademySoftwareFoundation:master Sep 13, 2023
23 checks passed
lgritz pushed a commit to lgritz/OpenImageIO that referenced this pull request Sep 16, 2023
…demySoftwareFoundation#3984)

When writing out EXIF headers in PNG files, the `tiff_version` field
wasn't handling endianness properly.

This lead to some tools like `exiv2` refusing to process the file at
all[1]

[1] Exiv2/exiv2#2745

---------

Signed-off-by: Jesse Yurkovich <jesse.y@gmail.com>
lgritz pushed a commit to lgritz/OpenImageIO that referenced this pull request Sep 20, 2023
…demySoftwareFoundation#3984)

When writing out EXIF headers in PNG files, the `tiff_version` field
wasn't handling endianness properly.

This lead to some tools like `exiv2` refusing to process the file at
all[1]

[1] Exiv2/exiv2#2745

---------

Signed-off-by: Jesse Yurkovich <jesse.y@gmail.com>
@jessey-git jessey-git deleted the fixpngexif branch September 26, 2023 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants