Re: g_warnings in png loader

[Matthias Clasen <maclas gmx de>]

Am Mon, 2002-03-25 um 00.34 schrieb Owen Taylor:

> If the chunk is actually invalid, then warning is probablyappropriate;
> something is wrong and if we don't complain the problem has no chance
> of ever getting fixed.

Well, some of the other loader are not overly correct either when it
comes to rejecting invalid images, and this is only ancillary
information anyway. I dislike the use of g_warning for this purpose
because a) --g-fatal-warnings b) output to the console is likely to go 
unnoticed anyway, unless we manage to crash. It would be better if the
loaders could return warnings of this kind back to the application but
still succeed in saving the image. The application could then bring up
a nice dialog: 

The following warnings have been produced while loading foo.png:
Couldn't convert ...


> > in gdk_pixbuf__png_image_load:
> >  g_warning ("Got multiple tEXt chunks for the same key.");
> > 
> > (This is not forbidden by the PNG spec - with 1.2 you can have localized
> > itxt chunks, so there could be a "Description" for "de" and another one
> > for "en". I think we should silently pick one of the chunks, either
> > simply the first one, or try to be fancy and match the locale. I guess
> > adding API for localized pixbuf options would be over the top)  
>  
> Well, if that warning is printed for itxt chunks it would indeed
> be inappropriate (not least because it refers to tEXt chunks).

Not for every itxt chunk, but for those which come with localized
variants. itxt chunks may not be in very common use though - I couldn't
find a single png on my system containing an itxt chunk. But one can
construct pngs conforming to PNG-1.2 which will trigger this warning.

> Whether matching the locale or picking something independent of the
> locale is correct really depends on what people are using the
> text chunk API for, which I don't have much feeling for. (It was
> adding for thumbnailing information, as I recall, where locale 
> is not relevant.)

The thumbnail draft at least mentions the "Description" key, where
localization would be an advantage if the thumbnail is shared among
users of different locales - which I think they normally aren't. 

>  
> > in gdk_pixbuf__png_image_save:
> >  g_warning ("Bad option name '%s' passed to PNG saver", *kiter);
> > 
> > (This is a programming error which should be signalled by setting the
> >  GError).
> 
> Programming errors are precisely the ones that should _never_ be
> set in the GError, because you'd never want to write code to handle
> a programming error. GTK+ should always g_warning() when it detects
> a programming error.

OK, got that now :-) 
But another question related to this warning, which complains about
options missing the "tEXt::" prefix. Why do we insist on this ugly
prefix in the first place ? It doesn't add any useful information, 
makes the code more complicated and adds more error conditions (as 
said warning shows). 
Plus, it will become misleading when we fix the png loader to use 
iTXt chunks for stuff which can't be converted to latin1. I have
attached a patch which does so, but I would like the patch much better
if it could remove the "tEXt::" prefix. 

Matthias

PS The patch will not have any effect unless your libpng's has been
compiled with #define PNG_iTXt_SUPPORTED added to pngconf.h. This will
be turned on default in libpng 1.3 (according to the docs).

--- io-png.c	Mon Mar 25 01:08:11 2002
+++ io-png.c+itxt	Mon Mar 25 01:06:15 2002
@@ -209,9 +209,14 @@
                            gchar    **key,
                            gchar    **value)
 {
-        *value = g_convert (text_ptr.text, -1, 
-                            "UTF-8", "ISO-8859-1", 
-                            NULL, NULL, NULL);
+        if (text_ptr.text_length > 0) {
+                *value = g_convert (text_ptr.text, -1, 
+                                    "UTF-8", "ISO-8859-1", 
+                                    NULL, NULL, NULL);
+        }
+        else {
+                *value = g_strdup (text_ptr.text);
+        }
         if (*value) {
                 *key = g_strconcat ("tEXt::", text_ptr.key, NULL);
                 return TRUE;
@@ -761,8 +766,8 @@
 
                for (kiter = keys; *kiter; kiter++) {
                        if (strncmp (*kiter, "tEXt::", 6) != 0) {
-                                g_warning ("Bad option name '%s' passed to PNG saver", *kiter);
-                                return FALSE;
+                               g_warning ("Bad option name '%s' passed to PNG saver", *kiter);
+                               return FALSE;
                        }
                        key = *kiter + 6;
                        len = strlen (key);
@@ -770,7 +775,7 @@
                                g_set_error (error,
                                             GDK_PIXBUF_ERROR,
                                             GDK_PIXBUF_ERROR_BAD_OPTION,
-                                            _("Keys for PNG tEXt chunks must have at least 1 and at most 79 characters."));
+                                            _("Keys for PNG text chunks must have at least 1 and at most 79 characters."));
                                return FALSE;
                        }
                        for (i = 0; i < len; i++) {
@@ -778,7 +783,7 @@
                                        g_set_error (error,
                                                     GDK_PIXBUF_ERROR,
                                                     GDK_PIXBUF_ERROR_BAD_OPTION,
-                                                    _("Keys for PNG tEXt chunks must be ASCII characters."));
+                                                    _("Keys for PNG text chunks must be ASCII characters."));
                                        return FALSE;
                                }
                        }
@@ -795,11 +800,23 @@
                                                      "ISO-8859-1", "UTF-8", 
                                                      NULL, &text_ptr[i].text_length, 
                                                      NULL);
+
+#ifdef PNG_iTXt_SUPPORTED 
+                       if (!text_ptr[i].text) {
+                               text_ptr[i].compression = PNG_ITXT_COMPRESSION_NONE;
+                               text_ptr[i].text = g_strdup (values[i]);
+                               text_ptr[i].text_length = 0;
+                               text_ptr[i].itxt_length = strlen (text_ptr[i].text);
+                               text_ptr[i].lang = NULL;
+                               text_ptr[i].lang_key = NULL;
+                       }
+#endif
+
                        if (!text_ptr[i].text) {
                                g_set_error (error,
                                             GDK_PIXBUF_ERROR,
                                             GDK_PIXBUF_ERROR_BAD_OPTION,
-                                            _("Value for PNG tEXt chunk can not be converted to ISO-8859-1 encoding."));
+                                            _("Value for PNG text chunk %s can not be converted to ISO-8859-1 encoding."), keys[i] + 6);
                                num_keys = i;
                                for (i = 0; i < num_keys; i++)
                                        g_free (text_ptr[i].text);