-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Libtiff metadata writing fixes #2288
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
Conversation
ecf193a
to
98268b8
Compare
98268b8
to
c69500e
Compare
Last few commits are the start of 2 and 3, but there's an issue with the sampleformat due to the differences between how we've had it defined and in tiff files, and how it's in the api. Additionally, I think the failures on appveyor are due to a libtiff5 vs libtiff4 difference. Upshot is, I think I've got to handle most of this to do it right, and doing just 1 isn't going to cut it. |
int stride = 256; | ||
TRACE(("Setting ColorMap\n")); | ||
if (len != 768) { | ||
PyErr_SetString(PyExc_ValueError, "Requiring 768 items for for Colormap"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PyErr_SetString(PyExc_ValueError, "Requiring 768 items for for Colormap"); | |
PyErr_SetString(PyExc_ValueError, "Requiring 768 items for Colormap"); |
|
||
reloaded = Image.open(out) | ||
# colormap/palette tag | ||
self.assertTrue(len(reloaded.tag_v2[320]), 768) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.assertTrue(len(reloaded.tag_v2[320]), 768) | |
self.assertEqual(len(reloaded.tag_v2[320]), 768) |
280: ("MinSampleValue", LONG, 0), | ||
281: ("MaxSampleValue", SHORT, 0), | ||
280: ("MinSampleValue", LONG, 1), | ||
281: ("MaxSampleValue", SHORT, 1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why this change was made - https://www.awaresystems.be/imaging/tiff/tifftags/minsamplevalue.html and https://www.awaresystems.be/imaging/tiff/tifftags/maxsamplevalue.html
Tiff Metadata | ||
============= | ||
|
||
Pillow currently reads TIFF metadata in pure python and writes either |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pillow currently reads TIFF metadata in pure python and writes either | |
Pillow currently reads TIFF metadata in pure Python and writes either |
324: ("TileOffsets", LONG, 0), | ||
325: ("TileByteCounts", LONG, 0), | ||
|
||
330: ("SubIFD", SHORT, 1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#6569 added this tag, but as "SubIFDs", LONG
and 0
, which matches https://web.archive.org/web/20240329145255/https://www.awaresystems.be/imaging/tiff/tifftags/subifds.html
340: ("SMinSampleValue", DOUBLE, 0), | ||
341: ("SMaxSampleValue", DOUBLE, 0), | ||
340: ("SMinSampleValue", DOUBLE, 1), | ||
341: ("SMaxSampleValue", DOUBLE, 1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why these changes were made -
https://web.archive.org/web/20240329145340/https://www.awaresystems.be/imaging/tiff/tifftags/sampleformat.html
https://web.archive.org/web/20240329145243/https://www.awaresystems.be/imaging/tiff/tifftags/sminsamplevalue.html
https://web.archive.org/web/20240329145243/https://www.awaresystems.be/imaging/tiff/tifftags/smaxsamplevalue.html
32995:("Matteing", SHORT, 1), | ||
32996:("DataType", SHORT, 1), | ||
32997:("ImageDepth", LONG, 1), | ||
32998:("TileDepth", LONG, 1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at https://gitlab.com/libtiff/libtiff/-/blob/master/libtiff/tif_dirinfo.c?ref_type=heads#L136, I've concluded "DataType" should be 0
rather than 1
.
I've created #9245 to add these four tags.
LIBTIFF_CORE.remove(320) # Array of short, crashes | ||
LIBTIFF_CORE.remove(301) # Array of short, crashes | ||
LIBTIFF_CORE.remove(532) # Array of long, crashes | ||
LIBTIFF_CORE.remove(330) # subifd, requires extra support for uint64 payload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that #5120 blocked the tag when saving with libtiff, I don't think this is needed anymore.
PIL/TiffTags.py
Outdated
|
||
LIBTIFF_CORE.remove(320) # Array of short, crashes | ||
LIBTIFF_CORE.remove(301) # Array of short, crashes | ||
LIBTIFF_CORE.remove(532) # Array of long, crashes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were since removed in #5384
All of the tests added here now pass in main. I've created #9276 to add the last functionality that I think this adds. |
Fixes #1765, #1147, #1597 (among others, likely)
Libtiff's metadata support is tricky and dangerous. This patch is 1/3 of the work to clean up our support for it.
Understand and support writing basic metadata through libtiff. This includes casting numbers to the correct format and passing in arrays correctly for known items. (e.g., this fixes Save compressed TIFF only accepts float values for dpi #1765, writing ints to dpi, and writing the colormap for
P
mode images)Consider supporting custom metadata tags safely. This is going to require reflection from libtiff to ensure that we're setting items correctly.
Coordinate this work with the python metadata implementation in two places: When reading images where the type conflicts with the spec, and when writing (in python) metadata of incorrect types.
While this is not finished, I believe that this is a material improvement in the support for tiff metadata, and can be merged (after I'm happy and rebased out some things) for 3.5.