Re: gdk-pixbuf-save

[Owen Taylor <otaylor redhat com>]

OK, I finally had a chance to look at your patch - I've just gotten around
to integrating gdk-pixbuf into GTK+-1.3 after spending a lot of time on
other issues. It looks pretty good, and I see no reason it shouldn't go
in. 

I do have a number of comments and suggestions, of course :-)

                                        Owen

"David N. Welton" <davidw@linuxcare.com> writes:

> Ok, I have taken the mini-library apart and put it back together as
> part of gdk-pixbuf.  It seems to work ok for jpeg's and png's, the two
> image types for which there exist save functions.
> 
> Being rather small, I have attached the patch to this message.  I will
> also post it to http://giga.prosa.it/~davidw/.

+}
+
+ /**
+ * gdk_pixbuf_save_to_file:
+ * @filehandle: FILE* to save to.
+ * @format: name of file format.
+ *
+ * Saves pixbuf to a filehandle.
+ * 
+ **/

You are missing the return value and a bunch of args from the DOC comment.

+gboolean gdk_pixbuf_save_to_filehandle (GdkPixbuf *pixbuf, FILE *filehandle, 
+					char *format, GdkPixbufInfo *imginfo)

The parts of gdk-pixbuf not directly integrated into GDK currently follow
GNOME coding guides; by these standards, the return value needs to be on a 
separate line. GTK+ standards which are slightly different also require
the args to be aligned:

gboolean 
gdk_pixbuf_save_to_filehandle (GdkPixbuf     *pixbuf,
                               FILE          *filehandle, 
			       char          *format, 
                               GdkPixbufInfo *imginfo)

It might be nice to do this for new code in gdk-pixbuf. (Yes, we are picky
bastards.)

The format argument needs to be const.

+{
+	int i;
+	GdkPixbufModule *image_module = NULL;
+	
+	if (filehandle == NULL)
+	{
+		g_warning ("Filehandle is null");
+		return FALSE;
+	}    

gdk-pixbuf has linux-kernel style indentation

	if (filehandle == NULL) {
		g_warning ("Filehandle is null");
		return FALSE;
	}    

[
 The rest of GTK+ follows GNU style indentation - 

	if (filehandle == NULL) 
          {
            g_warning ("Filehandle is null");
	    return FALSE;
	  }    
]

However, the GNOME and GTK+ way of writing this sort of check is:

 g_return_val_if_fail (filehandle != NULL, FALSE);


+	for (i = 0; file_formats[i].module_name; i++) 
+	{
+		if(!strncmp(file_formats[i].module_name, format, strlen(format)))

You need a space between the 'if' and the '('.

Do you really want 

 save_to_filehandle (pixbuf, filehandle, "pngASDFG", blah)

To work? I would suggest a simple strcmp, or maybe g_strcasecmp().

+	if (image_module->module == NULL)
+		gdk_pixbuf_load_module (image_module);
+	
+	if (image_module->save == NULL) 
+	{
+		fclose (filehandle);
+		return FALSE;
+	}

It would be nice if we had GException .. but anyways.
	
+	(* image_module->save) (filehandle, pixbuf, imginfo);

I suspect the save module should be allowed to fail internally and return
a status:

 return (* image_module->save) (filehandle, pixbuf, imginfo);

+	fclose (filehandle);

If you don't open the filehandle, don't close it. If the file handle came
from popen(), then you would need pclose() to close it, etc. If you were
closing the stream, you would need to check for error here.

+	return TRUE;
+}
+
+/**
+ * gdk_pixbuf_save_to_file:
+ * @filename: Name of file to save.
+ * @format: name of file format.
+ *
+ * Saves pixbuf to a file.
+ * 
+ **/

Again, this doc comment is going to give gtk-doc fits since it doesn't
match the prototype.

+gboolean gdk_pixbuf_save_to_file (GdkPixbuf *pixbuf, const char *filename, 
+				  char *format, GdkPixbufInfo *imginfo)
+{
+	int i;
+	FILE *f;
+	
+	f = fopen (filename, "w");
+	if (!f)
+	{
+		g_warning ("Cannot open for write: %s", filename);
+		return FALSE;

There should not be a g_warning() here, since this is a error that a program
needs to expect and should be able to handle in a user-friendly fashion. 
You should simply return FALSE. (It would be nice if we had GException...)

+	}    
+	return gdk_pixbuf_save_to_filehandle(pixbuf, f, format, imginfo);
 }

diff -r -N -u /home/davidw/download/gdk-pixbuf/gdk-pixbuf/gdk-pixbuf-io.h gdk-pixbuf/gdk-pixbuf/gdk-pixbuf-io.h
--- /home/davidw/download/gdk-pixbuf/gdk-pixbuf/gdk-pixbuf-io.h	Fri Jan  7 19:29:13 2000
+++ gdk-pixbuf/gdk-pixbuf/gdk-pixbuf-io.h	Tue May 23 22:39:22 2000
@@ -55,6 +55,7 @@
 	gboolean (* format_check) (guchar *buffer, int size);
 	GModule *module;
 	GdkPixbuf *(* load) (FILE *f);
+        int (* save) (FILE *f, GdkPixbuf *pixbuf, GdkPixbufInfo *imginfo);

Wait, you do have a return value here - you should observe it in your wrapper
routines then ... also, if it is a boolean, it should be gboolean here, not int.

         GdkPixbuf *(* load_xpm_data) (const char **data);
 
         /* Incremental loading */

@@ -45,6 +46,7 @@
 typedef struct _GdkPixbuf GdkPixbuf;
 typedef struct _GdkPixbufFrame GdkPixbufFrame;
 typedef struct _GdkPixbufAnimation GdkPixbufAnimation;
+typedef struct _GdkPixbufSaveModule GdkPixbufSaveModule;

You don't use this typedef any more...

 
 /* Handler that must free the pixel array */
 typedef void (* GdkPixbufDestroyNotify) (guchar *pixels, gpointer data);
@@ -100,6 +102,18 @@
 				     gpointer destroy_fn_data);
 
 GdkPixbuf *gdk_pixbuf_new_from_xpm_data (const char **data);
+
+/* It would probably be best if this were a void pointer to whatever
+   data we want to associate with an image */
+typedef struct {
+    int quality;
+} GdkPixbufInfo;

IMO, the right way to do it it with string pairs - this strikes a balance
between simplicity and flexibility:

gboolean gdk_pixbuf_save_to_file       (GdkPixbuf     *pixbuf,
					const char    *filename,
					const char    *format,
					...);
gboolean gdk_pixbuf_save_to_filev      (GdkPixbuf     *pixbuf,
					const char    *filename,
					const char    *format,
					char **        args);

The 'v' format would be the one that the module-interface would use.
(The args array is NULL terminated.)

So, you have:

 gdk_pixbuf_save_to_file (pixbuf, "my.jpg", "jpeg", "quality", "0.8", NULL);

Or something like that. People will sometimes have to do:

 char *val = g_strdup_printf ("%f", quality);
 gdk_pixbuf_save_to_file (pixbuf, "my.jpg", "jpeg", "quality", val, NULL);
 g_free (val);

But I think it is better than getting more complex, and better than fixing
the set of args.

+gboolean gdk_pixbuf_save_to_file (GdkPixbuf *pixbuf, const char *filename, 
+				      char *format, GdkPixbufInfo *imginfo);

+gboolean gdk_pixbuf_save_to_filehandle (GdkPixbuf *pixbuf, FILE *filehandle, 
+					char *format, GdkPixbufInfo *imginfo);
+

You definitely need to align your arguments here in the header file, luckily there is:

 http://people.redhat.com/otaylor/egtk.el

I think the names here should be a bit different. I would use:

 gdk_pixbuf_save (...const char *filename....)
 gdk_pixbuf_save_to_file (...FILE *filehandle...)

diff -r -N -u /home/davidw/download/gdk-pixbuf/gdk-pixbuf/io-jpeg.c gdk-pixbuf/gdk-pixbuf/io-jpeg.c
--- /home/davidw/download/gdk-pixbuf/gdk-pixbuf/io-jpeg.c	Tue Apr 11 12:02:27 2000
+++ gdk-pixbuf/gdk-pixbuf/io-jpeg.c	Wed May 24 20:03:33 2000
@@ -111,6 +111,28 @@
 	return;
 }
 
+static void 
+error_handler(j_common_ptr cinfo)
+{
+    struct error_handler_data *errmgr;
+    errmgr = (struct error_handler_data *) cinfo->err;
+    cinfo->err->output_message (cinfo);
+    siglongjmp (errmgr->setjmp_buffer, 1);
+    return;
+}
+
+static void 
+error_handler2(j_common_ptr cinfo, int msg_level)
+{
+    struct error_handler_data *errmgr;
+    errmgr = (struct error_handler_data *) cinfo->err;
+    cinfo->err->output_message (cinfo);
+    siglongjmp (errmgr->setjmp_buffer, 1);
+    msg_level = 0;
+    return;
+}
+
+
 /* Destroy notification function for the pixbuf */
 static void
 free_buffer (guchar *pixels, gpointer data)
@@ -168,6 +190,8 @@
 	/* setup error handler */
 	cinfo.err = jpeg_std_error (&jerr.pub);
 	jerr.pub.error_exit = fatal_error_handler;
+	jerr.pub.output_message = error_handler;
+	jerr.pub.emit_message = error_handler2;
 
 	if (sigsetjmp (jerr.setjmp_buffer, 1)) {
 		/* Whoops there was a jpeg error */
@@ -547,4 +571,89 @@
 	}
 
 	return TRUE;
+}
+
+int gdk_pixbuf__jpeg_image_save(FILE *f, GdkPixbuf *pixbuf, GdkPixbufInfo *imginfo)
+{
+	struct              jpeg_compress_struct cinfo;
+	guchar              *buf;
+	guchar              *ptr;
+	guchar              *pixels;
+	JSAMPROW           *jbuf;
+	int                 y = 0, quality = imginfo->quality;
+	int                 i, j;
+	int                 w, h = 0;
+	int rowstride       = 0;

This alignment is just a little odd, no?

+	struct              error_handler_data jerr;
+

diff -r -N -u /home/davidw/download/gdk-pixbuf/gdk-pixbuf/io-png.c gdk-pixbuf/gdk-pixbuf/io-png.c
--- /home/davidw/download/gdk-pixbuf/gdk-pixbuf/io-png.c	Tue Apr 11 12:02:27 2000
+++ gdk-pixbuf/gdk-pixbuf/io-png.c	Wed May 24 20:03:51 2000
@@ -522,3 +522,93 @@
         
         fprintf(stderr, "Warning loading PNG: %s\n", warning_msg);
 }
+
+int gdk_pixbuf__png_image_save (FILE *f, GdkPixbuf *pixbuf, GdkPixbufInfo *imginfo)
+{
+	png_structp         png_ptr;
+	png_infop           info_ptr;
+	guchar              *ptr;
+	guchar              *pixels;
+	int                 x, y, j;
+	png_bytep           row_ptr, data = NULL;
+	png_color_8         sig_bit;
+	int                 w, h, rowstride;
+	int                 has_alpha;
+	int                 bpc;
+
+	bpc = gdk_pixbuf_get_bits_per_sample(pixbuf);

'nother coding-style issue - need a space between 'sample' and '('.