Skip to content

Tiff ImageFileDirectory Rewrite#1419

Merged
wiredfool merged 38 commits intopython-pillow:masterfrom
wiredfool:tiff-ifd-rewrite
Sep 20, 2015
Merged

Tiff ImageFileDirectory Rewrite#1419
wiredfool merged 38 commits intopython-pillow:masterfrom
wiredfool:tiff-ifd-rewrite

Conversation

@wiredfool
Copy link
Member

@wiredfool wiredfool commented Sep 11, 2015

This is my take on #1059, which is a pretty full rewrite of the Tiff ImageFileDirectory for correctness. This is not complete, as there are still several issues that need to be addressed.

I had several issues with the PR, which I've attempted to fix here.

  • The implementation of the legacy api did not pass the tests out of the box on current master
  • Remerge/rebase was required
  • Bad Exif tolerance was removed at one point.
  • TiffTags api has changed

This PR:

  • Includes Fix setting of TIFF ExtraSamples tag #1059
  • Renames all accesses to the new api to _v2 suffixes
  • Keeps a shell that works with the original unversioned interface
  • Restores/adds checks for read length that would result in struct errors otherwise
  • Versions the TiffTags data and retains legacy tags without type information
  • Most of the current master branch tests pass, with these exceptions:
    • test_file_tiff.py:test_page_number_x_0 segfaults
    • test_file_tiff.py:test_xyres_tiff had a portion where integer resolution was tested, this is stubbed out.
    • There are two tests that don't have the im.tag.tags[tagname] populated, where they were before. This is arguably an internal interface that shouldn't be tested. If the tag had been read, it would be populated, and would be accessible. (note that this interface hasn't been documented as being private for very long)
    • There are several tests that test implementation specific details that can be dropped now.

Needs

  • One test has been disabled due to a segfault: test_file_tiff.py:test_page_number_x_0
  • test_file_libtiff is segfaulting
  • One test has less coverage than it did: test_file_tiff.py:test_xyres_tiff
  • Rebase
  • Documentation of the appropriate interfaces to use.
  • TiffTags
  • test_file_tiff_metadata.py
  • lingering test failures when run against master's test suite

My intention is that this land before 3.0.0. Please test and comment.

@wiredfool wiredfool added this to the 3.0.0 milestone Sep 11, 2015
@wiredfool wiredfool mentioned this pull request Sep 11, 2015
Closed
anntzer and others added 23 commits September 11, 2015 22:26
@anntzer @wiredfool
Fix setting of TIFF ExtraSamples tag.
aba7a34 
- force cast ExtraSamples to a list.
- fix calls to ImagingLibTiffSetField to include array length.
@anntzer @wiredfool
Major rewrite of TIFF ImageFileDirectory.
974bcc0 
Do not represent scalar tags as 1-element tuples.  Keep tag
type and count information in TiffTags.TAGS.  Normalize data in
ImageFileDirectory.__setitem__: wrap and unwrap tuples as needed,
convert rationals to floats.  (To ensure consistency, make the "tags"
attribute private.)  Interpret byte data as a series of integers rather
than a bytearray (which should only map to the "undefined" type).  On
Python3, if a str is assigned to an "undefined" tag, encode it as ASCII.

Note that a large number of tags have been removed from TiffTags.TAGS
because I do not have time to figure out the type and count of each of
them.  They should be restored before this gets merged in.

This obviously breaks backwards compatibility in a lot of ways...
@anntzer @wiredfool
Fix EXIF support.
56a3f0f
@anntzer @wiredfool
Fix MPO support, and Python2.6 support.
d5b46dc
@anntzer @wiredfool
Include tests of python-pillow#1113.
38f7e23
@anntzer @wiredfool
Restore legacy TIFF API.
93abbd0 
To have the old API that always returns tuples, and fractions as pairs,
set the `legacy_api` attribute of the IFD to True.

This should alleviate concerns about backwards compatibility.
@anntzer @wiredfool
Support too long ExtraSamples.
6309bfe 
Some programs generate SamplesPerPixel entries in ExtraSamples instead
of SamplesPerPixel-3, cf. python-pillow#1227.  This is a stopgap measure to support
them.  One could also decide to add generic code to always support
having SamplesPerPixel entries (by dropping the first 3).
@anntzer @wiredfool
Reorder OPEN_INFO to cover both endiannesses.
1b9b374 
By interleaving little and big-endian entries we make sure entries
exist for both cases.  Some additional entries created when the
big-endian was missing.  I am not sure of what entry to create for the
big-endian, 4-bit case (what is the order of the two entries within the
byte?).
@anntzer @wiredfool
Fail on invalid EXIF, reverting ed2cca1.
ba7b8d1
@anntzer @wiredfool
libtiff's rational precision is limited to C floats.
c113e8f
@wiredfool
fixed the rebase
0c94291
@wiredfool
module level default api level
9bb4c51
@wiredfool
Legacy/versioned interface
47a963c
@wiredfool
Restoring bad exif handling
5f9fff0
@wiredfool
test failing update
426c9d8
@wiredfool
In a twisty maze of bytes, text and arbitrary metadata, py2 and py3. …
b1fdff4 
…New IFD is putting textdata in type7 metadata and returning bytes, old one put it in type 2 string and returned a string. This may be an issue
@wiredfool
rewrite of python-pillow#1416 working
0c35194
@wiredfool
missed a set of _v2 versioning
1569728
@wiredfool
convert load_* functions to pure functions with no state
a9396ab
@wiredfool
v1/v2 tag storage in IFD, legacy_api as a parameter to _saveitem, sav…
e1236d7 
…e both _tags_v* when saving as legacy api
@wiredfool
updating tests for legacy_api api change
2636679
@wiredfool
Added doc comment
bb75b2d
@wiredfool
Rewrap intelligently
ca24a44
@wiredfool
Copy link
Member Author

wiredfool commented Sep 14, 2015

We're down to two classes of test failure against current master (f451e40).

This was an internal implementation detail that's been removed:

ERROR: TestFileTiff.test__cvt_res_float
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/erics/pillow-temp/Tests/test_file_tiff.py", line 326, in test__cvt_res_float
    from PIL.TiffImagePlugin import _cvt_res
ImportError: cannot import name _cvt_res

This test is looking into the cached, parsed tag dict, not using the public interface to populate that dict.

ERROR: TestFileTiff.test_deprecation_warning_with_spaces
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/erics/pillow-temp/Tests/test_file_tiff.py", line 408, in test_deprecation_warning_with_spaces
    self.assertEqual(im.tag.tags[X_RESOLUTION][0][0], 36)
KeyError: 282

I'm calling this good for lingering test failures.

@wiredfool
Copy link
Member Author

wiredfool commented Sep 14, 2015

Ok, I think I'm done here, pending anything else showing up.

wiredfool added a commit that referenced this pull request Sep 20, 2015
@wiredfool
Merge pull request #1419 from wiredfool/tiff-ifd-rewrite
de34547 
Tiff ImageFileDirectory Rewrite
@wiredfool wiredfool merged commit de34547 into python-pillow:master Sep 20, 2015
@radarhere radarhere mentioned this pull request Sep 20, 2015
Closed
@jensens jensens mentioned this pull request Oct 7, 2015
Merged
@radarhere radarhere mentioned this pull request Mar 25, 2016
Closed
@wiredfool wiredfool deleted the tiff-ifd-rewrite branch January 8, 2018 12:11
@radarhere radarhere mentioned this pull request Feb 8, 2019
Closed
This was referenced Aug 8, 2019
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Needs Review

Projects

None yet

Milestone

3.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@wiredfool @anntzer @homm