PAM loading not working #2

New Issue

goulash-lover · 2022-12-24T02:46:50-08:00

goulash-lover commented

2022-12-24 02:46:50 -08:00

Hello, using the recently updated 1.9.1 version in Gentoo, I tried to view a PAM image (freshly created via magick convert my.png my.pam) with the following header:

P7
WIDTH 674
HEIGHT 859
DEPTH 4
MAXVAL 255
TUPLTYPE RGB_ALPHA
ENDHDR

with imlib2_view but I got No loadable image and no further error. Same if I convert the image so that its TUPLTYPE becomes just RGB.

Reading the source code for it, I don't really understand that line:
https://git.enlightenment.org/old/legacy-imlib2/src/branch/master/src/modules/loaders/loader_pnm.c#L157
that seems to expect width to be 332.

Hello, using the recently updated 1.9.1 version in Gentoo, I tried to view a PAM image (freshly created via `magick convert my.png my.pam`) with the following header: ``` P7 WIDTH 674 HEIGHT 859 DEPTH 4 MAXVAL 255 TUPLTYPE RGB_ALPHA ENDHDR ``` with imlib2_view but I got `No loadable image` and no further error. Same if I convert the image so that its `TUPLTYPE` becomes just `RGB`. Reading the source code for it, I don't really understand that line: https://git.enlightenment.org/old/legacy-imlib2/src/branch/master/src/modules/loaders/loader_pnm.c#L157 that seems to expect width to be 332.

kw commented

2022-12-24 03:43:08 -08:00

Yes, imlib2 has never supported PAM ("P7\n" signature) images.
The code you are referring to is for loading a somewhat obscure image type ("P7 332\n" signature) used by xv for thumbnails.

Yes, imlib2 has never supported PAM ("P7\n" signature) images. The code you are referring to is for loading a somewhat obscure image type ("P7 332\n" signature) used by xv for thumbnails.

goulash-lover commented

2022-12-24 04:07:40 -08:00

I see. I might try my hand at it, would it be acceptable? If I were to do it, I'd probably dispatch to pbm/pgm/ppm on non-alpha cases and write a new loop for each alpha variant.

Might be worth to use inline functions (i.e. load_rgb, load_bw, etc...) with a boolean alpha argument to get all the speed advantages without having to duplicate each loop.

By the way, what is P8? Can't find anything about it on the web, and it seems similar to RGB_ALPHA PAM.

I see. I might try my hand at it, would it be acceptable? If I were to do it, I'd probably dispatch to pbm/pgm/ppm on non-alpha cases and write a new loop for each alpha variant. Might be worth to use inline functions (i.e. load_rgb, load_bw, etc...) with a boolean alpha argument to get all the speed advantages without having to duplicate each loop. By the way, what is P8? Can't find anything about it on the web, and it seems similar to RGB_ALPHA PAM.

kw commented

2022-12-25 02:07:37 -08:00

You are most welcome to contribute :)
I have now and then considered to add pam support but never got around to it.

I don't know where the P8 type comes from. I cannot find anything about it anywhere either. It looks like an imlib2 special used for images with alpha.

You are most welcome to contribute :) I have now and then considered to add pam support but never got around to it. I don't know where the P8 type comes from. I cannot find anything about it anywhere either. It looks like an imlib2 special used for images with alpha.

goulash-lover commented

2023-01-11 08:30:04 -08:00

I've finished and tested the feature, but before I make a PR, I want to know: is the code base C89 or C99? Since I see <stdbool.h> includes here and there but ANSI style variable declarations, I don't really know.

kw commented

2023-01-11 09:34:31 -08:00

Interesting :)

C99 is required as there are a few elements that are used here and there, such as stdbool, stdint, designated initializers, C++ style comments, and probably some more.
However, I'd like the codebase to be somewhat homogeneous, so please keep new code reasonably similar to what's already there.

I'm afraid I don't unsterstand what you mean by "ANSI style variable declarations".

When you run configure, please add --enable-werror to weed out the most trivial issues, in case there are any.

As for formatting, you can simply run like "$ indent src/modules/loaders/*.?" (or not and I will :) ).

Interesting :) C99 is required as there are a few elements that are used here and there, such as stdbool, stdint, designated initializers, C++ style comments, and probably some more. However, I'd like the codebase to be somewhat homogeneous, so please keep new code reasonably similar to what's already there. I'm afraid I don't unsterstand what you mean by "ANSI style variable declarations". When you run configure, please add --enable-werror to weed out the most trivial issues, in case there are any. As for formatting, you can simply run like "$ indent src/modules/loaders/*.?" (or not and I will :) ).

goulash-lover commented

2023-01-11 10:48:57 -08:00

Meant "variables declared at the top of the scope" with "ANSI style variable declarations".