[cairo] Pixel RGB Values...

[Chris Wilson]

On Wed, 2009-07-22 at 12:40 -0700, Carl Worth wrote:
> On Thu, 2009-07-09 at 12:07 +0100, Chris Wilson wrote:
> 
> > The first is a convenient method for direct access (prototyped in
> > cairo-drm, and used to good effect):
> > 
> >   cairo_surface_t *
> >   cairo_surface_map (cairo_surface_t *surface);
> > 
> >   void
> >   cairo_surface_unmap (cairo_surface_t *surface,
> >                        cairo_surface_t *image);
> 
> I do like the short name, but I'm a little bit concerned that the map
> function returns a new *image* surface without ever mentioning the word
> "image" in its function name. That's something I think we've avoided
> doing elsewhere, (note that all the various surface_create functions
> explicitly mention which sub-type of image they are creating).
> 
> "map_image" perhaps?

Taking a leaf out of other API, maybe cairo_surface_map_as_image() or
cairo_surface_map_to_image(). I think the 'to' is a good balance to the
'for' that is already in use in the other direction.

> > In use, mapping a surface returns an image surface that you can use to
> > manipulate pixels directly, and unmapping ensures that the changes are
> > flushed to the backing surface. Internally this works just like
> > acquire_dest_image()/release_dest_image() but with the extra caveat that
> > the surface is exported and so we need to redirect rendering whilst the
> > map is in effect. I like map/unmap (as used by pixel buffer objects in
> > OpenGL), but alternative names used elsewhere include lock/unlock.
> > (lock/unlock imply to me that any rendering to the original surface is
> > disabled - a side-effect I don't like, but may be cause less confusion
> > in the long run.)
> 
> If you don't disable rendering to the surface, then I'm not sure I
> completely understand the semantics of your proposed map. You are saying
> that rendering that I perform via cairo will be redirected to the mapped
> image surface, but that changes I make via direct pixel access won't be
> reflected to the underlying surface until the time of unmap? Is that it?
> 
> And maybe that's what we really want, but it seems oddly asymmetrix to
> me.

I'm misinterpreting what you're saying here, because I don't see the
confusion in what you are trying to point out. 

The choices that we have whilst the user has the surface mapped for
direct access are:

1. discard all rendering to the original surface.

2. keep rendering to the original surface, but without syncing to the
mapped image buffer, and overwriting the changes with the contents of
the image buffer when the surface is unmapped.

3. redirect the rendering to the image buffer such that the mapped
buffer stays in sync.

So far the only user code that has been written using map() has been to
apply a blur to a surface. For which the surface is mapped only for a
very brief period and there is no desire to mix vector drawing with
pixel manipulation.

However, looking at the wider picture (ignoring, for the purposes of
discussion, the complexity of multiple concurrent readers and writers)
then I can see that the ability to mix raster and vector drawing
operations could be quite useful. So to accommodate this, the mapped
surface must be kept in sync with vector operations performed in
parallel to the mapping, for which we need to redirect the rendering
from the original surface to the image surface for the duration of the
map. Then on unmap we flush all changes from the image surface back to
the original surface, be they as a result of direct pixel manipulation
or redirected rendering. We could even go as far as only flushing dirty
rectangles:

  surface = cairo_xlib_surface_create ();
  image = cairo_surface_map_to_image (surface);
  ... tweak ...
  cairo_surface_mark_dirty (image, x, y, width, height);
  cairo_surface_unmap (surface, image);

> > The second method I think we need is for efficient pixel upload. Already
> > win32 has such a scheme, but it is of general use. My proposal here is:
> > 
> >   cairo_surface_t *
> >   cairo_surface_create_similar_image (cairo_surface_t *target,
> >                                       cairo_content_t content,
> >                                       int width, int height);
> 
> We got really stuck on API the last time this idea came up, but the
> above proposal looks great to me. It uses "similar" in a similar way as
> the existing create_similar, (which is that "similar" means to do
> something in a way that's as efficient as possible for the given
> backend).
> 
> Like the existing create_similar, it uses an actual instance of a
> surface rather than just naming the backend of interest, and that might
> be annoying in some cases. For example, porting code that uses the
> current cairo_quartz_image_surface_create to this new API might require
> creating a dummy quartz surface here.

Yes, but I'm not convinced I understand the quartz-image surface yet. It
looks just to be an internal wrapper around an image for the quartz
backend. Its interaction with the regular quartz-surface looks akin to
the map/unmap situation described above.

The difficulty here is that our surfaces also encapsulation connection
state to the various devices, which we require in order to create a
similar image. With recent backends, we've started introducing specific
objects to track device connections, such as cairo_gl_context_t,
cairo_vg_context_t and cairo_drm_device_t. Similarly we could expose a
cairo_xlib_connection_t for a (Display, Screen) tuple (i.e just exposing
cairo_xlib_screen_t) so we wouldn't need a specific surface instance,
just the object describing the connection. For backends where there is
only one such possible device (quartz, win32, image) we could just have
a global object.

Following this scheme, we might end up with an API something like:

device = cairo_xlib_device_open (display, screen);
surface = cairo_xlib_surface_create_for_device (device, content, w, h);
image = cairo_image_surface_create_for_device (device, content, w, h);

Here image would an in-memory buffer suitable for quick upload to the
xlib surface. This also has the potential to allow cross-backend
surfaces, if that is ever desirable. (Smells like over-engineering.
clone_similar() should already be sufficient in that regard. The issue
at hand is fast uploading of pixel data.)

So using a surface rather than a backend type is a necessary evil.

> But we've already got a precedent for requiring such dummy surfaces in
> the case of font metrics, for example. And for font metrics, the "dummy"
> surface can actually hold necessary information, (such as antialiasing
> mode). And perhaps here as well, being able to access the surface's
> format/depth/etc. might also be important.

You've raised a good point here. Until now I've focussed on aiming for
efficient upload of the pixel data to the backend, but having the target
surface information available does gives us opportunity to optimise the
choice of similar surface for use as a source for compositing to the
target.

> > That returns an image that has appropriate backing storage to enable
> > efficient upload to the target surface. (This is the best name I've come
> > up with since the last time this proposal stumbled due to not finding a
> > good name... ;-) The tricky issue here is whether it should take a
> > content or a format. When uploading pixel data you really want to avoid
> > format conversion if possible, so maybe:
> > 
> >    typedef pixman_format_code_t cairo_wacky_format_t
> >    cairo_surface_t *
> >    cairo_surface_create_similar_image (cairo_surface_t *target,
> >                                        cairo_wacky_format_t format,
> >                                        int width, height);
> 
> I would imagine that this should accept a cairo_format_t first, and then
> have a named-variation of the function to accept the extended format.
> (Then we could use the same naming extension for
> cairo_image_surface_create if that makes sense.)
> 
> That is, I think adding cairo_extended_format_t should be wholly
> orthogonal to adding cairo_surface_create_similar_image.

On the subject of wacky_format_t, what did you think of Soeren's
suggestion to just use the pixman_format_code_t for cairo_format_t?
-ickle