ok, will check

We're going to need more than this to conform to the spec when writing tags. (ref: http://www.remotesensing.org/libtiff/man/TIFFSetField.3tiff.html)

This winds up calling TIFFVSetField with variable args -- with the arguments for specific tags being the correct types as listed in the doc referenced. Unfortunately, it's not consistent where arrays are represented as ct, [number]. Some of them are [number] with a fixed length (e.g. TIFFTAG_PRIMARYCHROMATICITIES), some are ct, [number]* (e.g. TIFFTAG_EXTRASAMPLES).

What we're going to need to do to do this properly is to list the known tags & types, and make sure that we're emitting the right types and data for those. Additionally, we'd then know which are the unknown tags, which would then need to be dealt with separately by adding them to the tiff types dictionary prior to adding the tag. (#983, #660)

I gave in and just rewrote the whole thing.

Wow, that you did.

Repository health decreased by 0.79% when pulling a6a6dbe on anntzer:tiff-extra-samples into cdfddc8 on python-pillow:master.

• 26 new problems were found (including 7 errors and 18 code smells).
• 10 problems were fixed (including 0 errors and 6 code smells).

I just noticed your comment on #520 about incompatibilities. Do you have any specific suggestions? It is not clear to me how to keep full backwards compatibility there...

I don't have specific suggestions yet -- I haven't gotten in the headspace to go through and see what all is failing from the tests (since right now, they're fail fast, there could be many hiding behind the one failure), and even if the tests are actually covering what needs to be covered.

Breaking user's code is something that I don't want to do, at least without some deprecation warnings and a phase in over some time. (From a quick look, the two things that concern me are changing some of the test items -- but I need to look closer to see if that's changing internal implementation details, or changing the only way people had to get that information, and the changes in the way that the TiffTags are done, especially removing a huge chunk of tags that we did have in there. )

I think what I'd probably look at doing is building up the IFD info in whatever api and structure works for it, including supporting EXIF (since it's the same thing), then blessing the new api as the way to do it. I'd then go back and make the old api work in a best-effort / best-guess approach, building on the new, working core.

Repository health decreased by 0.76% when pulling b58fdaa on anntzer:tiff-extra-samples into cdfddc8 on python-pillow:master.

• 25 new problems were found (including 7 errors and 17 code smells).
• 10 problems were fixed (including 0 errors and 6 code smells).

The removal of a huge chunk of tags is simply due to the fact that I was too lazy to figure out the type of each of the tags, as this is now needed. This can easily be added back once we have decided on the actual API, which I agree is the main issue.
I had not noticed that some tests were failing, because in fact, trying to run all the tests at once fails locally, but I can run single test files so I'm just catching whatever Travis is complaining about, one file at a time. So now MPO should be fixed (basically, now, values for unknown tags are returned as tuples, so I needed to enter the MPO tags in the TiffTags database. (The other test was due to the use of dict comprehensions and implicit format string indexing, not supported by Python 2.6).

Changes Unknown when pulling d1e2db6 on anntzer:tiff-extra-samples into * on python-pillow:master*.

@anntzer Some tips.

Make full use of Travis CI to run the full test suite on all Python versions. I recommend enabling Travis (and Coveralls, seeing as you're making a big change) for your own account, so you can have your own branches fully tested. They're free for open source.

See https://github.com/python-pillow/Pillow/blob/master/CONTRIBUTING.md for guidelines.

If you want to temporarily disable fail-first and see all test failures, remove the x option in this line of .travis.yml:

coverage run --append --include=PIL/* -m nose -vx Tests/test_*.py

It'll need to be re-enabled when everything is all done and the PR is ready for merging.

Ideally all changed code should be covered by tests, and especially for big changes, it's wise to make sure there are specific tests for the functionality being changed.

Repository health decreased by 0.63% when pulling 3b0e0bf on anntzer:tiff-extra-samples into 264f5d0 on python-pillow:master.

• 958 new problems were found (including 76 errors and 729 code smells).
• 952 problems were fixed (including 71 errors and 724 code smells).

I don't really want to shoehorn the old API on top of the new one, so what about the following: have a global tag (e.g., TIFF_API = "2.7" or "2.8", the default value being obviously a policy issue rather than a code issue) that'll switch between either API? The tag would live in the PIL namespace, so that it can be checked for when TiffImagePlugin is imported for the first time.

@anntzer Any progress on this?

Did you see the Feb 25th commits? I was waiting for your opinion on the approach (providing a legacy_api attribute on IFD objects, defaulting to False).

Ah, thanks. We probably need @wiredfool or @hugovk to review.

Currently seeing failures here

Sorry, I rebased mostly just to get a new appveyor build (I guess something went wrong during the transition).
I knew that there would be some failures, as mentioned in my previous message. I would like to revert the behavior recently introduced in ed2cca1 which leads to loading corrupted exif data with just a warning rather than with an exception. IMHO, there is no reason to guess what the exif data should be when it is corrupted.
On the other hand I wouldn't mind raising an exception other than struct.error (which can legitimately considered to be an implementation detail).

Can someone reproduce Travis's failures locally? Despite my best attempts I cannot reproduce them (on Linux 64bit).

In theory I could "fix" the failures by replacing calls to assertEqual by calls to assertAlmostEqual (what the tests previously did, which amounts to admitting that the code cannot actually roundtrip the data correctly), but obviously it would be better to avoid that.

I am unable to reproduce on OS X.

I can reproduce on Ubuntu 12.04/x64. Changing the assert to

      self.assertEqual(val, value, msg="%s didn't roundtrip. %s, %s" % (tag, val, value))

yields:

Traceback (most recent call last):
  File "/home/erics/Pillow/Tests/test_file_libtiff.py", line 144, in test_write_metadata
    self.assertEqual(val, value, msg="%s didn't roundtrip. %s, %s" % (tag, val, value))
AssertionError: YResolution didn't roundtrip. ((4294967295, 113653537),), ((634011008, 16777216),)

They are roughly equivalent, but different representations.

Sorry, I won't have the time to work on this in the next few days. We could switch back to assertAlmostEqual (which as I mentioned earlier was already used by the earlier tests) for now...

The problem is that I have no idea of where (634011008, 16777216) comes from. It is not that hard to see that the value actually stored in the file is (4294967295, 113653537) (hex dump ffff ffff 2137 c606 towards the end of the file) and this is also the value I see if I print the values in the test.
Likewise these are the values that appear when running master (before any TIFF patching). Can you run the tests with the current master, and print the values at lines 142 and 153 of test_file_libtiff.py?

Half-fudging it, but not really: libtiff's precision for "rationals" is limited to C floats, not C doubles, so it should be enough to require roundtripping up to that precision.

Sad.

See #1419

Closing, merged #1419 which contains this branch.