-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Correctly read JPEG compressed BLP images #4685
Conversation
Also this can break some code, if you want to save the picture in JPG, you will have to convert it to RGB mode before. |
Please could you give some more info/examples of what this could break? Also please can you add tests? |
Hello, a test would be having a reference blp file and comparing if the extracted file matches the original one? How is it possible to get a blp file without using blizzard tools, do we need to be able to generate a blp with pillow in first place? #3176 For what can break, it means that previously this is possible with Image.open("file.blp") as pil:
pil.save("file.jpg") Now you have to write with Image.open("file.blp") as pil:
conv = pil.convert("RGB")
conv.save("file.jpg") |
You can see the existing BLP tests in https://github.com/python-pillow/Pillow/blob/master/Tests/test_file_blp.py They test an opened BLP image is as expected by comparing against a PNG version. The BLP tests and files were added in #3007, and see the discussion there, that the files must be able to be shared under the Pillow licence, so it's most likely we cannot include files copyright someone else (unless specific permission is given). |
@Meithal Hi! This PR is now a year old, did you get a chance to look into adding tests? |
I'll try to tackle it this weekend. |
I've removed the alpha channel, which I think you weren't sure about in the first place. I did this for simplicity, and because there wasn't a test image demonstrating the need. @Meithal to be clear, is the image that you've included here one that can be included under our license and distributed with Pillow? There was a long discussion about this in #3007 |
I'm not sure, it should have the same license than the other ones, it is generated automatically by blizzard worldedit and included as is in a map archive. I have been trying to find an example of blp1 file with a transparent component to include it in the test, and cover those paths but got lost in a rabbit hole doing that (I was looping through thousands of existing archives), I think your patches removed the alpha support, it may still be useful to not do so, maybe add an extra function that keeps it so older code doesn't break? |
I found that we already have a BLP1 image in our repository, so I've pushed a commit to switch to that instead.
This PR exists because no BLP1 images were being read correctly, yes? Not just ones with alpha, but any of them. So surely all older code is broken, and this PR only makes things better? |
You may be interested to know that #6069 currently adds support for alpha in BLP1 when |
Fixes #4684
Changes proposed in this pull request: