Tiff seek by wiredfool · Pull Request #873 · python-pillow/Pillow

wiredfool · 2014-08-26T17:36:03Z

This is a cleaned up, single issue version of #865.

It's having some test issues OMM which go away when the tests are run one file at a time in debug mode, so something is likely subtly wrong.

When run as ./test-installed.py

======================================================================
FAIL: TestFileTiff.test_big_endian
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/erics/Pillow/Tests/test_file_tiff.py", line 96, in test_big_endian
    self.assertEqual(im.getpixel((0, 0)), 480)
AssertionError: 0 != 480

======================================================================
FAIL: TestFileTiff.test_little_endian
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/erics/Pillow/Tests/test_file_tiff.py", line 81, in test_little_endian
    self.assertEqual(im.getpixel((0, 0)), 480)
AssertionError: 0 != 480

======================================================================
FAIL: TestImageConvert.test_16bit
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/erics/Pillow/Tests/test_image_convert.py", line 44, in test_16bit
    self._test_float_conversion(im)
  File "/home/erics/Pillow/Tests/test_image_convert.py", line 36, in _test_float_conversion
    self.assertEqual(orig, converted)
AssertionError: 0 != 447.0

They don't fail when only running the one test file under debug mode:

./test-installed.py -v -s Tests/test_file_tiff.py
*** TiffImageFile._open ***
- __first: 8200
- ifh:  MM* 
tag: ImageWidth (256) - type: short (3) - value: (64,)
tag: ImageLength (257) - type: short (3) - value: (64,)
tag: BitsPerSample (258) - type: short (3) - value: (16,)
tag: Compression (259) - type: short (3) - value: (1,)
tag: PhotometricInterpretation (262) - type: short (3) - value: (1,)
tag: FillOrder (266) - type: short (3) - value: (1,)
tag: DocumentName (269) - type: ascii (2) - value: 12bit.MM.cropped.tif
tag: StripOffsets (273) - type: long (4) - value: (8,)
tag: Orientation (274) - type: short (3) - value: (1,)
tag: SamplesPerPixel (277) - type: short (3) - value: (1,)
tag: RowsPerStrip (278) - type: short (3) - value: (64,)
tag: StripByteCounts (279) - type: long (4) - value: (8192,)
tag: XResolution (282) - type: rational (5) - value: ((1207959552, 16777216),)
tag: YResolution (283) - type: rational (5) - value: ((1207959552, 16777216),)
tag: PlanarConfiguration (284) - type: short (3) - value: (1,)
tag: ResolutionUnit (296) - type: short (3) - value: (2,)
tag: PageNumber (297) - type: short (3) - value: (0, 1)
*** Summary ***
- compression: raw
- photometric_interpretation: 1
- planar_configuration: 1
- fill_order: 1
- size: (64, 64)
format key: ('MM', 1, 1, 1, (16,), ())
- raw mode: I;16B
- pil mode: I;16B
tiles:  [('raw', (0, 0, 64, 64), 8, ('I;16B', 0, 1))]
Are we generating the same interpretation ... ok
TestFileTiff.test_32bit_float ... ok
TestFileTiff.test___str__ ... ok
TestFileTiff.test__cvt_res_float ... ok
TestFileTiff.test__cvt_res_int ... ok
TestFileTiff.test__cvt_res_sequence ... ok
TestFileTiff.test__delitem__ ... ok
TestFileTiff.test_big_endian ... ok
TestFileTiff.test_gimp_tiff ... ok
TestFileTiff.test_little_endian ... ok
TestFileTiff.test_load_byte ... ok
TestFileTiff.test_load_double ... ok
TestFileTiff.test_load_float ... ok
TestFileTiff.test_load_string ... ok
TestFileTiff.test_mac_tiff ... ok
TestFileTiff.test_multipage ... ok
TestFileTiff.test_multipage_last_frame ... ok
TestFileTiff.test_sanity ... ok
TestFileTiff.test_seek ... ok
TestFileTiff.test_seek_eof ... ok
TestFileTiff.test_xyres_tiff ... ok

----------------------------------------------------------------------
Ran 21 tests in 0.128s

OK

…reusing palettes and image sizes

coveralls · 2014-08-26T17:55:33Z

Coverage decreased (-0.71%) when pulling 633660f on wiredfool:tiff-seek into 0c94adc on python-pillow:master.

coveralls · 2014-08-27T03:24:26Z

Coverage increased (+0.01%) when pulling 40e5265 on wiredfool:tiff-seek into 0c94adc on python-pillow:master.

hugovk · 2014-08-27T06:38:20Z

@wiredfool Looks good. Ready for merge?

wiredfool · 2014-08-27T14:47:56Z

Actually, It's only passing the test when debugging is on (OMM). There's a really screwy failure where fp.tell is giging a negative number, so that when we seek to it later, it's an error. Reading the opsition before hand seems to be enough that it doesn't fail. I need to take another pass through it to see if there's anything that I'm missing.

kostrom · 2014-08-27T15:18:23Z

Is it returning -1 for error? I think tell() should not give an error unless files were opened with 'r' on windows, which is not the case here.

There aren't any 2gig tiff files so the ifdoffset gets turned into a bad signed offset in decode.c, because I didn't know how to pass an unsigned int through PyArg_ParseTuple. There are no such files in the test suite, so that can't be it.

In libImaging/TiffDecode.c, filename = "tempfile.tif" is just a descriptive string for a memory buffer, I think, so it is not actually reusing the same physical temp file.

None of those seem like a problem. Since it is release only, check to see if the new value, clientstate->ifd is initialized to zero everywhere. Maybe the negative offset you are seeing is not from fp.tell() but is an uninitialized ifdoffset in the TiffDecode.c

Maybe old code is calling PyImaging_LibTiffDecoderNew with an args tuple that does not include the ifdoffset. I don't know if that would throw an exception for too few arguments, or if it will give you an undefined value for the last arg. No -- that would fail in debug, too.

wiredfool · 2014-08-27T16:04:23Z

It's not -1, it's numbers like -380 or -590. Only on Py3, only when Image.DEBUG is false.

It's failing in reading the IFD, called from TiffImagePlugin:_seek. This is in the python only portion of the code, so the libtiff C level changes aren't even part of it.

I've pushed the imagesequence test so that it runs both with Image.DEBUG on successfully and fails when it's off. We'll see what travis thinks of it. (though, I'm running on a system that's 99% like travis, so I'm expecting the same thing)

kostrom · 2014-08-27T16:51:15Z

How is it different from the previous code?
Is the initial ifdoffset working as expected
self.offset = fp.tell()
and then it has problems calling tell again to parse the ifd entries?

wiredfool · 2014-08-28T16:52:12Z

When I've gone through it in the debugger, I'm getting the first IFD ok, then the second fails. I think I'll make a minimal test case and go through each commit looking for where it starts to fail that way.

kostrom · 2014-08-28T18:31:28Z

I'm caught up on reproducing this on python3

have fileno, calling fileno version of the decoder.
Seeking to frame 1, on frame 0, __next 284, location: -808

kostrom · 2014-08-28T21:07:36Z

I made a separate flag
TiffImagePlugins.DEBUG_OVERRIDE
and verified that the test passes with
Image.DEBUG = False and TiffImagePlugins.DEBUG_OVERRIDE = True

now I will try eliminating side effects, or just commenting out debug messages, in case there is some sort of timing issue with python3's io

kostrom · 2014-08-28T21:22:31Z

in TiffImageFile._seek(self, frame)
one of the debug messages has a side effect of calling fp.tell()

if I just call self.fp.tell() immediately before calling self.fp.seek(self.__next) then everything is magically fixed.

kostrom · 2014-08-28T21:33:58Z

def _seek(self, frame):
    self.fp = self.__fp
    if frame < self.__frame:
        # rewind file
        self.__frame = -1
        self.__next = self.__first
    while self.__frame < frame:
        if not self.__next:
            raise EOFError("no more images in TIFF file")
        # stabilize python3 by calling fp.tell()
        current_location = self.fp.tell()
        if Image.DEBUG:
            print("Seeking to frame %s, on frame %s, __next %s, location: %s"%
                  (frame, self.__frame, self.__next, current_location))
        self.fp.seek(self.__next)
        if Image.DEBUG:
            print("Loading tags, location: %s"%self.fp.tell())
        self.tag.load(self.fp)
        self.__next = self.tag.next
        self.__frame += 1
    self._setup()

wiredfool · 2014-08-29T17:42:25Z

Yeah. That's kind of where I ended up. Apparently fp.tell has side effects on py3.

I hate spooky side effects.

kostrom · 2014-08-29T18:36:14Z

I'm submitting it as a possible bug to python3, as soon as I get registered. I have a test script that shows the basics of what's happening. I'll link the bug report shortly.

Go ahead and get this merged when it passes, and I will start on the tiff tag issues, now that I know how to contribute and test properly.

kostrom · 2014-08-29T19:21:05Z

I couldn't register at python's bug tracker, so
http://stackoverflow.com/questions/25574575/is-this-a-python-3-file-bug

It is because of python's buffered io.
It could have been prevented by ImageFile using
open(filename, "rb", buffering=0)
or RawIOBase, but we don't want to change the base class for all images just to accommodate the few cases where pillow needs to call another library to process the image.

I'll wait for a better response on SO than "don't use python with established code libraries you moron." I'd really like to know why tell fixes it, but flush does not.

kostrom · 2014-08-29T19:45:13Z

tell is documented as reinitializing py3's buffered io file object
which otherwise is relying on buffered info about the file
which was changed by the os.dup(fp) handle we needed for libtiff

wiredfool · 2014-08-29T19:53:41Z

Ah, ok. That does explain it then. That makes me feel a lot better.

wiredfool · 2014-09-02T22:31:04Z

See #885

kostrom and others added 5 commits August 20, 2014 20:19

multipage tiff for iterator testing

196dd08

Tests for multipage tiffs

f524333

Don't close fd for multipage files

84013b3

Allocate a new core image object for each frame, fixes problems with …

ab1e809

…reusing palettes and image sizes

added ifd offset to the tiff decoder args, call TIFFSetSubDirectory

633660f

wiredfool added 3 commits August 26, 2014 14:16

Testing the imagesequence module with the multipage tiffs

c5ac6d3

Partial fix for test failures, works on py2.x

f90a515

Debugging on tests

40e5265

Same test, debugging,not debugging

e26c620

wiredfool mentioned this pull request Sep 2, 2014

Fix for reading Multipage Tiffs #885

Merged

wiredfool closed this Sep 2, 2014

wiredfool deleted the tiff-seek branch September 23, 2014 16:21

Uh oh!

Conversation

wiredfool commented Aug 26, 2014

Uh oh!

coveralls commented Aug 26, 2014

Uh oh!

coveralls commented Aug 27, 2014

Uh oh!

hugovk commented Aug 27, 2014

Uh oh!

wiredfool commented Aug 27, 2014

Uh oh!

kostrom commented Aug 27, 2014

Uh oh!

wiredfool commented Aug 27, 2014

Uh oh!

kostrom commented Aug 27, 2014

Uh oh!

wiredfool commented Aug 28, 2014

Uh oh!

kostrom commented Aug 28, 2014

Uh oh!

kostrom commented Aug 28, 2014

Uh oh!

kostrom commented Aug 28, 2014

Uh oh!

kostrom commented Aug 28, 2014

Uh oh!

wiredfool commented Aug 29, 2014

Uh oh!

kostrom commented Aug 29, 2014

Uh oh!

kostrom commented Aug 29, 2014

Uh oh!

kostrom commented Aug 29, 2014

Uh oh!

wiredfool commented Aug 29, 2014

Uh oh!

wiredfool commented Sep 2, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants