Accessing palette data

[connorjclark]

In png.c and bmp.c, there is a PalEntry pal[256] value that is read into when applicable for those formats. I'd like a way to access this palette data.

Use case: My program allows artists to import images for indexed-color manipulation, so maintaining the exact same color palette is ideal. I currently have a roundabout way to restore the palette data from an ALLEGRO_BITMAP here: https://github.com/ArmageddonGames/ZQuestClassic/blob/f42d383f9b2b1972ae22f0c3e41c04f46bddb18e/allegro-addons/al5img/src/al5img.c#L57 which reads the bitmap pixel-by-pixel and recreates a palette. The problems are:

1) Only the colors utilized in the bitmap exist in the restored palette 2) The order is whatever order the colors are encountered in the bitmap, not the original palette order

I can fallback to hacky approaches to grab this data (I may fork allegro and stash these things into globals or something), but first I wanted to run this by y'all. Is there room in the official a5 API for something like al_get_indexed_palette for applicable images?

[SiegeLord]

Maybe al_load_{png/bmp}_with_pallete

[TRDario]

I'm currently working on a few drawing related functions, so I could try my hand at making it. How should I go about the function(s)? After looking at the code, I see that a few other image formats have a palette being read into, so maybe it would be easier to just have one al_load_image_with_palette and write in the docs which image formats we support (I've checked that bmp, png, tga, pcx all have it, while dds doesn't)? (In which case, what would be done with unsupported formats, load normally and zero-out the palette or make it fail somehow?)

Here's my idea in short: struct ALLEGRO_PALETTE { ALLEGRO_COLOR p[256]; }; al_load_image_with_palette(const char *filename, ALLEGRO_PALETTE *palette); al_load_image_with_palette_f(ALLEGRO_FILE *fp, const char *ident, ALLEGRO_PALETTE *palette); al_load_image_flags_with_palette(const char *filename, int flags, ALLEGRO_PALETTE *palette); al_load_image_flags_with_palette_f(ALLEGRO_FILE *fp, const char *ident, int flags, ALLEGRO_PALETTE *palette);

[pedro-w]

Just thinking out loud but how about storing the palette in the ALLEGRO_BITMAP during loading and providing an accessor function for it?

int al_get_bitmap_palette(ALLEGRO_BITMAP*, ALLEGRO_PALETTE*); // return value = number of entries / zero for no palette

There would be a bit of overhead due to adding a field to the bitmap structure, and more (1024 bytes?) if you loaded a bitmap with a palette which you didn't care about. The latter could be eliminated by having a flag ALLEGRO_WANT_PALETTE which would be passed to the loader like ALLEGRO_KEEP_INDEX et al are now. Without the flag, the palette would not be stored, same as the current situation.

[TRDario]

I've written a way to load bitmaps and copy the palette data over at my fork of Allegro (though not the get_bitmap_palette suggestion by pedro-w), it's currently functional for bmp, pcx and tga. I'd like to hear what people here think before i open a pull request.

[pedro-w]

A bit of code beats pontification any day! My only concern was that it was doubling up the number of bitmap API functions.

One more thought - would we ever want to save an image with a palette?

[TRDario]

Unless we go through with integrating the palette into the bitmap structure, doubling up seems to be the only way to me (unless we redefine all the non palette loading versions as passing null, which could be possible but would break compatibility with older code).

Personally, after writing this implementation I'm liking your idea of storing it in the struct more. Seems much less messy and it's not too much overhead (1kb at worst, but we could probably implement it as a pointer to optionally allocate anyway).

As for the saving with palette thing, it sounds very niche. Getting the palette is already quite borderline in my opinion, and it wouldn't exactly be a simple addition. Maybe I'm just forgetting a use case.

[connorjclark]

If a bitmap is loaded with ALLEGRO_KEEP_INDEX, and you modify some pixel's index values, does saving the bitmap preserve the palette (with the current impl.)?

Anyway, for my usecase I wouldn't need to save an allegro 5 bitmap. Since I'm going through allegro 4 anyway, I can just pass the palette data along to the allegro 4 bitmap APIs :)

[TRDario]

Hm, I haven't actually checked that. I'll take a look tomorrow.

[pedro-w]

I had a go at implementing a "get palette" function, just to see how it might pan out. Only implemented for Windows BMP at the moment and it also uses an egregious hack "internal function" to put the palette data into the bitmap. Also I'm not 100% sure I've covered all the memory management cases. Have a look: https://github.com/pedro-w/allegro5/tree/palette-access

[TRDario]

I'm not sure if I'm just not seeing it in the docs but I don't see a way to save indexed bitmaps in their original format? If I save it, it just gets saved as a 24-bit image with the indexes as the red color channel.

Edit: After thinking about it, it makes sense since we currently throw the palette away. If we embed it into the bitmap we could also add a way to save them in the original format.

[connorjclark]

Yeah, you aren't missing anything. I was only like 95% sure it would just be saved as true color with the data stuffed in a single color.

@pedro-w code seems reasonable to me.

Might this need to be a while loop: https://github.com/liballeg/allegro5/compare/master...pedro-w:allegro5:palette-access#diff-58f8fda2e5a0957ccd13c2519cb6c42acc4c58b17ce91406eef2db86217f0b0bR849 line 849

[TRDario]

bitmap->parent always stores the top level bitmap according to the documentation, so a while loop isn't necessary in this case.

Anyway, this implementation certainly looks reasonable to me.

[pedro-w]

I just wanted to see if it was feasible/reasonable; those who are Skilled In The Art will be able to implement it more nicely I'm sure.

Regarding saving indexed bitmaps, I think if someone wants general multi-format image IO in their Allegro program they should probably make use of another library e.g. devil or whatever is current these days, it's a bit out of scope for allegro_image