After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 159947 - saving 1bpp PCX Files
saving 1bpp PCX Files
Status: RESOLVED FIXED
Product: GIMP
Classification: Other
Component: Plugins
2.1.x
Other All
: Low enhancement
: 2.10
Assigned To: GIMP Bugs
GIMP Bugs
: 777566 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2004-11-30 08:38 UTC by axel schwarz
Modified: 2017-06-11 22:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch candidate for allowing indexed PCX images to save as 1bpp images. (13.00 KB, patch)
2017-05-10 12:23 UTC, Nikc M.
committed Details | Review
2nd patch - cleans up code, combines two methods into one, loads 1bpp palette from file instead of assuming BW (8.02 KB, patch)
2017-05-14 13:50 UTC, Nikc M.
committed Details | Review
Test PCX images for reviewing purposes (245.35 KB, application/x-zip-compressed)
2017-05-14 13:51 UTC, Nikc M.
  Details
Color Monochrome test image (19.58 KB, application/x-zip-compressed)
2017-06-07 23:20 UTC, Nikc M.
  Details

Description axel schwarz 2004-11-30 08:38:56 UTC
Create a PCX File
save as rgb, ok , saved with 24bpp colors

change image mode to greyscaled
save it, ok 8bpp colors

change image mode to indexed (1bit)
save it, ooops, saved with 8bpp colors.

in short: gimp 2.05 or / and 2.2 pre are not able
to save 1bpp pcx files. (pcx version 0)
Comment 1 Sven Neumann 2004-11-30 10:08:59 UTC
GIMP doesn't have an image format for 1bpp so it's not surprising that the PCX
plug-in doesn't support this. If you really need this feature, I suggest that
you add the missing code to the plug-in that tests the number of colors used in
the colormap and saves as 1bit PCX if that is the case.
Comment 2 axel schwarz 2004-11-30 10:27:37 UTC
Hmm, great solution.

Cause I am not a real script writer and I am not so familiar with gimp
than yours, so please can you give a little bit more information
which plugin I need to edit?

Anyway, I think it is a bug in gimp, and not only a missing feature.
But I know that pcx is not the favourit image format todays.
Comment 3 Sven Neumann 2004-11-30 11:49:36 UTC
It is a missing feature and if you need it, you should either implement it
yourself or get someone else to do it for you. The code in question is in
plug-ins/common/pcx.c (obviously).
Comment 4 Joao S. O. Bueno 2004-11-30 12:47:15 UTC
See also the discussion on bug 150865. 
And the PNG file format saves as 1 bpp already. 
Comment 5 axel schwarz 2004-11-30 13:11:15 UTC
Ok, thanks for the fast help.
I got the tiff patch.
maybe I can use it for the pcx.c

Comment 6 Nikc M. 2017-05-10 12:23:27 UTC
Created attachment 351540 [details] [review]
Patch candidate for allowing indexed PCX images to save as 1bpp images.

Here's my attempt at a solution. It allows file-pcx.c to save 1bpp and 4bpp/16 color files (if indexed) based on number of colors used. Also created a parameter-based version of the load function to allow 2bpp PCX files to be loaded.

To test functionality, I saved and opened PCX files between GIMP and IrfanView 4.44 (64bit), which can open 1bpp+ images.
Comment 7 Jehan 2017-05-10 22:35:53 UTC
Review of attachment 351540 [details] [review]:

Looks quite good, thanks!

There are a few syntax/coding style bugs here and there: try to avoid white spaces at end of line, have a space between for/if and parentheses, indent blocks by 2 characters (GNU coding style).
Also in save_less_than_8(), you didn't free the buffer "line". Nothing I can't fix myself, so don't worry about these.

Other than these, I am trying to understand the format a little more.
You are adding 2bpp and 4bpp. Should we interpret these as greyscale with 2bpp and 4bpp?
Is 3 planes of 1bpp a RGB with only 2 values per channel? What about 2 planes of 1bpp?
Checking Wikipedia, there is a "Common PCX Image Formats" table, but it does not propose these values.

Finally do you have example files of PCX with these different formats and do you know by any chance which other Free Software (under Linux) can open and create PCX files? Irfanview seems to be Windows-only unfortunately and I fail to find another image editor which supported these.
Comment 8 Jehan 2017-05-10 22:52:24 UTC
Answering to myself… though I can't find a clear spec, I can find a few pages explaining the format, but never exhaustively.

This for instance says that 3 planes of 1 bpp is 8 color and 2 planes of 1bpp is a 4 color image:
http://www.fysnet.net/pcxfile.htm
So I assume it uses the header palette? What about this CGA vs EGA color palettes?

This is all very unclear and I can't find a good and exhaustive reference. Examples files would really help too (of course not example files created by your patched GIMP. Otherwise we can't compare and it's meaningless :p).

Thanks again!
Comment 9 Nikc M. 2017-05-11 13:05:35 UTC
Hello! Thanks for the feedback - I'll make those formatting changes and resubmit (I also realized I could combine load_by_planes() and load_by_bpp into one function, which would be a better use of space as well).

You may already know this from your own research and experience, but I'll post it here since it confused me for quite a while.
Essentially, 'X' bits per pixel means you read in 'X' many bytes sequentially and determine the color from that (E.g. 11 is the fourth color in the indexed palette for a 2bpp image). 'X' number of planes means you have that many rows (usually one bit per row), and the bits are added up to form the pixel.
For instance, imagine we have a 5 pixel wide image (16 possible colors), and we want the first pixel to be 0110. We would write that like this:

00000
10000
10000
00000

Those four lines are equal to one row of pixels - it runs through them and "adds" the color to each pixel until it's done and has the final value.

Basically, EGA is an older version of IBM's VGA screen display, and CGA is an older version of EGA. It's a hardware limitation that most people don't have to deal with anymore. From my understanding and testing, the end of file palette supercedes the one in the header if it exists, but as long as you don't have more than 16 colors, the header palette is used. 

Apologies for not posting the test PCX files from outside sources - I'll add them here when I get back to the computer I have them saved on. I'll also try to find another open-source editor that saves in those formats so you don't have to take my word for it, haha.
Comment 10 Michael Schumacher 2017-05-11 13:32:36 UTC
The Wikipedia PCX article offers a nice overview of the file format: https://en.wikipedia.org/wiki/PCX
Comment 11 Jehan 2017-05-11 13:34:13 UTC
> Hello! Thanks for the feedback - I'll make those formatting changes and resubmit (I also realized I could combine load_by_planes() and load_by_bpp into one function, which would be a better use of space as well).

Cool. :-)

> Essentially, 'X' bits per pixel means you read in 'X' many bytes sequentially and determine the color from that (E.g. 11 is the fourth color in the indexed palette for a 2bpp image). 'X' number of planes means you have that many rows (usually one bit per row), and the bits are added up to form the pixel.

From my understanding, you can also have non-indexed pixel, no? For instance 8bpp with 3 planes means basically 32bit RGB and each plane is actually a color channel. There is no index.
But then all other combination of bpp/plane would be indexed colors (either from header when color < 16 or EOF index otherwise)?

P.S.: adding the 2.10 milestone.
Comment 12 Jehan 2017-05-11 13:36:52 UTC
(In reply to Michael Schumacher from comment #10)
> The Wikipedia PCX article offers a nice overview of the file format:
> https://en.wikipedia.org/wiki/PCX

Yes saw it, but unfortunately not very extensive. But I'm starting to understand it better now with the few descriptions I found online (like my link in comment 8) and comment 9 from Nikc.
Comment 13 Nikc M. 2017-05-14 13:50:33 UTC
Created attachment 351831 [details] [review]
2nd patch - cleans up code, combines two methods into one, loads 1bpp palette from file instead of assuming BW

I believe this patch has to be applied after the first one (351540: Patch candidate for allowing indexed PCX images to save as 1bpp images). It should be closer to the coding standard, and combines load_by_bpp and load_by_plane into one method (load_sub_8).

I also removed the hard-coded Black & White palette for loading 1bpp images, since a lot of images I came across had color monochrome palettes.

Test images in the next comment.
Comment 14 Nikc M. 2017-05-14 13:51:33 UTC
Created attachment 351832 [details]
Test PCX images for reviewing purposes

Attached are a few PCX test images, and PNG versions for comparison. It should cover the range of new PCX types that can be loaded.

ImageMagick seems to be an open source program that can view PCX files. https://www.imagemagick.org/script/download.php
I have not installed or tested it, so I am unsure of its range.
Comment 15 Jehan 2017-05-14 19:19:27 UTC
Thanks, I'll have a look soon.

Just a note. I notice your comment:

> /* Bytes per Line must be an even number, according to spec */

Is there a real spec somewhere which I could read? I could not find one, but if there were, that would make the review much easier! :-)
Comment 16 Nikc M. 2017-05-15 17:45:17 UTC
This is the most official document I could find, since the company itself went under:
http://bespin.org/~qz/pc-gpe/pcx.txt

Unfortunately, a lot of PCX files in the wild don't follow every part of this spec, so I've tried to aggregate it with other sources (such as the ones you've linked) and test images I've found. Hooray for old image formats!
Comment 17 Jehan 2017-05-23 21:03:09 UTC
For info, I did not forget you. Just trying to make the time to properly review.
Comment 18 Nikc M. 2017-05-25 18:19:04 UTC
Thanks for the update! But don't worry about the time - I realize this isn't exactly the most important thing to be worked on for GIMP. :)
Comment 19 Michael Natterer 2017-06-06 22:18:57 UTC
*** Bug 777566 has been marked as a duplicate of this bug. ***
Comment 20 Jehan 2017-06-07 16:55:35 UTC
Review of attachment 351831 [details] [review]:

Ok I have reviewed today. I will clean a few things, but it looks good.
Just want to make sure of a few things though:

(In reply to Nikc M. from comment #9)
> From my understanding and testing, the end of
> file palette supercedes the one in the header if it exists, but as long as
> you don't have more than 16 colors, the header palette is used.

According to this link: http://www.fysnet.net/pcxfile.htm
Check "Interpretation of the PCX data" section: 2bpp/1plane and 4bpp/1plane would use the palette at EOF (not the one from the header) if we are in version 5. In your code, you just always use the header palette (and pcx version is never checked upon loading), which corresponds to your assumption above (header palette always used for less than 16 colors).

Are you sure about this assumption? If you are sure, let's keep it as-is. I just have no idea.

> I also removed the hard-coded Black & White palette for loading 1bpp images, since a lot of images I came across had color monochrome palettes.

Is this sure as well? For instance, I see that ImageMagick sees the file "CGA_1bpp - 1plane.PCX" from your examples as a B&W image. But of course, it is highly possible that ImageMagick implementation is wrong as well.
Where did you get this image from? I could not find any decisive information on this particular case.

Anyway it looks nice.
Comment 21 Jehan 2017-06-07 16:55:38 UTC
Review of attachment 351831 [details] [review]:

Ok I have reviewed today. I will clean a few things, but it looks good.
Just want to make sure of a few things though:

(In reply to Nikc M. from comment #9)
> From my understanding and testing, the end of
> file palette supercedes the one in the header if it exists, but as long as
> you don't have more than 16 colors, the header palette is used.

According to this link: http://www.fysnet.net/pcxfile.htm
Check "Interpretation of the PCX data" section: 2bpp/1plane and 4bpp/1plane would use the palette at EOF (not the one from the header) if we are in version 5. In your code, you just always use the header palette (and pcx version is never checked upon loading), which corresponds to your assumption above (header palette always used for less than 16 colors).

Are you sure about this assumption? If you are sure, let's keep it as-is. I just have no idea.

> I also removed the hard-coded Black & White palette for loading 1bpp images, since a lot of images I came across had color monochrome palettes.

Is this sure as well? For instance, I see that ImageMagick sees the file "CGA_1bpp - 1plane.PCX" from your examples as a B&W image. But of course, it is highly possible that ImageMagick implementation is wrong as well.
Where did you get this image from? I could not find any decisive information on this particular case.

Anyway it looks nice.
Comment 22 Nikc M. 2017-06-07 23:20:20 UTC
Created attachment 353365 [details]
Color Monochrome test image

A color monochrome image (with a PNG comparision shot) to show a non-black and white 1 bpp/1 plane PCX sample.
Comment 23 Nikc M. 2017-06-07 23:21:07 UTC
Thanks for looking over the patch again!  

To answer your questions:
=============================
Palette: Unfortunately, this is another case where the available information differs. :)

This article (http://www.drdobbs.com/pcx-graphics/184402396) mentions that the expanded EOF palette is only used if there are more than 16 colors, which makes logical sense (why tack on an additional 768 bytes to the file that your image will never be able to use?).

I haven't come across a PCX file in the wild that had less than 16 colors and also had an EOF palette. 

Therefore, I went with what I saw. I wouldn't stake my life on it, but I think it's a solid assumption.
=============================
=============================
Black/White: I wouldn't say ImageMagick's implementation is "wrong", they just decided not to handle special cases and just assumed everything to be black/white. 

Using the same link, (http://www.fysnet.net/pcxfile.htm), 1bpp/1 plane is specified as "monochrome" which doesn't automatically mean black & white. It just means that you have only two colors you can use. For instance, old green and black monitors were monochrome.

In ~95% of the 1bpp/1plane cases, they were intended to be black and white. The PCX header palette bytes read (00,00,00) then (FF,FF,FF) - so, whether we assume a black/white palette in GIMP or read the palette in from the header, it still produces a black/white image in the standard case.

Sometimes however, the two colors in the header palette are different, and I wanted to respect that information from the file rather than force the palette to be black and white.
I've attached another sample of lena.pcx I found to demonstrate. Its header palette (starting at hex 10) is BF,AC,74 and 91,32,5A, creating a purple/yellow monochrome image instead of the assumed black/white.

Since the standard case renders properly either way, I figured relying on the PCX file's palette for colors in 1bpp/1plane images would produce the most accurate images on load.
=============================

Let me know if you have further questions or if I need to provide more details!
Comment 24 Jehan 2017-06-09 11:20:46 UTC
Ok pushed.

commit 8d4642f4bad38249d17cf977abbea15c76998193
Author: Nikc M <nikc.gimp@gmail.com>
Date:   Wed May 10 07:56:28 2017 -0400

    Bug 159947 - saving 1bpp PCX Files
    
    Allows saving 1bpp and 4bpp indexed PCX files based on number of colors
    used in image. Also provides ability to load additional PCX formats:
    2bpp, 2 planes 1bpp, 3 planes 1bpp, 4bpp.

 plug-ins/common/file-pcx.c | 295 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 233 insertions(+), 62 deletions(-)

commit daa5611e6f353e2887900af3add91915da0ef852
Author: Nikc M <nikc.gimp@gmail.com>
Date:   Sat May 13 23:30:53 2017 -0400

    Fix for Bug 159947 - saving 1bpp PCX files
    
    +Allows user to save indexed 1bpp and 4bpp PCX files automatically,
    depending on the number of colors used.
    
    +Creates a general function with parameters for both bpp and # of
    planes, to handle more PCX file types (1bpp, 2planes; 1bpp, 2planes;
    4bpp, 1plane).
    
    +Removes assumption when loading colormap that 1bpp files are black &
    white only; loads both colors from PCX palette header instead.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=159947

 plug-ins/common/file-pcx.c | 101 +++++++++++++++++++++++++++++++++++++++--------------------------------------------------------------
 1 file changed, 39 insertions(+), 62 deletions(-)

commit dc069e424ab0c38c3f4ed0d91630a4d3768ae457
Author: Jehan <jehan@girinstud.io>
Date:   Fri Jun 9 12:56:03 2017 +0200

    plug-ins: coding-style fix, adding comments and removing unused var.
    
    Commits 8d4642f and daa5611 reviewed. I was only unsured on usage of
    header vs EOF palette as well as the non-B&W monochrome palette since we
    don't find a single authoritative spec. Nevertheless the implementer has
    good argumentation so let's go with this. Simply I add some comments
    about these 2 points in the code, just in case for future references.

 plug-ins/common/file-pcx.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------
 1 file changed, 56 insertions(+), 41 deletions(-)
Comment 25 Jehan 2017-06-09 11:30:48 UTC
- About the B&W Monochrome, I was mostly afraid of the case where a software would assume that 1bpp/1plane means B&W and would not even bother filling the header palette (so maybe the values there would just all be 0s or random or whatever. But if you think this would not happen, let's go with your implementation.

- About the header vs EOF palette, I noticed this in the article you linked in comment 23: http://www.drdobbs.com/pcx-graphics/184402396

>  Only Version 5 PCX-format files support 256-color palettes, and then only when the image has more than 16 colors. ZSoft recommends the following technique to determine if a 256-color palette is present: first verify that the file version number is 5, then count back 769 bytes from the end of the file. If the value of this byte is not 0x0c, the optional 256- color palette is not present and the EGA/VGA 16-color file header palette should be used.
>
> It is possible that a Version 5 PCX-format file with a valid file header palette can have the value 0x0c in the 769th byte from the end of the encoded image data. The above technique would then falsely indicate the presence of an appended 256-color palette. To avoid this problem, it is necessary to first decode the image and note the file position where the encoded image data section ends before counting back 769 bytes from the end of the file. If the supposed 256-color palette flag is located in the image data section, then the file header palette should be used instead.

So this agrees mostly with you and we should likely continue to "save" the header palette with less than 16 colors. Yet it seems to imply that it is *possible* to have an EOF palette even with less than 16 colors (otherwise why bother checking the existence? Just check the number of colors as we do) and that's a double security to check the palette presence on "loading".

Therefore I wonder if we should implement their proposed algorithm upon loading PCX files.

What do you think? If you believe your current implementation is all right, let's just close this bug report as-is. If you think it is worth adding this additional EOF palette check upon loading, a new patch is welcome. Just tell me. :-)
Comment 26 Jehan 2017-06-09 11:40:08 UTC
Review of attachment 351540 [details] [review]:

Pushed.
Comment 27 Jehan 2017-06-09 11:40:24 UTC
Review of attachment 351831 [details] [review]:

Pushed.
Comment 28 Jehan 2017-06-11 22:01:47 UTC
Anyway your patches are good and pushed. I'll just close this report as FIXED.

If you believe my remark from comment 25 makes sense and that we should implement the algorithm to test the presence of the EOF palette, feel free to open a new feature request with a patch. For the time being, that looks good! :-)