Re: PCX support for gdk-pixbuf

[Owen Taylor <otaylor redhat com>]

"Terry 'Mongoose' Hendrix II" <stu7440 westga edu> writes:

> Well, I figured I'd dump the PCX loader on you guys too - I'll be moving
> these to tmp file less progressive loaders this week as I port my
> modeler to gnome.  This prob isn't useful as the TGA, unless you do a
> lot of quakeforge development.  =)

Thanks for the submission. I took a scan through it, and through
the PCX spec and had a few comments:

 * As has been mentioned before, there really is not much point in
   writing this for gdk-pixbuf-0.x, which will be obsolete
   in a few months.
   
   There are various porting issue for the gdk-pixbuf included in
   GTK+ 1.3.x, and someone is going to have to do the port, or
   the work you've done here will be lost.

 * Allocation of the image data should be done with malloc()
   [ old gdk-pixbuf ] or g_try_malloc() [ GTK+-1.3.x ].

   g_malloc() never fails; it aborts on out-of-memory

 * Errors should be reported back through the g_error() 
   mechanism for GTK+-1.3.x.

 * I'm not sure it makes sense to have a loader for only a subset
   of PCX files as this one is. (That is, it seems to me 
   that since it wouldn't be more than maybe 50 lines of code, 
   supporting non-indexed PCX files as well would be
   a good idea.)

 * You don't handle BytesPerLine as described in the PCX spec, so I
   think you won't correctly load odd-width images.

 * When decoding RLE data, you don't protect against over-running
   your buffer on an invalid image which has more data then
   its size.

 * Using the TIFF save-to-temp file hack is pretty horrendous
   here; for TIFF, the excuse is limitations of the tiff library,
   but you don't have that excuse ;-)

Regards,
                                        Owen