Add PCX decoder support #2364

lupine · 2024-10-25T22:58:36Z

This PR adds support for decoding PCX images. This is a raster format introduced in 1985. Further details on Wikipedia: https://en.wikipedia.org/wiki/PCX; it saw fairly extensive use up to the end of the 20th century, but is definitely obsolete now. However, there is still extant data using the format.

My hobby project is a game engine recreation that happens to need to load a range of assets in the PCX format; using the image crate through bevy to (attempt to) load PCX files is how I ended up writing this PR.

Since it's an obsolete format, I've opted to only implement the decoder side, but the pcx crate does have an encoder as well if we wanted to wire that up. Perhaps "historic format" sounds better!

I license past and future contributions under the dual MIT/Apache-2.0 license, allowing licensees to choose either at their option.

lupine

Just some initial self-review.

Cargo.toml

src/codecs/pcx.rs

src/image_reader/free_functions.rs

deny.toml

tests/reference/pcx/images/cga_bw.pcx.620c687c.png

tests/reference/pcx/images/cga_tst1.pcx.2a36d106.png

tests/reference/pcx/images/gmarbles.pcx.1359bee5.png

fintelia · 2024-10-26T20:46:26Z

I wrote up some thoughts on format support in #2367.

High level, I don't think we should add PCX, but do want to think about making it easier for users to deal with formats not directly supported by this crate.

lupine · 2024-10-26T22:43:49Z

@fintelia just so I know, does that decide the fate of this PR or is it still up in the air? I just want bevy to load my files; if image doesn't want to support a particular format, that's entirely fair, and I can make it happen via a custom asset loader over there (I think).

FWIW, I was planning on adding ANI support as well ^^. In my head, an image crate naturally wants to support all the image formats, but I appreciate the reality is more complicated than that.

A tidy alternative would be a way to register codecs at build time (runtime probably isn't necessary), as go's image stdlib package does; I don't quite know offhand how that would look in Rust. Right now, this PR would need to be followed up with a PR to bevy to wire up the new format, which really isn't ideal even for the case of popular new formats.

fintelia · 2024-10-26T23:43:40Z

Yes. If @HeroicKatora feels differently we can discuss further, but otherwise we can go ahead and close this PR. I'll add that from skimming the code quality does look quite good, and I've also just opened #2369 to encourage having discussions prior to doing all the implementation effort.

(As a side note, supporting all image formats is impossible because there's no official list: anybody can invent their own format and people do so pretty often.)

sorairolake · 2024-10-27T11:33:38Z

I think it would be good to implement image::ImageDecoder for pcx::Reader. I think this will help you achieve your goal to some extent. The problem is that pcx crate is not being actively developed.

HeroicKatora · 2024-10-27T21:36:53Z

First of all, I do agree with code quality :+1.

Following the thoughts raised out in #2367, and in particular I like the tiered view #2363 (comment) , I think we should support this format in some capacity. And in particular, by having the trait implemented but not dispatching onto the file without explicit opt-in (as written in the linked comment, I'd liken it to tier 3). I tend to agree that some image-level configuration mechanism will be necessary at some point, but let's design that in an actual issue (#2368 seems appropriate). I wouldn't want to hold up the PR over that, we have enough leverage to fine-tune the integration when it lands and can use having validation targets of all tiers anyways, right?

It would be most beneficial to users, that's the crucial part. My argument would be it doesn't matter so much to me if a format itself is fringe, as rather if image would be an appropriate tool to use for the people that need that format. That's purely a maintenance question for this code. In terms of lines of code, already including a test suite_ (!), it doesn't seem to put much strain on image. Rather on the underlying implementation but really that doesn't need to be our immediate problem?

So 👍 me with the licensing comment resolved.

fintelia · 2024-10-31T18:54:54Z

Just to double check, what is the source of the test images and what license are they under?

lupine · 2024-10-31T21:45:46Z

@fintelia they're from the pcx crate - #2364 (comment) -> https://github.com/kryptan/pcx/tree/master/test-data . They didn't originate there though. https://github.com/drewnoakes/metadata-extractor-images/tree/main/pcx has them, for instance.

https://netghost.narod.ru/gff/sample/images/pcx/index.htm has a 1996 imprint and has marbles.pcx and gmarbles.pcx 🤔

fintelia · 2024-11-01T02:17:39Z

lupine · 2024-11-01T18:16:29Z

That's fair. I wrongly assumed that since they were in a WTFPL crate, it'd be fine to reuse them! I'll put together a PR that replaces them.

I tracked down the CGA PCX files to https://samples.ffmpeg.org/image-samples/pcx/cga/cga-pcx.txt - they also appear in the MPlayer samples directory. No explicit copyright grant in that file; ffmpeg itself is lgpl, mplayer gpl. I'll just yoink those out as well.

All the obscure parts of the format are still tested in the pcx crate; I think we just need a single image that allows us to verify that we're up to that crate correctly.

Oh, I also found a "proper" use of PCX in the wild - according to https://convertimage.net/convert-a-picture/to-pcx/ :

While PCs do not generally use PCX anymore, the worldwide airline industry does. In fact, PCX is still the only valid format for logos and graphics printed on baggage tags and tickets, as defined in the AEA (Association of European Airlines) ATB Technical Specs.

A brief look tells me this was true in 2007, at least.