Packing tests #2694

homm · 2017-08-22T23:03:01Z

Decided to implement all and improved tests for packing and unpacking.

Added many missed tests. At the end all possible modes should be covered. Most tests improved, for example with line interleave.

wiredfool · 2017-08-23T12:10:31Z

From the bigendian machine:

======================================================================
FAIL: TestLibUnpack.test_F_float
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/erics/Pillow/Tests/test_lib_pack.py", line 318, in test_F_float
    1.539989614439558e-36, 4.063216068939723e-34)
  File "/home/erics/Pillow/Tests/test_lib_pack.py", line 78, in assert_unpack
    self.assertEqual(pixel, im.getpixel((x, 0)))
AssertionError: 1.539989614439558e-36 != 2.387939260590663e-38

======================================================================
FAIL: TestLibUnpack.test_I
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/erics/Pillow/Tests/test_lib_pack.py", line 254, in test_I
    self.assert_unpack("I", "I", 4, 0x04030201, 0x08070605)
  File "/home/erics/Pillow/Tests/test_lib_pack.py", line 78, in assert_unpack
    self.assertEqual(pixel, im.getpixel((x, 0)))
AssertionError: 67305985 != 16909060

======================================================================
FAIL: TestLibUnpack.test_I16
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/erics/Pillow/Tests/test_lib_pack.py", line 347, in test_I16
    self.assert_unpack("I;16", "I;16N", 2, 0x0201, 0x0403, 0x0605)
  File "/home/erics/Pillow/Tests/test_lib_pack.py", line 78, in assert_unpack
    self.assertEqual(pixel, im.getpixel((x, 0)))
AssertionError: 513 != 258

homm · 2017-08-23T20:11:28Z

It's interesting. I've fixed test_I16, it's my fault. The problem with test_F_float and test_I, because in both cases just copy4 is used. So one of the following is correct: rawmode F and I actually means F;32N and I;32N (native) or we should replace copy4 with unpackI32 unpacker.

Could you test the latest commit?

wiredfool · 2017-08-24T12:49:26Z

I'm getting this now:

 ======================================================================
FAIL: TestImageArray.test_fromarray
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/erics/Pillow/Tests/test_image_array.py", line 50, in test_fromarray
    self.assertEqual(test("I"), ("I", (128, 100), True))
AssertionError: Tuples differ: ('I', (128, 100), False) != ('I', (128, 100), True)

First differing element 2:
False
True

- ('I', (128, 100), False)
?                   ^^^^

+ ('I', (128, 100), True)
?                   ^^^

There's also an ongoing error that's also in the latest releases:

======================================================================
FAIL: TestImageFile.test_parser
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/erics/Pillow/Tests/test_imagefile.py", line 66, in test_parser
    self.assert_image_similar(im1, im2.convert('L'), 20)
  File "/home/erics/Pillow/Tests/helper.py", line 110, in assert_image_similar
    ave_diff, epsilon))
AssertionError:  average pixel value difference 84.7002 > epsilon 20.0000

homm · 2017-08-24T14:11:29Z

@wiredfool What do you think about this in general?

So one of the following is correct: rawmode F and I actually means F;32N and I;32N (native) or we should replace copy4 with unpackI32 unpacker.

Should be consider F as F;32N and add at least comments, or fix it and continue investigate why tests failed?

wiredfool · 2017-08-24T15:33:23Z

I think that for any sort of resampling or image ops, it would be completely goofy to store non-native endian in I or F storage. It has to be native mode.

Historically, the native versions were added for integer modes because of libtiff, which returns multibyte pixels in the native format, so it's just a copy operation to add it to storage. Our tiff implementation needed to decode the endianness to get it right.

homm · 2017-08-24T23:16:38Z

I think that for any sort of resampling or image ops, it would be completely goofy to store non-native endian in I or F storage.

For sure. In Pillow objects we store all data only in native order. The question is how to interpret the data from external sources in 'F' and 'I' rawmodes. There are two sources which can contain ramodes:

in our file decoders. In this case that rawmodes definitely means non-native endian. As the vast majority of systems is little endian, it should be.
in user code with Image.frombytes(). Users can get data arrays from numpy, for example, and apply them with Image.frombytes(). Depending on numpy behavior, user can expect that we will decode bytes as native endings.

For me the best solution is align those modes with current rawmode format (where "default is little endian") and add appropriate section in Release Notes.

I've installed QEMU with powerpc architecture, it is damn slow but works. I see the error in TestImageArray.test_fromarray, but I have no errors in TestImageFile.test_parser.

radarhere · 2017-08-25T12:14:46Z

Tests/test_lib_pack.py

-        self.assertRaises(ValueError, lambda: unpack("CMYK", "CMYK", 2))
+    def assert_pack(self, mode, rawmode, data, *pixels):
+        """
+        data - eather raw bytes with data or just number of bytes in rawmode.


radarhere · 2017-08-25T12:15:13Z

Tests/test_lib_pack.py

+class TestLibUnpack(PillowTestCase):
+    def assert_unpack(self, mode, rawmode, data, *pixels):
+        """
+        data - eather raw bytes with data or just number of bytes in rawmode.


homm · 2017-08-26T08:10:57Z

Changed packers and unpackers for I and F modes to platform-independent format. All tests passed on my big-endian qemu-ppc system.

wiredfool · 2017-08-26T15:25:55Z

Ok, mostly afk this weekend, but will check on mon.

wiredfool · 2017-08-28T14:35:35Z

(fwiw, my mini is at ~ 70 seconds for a test run, which is roughly equivalent to a raspberry pi)

Ok, I get your point now.

We should not change the meaning of the I and F modes now from copy4 to byteswap. This will break anyone on PPC who's using I or F images. It does trip one test on my PPC which you're probably not hitting:

FAIL: TestNumpy.test_to_array
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/erics/Pillow/Tests/test_numpy.py", line 185, in test_to_array
    _to_array(*mode)
  File "/home/erics/Pillow/Tests/test_numpy.py", line 166, in _to_array
    self._test_img_equals_nparray(img, np_img)
  File "/home/erics/Pillow/Tests/test_numpy.py", line 122, in _test_img_equals_nparray
    self.assert_deep_equal(px[x, y], np[y, x])
  File "/home/erics/Pillow/Tests/helper.py", line 66, in assert_deep_equal
    self.assertEqual(a, b, msg)
AssertionError: 26 != 436207616

re: "default is little endian" comment

There's something of a disconnect between the storage modes, which are limited in number and expressiveness and fixed to native endian and the pack/unpack raw modes, where the flagged details default to LE if not specified. The disconnect comes when there's identically named storage and raw modes with conflicting defaults.

There are modes that do what you're proposing, I;32 and F;32 that are the default LE versions of the unpackers corresponding to the F;32B and I;32B modes.

homm · 2017-08-28T16:06:32Z

It does trip one test on my PPC which you're probably not hitting

You are right, I didn't install numpy.

We should not change the meaning of the I and F modes now from copy4 to byteswap.

Ok, reverted. Could you check again?

wiredfool · 2017-08-28T16:19:44Z

Tests/test_lib_pack.py

        self.assert_pack("I", "I;16B", 2, 0x0102, 0x0304)
        self.assert_pack("I", "I;32S",
            b'\x83\x00\x00\x01\x01\x00\x00\x83',
            0x01000083, -2097151999)

        if sys.byteorder == 'little':
+            self.assert_pack("I", "I", 4, 0x04030201, 0x08070605)


I think you missed moving the unpack version of these tests.

Indeed. Done.

wiredfool · 2017-08-28T16:37:36Z

Ok, tests are passing here.

homm added 2 commits August 23, 2017 01:32

First part of unpack tests

f012fa1

Second part of unpack tests

5c2199c

This was referenced Aug 22, 2017

Load 16-bit RGB and RGBA TIFF image #2692

Merged

Fast unpacking and partially packing #2693

Merged

homm added 2 commits August 23, 2017 10:27

Third part of unpack tests

29aca08

byte strings

d33eab5

homm added 2 commits August 23, 2017 23:09

fix tests test_I16

a812f6c

possible fix I→I and L→L unpacking on bigendian

2439c9b

homm added 2 commits August 24, 2017 00:58

Packing tests, first part. Fix RGBX packers

8cb9cba

Fix error in pypy

ebb10e8

radarhere reviewed Aug 25, 2017

View reviewed changes

homm added 2 commits August 26, 2017 10:49

Second part of pack tests

f43b9f3

fix py3, change test

922407e

homm changed the title ~~[WIP] Packing tests~~ Packing tests Aug 26, 2017

homm requested a review from wiredfool August 26, 2017 08:36

homm added this to the 4.3.0 milestone Aug 26, 2017

homm mentioned this pull request Aug 26, 2017

Some release notes for 4.3 #2697

Closed

homm added the Do Not Merge label Aug 28, 2017

Revert little-endian byte order for "I" and "F" rawmodes

aac0869

homm removed the Do Not Merge label Aug 28, 2017

wiredfool reviewed Aug 28, 2017

View reviewed changes

unpacking tests for "I" and "F" modes

40fe1f7

homm merged commit 6b4b1dc into python-pillow:master Aug 28, 2017

homm deleted the packing-tests branch August 28, 2017 16:42

radarhere mentioned this pull request Feb 14, 2022

Endian tests #391

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Packing tests #2694

Packing tests #2694

homm commented Aug 22, 2017

wiredfool commented Aug 23, 2017

homm commented Aug 23, 2017

wiredfool commented Aug 24, 2017

homm commented Aug 24, 2017

wiredfool commented Aug 24, 2017

homm commented Aug 24, 2017 •

edited

Loading

radarhere Aug 25, 2017

radarhere Aug 25, 2017

homm commented Aug 26, 2017

wiredfool commented Aug 26, 2017

wiredfool commented Aug 28, 2017

homm commented Aug 28, 2017

wiredfool Aug 28, 2017

homm Aug 28, 2017

wiredfool commented Aug 28, 2017

Packing tests #2694

Packing tests #2694

Conversation

homm commented Aug 22, 2017

wiredfool commented Aug 23, 2017

homm commented Aug 23, 2017

wiredfool commented Aug 24, 2017

homm commented Aug 24, 2017

wiredfool commented Aug 24, 2017

homm commented Aug 24, 2017 • edited Loading

radarhere Aug 25, 2017

Choose a reason for hiding this comment

radarhere Aug 25, 2017

Choose a reason for hiding this comment

homm commented Aug 26, 2017

wiredfool commented Aug 26, 2017

wiredfool commented Aug 28, 2017

homm commented Aug 28, 2017

wiredfool Aug 28, 2017

Choose a reason for hiding this comment

homm Aug 28, 2017

Choose a reason for hiding this comment

wiredfool commented Aug 28, 2017

homm commented Aug 24, 2017 •

edited

Loading