Add support for BLP file format #3007

jleclanche · 2018-02-17T00:54:30Z

This actually uses a custom python decoder for dxt1/3/5 which is... silly, given that we added a cpython decoder in #1652. But this is some very old work I happen to be able to backport now, and I just tested to be working, so I really want to land it even if the decoder could be ported to use the bcn decoder for better performance.

radarhere · 2018-02-17T01:03:40Z

src/PIL/BlpImagePlugin.py

+
+
+Image.register_open("BLP", BlpImageFile)
+Image.register_extension("BLP", ".blp")


Hi. Would you be interested in replacing "BLP" with BlpImageFile.format in these two lines, to avoid duplication, as per #1333?

jleclanche · 2018-02-17T16:37:52Z

@radarhere any idea why i'm getting "Cannot identify..." errors?

radarhere · 2018-02-18T01:53:35Z

Tests/test_file_blp.py

+    def test_load_blp2_dxt1(self):
+        im = Image.open("Tests/images/blp2_dxt1.blp")
+        target = Image.open("Tests/images/blp2_dxt1.png")
+        self.assert_image_similar(im, target.convert("RGBA"), 15)


In my testing, I find that .convert("RGBA") causes an error, and the test passes once it is removed.

radarhere · 2018-02-18T01:58:24Z

Adding your plugin to the list in https://github.com/python-pillow/Pillow/blob/master/src/PIL/__init__.py fixes the error for Python 3.

However, in Python 2, the error persists, because of

File "PIL/BlpImagePlugin.py", line 412, in _decode_blp2
    data = b"".join(data)
TypeError: sequence item 0: expected string, bytearray found

radarhere · 2018-02-18T02:00:19Z

CHANGES.rst

@@ -35,6 +35,9 @@ Changelog (Pillow)
 - Docs: Changed documentation references to 2.x to 2.7 #2921
  [radarhere]

+- Add a loader for the BLP format from Warcraft and World of Warcraft #3007
+  [jleclanche]
+


PRs do not usually have their CHANGES entry as part of them - this is inviting merge conflicts for your PR if another PR is merged first. Whoever merges it in will add the entry.

jleclanche · 2018-02-18T05:51:33Z

All issues fixed and ready to merge.

jleclanche · 2018-02-19T14:41:20Z

@radarhere good to go?

radarhere · 2018-02-19T19:31:00Z

While I'm able to provide comments and help with problems, I'd like to leave it up to a longer standing member of Pillow team to do the final review and merge.

jleclanche · 2018-02-19T20:13:03Z

Paging @wiredfool @hugovk

jleclanche · 2018-02-28T09:06:42Z

Paging @wiredfool @hugovk again. Can this land please? or is there anything blocking it?

jleclanche · 2018-03-03T10:45:29Z

Since this has been approved, anything blocking merging it? :)
Sorry to keep bugging but I just want to make sure it lands while all this is still fresh in my head. This is a pretty important and long-awaited decoder for the WoW community.

wiredfool

What is the source/license of the test images? They're not from WoW are they?

Other than that, and the _open issue, I think this is reasonable.

wiredfool · 2018-03-03T12:42:00Z

src/PIL/BlpImagePlugin.py

+    format = "BLP"
+    format_description = "Blizzard Mipmap Format"
+
+    def _open(self):


_open should not be doing the decoding work, it should only be setting up tiles and deferring to a decoder.

See https://github.com/python-pillow/Pillow/blob/master/docs/example/DdsImagePlugin.py

I know it can be improved using the bcn decoder, but I wanted to at least land the exhaustive tests.

The example above uses the python decoder framework to split the _open and load phases in pure python. It's a upgrade of your old DDS plugin (which was then obsoleted by the C version)

jleclanche · 2018-03-03T13:13:26Z

blp2_dxt1 and blp2_raw are not copyrightable but other than that, the images are © Blizzard Entertainment. They just need a copyright notice, but I wasn't sure where to put it. Is it ok in the Tests/images/blp folder?

wiredfool · 2018-03-03T13:22:38Z

We can't use them if they're copyright Blizzard, unless there's a very specific license that does give us that ability. Depending on the actual license, we may need to add them to the extra images in the depends repo rather than in this repo.

jleclanche · 2018-03-03T14:14:02Z

Fair use applies here, as the assets are copyrighted but:

They're used for a very limited purpose
They are not redistributed (they live in tests)
They are meaningless in the format they're used as in the tests

I was careful to avoid picking any asset that could potentially not be usable (logos, recognizable textures, etc). So I think a copyright notice should be enough, but I'm fine moving them to a separate repo if you prefer, just let me know how.

Btw @wiredfool are you still on freenode? I pinged you there but no answer.

wiredfool · 2018-03-03T14:22:38Z

So, no license. That's not going to fly with Debian legal (who has patched out an sRGB icc profile because it contained a copyright string), and it's not nearly open enough to distribute.

Fair Use isn't a license, it's a potential defense.

jleclanche · 2018-03-03T14:23:01Z

Why are the tests distributed in the packaged versions?

Edit: Either way as I said just let me know how to move them to a separate repo, I'm fine with that.

wiredfool · 2018-03-03T14:36:44Z

Tests are distributed in the source version, which is the canonical reference for all of the distro packagers.

There have to be open source licensed images of this format somewhere, or they should be possible to create from non-copyrighted images.

jleclanche · 2018-03-03T14:44:07Z

The two files I mentioned above are non-copyrightable, but I wanted to make sure to have coverage for all the variations of blp1 and blp2 (most of which the various blp encoders on the market can't generate right now). I'd rather move these wherever is more appropriate to avoid having it in the source.

jleclanche · 2018-03-03T17:00:09Z

@wiredfool I'm a little overwhelmed on fixing the _open issue you pointed out. The PIL API isn't exactly obvious... (even though I've written several decoders I still don't understand it). Is this a merge-blocking issue? The plugin works as is.

Regarding the blp files, please advise.

wiredfool · 2018-03-03T20:10:41Z

Yes. _open shouldn't do anything expensive, including reading the whole file or decoding it. We've come a good way since the original decoders that you wrote.

The example I linked is your original DDS encoder, redone with the pydecoder interface. Unfortunately, the online diff between it and the original got lost in the move and the conversion of the original into a C form.

However, the basics are here:

def _open():
...
        if fourcc == b"DXT1":
            self.decoder = "DXT1"
            codec = _dxt1
        elif fourcc == b"DXT5":
            self.decoder = "DXT5"
            codec = _dxt5
        else:
            raise NotImplementedError("Unimplemented pixel format %r" % fourcc)

        self.tile = [
            (self.decoder, (0, 0) + self.size, 0, (self.mode, 0, 1))
        ]

_open's responsibility is to set self.size, self.mode, and self.tile. It should do as little as possible to that end, and then return.

and one of the sample decoders:

class DXT1Decoder(ImageFile.PyDecoder):
    _pulls_fd = True

    def decode(self, buffer):
        try:
            self.set_as_raw(_dxt1(self.fd, self.state.xsize, self.state.ysize))
        except struct.error:
            raise IOError("Truncated DDS file")
        return 0, 0

If you're reading from the fd yourself, then you want to set the _pulls_fd, and manage the buffer yourself.

self.set_as_raw sets the entire image as a raw byte stream.

In your case, _open should look something like:

     def _open(self):
        self.magic = self.fp.read(4)
        self._read_blp_header()

        if self.magic == b"BLP1":
            decoder = 'BLP1'
        elif self.magic == b"BLP2":
            decoder = 'BLP2'
        else:
            raise BLPFormatError("Bad BLP magic %r" % (self.magic))
        
        self.tile =  [
            (decoder, (0, 0) + self.size, 0, (self.mode, 0, 1))
        ]

(though, I think you'd have to hoist the self.mode logic up from where it is currently)

Then you'd need two pydecoders that basically defer to the _open_blp* functions, and call self.set_as_raw on the returned bytestream.

jleclanche · 2018-03-04T08:50:56Z

This seems a little silly given that BLP is a container format for DXT and JPG in the first place... it should be pulling in the bcn decoder as I said.

Now don't get me wrong, I'm all for this better interface that doesn't fully read the file on open(), but here's the thing: You're one of the only people in the world who understands that API. Pillow, as I've ranted several times in the past, is extremely hard to understand (I say that having contracted on refactoring 10 year old million-line python codebases), with obscure incantations and voodoo attribute names all over the place.

I get that this isn't your fault, but as it is, you're asking me to spend a lot of time understanding and refactoring this BLP plugin, when you can open just about almost any of the other plugins and find a million much worse offenders; and as far as I can see, only two of them use the PyDecoder interface, making example usage extremely sparse. The DXT decoder you linked in example docs does not seem to even work (self.fd isn't a thing on decoders).

Feel free to do this; I made sure to have exhaustive tests so it can be done in the future (as I said, this should use the c decoder, not the python one), and the plugin itself is as clean as I could make it. But I'd like to merge in the current version as soon as you let me know where to push the sample images to.

wiredfool · 2018-03-04T20:31:07Z

@jleclanche

That example was the workpiece for the PyDecoder. it was ready to merge when you converted the decoders it was based on to C. I assure you that it works, and some of the changes, such as self.fd, are there specifically because the API was hard to work with.

So, here you go, 1 hour while the kids are having stories. wiredfool@196717f#diff-ba00046598bc3bd2e09dd786d7f74950L252

I'm not 100% happy with it due to code duplication, and the format detection probably could be pushed back into the _open method, and then delegate directly to the raw, jpeg, and other decoders in self.tile.

jleclanche · 2018-03-07T21:39:51Z

@wiredfool Forgot to ping back! Thank you for the help with this. I have rebased it against master.

Now what do we do about the images? If moving them to another repo is a problem then I'll drop the copyrighted ones just to get this landed; but I'd rather they live somewhere.

jleclanche · 2018-03-10T19:44:27Z

@wiredfool poke

hugovk · 2018-03-12T07:00:16Z

@jleclanche Have a look at test_images in https://github.com/python-pillow/pillow-depends and how, for example, the picins dir is skipped https://github.com/python-pillow/Pillow/search?utf8=%E2%9C%93&q=picins&type=.

It would be good to have at least some redistributable ones in this repo.

wiredfool · 2018-03-12T07:39:04Z

I've thought about this some more, and I don't think we should be adding images that are Copyright Blizzard in that repo either. The ones that are there are there are included from other test image archives and have unclear licenses or licenses that wouldn't pass Debian Legal. This is a different case, and we shouldn't be hosting these in one of our repos.

hugovk · 2018-03-12T07:44:09Z

Sounds reasonable, then let's try and find or create "free" replacements for the copyright ones.

jleclanche · 2018-03-12T07:49:18Z

I've removed the copyrighted images. The three that remain are good to go.

jleclanche · 2018-03-17T03:30:22Z

Ping for a merge @wiredfool

wiredfool

LGTM

wiredfool · 2018-03-21T07:54:21Z

Still needs documentation and release notes #3018

radarhere · 2022-02-18T02:42:00Z

I've removed the copyrighted images. The three that remain are good to go.

@jleclanche I realise it's been a long time since this PR, but you actually have added four images here, not three. Should the fourth one, blp1_jpeg.blp, be removed, or are we ok to use it for testing from a copyright perspective?

jleclanche · 2022-02-18T06:38:10Z

Everything I added here is ok to use yep

radarhere · 2022-02-18T06:38:56Z

Thanks very much for the reply.

radarhere reviewed Feb 17, 2018

View reviewed changes

radarhere reviewed Feb 18, 2018

View reviewed changes

hugovk approved these changes Feb 28, 2018

View reviewed changes

wiredfool reviewed Mar 3, 2018

View reviewed changes

jleclanche added 6 commits March 6, 2018 18:16

Add support for BLP file format

adaa703

blp: Simplify dxt1/dxt3/dxt5 decoding

fcba358

blp: Separate BLP1 and BLP2 decoding more cleanly

73d7fff

blp: Add more test images

4d685ac

tox: Add pytest to test deps

d2c73c4

tox: Allow passing arguments from tox to pytest

31409bc

Convert BlpImagePlugin to use Pydecoder framework

3a77758

blp: Remove images with unclear licenses

ef546cd

wiredfool approved these changes Mar 21, 2018

View reviewed changes

wiredfool added Needs Release Notes Needs Documentation labels Mar 21, 2018

wiredfool merged commit f504cbe into python-pillow:master Mar 21, 2018

radarhere mentioned this pull request Mar 21, 2018

Release Pillow 5.1.0 on April 1, 2018 #3018

Closed

1 task

hugovk mentioned this pull request Jul 23, 2018

Tests in the pypi .tar.gz archive #3269

Closed

hugovk mentioned this pull request Jun 30, 2020

Correctly read JPEG compressed BLP images #4685

Merged

jleclanche deleted the feat/blp branch September 4, 2020 07:45



		Image.register_open("BLP", BlpImageFile)
		Image.register_extension("BLP", ".blp")

Add support for BLP file format #3007

Add support for BLP file format #3007

Conversation

jleclanche commented Feb 17, 2018

radarhere Feb 17, 2018

Choose a reason for hiding this comment

jleclanche Feb 17, 2018

Choose a reason for hiding this comment

jleclanche commented Feb 17, 2018

radarhere Feb 18, 2018

Choose a reason for hiding this comment

radarhere commented Feb 18, 2018

radarhere Feb 18, 2018

Choose a reason for hiding this comment

jleclanche commented Feb 18, 2018

jleclanche commented Feb 19, 2018

radarhere commented Feb 19, 2018

jleclanche commented Feb 19, 2018

jleclanche commented Feb 28, 2018

jleclanche commented Mar 3, 2018

wiredfool left a comment

Choose a reason for hiding this comment

wiredfool Mar 3, 2018

Choose a reason for hiding this comment

jleclanche Mar 3, 2018

Choose a reason for hiding this comment

wiredfool Mar 3, 2018

Choose a reason for hiding this comment

jleclanche commented Mar 3, 2018

wiredfool commented Mar 3, 2018

jleclanche commented Mar 3, 2018

wiredfool commented Mar 3, 2018

jleclanche commented Mar 3, 2018 • edited Loading

wiredfool commented Mar 3, 2018

jleclanche commented Mar 3, 2018

jleclanche commented Mar 3, 2018

wiredfool commented Mar 3, 2018 • edited by hugovk Loading

jleclanche commented Mar 4, 2018

wiredfool commented Mar 4, 2018

jleclanche commented Mar 7, 2018

jleclanche commented Mar 10, 2018

hugovk commented Mar 12, 2018

wiredfool commented Mar 12, 2018

hugovk commented Mar 12, 2018

jleclanche commented Mar 12, 2018

jleclanche commented Mar 17, 2018

wiredfool left a comment

Choose a reason for hiding this comment

wiredfool commented Mar 21, 2018

radarhere commented Feb 18, 2022

jleclanche commented Feb 18, 2022

radarhere commented Feb 18, 2022

jleclanche commented Mar 3, 2018 •

edited

Loading

wiredfool commented Mar 3, 2018 •

edited by hugovk

Loading