Re: inline pixbufs

[Darin Adler <darin eazel com>]

on 7/26/00 9:54 AM, Havoc Pennington at hp@redhat.com wrote:

> - 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.

I agree that we can choose to use your new format instead of what Bonobo
already does, but I wish you'd take more responsibility for making the two
match since you introduced the new format. Once Bonobo 1.0 is released we'll
have a binary compatibility issue if we want to change the format.

Unfortunately, I'm not certain the format you've invented is suitable for
Bonobo (details below).

> - 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.

A problem with putting row stride in is this: There may be restrictions
about which row strides can be efficiently supported on different platforms.
If the code that reads the image uses the row stride stored in the flattened
form without taking into account what's supported on the platform, you might
get a slow-to-draw image (or perhaps even an unusable one?). So in the
general case the reading code needs to select an appropriate row stride.

Leaving the row stride out of the flattened form entirely helps keep this
issue clear, which is why we did so in the Bonobo format. Even if you don't
leave it out, because you want the option of using the data in place, you
might need code to handle the case where the row stride is not usable.

A separate issue: With redundant data in the flattened form, you end up with
a format that is much more complex for reading flattened data from an
untrusted place like a network stream. Unlike the code in Bonobo, which does
reality checks on all the parameters and returns an error if there's
anything wrong, read_raw_inline will construct an incorrect pixbuf if passed
bad data, which can result in unpredictable behavior later on when GdkPixbuf
routines are called on an invalid pixbuf. For example, has_alpha can be
incompatible with n_channels, bits_per_sample is ignored when computing the
size of the buffer, width and height might not match the actual size of the
data, width might not match rowstride.

In the Bonobo code, we chose a simple subset of GdkPixbuf, which is harder
to write, but much easier to read with complete error checking.

> +/* Is this Evil? I'm not sure. */
> +#define GDK_PIXBUF_INLINE_MAGIC_NUMBER (* (guint32 *) "GdkPixbufMagic")

There are at least 3 big problems. Problem 1 is that it does different
things on big vs. little endian machines. Problem 2 is that this just gets
the "GdkP" part, so it looks a lot more unique than it is. Problem 3 is that
on platforms where guint32 is not exactly 4 bytes, you end up with a
different number.

You should just write this out as a hex constant.

(I have a lot of experience with this sort of thing, since we used these
kinds of magic numbers extensively on Macintosh.)

> +void
> +output_int (FILE *outfile, guint32 num, const char *comment)
> +{
> +  guchar bytes[4];
> +
> +  g_assert (sizeof (bytes) == sizeof (num));

This may be an overzealous assert. On some platforms, guint32 can be bigger
than 32 bits and most glib/gtk code will still work. This assert guarantees
a core dump on those systems. But if the number fits in 32 bits, the
function will work just fine, and sizeof (num) is beside the point.

    -- Darin