568189 - Implement CGLayer support in cairo-quartz

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

To squeeze out the maximum performance for blitting using cairo-quartz, I implemented API to create cairo surfaces backed by CGLayers.

I also have a small patch to speed up self-copy operations for CGBitmap-based cairo surfaces.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: Make self-copy fast for CGBitmapContext surfaces

We actually don't need this patch, I'm just putting it here for safekeeping.

Note the comment: we probably shouldn't take this patch since it makes behaviour less correct until pixman supports self-copies properly.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: CGLayer surfaces

Creates new API cairo_quartz_cglayer_surface_create_similar. This is like cairo_surface_create_similar but if possible it creates a CGLayer-backed surface that's optimized for drawing into the given surface.

The only thing a CGLayer-backed surface is really good for, as a source, is being drawn into a destination without tiling. But it's really good for that! For everything else we fall back through painfully slow and not really tested paths.

I've removed _cairo_quartz_surface_snapshot. It doesn't seem to be useful at all currently and I didn't want to try to extend it to handle CGLayer surfaces.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3: expose gfxQuartzSurface constructor to create a CGLayer surface

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 447497 [details] [diff] [review]
Part 2: CGLayer surfaces

>@@ -1339,33 +1338,39 @@ _cairo_quartz_cairo_repeating_surface_pa
> }
> 

> 
> /* State used during a drawing operation. */
> typedef struct {
>     CGContextRef context;
>     cairo_quartz_action_t action;
> 
>-    // Used with DO_SHADING, DO_IMAGE and DO_TILED_IMAGE
>+    // Used with DO_SHADING, DO_IMAGE, DO_TILED_IMAGE and DO_LAYER
>     CGAffineTransform transform;
> 
>     // Used with DO_IMAGE and DO_TILED_IMAGE
>     CGImageRef image;
>     cairo_surface_t *imageSurface;
>+
>+    // Used with DO_IMAGE, DO_TILED_IMAGE and DO_LAYER
>     CGRect imageRect;
> 
>+    // Used with DO_LAYER
>+    CGLayerRef layer;
>+
>     // Used with DO_SHADING
>     CGShadingRef shading;
> 
>     // Used with DO_PATTERN
>     CGPatternRef pattern;

Is it worth putting these things in a union?


>-	status = _cairo_surface_to_cgimage ((cairo_surface_t *) surface, pat_surf, &img);
>+        cairo_matrix_invert(&m);
>+        _cairo_quartz_cairo_matrix_to_quartz (&m, &state.transform);
>+
>+        if (cairo_surface_get_type (pat_surf) == CAIRO_SURFACE_TYPE_QUARTZ) {
>+            cairo_quartz_surface_t *quartz_surf = (cairo_quartz_surface_t *) pat_surf;
>+            if (quartz_surf->cgLayer && source->extend == CAIRO_EXTEND_NONE) {
>+         	state.imageRect = CGRectMake (0, 0, quartz_surf->extents.width, quartz_surf->extents.height);
>+                state.layer = quartz_surf->cgLayer;

The indentation here is inconsistent. One line uses tabs the other spaces. Not sure if it matters though


>@@ -1820,33 +1836,44 @@ _cairo_quartz_teardown_state (cairo_quar
> 
>     CGContextRestoreGState(state->context);
> }
> 
> 

>-static cairo_surface_t *
>-_cairo_quartz_surface_snapshot (void *abstract_surface)
>-{
>-    cairo_int_status_t status;
>-    cairo_quartz_surface_t *surface = abstract_surface;
>-    cairo_image_surface_t *image;
>-
>-    if (surface->imageSurfaceEquiv)
>-	return NULL;
>-
>-    status = _cairo_quartz_get_image (surface, &image);
>-    if (unlikely (status))
>-        return _cairo_surface_create_in_error (CAIRO_STATUS_NO_MEMORY);
>-
>-    return &image->base;
>-}
>-

It wouldn't hurt if this part was a separate patch.

>-    _cairo_quartz_surface_will_change (surface);
>-
>-    status = _cairo_quartz_get_image (surface, image_out);
>-    if (status)
>-	return _cairo_error (CAIRO_STATUS_NO_MEMORY);
>-
>     *image_rect = surface->extents;
>     *image_extra = NULL;
> 
>-    return CAIRO_STATUS_SUCCESS;
>+    _cairo_quartz_surface_will_change (surface);
>+
>+    return _cairo_quartz_surface_acquire_source_image (abstract_surface,
>+        image_out, image_extra);
> }

I don't really like pairing _cairo_quartz_surface_acquire_source_image with *_release_dest_image.
Perhaps rename _cairo_quartz_surface_acquire_source_image to _cairo_quartz_surface_acquire_image.

> 
> static cairo_surface_t *
> _cairo_quartz_surface_create_similar (void *abstract_surface,
> 				       cairo_content_t content,
> 				       int width,
> 				       int height)
> {
>-    /*cairo_quartz_surface_t *surface = (cairo_quartz_surface_t *) abstract_surface;*/
>-
>+    cairo_quartz_surface_t *surface = (cairo_quartz_surface_t *) abstract_surface;
>     cairo_format_t format;
> 
>+    if (surface->cgLayer)
>+        return cairo_quartz_cglayer_surface_create_similar (abstract_surface, width, height);
>+

I was surprised by this at first, thinking that it would break if you draw a layer
with a different context than the one it was created with. However, it seems to work.

I think it's worth writing about drawing a layer on to a different context than
it was created with.  I tested this with CGBitmapContexts and it seemed to
work, in fact using a NULL context for creation even worked. However, I don't
know whether it works with different CGWindowContexts or between
CGWindowContexts and CGBitmapContexts. 

>@@ -2211,17 +2275,18 @@ _cairo_quartz_surface_paint (void *abstr
>     if (state.action == DO_SOLID || state.action == DO_PATTERN) {
> 	CGContextFillRect (state.context, CGRectMake(surface->extents.x,
> 						     surface->extents.y,
> 						     surface->extents.width,
> 						     surface->extents.height));
>     } else if (state.action == DO_SHADING) {
> 	CGContextConcatCTM (state.context, state.transform);
> 	CGContextDrawShading (state.context, state.shading);
>-    } else if (state.action == DO_IMAGE || state.action == DO_TILED_IMAGE) {
>+    } else if (state.action == DO_IMAGE || state.action == DO_TILED_IMAGE ||
>+               state.action == DO_LAYER) {

Maybe it's worth rolling these conditions into an inline function?

> 	_cairo_quartz_draw_image (&state, op);
>     } else if (state.action != DO_NOTHING) {
> 	rv = CAIRO_INT_STATUS_UNSUPPORTED;
>     }
> 
>     _cairo_quartz_teardown_state (&state);
> 
>     ND((stderr, "-- paint\n"));
> /**
>+ * cairo_quartz_cglayer_surface_create_similar
>+ * @surface: a surface that we will eventually want to blit the returned
>+ * surface to

This explanation is a unclear. It's not clear what the relationship
between this associated CGContextRef and the Layer actually is.

>+ * @width: width of the surface, in pixels
>+ * @height: height of the surface, in pixels
>+ *
>+ * Creates a Quartz surface backed by a CGLayer, if the given surface
>+ * is a Quartz surface; the CGLayer is created to match the surface's
>+ * Quartz context. Otherwise just calls cairo_surface_create_similar
>+ * with CAIRO_CONTENT_COLOR_ALPHA.
>+ * The returned surface can be efficiently blitted to the given surface,
>+ * but tiling and 'extend' modes other than NONE are not so efficient.
>+ *
>+ * Return value: the newly created surface.
>+ *
>+ * Since: 1.4

1.10 is probably more accurate :)

>+ **/
>+cairo_surface_t *
>+cairo_quartz_cglayer_surface_create_similar (cairo_surface_t *surface,
>+              unsigned int width,
>+              unsigned int height)
>+{

I'm not a huge fan of this name. How about cairo_quartz_create_layer()?

>+    cairo_quartz_surface_t *surf;
>+    CGLayerRef layer;
>+    CGContextRef ctx;
>+    CGContextRef cgContext;
>+
>+    cgContext = cairo_quartz_surface_get_cg_context (surface);
>+    if (!cgContext)
>+        return cairo_surface_create_similar (surface, CAIRO_CONTENT_COLOR_ALPHA,
>+                                             width, height);
>+
>+    if (!_cairo_quartz_verify_surface_size(width, height))
>+  return _cairo_surface_create_in_error (_cairo_error (CAIRO_STATUS_INVALID_SIZE));
>+
>+    if (width == 0 || height == 0) {
>+  return (cairo_surface_t*) _cairo_quartz_surface_create_internal (NULL, CAIRO_CONTENT_COLOR_ALPHA,
>+                   width, height);
>+    }

What happens if you pass a zero size to CGLayerCreateWithContext()?

>+
>+    layer = CGLayerCreateWithContext (cgContext,
>+                                      CGSizeMake (width, height),
>+                                      NULL);
>+    if (!layer)
>+      return _cairo_surface_create_in_error (_cairo_error (CAIRO_STATUS_NO_MEMORY));
>+
>+    ctx = CGLayerGetContext (layer);
>+    /* Flip it when we draw into it, so that when we finally composite it
>+     * to a flipped target, the directions match and Quartz will optimize
>+     * the composition properly
>+     */
>+    CGContextTranslateCTM (ctx, 0, height);
>+    CGContextScaleCTM (ctx, 1, -1);
>+
>+    CGContextRetain (ctx);
>+    surf = _cairo_quartz_surface_create_internal (ctx, CAIRO_CONTENT_COLOR_ALPHA,
>+              width, height);
>+    if (surf->base.status) {
>+  CGLayerRelease (layer);
>+  // create_internal will have set an error
>+  return (cairo_surface_t*) surf;

Looks like something went wrong with the indentation in this function.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 447498 [details] [diff] [review]
Part 3: expose gfxQuartzSurface constructor to create a CGLayer surface

> 
>+gfxQuartzSurface::gfxQuartzSurface(gfxASurface* dest, const gfxSize& size,
>+                                   PRBool aForPrinting)
>+    : mCGContext(NULL), mSize(size), mForPrinting(aForPrinting)
>+{
>+    unsigned int width = (unsigned int) floor(size.width);
>+    unsigned int height = (unsigned int) floor(size.height);
>+

You wouldn't happen to know why these take a gfxSize instead of gfxIntSize would you?

Comment 6

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #4)
> >+ **/
> >+cairo_surface_t *
> >+cairo_quartz_cglayer_surface_create_similar (cairo_surface_t *surface,
> >+              unsigned int width,
> >+              unsigned int height)
> >+{
> 
> I'm not a huge fan of this name. How about cairo_quartz_create_layer()?

I also wonder if this should take a CGContextRef instead of a cairo_surface_t*.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #4)
> Is it worth putting these things in a union?

I don't think so.

> It wouldn't hurt if this part was a separate patch.

OK, I'll do that.

> I don't really like pairing _cairo_quartz_surface_acquire_source_image with
> *_release_dest_image.
> Perhaps rename _cairo_quartz_surface_acquire_source_image to
> _cairo_quartz_surface_acquire_image.

Sure, will do.

> I was surprised by this at first, thinking that it would break if you draw a
> layer with a different context than the one it was created with. However, it
> seems to work.
> 
> I think it's worth writing about drawing a layer on to a different context
> than it was created with.  I tested this with CGBitmapContexts and it seemed
> to work, in fact using a NULL context for creation even worked. However, I
> don't know whether it works with different CGWindowContexts or between
> CGWindowContexts and CGBitmapContexts.

Apple docs say:

| Although layer uses this graphics context as a reference for initialization,
| you are not restricted to drawing the layer to this graphics context. You can
| draw the layer to other graphics contexts, although any limitations of the
| original context are imposed. For example, if you create a CGLayer object
| using a bitmap context, the layer is rendered as a bitmap when drawn to any
| other graphics context.

I'll add a comment to the places where we call CGContextDrawLayerAtPoint.

> >@@ -2211,17 +2275,18 @@ _cairo_quartz_surface_paint (void *abstr
> >     if (state.action == DO_SOLID || state.action == DO_PATTERN) {
> > 	CGContextFillRect (state.context, CGRectMake(surface->extents.x,
> > 						     surface->extents.y,
> > 						     surface->extents.width,
> > 						     surface->extents.height));
> >     } else if (state.action == DO_SHADING) {
> > 	CGContextConcatCTM (state.context, state.transform);
> > 	CGContextDrawShading (state.context, state.shading);
> >-    } else if (state.action == DO_IMAGE || state.action == DO_TILED_IMAGE) {
> >+    } else if (state.action == DO_IMAGE || state.action == DO_TILED_IMAGE ||
> >+               state.action == DO_LAYER) {
> 
> Maybe it's worth rolling these conditions into an inline function?

Maybe, but it's nice to see in the caller that all the various cases are handled, so I'd prefer not to. OK?

> > 	_cairo_quartz_draw_image (&state, op);
> >     } else if (state.action != DO_NOTHING) {
> > 	rv = CAIRO_INT_STATUS_UNSUPPORTED;
> >     }
> > 
> >     _cairo_quartz_teardown_state (&state);
> > 
> >     ND((stderr, "-- paint\n"));
> > /**
> >+ * cairo_quartz_cglayer_surface_create_similar
> >+ * @surface: a surface that we will eventually want to blit the returned
> >+ * surface to
> 
> This explanation is a unclear. It's not clear what the relationship
> between this associated CGContextRef and the Layer actually is.

OK, how about
"@surface: The returned surface can be efficiently drawn into this destination surface (if tiling is not used)."
?

> 1.10 is probably more accurate :)

I'll fix.

> >+ **/
> >+cairo_surface_t *
> >+cairo_quartz_cglayer_surface_create_similar (cairo_surface_t *surface,
> >+              unsigned int width,
> >+              unsigned int height)
> >+{
> 
> I'm not a huge fan of this name. How about cairo_quartz_create_layer()?

cairo_quartz_surface_create_cg_layer()? That matches cairo_quartz_surface_create and cairo_quartz_surface_create_for_cg_context better...

> What happens if you pass a zero size to CGLayerCreateWithContext()?

It fails, so we return a surface in an error state. I hit exactly that bug last night and it took hours to track down :-(

> Looks like something went wrong with the indentation in this function.

I'll fix.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #5)
> You wouldn't happen to know why these take a gfxSize instead of gfxIntSize
> would you?

Ask Vlad I guess :-)

(In reply to comment #6)
> (In reply to comment #4)
> > >+ **/
> > >+cairo_surface_t *
> > >+cairo_quartz_cglayer_surface_create_similar (cairo_surface_t *surface,
> > >+              unsigned int width,
> > >+              unsigned int height)
> > >+{
> > 
> > I'm not a huge fan of this name. How about cairo_quartz_create_layer()?
> 
> I also wonder if this should take a CGContextRef instead of a cairo_surface_t*.

It could. I was just thinking of this as a special version of cairo_surface_create_similar, and thought that the caller probably already has a cairo surface for their destination, so why not just take that instead of requiring them to extract the CGContext manually?

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #7)
> (In reply to comment #4)

> 
> I'll add a comment to the places where we call CGContextDrawLayerAtPoint.
> 

Great.

> > >@@ -2211,17 +2275,18 @@ _cairo_quartz_surface_paint (void *abstr
> > >     if (state.action == DO_SOLID || state.action == DO_PATTERN) {
> > > 	CGContextFillRect (state.context, CGRectMake(surface->extents.x,
> > > 						     surface->extents.y,
> > > 						     surface->extents.width,
> > > 						     surface->extents.height));
> > >     } else if (state.action == DO_SHADING) {
> > > 	CGContextConcatCTM (state.context, state.transform);
> > > 	CGContextDrawShading (state.context, state.shading);
> > >-    } else if (state.action == DO_IMAGE || state.action == DO_TILED_IMAGE) {
> > >+    } else if (state.action == DO_IMAGE || state.action == DO_TILED_IMAGE ||
> > >+               state.action == DO_LAYER) {
> > 
> > Maybe it's worth rolling these conditions into an inline function?
> 
> Maybe, but it's nice to see in the caller that all the various cases are
> handled, so I'd prefer not to. OK?
> 

Sure.

> > > 	_cairo_quartz_draw_image (&state, op);
> > >     } else if (state.action != DO_NOTHING) {
> > > 	rv = CAIRO_INT_STATUS_UNSUPPORTED;
> > >     }
> > > 
> > >     _cairo_quartz_teardown_state (&state);
> > > 
> > >     ND((stderr, "-- paint\n"));
> > > /**
> > >+ * cairo_quartz_cglayer_surface_create_similar
> > >+ * @surface: a surface that we will eventually want to blit the returned
> > >+ * surface to
> > 
> > This explanation is a unclear. It's not clear what the relationship
> > between this associated CGContextRef and the Layer actually is.
> 
> OK, how about
> "@surface: The returned surface can be efficiently drawn into this destination
> surface (if tiling is not used)."
> ?

I like that.

> 
> > 1.10 is probably more accurate :)
> 
> I'll fix.
> 
> > >+ **/
> > >+cairo_surface_t *
> > >+cairo_quartz_cglayer_surface_create_similar (cairo_surface_t *surface,
> > >+              unsigned int width,
> > >+              unsigned int height)
> > >+{
> > 
> > I'm not a huge fan of this name. How about cairo_quartz_create_layer()?
> 
> cairo_quartz_surface_create_cg_layer()? That matches
> cairo_quartz_surface_create and cairo_quartz_surface_create_for_cg_context
> better...

Good enough for me.

> 
> > What happens if you pass a zero size to CGLayerCreateWithContext()?
> 
> It fails, so we return a surface in an error state. I hit exactly that bug last
> night and it took hours to track down :-(

Maybe add that as a comment.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

We should be able to use this to improve the performance of blitting the canvas backing store to the screen correct?

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #8)
> (In reply to comment #5)
> > You wouldn't happen to know why these take a gfxSize instead of gfxIntSize
> > would you?
> 
> Ask Vlad I guess :-)

If I remember right, it's because the same surface/constructor is used for printing, and at some point we needed to be able to use fractional sizes in order to represent paper sizes correctly.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #10)
> We should be able to use this to improve the performance of blitting the canvas
> backing store to the screen correct?

Probably. I haven't actually measured the performance of blitting from CGBitmapContext to CGLayer or CGWindowContext via CGImage, vs blitting from CGLayer to CGLayer or CGWindowContext. I do know that self-copies using CGBitmapContext are a complete disaster, and patch 1 doesn't work because pixman is broken as noted in comment #1. Self-copies using CGLayer are fast (turns into memmoves), and that's what I need right now.

(Possibly the simplest way to get what I need right now would be patch 1 + Matt Woodrow's fixes to pixman (bug 563488), but by the time Matt's fixes existed I had already written this CGLayer code, and using CGLayers seems like more "the right thing".)

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 447498 [details] [diff] [review]
Part 3: expose gfxQuartzSurface constructor to create a CGLayer surface

Actually I've got a better idea...

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3: remove snapshot

Calling _cairo_quartz_get_image doesn't work for two reasons:
-- for CGBitmapContexts that we created, we always have an imageSurfaceEquiv, and _cairo_quartz_get_image doesn't work for any other kind of context than CGBitmapContexts, so _cairo_quartz_surface_snapshot only does something remotely useful when it gets called on cairo surfaces that wrap foreign CGBitmapContexts (i.e., not much)
-- since the resulting image references the underlying data for the CGBitmapContext, it's not actually a snapshot! oops.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: CGLayer surfaces

Addressed comments.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Note, part 3 needs to be applied before part 2. Sorry.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 4: Create gfxASurface::CreateOffscreenSurface

Exposes this for Thebes users by creating a new gfxASurface API that basically calls cairo_surface_create_similar or cairo_surface_create_cg_layer.

Also adds an AreOffscreenSurfacesSensitiveToContentType API that retained layers needs.

Comment 18

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 447915 [details] [diff] [review]
Part 4: Create gfxASurface::CreateOffscreenSurface

I'm worried about this having the same name as gfxPlatform::CreateOffscreenSurface()

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, how about CreateSimilarSurface()?

Comment 20

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 447915 [details] [diff] [review]
Part 4: Create gfxASurface::CreateOffscreenSurface

Looks good with the name change.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/dd0f1f0a96b3
http://hg.mozilla.org/mozilla-central/rev/e1964291f8ff
http://hg.mozilla.org/mozilla-central/rev/89435e22d7ca

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Also
http://hg.mozilla.org/mozilla-central/rev/03a73eb7f3ec
to add the patches to our list.