Skip to content

Fix 32-bit BMP loading (RGBA or RGBX) - #1125

Merged
aclark4life merged 18 commits into
python-pillow:masterfrom
artscoop:patch-1
Apr 1, 2015
Merged

aclark4life merged 18 commits into
python-pillow:masterfrom
artscoop:patch-1

Conversation

@artscoop

@artscoop artscoop commented Mar 4, 2015

Copy link
Copy Markdown
Contributor

PIL choked on perfectly valid BMP files (32 bits with Alpha). It could not handle valid RGBA masks to determine the raw format.
To clarify things, I:

  • Rewrote the BmpImagePlugin.BmpImageFile class to be far more readable
  • Made error messages more explicit (e.g. say that RLE bitmaps are unsupported)
  • Made a readable dict to contain BMP header information
  • Kept the existing security checks
  • Instead of reading palette info by chunks of 3/4 bytes, read the whole palette info at once and parse the data.
  • Now works with BMPv4/5 with Alpha (and can be exported to alpha PNG for example)
  • Tested load and save with RGB24, RGB8, RGB8L, RGB32 and RGBA32.
  • Tested with one bogus file. File not accepted, as expected.
  • Size of header attributes is made explicit in this code

I wanted to test more BMP formats, but I could not find that many images.
But for all the types I tested, it worked flawlessly.

PIL choked on perfectly valid BMP files (32 bits with Alpha). It could not handle valid RGBA masks to determine the raw format.
To clarify things, I:
- Rewrote the `BmpImagePlugin.BmpImageFile` class to be far more readable
- Made error messages more explicit (e.g. say that RLE bitmaps are unsupported)
- Made a readable dict to contain BMP header information
- Kept the existing security checks
- Instead of reading palette info by chunks of 3/4 bytes, read the whole palette info at once and parse the data.
- Now works with BMPv4/5 with Alpha (and can be exported to alpha PNG for example)
- Tested load and save with RGB24, RGB8, RGB8L, RGB32 and RGBA32.
- Tested with one bogus file. File not accepted, as expected.

I wanted to test more BMP formats, but I could not find that many images.
But for all the types I tested, it worked flawlessly.
Comment thread PIL/BmpImagePlugin.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

debug print

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh my goodness. :/

Debug print left in _save. Removed.
@artscoop

artscoop commented Mar 4, 2015

Copy link
Copy Markdown
Contributor Author

Seems like a test with a BMP image has failed (Tests/images/bmp/g/pal1wb.bmp, too different from expected result). Investigating.

artscoop added 6 commits March 4, 2015 19:26
It appears that
{{{
  The handling of 1bpp bitmaps is a little complicated.
  When reading 1bpp bitmaps, the palette is ignored.
  1's are considered foreground, and they are considered black.
  0's are considered background, and they are considered white.
}}}
so the raw mode has to be `1;I`
Use the provided image offset if there is palette data while the image is bitfielded.
Until tests pass.
Fixed loading of all types of provided images (+rgba). Added edge case where the header is reported as 40 bytes long with BITFIELDS (they start past the 40 bytes of the header). Loading fails for RLE, but IIRC, they're unsupported so it's normal.
Choked on roundtrip, where a P;1 image was returned instead of a 1 image.
@artscoop artscoop closed this Mar 5, 2015
@artscoop artscoop reopened this Mar 5, 2015
artscoop added 9 commits March 5, 2015 08:19
Readapted some original code.
Getting bonkers but I need to know
I suspect the tests to check against an exact string when expecting an error
Various fixes on code broken or not passing tests
Some .cur file with alpha was loaded fully opaque with PIL. Fixed, and fixed the test to take that into account.
Fails on Python 3, tried some fixes before going the virtualenv3 route
Error happening in Python 3.x with P images:
in original code, palette data was created from a list of bytestrings. Changed to a full bytestring.
- `b"".join(list of bytestrings)` works in python 2.7 and 3.x
- `b"".join(bytestring)` works in python 2.7 but fails in python 3.x
No need to `join` anymore. Works in 3.x
@wiredfool

Copy link
Copy Markdown
Member

There are a ton of BMP images in the test suite, Tests/images/bmp/*. G == good, q== questionable, b==bad. Look at tests/test_bmp_reference.py (which you've probably seen, from the test failures).

@artscoop

artscoop commented Mar 7, 2015

Copy link
Copy Markdown
Contributor Author

Exactly. At first, I did not know how to test a thorough set of images. Then I found the images, understood how Travis CI works, and then I had a very hard time catering for every test (I even had to fix a test, since before the fix, PIL could not detect the A channel of a valid 32 bit RGBA .cur file and expected this behaviour)

Does not change testing on other files, but fixes a case which previously made PIL collapse.
The Bitmap was a 1x1 RGBA and provoked an exception in PIL, but every Image viewer can load it.
Fixed code with comparison of header size, compression type and loading type of masks and fixed it.
@aclark4life

Copy link
Copy Markdown
Member

Is this ready for 2.8.0?

@aclark4life aclark4life added this to the 2.8.0 milestone Apr 1, 2015
Comment thread PIL/BmpImagePlugin.py

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Very strange comments style

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, sorry, gonna fix it very soon. This is the comment style automatically used by Eclipse, and it did fit me until very recently, when I started using flake8 to check coding style.

aclark4life added a commit that referenced this pull request Apr 1, 2015
Fix 32-bit BMP loading (RGBA or RGBX)
@aclark4life
aclark4life merged commit ea65087 into python-pillow:master Apr 1, 2015
@aclark4life

Copy link
Copy Markdown
Member

Thanks, I'll fix the comments

@techtonik

Copy link
Copy Markdown
Contributor

@artscoop can you investigate the regression in #1293 that is supposedly caused by this commit. You seem to be the best person to understand what is wrong there.

@artscoop

Copy link
Copy Markdown
Contributor Author

Yes, will have a look at it today.

@techtonik

Copy link
Copy Markdown
Contributor

@artscoop thanks! ;) 💃

@artscoop
artscoop deleted the patch-1 branch December 30, 2015 16:24
@artscoop

Copy link
Copy Markdown
Contributor Author

@techtonik , can you check if the version of Pillow in the https://github.com/artscoop/Pillow repo fixes or mitigates your problem ?

@techtonik

Copy link
Copy Markdown
Contributor

@artscoop I can only check this change https://github.com/artscoop/Pillow/commit/6216d079f5bac0a7f13a43b76bf276542c1abd74 because I don't have Visual Studio to compile the whole extension. Let me check.

@techtonik

Copy link
Copy Markdown
Contributor

Didn't help. The error message is the same:

Traceback (most recent call last):
  File "C:\Bin\saveclip.py", line 31, in <module>
    im = ImageGrab.grabclipboard()
  File "C:\Python27\lib\site-packages\PIL\ImageGrab.py", line 66, in grabclipboard
    return BmpImagePlugin.DibImageFile(io.BytesIO(data))
  File "C:\Python27\lib\site-packages\PIL\ImageFile.py", line 100, in __init__
    self._open()
  File "C:\Python27\lib\site-packages\PIL\BmpImagePlugin.py", line 208, in _open
    self._bitmap()
  File "C:\Python27\lib\site-packages\PIL\BmpImagePlugin.py", line 148, in _bitmap
    raise IOError("Unsupported BMP bitfields layout")
IOError: Unsupported BMP bitfields layout

and saveclip.py is:

#!/usr/bin/env python
#
# the code is in public domain
#
# history:
#   1.1  - mention what library should be installed
#   1.0  - make get_avail_filename() function

__version__ = '1.1'

import os, sys
try:
    from PIL import ImageGrab
except ImportError:
    # https://github.com/python-pillow/Pillow/issues/1293
    raise ImportError("Pillow is not installed (use Pillow==2.7.0)")

def get_avail_filename(basename, suffix='%0.2d.png'):
    """Find first non-existing filename with given numerical suffix"""
    counter = 1
    while True:
        newname = basename + suffix % counter
        if not os.path.exists(newname):
            return newname
        counter += 1

filename = 'clipboard.png'
if os.path.exists(filename):
  filename = get_avail_filename('clipboard', suffix='%0.2d.png')

im = ImageGrab.grabclipboard()
if im is None:
  sys.exit('Error: No image data in clipboard')
else:
  im.save(filename, 'PNG')
  print('Saved as %s' % filename)

@techtonik

Copy link
Copy Markdown
Contributor

Uncommented line 147 just before the raise IOError("Unsupported BMP bitfields layout"), which is:

print(file_info['bits'], [hex(value) for value in file_info['rgb_mask']])

It gives this info: (32, ['0xff0000', '0xff00', '0xff'])

@artscoop

Copy link
Copy Markdown
Contributor Author

I was just going to ask you to uncomment that line :)
This is exactly what I wanted to know.
I pushed an edit with a new mask added. Could you try with the latest master ? (you can just copy BMPImagePlugin.py from my repository)

@techtonik

Copy link
Copy Markdown
Contributor

The error exception with the latest code is strange:

...
  File "C:\Python27\lib\site-packages\PIL\ImageGrab.py", line 66, in grabclipboard
    return BmpImagePlugin.DibImageFile(io.BytesIO(data))
  File "C:\Python27\lib\site-packages\PIL\ImageFile.py", line 107, in __init__
    raise SyntaxError(v)
SyntaxError: (32, (16711680, 65280, 255, 4279440405L))

I reverted and changed the line to dump rgba_mask and got this:

(32, ['0xff0000', '0xff00', '0xff', '0xff131415L'])

@hugovk

hugovk commented Dec 30, 2015

Copy link
Copy Markdown
Member

(Top tip: if there's unit tests for this, you can enable AppVeyor for your repo and it'll build and test it on Windows. And good to do so on Travis CI, for Linux. They're free. And they will also help guard against any other regressions.)

@techtonik

Copy link
Copy Markdown
Contributor

@hugovk yes, but it is not clear what unit test is for. Right now the data is in Windows clipboard. I can dump the binary for unit test if somebody tells me how.

@techtonik

Copy link
Copy Markdown
Contributor

Tracing with pdb..

140                 import pdb; pdb.set_trace()
141                 if file_info['bits'] in SUPPORTED:
142                     if file_info['bits'] == 32 and (file_info['rgba_mask'] in SUPPORTED[file_info['bits']]) or (file_info['rgb_mask'] in SUPPORTED
[file_info['bits']]):
143  ->                     raw_mode = MASK_MODES[(file_info['bits'], file_info['rgba_mask'])]
144                         self.mode = "RGBA" if raw_mode in ("BGRA",) else self.mode
145                     elif file_info['bits'] in (24, 16) and file_info['rgb_mask'] in SUPPORTED[file_info['bits']]:
146                         raw_mode = MASK_MODES[(file_info['bits'], file_info['rgb_mask'])]
147                     else:
148                         print(file_info['bits'], [hex(value) for value in file_info['rgb_mask']])
(Pdb) file_info['bits']
32
(Pdb) file_info['rgba_mask']
(16711680, 65280, 255, 4279440405L)
(Pdb) n
KeyError: ((32, (16711680, 65280, 255, 4279440405L)),)
> c:\python27\lib\site-packages\pil\bmpimageplugin.py(209)_open()
-> self._bitmap()
(Pdb)

@techtonik

Copy link
Copy Markdown
Contributor

And here comes the SyntaxError:

> c:\python27\lib\site-packages\pil\imagefile.py(101)__init__()
-> except (IndexError,  # end of data
(Pdb) n
> c:\python27\lib\site-packages\pil\imagefile.py(102)__init__()
-> TypeError,  # end of data (ord)
(Pdb) n
> c:\python27\lib\site-packages\pil\imagefile.py(103)__init__()
-> KeyError,  # unsupported mode
(Pdb) n
> c:\python27\lib\site-packages\pil\imagefile.py(104)__init__()
-> EOFError,  # got header but not the first frame
(Pdb) n
> c:\python27\lib\site-packages\pil\imagefile.py(105)__init__()
-> struct.error) as v:
(Pdb) n
> c:\python27\lib\site-packages\pil\imagefile.py(106)__init__()
-> logger.exception("%s")
(Pdb) n
No handlers could be found for logger "PIL.ImageFile"
> c:\python27\lib\site-packages\pil\imagefile.py(107)__init__()
-> raise SyntaxError(v)

@techtonik

Copy link
Copy Markdown
Contributor

Here is the binary data from self.fp
dumpfp.zip

@MXS2514

MXS2514 commented Jul 6, 2016

Copy link
Copy Markdown

Hello, the latest BmpImagePlugin.py seems can't work with me Test32bitbmp
When checking mode it is still RGB not expected RGBA
However, after googling this worked, both reading and saving to 32-bit png.

@wiredfool

wiredfool commented Jul 6, 2016 •

Copy link
Copy Markdown
Member

From the code:

     # 40 byte headers only have the three components in the bitfields masks,
     # ref: https://msdn.microsoft.com/en-us/library/windows/desktop/dd183376(v=vs.85).aspx
     # See also https://github.com/python-pillow/Pillow/issues/1293
     # There is a 4th component in the RGBQuad, in the alpha location, but it
     # is listed as a reserved component, and it is not generally an alpha channel

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.

8 participants