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

Combined BMP RLE decoders #1

Merged
merged 2 commits into from Oct 24, 2022
Merged

Combined BMP RLE decoders #1

merged 2 commits into from Oct 24, 2022

Conversation

radarhere
Copy link

Two suggestions for python-pillow#6674

  1. Corrected the BMP compression setting in the documentation. This was already incorrect in the documentation.
with Image.open("Tests/images/hopper_rle4.bmp") as im:
        print(im.info["compression"])

prints 2.

  1. I noticed that you copied much of the existing BMP RLE decoder, so I've combined your decoder with the existing decoder to reduce duplicate code.

Comment on lines +329 to +339
if rle4:
# 2 pixels per byte
byte_count = byte[0] // 2
bytes_read = self.fd.read(byte_count)
for byte_read in bytes_read:
data += o8(byte_read >> 4)
data += o8(byte_read & 0x0F)
else:
byte_count = byte[0]
bytes_read = self.fd.read(byte_count)
data += bytes_read
Copy link

Choose a reason for hiding this comment

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

The first two lines of code in each branch here are the same (other than the comment), so they could be pulled out of the if..else.

Copy link
Author

Choose a reason for hiding this comment

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

The first line of the first branch is byte_count = byte[0] // 2 and the first line of the second branch is byte_count = byte[0]. As I noted elsewhere, // 2 isn't a comment, it's division.

@radarhere radarhere merged commit 4b31a7b into npjg:main Oct 24, 2022
@radarhere
Copy link
Author

I've merged this in. Normally I'd wait longer to see if you had any thoughts on my suggestions, but Pillow 9.3.0 is fast approaching, and I expect you're more interested in this functionality making it into that release.

@radarhere radarhere deleted the bmp_rle4 branch October 24, 2022 04:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants