Some problems with "csource"

[Owen Taylor <otaylor redhat com>]

The pressing problem is that the name gdk_pixbuf_new_from_stream():

  * A stream is something like a FILE * or a GIOChannel. GdkPixbufLoader is
    our stream API currently.

  * This function can't load arbitrary types of content from memory,
    but only the "gdkpixbuf inline format".

Considering only the first principle, a name like
gdk_pixbuf_new_from_memory() sounds OK, but it isn't so great on the
second principle. I'd prefer the original name,
gdk_pixbuf_new_from_inline() which is mnemonic for the most common
case, and doesn't imply that arbitrary formats can be loaded.

There is also the problem with the ordering of stream_length/stream
in:

GdkPixbuf* gdk_pixbuf_new_from_stream	(gint          stream_length,
					 const guint8 *stream,
					 gboolean      copy_pixels,
					 GError      **error);
       
but I guess my chance of convincing you of that is minimal. :-(


I also had some issues with gdk-pixdata.h and the "gdk-pixbuf-csource"
program, but I'm not so concerned with those, since I think most
people will just ignore gdk-pixdata.h, and will just cut-and-paste
the magic gdk-pixbuf-csource command line from some other module.

 * I don't really understand the reason for exposing GdkPixdata at all... 
   if you want an in-memory, manipulable representation, you have GdkPixbuf. 
   If you want a representation to store as static data, you have
   the serialied form.

 * SAMPLE_WIDTH strikes me as a non-standard name. bits_per_sample is
   the name in gdk-pixbuf, so SAMPLE_BITS would be a reasonable name.

  typedef struct _GdkPixdata GdkPixdata;
  struct _GdkPixdata
  {
    gint32  length;       /* <1 to disable length checks, otherwise:
		  	   * GDK_PIXDATA_HEADER_LENGTH + pixel_data length
			   */

 * I don't quite understand why this is optional ... this seems that it 
   sort of ruins the point of having it if the caller can't depend   
   on it being there.

  guint32 rowstride;    /* maybe 0 to indicate non-padded data */

 * Again, why not just require it to be set and correct, at least in the 
   non-RLE case?

 * As a more general point, I tend to think the choice of what to 
   make the return value for:

   guint8* gdk_pixdata_serialize (const GdkPixdata	*pixdata,
				  guint			*stream_length_p);

   is backwards. The bytes are not useful without the length, but    
   the length could, in some cases, be useful before you retrieve the
   data. (Even more the case for many similar functions.) I know you use 
   this ordering in GObject, but in most parts of GTK+ we just make both 
   out parameters.
   
 * GString* gdk_pixdata_to_csource (GdkPixdata		*pixdata,
			    	    const gchar		*name,
				     GdkPixdataDumpType	 dump_type);

   What's the reason for making this part of the API? Why does it
   return a GString unlike all the rest of the functions in our APIs?


And gdk-pixbuf-csource has more options than I really think are necessary.

     --struct                   generate GdkPixdata structure

Why would you want this?

     --macros                   generate image size/pixel macros

Why would you want this? The output in this case seems unrelated
to anything that is useful in conjunction with gdk-pixbuf.

     --decoder                  provide rle decoder

Same as the last. If you do have it, specifying it with --macros
should be an error, it should not silently do nothing.

     --g-fatal-warnings         make warnings fatal (abort)

Shoudl this be in the --help output?

Also, I'd like to see --raw as the default. If you don't care about
saving a copy, then you probably should be using PNG, not RLE anyways.

Regards,
                                        Owen