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

SunImagePlugin fixes #2241

Merged
merged 8 commits into from
Nov 23, 2016
Merged

SunImagePlugin fixes #2241

merged 8 commits into from
Nov 23, 2016

Conversation

wiredfool
Copy link
Member

Fixes #2231

Changes proposed in this pull request:

  • SunRle was borked, it couldn't have worked in the first place.
  • ImageFile.open would attempt to pass a too short args tuple into mmap.
  • Ran a sample of 10 other images through the decoder, verified visually. Pre-fix, 2 worked. All work now.

Tests. I'd like to include the set of test images from https://samples.libav.org/image-samples/sunrast/ so that we don't regress. Libav is ok with mirroring/distribution, though half of the images are lena, and the others are of unclear license from upstream. So at least debian would have issues with them.

SunImagePlugin now loads all the images here:
https://samples.libav.org/image-samples/sunrast/
without LOAD_TRUNCATED_IMAGES set, verified visually.

Prior to this commit:
Could not open 32bpp.ras
Could not open 4bpp.ras
Could not open gray.ras
Could not open lena-1bit-rle.sun
Could not open lena-24bit-rle.sun
Could not open lena-8bit-raw.sun
Could not open lena-8bit-rle.sun
Could not open MARBLES.SUN
@hugovk
Copy link
Member

hugovk commented Nov 22, 2016

If we can't include the test images in the source, one option is to zip them up onto https://github.com/python-pillow/pillow-depends and fetch them as part of the CI builds (and skip relevant tests if not present).

@wiredfool
Copy link
Member Author

Ok, tests should be good, I've rebased away the two extra xcopy flag commits, so I'm done with this one.

os.listdir(EXTRA_DIR) if os.path.splitext(f)[1]
in ('.sun', '.SUN', '.ras'))
for path in files:
print (path)
Copy link
Member

Choose a reason for hiding this comment

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

Is this print needed?

self.mode, rawmode = "RGB", "BGR"
elif depth == 32:
if file_type == 3:
self.mode, rawmode = 'RGB', 'RGBX'
Copy link
Member

Choose a reason for hiding this comment

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

No tests for this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

Feel free to add a 32 bit type 3 file if you can find one.

length = i32(s[28:32])
offset = offset + length
self.palette = ImagePalette.raw("RGB;L", self.fp.read(length))
raise SyntaxError("Unsupported Mode/Bit Depth")
Copy link
Member

Choose a reason for hiding this comment

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

No tests for this line.


if palette_length:
if palette_length > 1024:
raise SyntaxError("Unsupported Color Palette Length")
Copy link
Member

Choose a reason for hiding this comment

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

No tests for this line.

raise SyntaxError("Unsupported Color Palette Length")

if palette_type != 1:
raise SyntaxError("Unsupported Palette Type")
Copy link
Member

Choose a reason for hiding this comment

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

No tests for this line.

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 palette bits are simple defensiveness. We're doing an unprotected read here, not even a saferead.

self.tile = [("sun_rle", (0, 0)+self.size, offset, rawmode)]

else:
raise SyntaxError('Unsupported Sun Raster file type')
Copy link
Member

Choose a reason for hiding this comment

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

No tests for this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

For all of the no-tests -- The values are in the spec, I have tested every sun raster file I've found. In this case, I'm raising a syntax error instead of just leaving the tile blank. It's an explanation rather than a silent failure. Previously hopper.ras silently here failed because it was a type 3, not type 1. So it passed through our open test, but when load was called, it barfed with a strange error. Fixing support for it meant that this change didn't get covered.

raise SyntaxError("Unsupported Mode/Bit Depth")



Copy link
Member

Choose a reason for hiding this comment

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

Can delete some of these blank lines.

@hugovk hugovk merged commit 6fa7f3f into python-pillow:master Nov 23, 2016
@wiredfool wiredfool deleted the sunrle branch October 2, 2017 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sun Raster files with RLE encoding and depth 1 fail to load
2 participants