Bug 60757 – [CAIRO] Don't copy the surface before drawing it into the context in ShareableBitmap::paint()

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

cairo_image_surface_create_for_data() is already a shallow copy of the image, so we don't need to copy it again.

[Carlos Garcia Campos]

Created attachment 93434 [details]
Patch

[Martin Robinson]

(In reply to comment #0)
> cairo_image_surface_create_for_data() is already a shallow copy of the image, so we don't need to copy it again.

Notice that in ShareableBitmapCG.cpp (the CG version of this file), the code does a copy of the image. Perhaps that this is just to support self copies though. If that's the case, it would be better to support self-copies by detecting when the target of the context is the same as the ShareableBitmap. Please contect the Apple team to see what the correct behavior is here.

[Martin Robinson]

Comment on attachment 93434 [details]
Patch

Removing review flag until we get some input from the authors of the CG version of ShareableBitmap.

[Carlos Garcia Campos]

(In reply to comment #2)
> (In reply to comment #0)
> > cairo_image_surface_create_for_data() is already a shallow copy of the image, so we don't need to copy it again.
> 
> Notice that in ShareableBitmapCG.cpp (the CG version of this file), the code does a copy of the image. Perhaps that this is just to support self copies though. If that's the case, it would be better to support self-copies by detecting when the target of the context is the same as the ShareableBitmap. Please contect the Apple team to see what the correct behavior is here.

But we are creating a new surface and copying that new surface, CG copies the already existing image, I think, which is the same than simply create a new surface with the image data.

[Martin Robinson]

(In reply to comment #4)
> > Notice that in ShareableBitmapCG.cpp (the CG version of this file), the code does a copy of the image. Perhaps that this is just to support self copies though. If that's the case, it would be better to support self-copies by detecting when the target of the context is the same as the ShareableBitmap. Please contect the Apple team to see what the correct behavior is here.
> 
> But we are creating a new surface and copying that new surface, CG copies the already existing image, I think, which is the same than simply create a new surface with the image data.

The documentation for CGBitmapContextCreateImage states: 

The CGImage object returned by this function is created by a copy operation. Subsequent changes to the bitmap graphics context do not affect the contents of the returned image. In some cases the copy operation actually follows copy-on-write semantics, so that the actual physical copy of the bits occur only if the underlying data in the bitmap graphics context is modified. As a consequence, you may want to use the resulting image and release it before you perform additional drawing into the bitmap graphics context. In this way, you can avoid the actual physical copy of the data.

In the case of Cairo, unless you explicitly create a copy of the original surface, all surfaces will be backed by the same block of data. Thus it seems the semantics of the blit could deviate if the ShareableBitmap is painting onto itself.

In my opinion, we should check with the authors of the CG version. If the bitmap will never do a self-paint we can make your optimization, otherwise we can optimize only when the backing surface of the GraphicsContext does not contain the same bytes as the ShareableBitmap.

[Carlos Garcia Campos]

Committed r87517: <http://trac.webkit.org/changeset/87517>