Re: inline pixbufs

[Tim Janik <timj gtk org>]

On 26 Jul 2000, Havoc Pennington wrote:

> 
> Hi,
> 
> Changes I just made to my local copy of inline pixbuf code:
> 
>  - fixed the unaligned read issue Darin mentioned, or at least made
>    some attempt to do so
> 
>  - use "GdkPixbuf" instead of random number for the magic (though, 
>    I just cast the string bytes to an int, and that might be sort 
>    of busted ;-)

it doesn't, has endian problems, but i think darin or someone pointed that out
already.

> The changes aren't tested, I'm building GTK again now to do that.
> 
> Changes I don't think we should make:
> 
>  - Match Bonobo implementation; the Bonobo implementation is almost
>    the same, but lacks the format tag, and I think the format tag is
>    important; so I'd like to see Bonobo cut-and-paste the stuff from
>    gdk-pixbuf once we finalize it.

where's that bonobo format you refer to?
i'd also like to get that stuff in sync with the inlined images
that gimp can save.

also i think you're still using a byte array:

{ 0x01, 0x02, 0x03, 0x04, ... }

etc when writing out code, this has severe disadvantages on the
compiler resource side, we _really_ have to use strings here.

>  - GdkPixdataType; we already have a data type that stores pixdata,
>    it's called GdkPixbuf. The point of new_from_inline() is to take a
>    GdkPixbuf to and from a byte stream. I don't understand the
>    advantage of an intermediate type; certainly we shouldn't have an
>    intermediate type in the public API.

well, the basic thing you do is to create a pixbuf from a stream
of RGB(A) data. independant of where that came from, say i just
got an RGB buffer from some third party code and know it's size,
or i got it from a selection, or i've got a stream compiled-in
available, it doesn't really matter.
with your (and the existing) API i'd either have to use
gdk_pixbuf_from_inline() if its an opaque stream, or sometimes
gdk_pixbuf_new_from_data() if i have the RGB data and the sizes
seperatedly available (e.g. from an inlined image saved by gimp).
that's what i'd like to unify by having a seperate auxillary structure
that can easily be filled in, and where the serialization/deserialization
is obvious from the structures contents itself (give/take g_ntoh/g_hton
for the non-byte members and write/read it member by member).

GdkPixdataType is not another object or structure type like you
make it sound here, instead it'd be what
1) takes over the purpose of your GdkPixbufInlineFormat enum (in gtk
   we simply have the convention to name enum types *Type at the end,
   and it's adheared to throught most of the code)
2) incorporates the various fields of gdk_pixbuf_new_from_data()

so instead of having:

typedef enum
{
  GDK_PIXBUF_INLINE_RAW = 0,
  GDK_PIXBUF_INLINE_RLE = 1
} GdkPixbufInlineFormat;

GdkPixbuf *gdk_pixbuf_new_from_inline   (const guchar *inline_pixbuf,
                                         gboolean      copy_pixels,
                                         int           length);
GdkPixbuf *
gdk_pixbuf_new_from_data (const guchar *data, GdkColorspace colorspace, gboolean has_alpha,
                          int bits_per_sample, int width, int height, int rowstride,
                          GdkPixbufDestroyNotify destroy_fn, gpointer destroy_fn_data);


with GdkPixbufInlineFormat being exposed in the API but nowhere to pass,
and an opaque stream format, we'd have _one_ function for creation,
and helper functions that can do the (de-)serialization into pure
bytestreams like you'd want it for selections (i think i cooked something
like this up before in email):

typedef enum
{
  /* colorspace + alpha */
  GDK_PIXDATA_COLOR_TYPE_RGB	= 0x01,
  GDK_PIXDATA_COLOR_TYPE_RGBA	= 0x02,
  GDK_PIXDATA_COLOR_TYPE_MASK	= 0x0f,
  /* should probably leave GDK_PIXDATA_SAMPLE_WIDTH_8 out and default to that
  GDK_PIXDATA_SAMPLE_WIDTH_8	= 0x01 << 8,
  GDK_PIXDATA_SAMPLE_WIDTH_16	= 0x02 << 8,
  GDK_PIXDATA_SAMPLE_WIDTH_MASK	= 0x0f << 8,
  */
  GDK_PIXDATA_ENCODING_RAW	= 0x01 << 26,
  GDK_PIXDATA_ENCODING_RLE	= 0x02 << 26,
  GDK_PIXDATA_ENCODING_MASK	= 0x0f << 26
} GdkPixdataType;

typedef struct _GdkPixdata GdkPixdata;  
struct _GdkPixdata
{
  guint32 magic;        /* 0x47646B50 'GdkP' */
  gint32  length;       /* -1 to disable length checks,
                         * sizeof (GdkPixdata) + pixel_data length otherwise
                         */
  gint32  pixdata_type; /* GdkPixdataType */
  guint32 rowstride;    /* maybe 0 to indicate non-padded data */
  guint32 witdh;
  guint32 height;
  guint8 *pixel_data;
};

GdkPixbuf* gdk_pixbuf_new_from_pixdata (GdkPixdata             *pixdata,
                                        GdkPixbufDestroyNotify *pixel_dtor,
                                        gpointer                dtor_data);
gint8*     gdk_pixdata_serialize   (GdkPixdata *pixdata,
                                    guint      &stream_length);
gboolean   gdk_pixdata_deserialize (GdkPixdata *pixdata,
                                    guint       stream_length,
                                    guint8     *stream);
                                     

there you go, extremely easy to handle, for the cases above:

third party renderer:
gint width, height; guchar *data;
data = third_party_render_rgb (&width, &height);
GdkPixdata pixdata = { GDK_PIXDATA_MAGIC, -1 /* length */,
                       GDK_PIXDATA_COLOR_TYPE_RGB | GDK_PIXDATA_ENCODING_RAW,
                       0 /* rowstride */,
                       width, height, data };
pixbuf = gdk_pixbuf_new_from_pixdata (&pixdata, g_free /* for pixel_data */, NULL);

write it out:
selection->data = gdk_pixdata_serialize (&pixdata,
                                         &selection->length);

read it in:
GdkPixdata pixdata2;
if (!gdk_pixdata_deserialize (&pixdata2, selection->length, selection->data))
  g_error ("got invalid pixdata stream");
pixbuf = gdk_pixbuf_new_from_pixdata (&pixdata, NULL, NULL);
/* copy pixbuf since data still belongs to selection */
pixbuf2 = gdk_pixbuf_copy (pixbuf);
g_object_unref (pixbuf);
selection_free (selection);

that interface is easy to handle, even for obscure cases, and
more so for the regular ones. gimp's CSource plugin can be hacked
to support this format in like 10 minutes, featuring RGB, RGBA, RAW
and RLE. another 30 minutes or so, and it's working as a standalone
program, doing sane string writeouts, instead of compiler-horror
byte arrays.
the stream format is straight forward from the structure layout (and
with 4 bytes magic then 4 bytes length, it's pretty standard and can
interact with other wire protocols from what i've seen so far), and
unambigous, e.g. you won't have COLORSPACE_RGB and n_channels=2.
also GdkPixdataType leaves enough room for future extensions, e.g.
different colorspaces, HSV, CMYK, or even more advanced compression
types in the future (once mpeg compression algorithms are part of
glib ;) in an unambigous way also.

if you see shortcomings with this, please name them, especially the
bonobo types, if they think they'd have problem with this streaming
format.

maybe gdk_pixdata_deserialize() should return a GError, and maybe you
want a copy_pixels arg in gdk_pixbuf_new_from_pixdata() as well,
but that's about it.

>    Conceivably it makes the autogenerated inline code files more
>    human-editable, but it also adds complexity to them and makes the
>    code harder to understand, and I think it's pretty useless to
>    human-edit these files anyway.

think "human readable". everythink needs to be human readable, for debugging
you even want that for the raw stream format, and that's very straight
forward after you've seen the structure and know g_hton() on intel.

>  - Remove rowstride; we're just dumping a pixbuf to a stream, the
>    stream shouldn't lose information that was present in the original
>    pixbuf. Also, as Owen points out, we need this to be able to align
>    read-only static data on natural boundaries. Clearly rowstride only
>    exists in GDK_PIXBUF_INLINE_RAW format, however, and the loader for
>    the inlined pixbuf could choose to ignore the rowstride in the
>    copy_pixels case.

ok, ok, it's in there, it's optional, what more do you want? ;)

>  - Use g_malloc(); we really want g_try_malloc() for pixel buffers,
>    because the buffers are potentially quite huge, and an image viewer
>    app (for example) shouldn't simply fall over if you try to load a
>    huge image. Granted, this probably won't happen in the inline case,
>    if you inline an image much over 48x48 you are likely insane. So
>    there it's just consistency with the other gdk_pixbuf_new()
>    routines.

yes, you're getting there, i'm working on that as well.
for beast, i use inline RGBA images of 64x64 to make the
canvas items in the synthesis networks look decent. with
RLE it's far from insane ;)

> Changes that are pending:
> 
>  - RLE encoding; this would go in gdk_pixbuf_new_from_inline(), add
>    GDK_PIXBUF_INLINE_RLE to the format switch, then write a function
>    to load it. To make it useful you'd also need to hack
>    make-inline-pixbuf.c to write one of these, the problem is that
>    make-inline-pixbuf.c has no real option parsing, which makes it
>    annoying to add a --encode=rle option. Because the public API
>    contains no way to create an inline pixbuf, there is currently no
>    public API change involved. If there were such an inline-creation
>    function, the API change would just be adding the
>    GDK_PIXBUF_INLINE_RLE enum value.
> 
>    Anyway, basically this is trivial except for the annoying
>    option-parsing issue.

you're code has more problems than option parsing, you have to use
_strings_! ;)
if it'd be just for the option parser, i'd say c'n'p this:

static void
parse_args (gint    *argc_p,
            gchar ***argv_p)
{
  guint argc = *argc_p;
  gchar **argv = *argv_p;
  guint i, e;

  for (i = 1; i < argc; i++)
    {
      if (strcmp (argv[i], "--g-fatal-warnings") == 0)
        {
          GLogLevelFlags fatal_mask;

          fatal_mask = g_log_set_always_fatal (G_LOG_FATAL_MASK);
          fatal_mask |= G_LOG_LEVEL_WARNING | G_LOG_LEVEL_CRITICAL;
          g_log_set_always_fatal (fatal_mask);

          argv[i] = NULL;
        }
      else if (strcmp ("-h", argv[i]) == 0 ||
               strcmp ("--help", argv[i]) == 0)
        {
          print_blurb (stderr);
          argv[i] = NULL;
          exit (0);
        }
    }

  e = 0;
  for (i = 1; i < argc; i++)
    {
      if (e)
        {
          if (argv[i])
            {
              argv[e++] = argv[i];
              argv[i] = NULL;
            }
        }
      else if (!argv[i])
        e = i;
    }
  if (e)
    *argc_p = e;
}

it's similar to what gtk uses, i think you'd know how to add
new if() statements ;)

but as i said above, that doesn't solve all your problems, instead,
if we can agree on something like the above, i'll give you your
standalone program and the pixbuf function implementations in about
an hour.

>  - Documentation. I would like to see gtk-reference sorted out (or is
>    it already sorted out?) for 2.0, so we can put docs in there. I'm
>    not sure just adding more text files is sensible.
> 
> My overall thought on the API is that it should be kept super-minimal;
> right now the public interface is "make-inline-pixbuf src_image" and
> gdk_pixbuf_new_from_inline(), and that's it. I'm pretty sure we don't
> want this to become some sort of alternative to PNG, and I wouldn't
> even like the Gimp for example to be able to load/save it directly,

great, the gimp already has support for writing one inline format (CSource,
close to what we have above) and reading another (header files with inlined
string macros, used for the splashes). it'd be good to make one of these
quasi-standard, rather then inventing yet another for pixbufs.
besides that, i or someone else might want to pass streamed pixbufs across
sockets or similar wires (in fact i'll need something like this for threaded
BSE) without bonobo deps, and for that they'll need the serialization and
deserialization easily accessible and debuggable.

> it's better to use PNG as the disk format and convert to/from inline
> data in the Makefile. The world does not need another format...

well, we already got suitable ones, bend them as necessary ;)

> 
> Havoc
> 

---
ciaoTJ