Re: patch for gdk-pixbuf to read and write PNG tEXt chunks

[Owen Taylor <otaylor redhat com>]

Sven Neumann <sven gimp org> writes:

> Hi,
> 
> Owen Taylor <otaylor redhat com> writes:
> 
> > > +        creator = (const gchar *) g_object_get_data (G_OBJECT (pixbuf), 
> > > +                                                     "tEXt::Software");
> > > +        if (creator)
> > > +                g_print ("%s was created by '%s'\n", argv[1], creator);
> > 
> > I don't like this -- better to add a real API:
> > 
> > Something along the lines of:
> > 
> >  G_CONST_RETURN char *gdk_pixbuf_get_option (GdkPixbuf *pixbuf,
> >                                              const char *key);
> > 
> > And as part of this, you probably should hang all options off of
> > a single object data key rather than using a bunch of object data
> > keys.
> > 
> > Something like:
> > 
> >  void gdk_pixbuf_get_options (GdkPixbuf         *pixbuf,
> >                               GdkPixbufOption ***options,
> >                               gint              *n_options);
> >  void gdk_pixbuf_free_options (GdkPixbufOption *options,
> >                                gint             n_options);
> > 
> > Might be nice as well, though that could (should?) be added later.
> > 
> > The final comment is that I think that the code in io-png should
> > translate the values of the keys to/from UTF-8 and error if
> > the key value is not representable in ISO-8859-1.
> > 
> > (tEXt is specified to be in iso-8859-1, ugh. There is a "internationalized
> > text segment in later versions of png, but we presumbaly don't want 
> > to assume sufficiently new libpng.)
> 
> attached is a revised patch that addresses the encoding conversion and
> introduces a GdkPixbufOption struct that is however only used internally.
> It adds the gdk_pixbuf_get_option() API that you suggested. The helper
> functions _gdk_pixbuf_set_options() and _gdk_pixbuf_free_options() are
> not exported since I think the user shouldn't have to know anything about
> the implementation. 

Hmmm, this doesn't work does it? If the symbols aren't exported
then they won't be available for dynamically loaded modules to
access.

For Pango, what I did was to have 

  -DPANGO_ENABLE_BACKEND  - enable API only useful for backends
  -DPANGO_ENABLE_ENGINE	  - enable API only useful for language engines

But I don't think that is necessary here - simply putting the functions
into gdk-pixbuf-private.h should be fine.

I do think we should try to make the API as simple and sane as 
possible though, even if it is for modules only. See below.
 
> I have not addressed the error handling issues since I would prefer to
> discuss this problem first and if necessary handle it in a seperate 
> patch. At the moment there are a few places where gdk_pixbuf functions 
> return FALSE without setting GError. Havoc mentioned that this might be
> intentional as it indicates a bug in the application that is not 
> recoverable and I think I agree with him. What do you think?

Returning FALSE without setting GError is fine only in the sense
that it's fine for a function to have:
 
 g_return_if_fail (GTK_IS_WIDGET (widget), NULL);

Even if the docs say that it will never return NULL. If the programmer
violates his side of his contract, then we don't need to worry
about our side of the contract.

For options, there is a continuum of whether its a programmer error
or something that we should return as an Error:

 - tEXt:: value not representable as ISO-8859-1. GError useful.
 - tEXt:: key longer than 79 characters or not ascii. Probably
   programmer error, but GError might be useful.
 - unknown key not beginning with tEXt::. Programmer error,
   GError not really useful.

I think we can afford to be liberal with GError setting here and
correspondingly sparse with g_warning(), since people _must_ be
checking the return value from saving a pixbuf; it's something
that always could be failing for out-of-disk errors if nothing
else.

It's probably OK to make the last a g_warning() and save the poor
translators one string to translate, but I'd like to see GError for
the first two.

> - * The only save parameter that currently exists is the "quality" field
> - * for JPEG images; its value should be in the range [0,100].
> + * Currently only few parameters exist. JPEG images can be saved with a 
> + * "quality" parameter; its value should be in the range [0,100]. 
> + * Text chunks can be attached to PNG images by specifying parameters of
> + * the form "tEXt::key", where key is an ASCII string of length 1-79;
> + * the values being UTF-8 encoded strings. Note however that PNG text

This is worded a bit oddly, and confused me the first few times I
read it. 

 Text chunks can be attached to PNG images by specifying parameters of
 the form "tEXt::key", where key is an ASCII string of length 1-79.
 The values are UTF-8 encoded strings. [...]

> +/*  key/value pairs that can be attached by the pixbuf loader  */
> +
> +typedef struct _GdkPixbufOption GdkPixbufOption;
> +struct _GdkPixbufOption {
> +        gchar *key;
> +        gchar *value;
> +};
> +
> +void _gdk_pixbuf_set_options  (GdkPixbuf        *pixbuf,
> +                               GdkPixbufOption **options);
> +void _gdk_pixbuf_free_options (GdkPixbufOption **options);

Why don't we make this more pleasant and simply have:

 gtk_pixbuf_set_option (GdkPixbuf    *pixbuf,
                        const gchar  *key,
                        const gchar  *value);

A few calls to g_renew() and a few strcmp's won't hurt significantly,
and this avoids:

 - the need for free_options()
 - the need for the caller to g_new0 the list of options
 - the need to have a separate structure

etc.

> +void
> +_gdk_pixbuf_set_options (GdkPixbuf        *pixbuf,
> +                         GdkPixbufOption **options)
> +{
> +        g_object_set_qdata_full (G_OBJECT (pixbuf), 
> +                                 g_quark_from_static_string ("gdk_pixbuf_options"),
> +                                 options, 
> +                                 (GDestroyNotify) _gdk_pixbuf_free_options);
> +}
> +
> +void
> +_gdk_pixbuf_free_options (GdkPixbufOption **options)
> +{
> +        while (*options) {
> +                g_free ((*options)->key);
> +                g_free ((*options)->value);
> +                options++;
> +        }
> +}

You need to g_free (*options) as well. I'd use GdkPixbufOption *options
instead, I think. If you keep it private, it should be fine to use
key == NULL as the flag for the last option; if it was public we'd
typically return the length separately.

> +static GdkPixbufOption **
> +png_text_to_pixbuf_options(png_textp text_ptr,
> +                           gint      num_texts)
> +{
> +        GdkPixbufOption **options;
> +        gchar *value;
> +        gint   i, n;
> +
> +        options = g_new0 (GdkPixbufOption *, num_texts + 1);
> +        for (i = 0, n = 0; i < num_texts; i++) {
> +                value = g_convert (text_ptr[i].text, -1, 
> +                                   "UTF-8", "ISO-8859-1", 
> +                                   NULL, NULL, NULL);
> +                if (value) {
> +                        options[n] = g_new (GdkPixbufOption, 1);
> +                        options[n]->key = g_strconcat ("tEXt::", 
> +                                                       text_ptr[i].key, 
> +                                                       NULL);
> +                        options[n]->value = value;
> +                        n++;
> +                }
> +        }
> +        
> +        return options;
> +}

I'd at least g_warning() here if the conversion fails, since it indicates
that something is seriously wrong.

> +               for (kiter = keys; *kiter; kiter++) {
> +                       len = strlen (*kiter);
> +                       if (len < 6 || strncmp (*kiter, "tEXt::", 6)) {

As a style point, 'strncmp (*kiter, "tEXt::", 6) != 0', to make the sense
of the comparison clear. 

The len < 6 part is also unecessary. 

> +                                g_warning ("Bad option name '%s' passed to PNG saver",
> +                                          *kiter);
> +                               return FALSE;
> +                       }
> +                       len -= 6;
> +                       if (len < 1 || len > 79) {
> +                               g_warning ("tEXt keys passed as option to PNG saver must be of length 1-79");
> +                               return FALSE;
> +                       }
> +                       num_keys++;
>                 }
> -#endif
>         }
> +
> +       if (num_keys > 0) {
> +               text_ptr = g_new0 (png_text, num_keys);
> +               for (i = 0; i < num_keys; i++) {
> +                       text_ptr[i].compression = PNG_TEXT_COMPRESSION_NONE;
> +                       text_ptr[i].key         = keys[i]+6;

I think it would be good to check that the key actually is ASCII 
here, and not add it if it is not ASCII.

> +                       text_ptr[i].text        = g_convert (values[i], -1, 
> +                                                            "ISO-8859-1", "UTF-8", 
> +                                                            NULL, &text_ptr[i].text_length, 
> +                                                            NULL);
> +                       if (!text_ptr[i].text) {
> +                               g_warning ("Value for key '%s' can not be represented in ISO-8859-1 encoding as specified for PNG tEXt chunks.", keys[i]);
> +                               return FALSE;

Probably slightly better to say 'cannot be converted to the ISO-8859-1
encoding' and then include error->msg here. 

But I think in this case, setting GError is clearly the right thing to
do rather than g_warning - imagine an entry to enter some text key to
save with a PNG.

Regards,
                                        Owen