Skip to content
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

More MSP tests #2384

Closed
wants to merge 6 commits into from
Closed

More MSP tests #2384

wants to merge 6 commits into from

Conversation

hugovk
Copy link
Member

@hugovk hugovk commented Jan 29, 2017

Changes proposed in this pull request:

  • Test opening MSP file with a bad checksum. The test file was created by Pillow, temporarily hacked to save with a bad checksum.
  • Test Windows v2 MSP files. Fetches files via Add MSP files pillow-depends#19.
  • Helps towards Test coverage >= 90% #722:
    • 0% -> 82.14% MspDecode.c
    • 95.12% -> 100% MspImagePlugin.py
    • 83.81% -> 85.97% decode.c
    • 79.977% -> 80.131% overall

self.assertRaises(SyntaxError,
lambda: MspImagePlugin.MspImageFile(bad_checksum))

def test_open_windows_v1(self):
# Arrange
# Act
im = Image.open(TEST_FILE)

# Assert
self.assertEqual(im.size, (128, 128))
self.assert_image_equal(im, hopper("1"), 4)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the message here '4'?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mistake. Removed.

"Extra image files not installed")
def test_open_windows_v2(self):
# Arrange
ImageFile.LOAD_TRUNCATED_IMAGES = True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we suppressing errors here and not checking for correctness below?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The files are truncated and otherwise give:

Traceback (most recent call last):
  File "Tests/test_file_msp.py", line 60, in test_open_windows_v2
    im.load()
  File "/usr/local/lib/python2.7/site-packages/PIL/ImageFile.py", line 226, in load
    "(%d bytes not processed)" % len(b))
IOError: image file is truncated (0 bytes not processed)

These are the only example files I can find on the internet. I can't find any other software to generate them, or open these, and I've only got Windows 7 not Windows 1 or 2 :)

im.show()ing them gives reasonable-looking images, although with a black bar below the main image. The size fetching inside MspImageFile.py is the same for both Windows 1 and 2 formats, which looks sound for Windows 1, but MspDecode.c is only used for Windows 2 to do the actual decoding.

I've added some getpixel assertions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. It's YAFRLE implementation.

That sort of thing really sounds like an RLE decoding error in MspDecode.c, as RLE is prone to underrun or overrun if it's interpreted incorrectly. (e.g. #2231). I'd call this probably broken unless I can prove otherwise.

@wiredfool
Copy link
Member

  • confirm correctness or failure of msp parsing re: truncated images

@hugovk
Copy link
Member Author

hugovk commented Feb 19, 2017

Unless the fix is simple this MSP code is probably a good candidate for removal, seeing as it's quite never worked in 20 years and no-one has complained about it, the software to create these files (Windows 1 and 2) have been unsupported since 2001, it was very hard to find any existing test files and it looks like no-one is using it anymore. Thoughts?

@wiredfool
Copy link
Member

wiredfool commented Feb 21, 2017

So, basically, the MSP decoder wan't handling all the cases properly. There's a structure that was being ignored, the scan line map. That map gave an encoded byte length for each scan line, where some of them were 0. Those scan lines were not included in the decoded image. (Not that that method was actually mentioned in the documentation I could find.)

If we assume that the background color is white and add that for each 0 length line, all of the images from this PR (and all the images I could find) decompress to something that looks roughly intentional. Admittedly, some of the ones that I found look a lot like b/w dither/halftoning corresponding to colors, but the marbles one looks pretty good.

This PR is now included in the Pure Python Decoder PR (#1938), as it's a low volume not terribly performance critical decoder, and I didn't really relish implementing the scan line map in C.

@wiredfool wiredfool closed this Mar 12, 2017
@hugovk hugovk deleted the more-msp-tests branch March 12, 2017 20:17
@hugovk
Copy link
Member Author

hugovk commented Mar 12, 2017

Merged in #1938.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants