[cairo] mime-surface

[Chris Wilson]

On Wed, 2008-11-19 at 10:58 +0100, Benjamin Otte wrote:
> Yay, that was fast. And it looks pretty much perfect to me. Some tiny
> nitpicks on the API:
> 
> > static cairo_status_t
> > decode_png (cairo_surface_t *surface,
> >            const char *mime_type,
> >            cairo_surface_t *_image)
> >
> I don't think we need to pass the mime type here. And if you need it
> you can cairo_surface_set_user_data() it beforehand.
> I'm also not sure if passing an image surface or data/stride is better
> here. I guess an image surface is preferrable, even though it allows
> more nasty tricks by the decoding function (like creating a cairo_t on
> it or acquiring a reference and using the image surface for other
> nefarious purposes later).

Aside from the nefarious purposes (which can be useful in testing ;-),
the primarily reason to pass an image surface here is that it is a
complete container for pixel data - which allows the width/height/stride
to be checked against pre-conditions.

> You didn't investigate the idea of giving the decoder a rectangle of
> the area of interest to decode?
> I think the usefulness of this was one of the open points we still had.
> But we can still add it later using
> cairo_mime_surface_set_region_decode_function() or so.

I briefly looked at it, but it did not marry well with the current
internal API. However, converting acquire_source_image() to use a ROI is
not intractable and needs to be investigated for at the moment if we
fallback but try to use an xlib-surface as a source we pull the entire
XImage back over the wire.

The advantage of decompressing the full image is that we can keep the
surface around for future requests. For a region decoder, I suspect I
may still allocate the full image, but track the current decompressed
region as well (and first check whether the current request can be
satisfied from the cache, otherwise request decompression of the minimum
covering rectangle).

So, I think a region decompressor would look like:
  cairo_status_t
  decode_png (cairo_surface_t *mime_surface [in],
              cairo_surface_t *image_surface [out],
              cairo_rectangle_t *region_of_interest [inout]);
whereby the region_of_interest specifies the minimum rectangle that must
be decompressed on entry, and the rectangle actually decompressed on
exit.

> 
> >    if (mime_data == NULL)
> >        return _cairo_error (CAIRO_STATUS_NULL_POINTER);
> >
> _cairo_error() is internal API, so you likely want to just return
> CAIRO_STATUS_NULL_POINTER here and do an
> if (status)
>   _cairo_error(status);
> after returning from the user-provided callback, right?

Whoops, missed propagating the error from the callback to the
mime-surface.

> >   surface = cairo_mime_surface_create (content,
> >                                        CAIRO_MIME_TYPE_PNG,decode_png,
> >                                        width, height);
> >
> I think specifying width/height on surface creation is fine, but I'm
> just one user - or two, if you count my gdk-pixbuf knowledge, where it
> should be fine, too - You'd defer creating the surface until the size
> of the image is known. But is there a use case for determining the
> size using a callback and/or during the decode function?
> 
> > Is the simple interface both minimal and sufficient for expected users?
> >
> It's certainly minimal enough. I think other things could be added
> later by extending the mime surface API if we figure out we need
> something.

I'd rather avoid adding more constructors though, so I think switching
to a region decoder (even though it will currently only request the full
image) is better to do now.

Thanks for the feedback, am already incorporating it into the code :-)
-- 
Chris