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.

Sign up for GitHub

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

Jump to bottom

Corrected BLP1 alpha depth handling #8651

Merged
merged 3 commits into from
Jan 1, 2025
Merged

Corrected BLP1 alpha depth handling #8651

merged 3 commits into from
Jan 1, 2025

Conversation

radarhere
Copy link
Member

Resolves #8650

https://www.hiveworkshop.com/threads/blp-specifications-wc3.279306/ describes the start of the 'BLP File Header' as

uint32_t magic;
uint32_t content;
if (version >= 2) {
    uint8_t encodingType; // not documented
    uint8_t alphaBits;
    uint8_t sampleType; // not documented
    uint8_t hasMipmaps;
} else {
    uint32_t alphaBits;
}

However, we don't currently perform different behaviour for BLP1, and only treat the data as if it was BLP2.

Pillow/src/PIL/BlpImagePlugin.py

Lines 300 to 303 in aa0f412

(self._blp_encoding,) = struct.unpack("<b", self._safe_read(1))
(self._blp_alpha_depth,) = struct.unpack("<b", self._safe_read(1))
(self._blp_alpha_encoding,) = struct.unpack("<b", self._safe_read(1))
self.fd.seek(1, os.SEEK_CUR) # mips

This PR fixes updates how the header data is handled for BLP1, for both reading and writing.

@radarhere radarhere mentioned this pull request Jan 1, 2025
Closed
sminjard
sminjard approved these changes Jan 1, 2025
View reviewed changes
Copy link

@sminjard sminjard left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Wow, I didn't expect to hear from you this soon.
Thanks!

Everything seems fine. A bit confused about why you had to change things in the _openfunction. I think there is a way to call _read_blp_header so that you don't have that logic at two places, but this will go in another PR as this one seems to resolve the issue.

Happy New Year !

@radarhere
Copy link
Member Author

Thanks for pointing out the duplicate code. As you can see, I've pushed a commit to fix it.

I can see you've approved this PR three times in the last ten minutes. I imagine you're experiencing a bug on your device and not seeing that it worked? If so, I can confirm you have approved this.

@hugovk hugovk merged commit dfb368a into python-pillow:main Jan 1, 2025
50 checks passed
@sminjard
Copy link

sminjard commented Jan 1, 2025

Somehow GitHub was not checking the "#approved-reviews-by>=1" for auto merge check.

@hugovk
Copy link
Member

hugovk commented Jan 1, 2025

This was a manual merge by me, so it didn't use automerge, which would have also needed the label adding and approval by a committer.

@radarhere radarhere deleted the blp branch January 1, 2025 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sminjard sminjard sminjard approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BLP 1 header is not read correctly
3 participants
@radarhere @sminjard @hugovk