575521 - Make TextureImageCGL fast

Status: RESOLVED FIXED

(Core :: Graphics, enhancement)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The current implementation uses gfxPlatform::CreateOffscreenSurface() to create a gfxQuartzSurface as a scratch surface, and then draws that into a gfxImageSurface for texture upload.  This code isn't optimized for OS X at all.

Says vlad:

It's been a while, but you can create a gfxImageSurface and wrap it in a
gfxQuartzImageSurface.  I can't remember how fast that is to draw into, though.
 Another option is to grab the quartz surface's CGContext, and try
CGBitmapContextGetData etc. on it in case it's a bitmap context.

Comment 1

[Joe Drew (not getting mail)]

Attachment: Part 1: Add cairo_quartz_surface_get_image

We can get direct access to the image data backing the quartz surfaces in a lot of cases. Make that accessible to users.

Comment 2

[Joe Drew (not getting mail)]

Attachment: Part 2: Expose GetImage on gfxQuartzSurface

Comment 3

[Joe Drew (not getting mail)]

Attachment: Part 3: use GetImage in TextureImageCGL

Now that we have GetImage, we should just use it to avoid a copy and make textures faster.

Comment 4

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

How about this?

    cairo_surface_t *surface = cairo_quartz_surface_get_image(mSurface);
    // surface is addrefed here
    if (surface) {
        // gfxImageSurface shares the refcount of 'surface', so it's already
        // addrefed
        return new gfxImageSurface(surface);
    }
    return nsnull;

Comment 5

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

ok, that doesn't handle the case where 'surface' is already wrapped.

Comment 6

[Joe Drew (not getting mail)]

Attachment: part 0: call CGContextFlush in _cairo_quartz_get_image

We should really call CGContextFlush in get_image, so we're sure we have the latest contents.

Comment 7

[Joe Drew (not getting mail)]

Attachment: Part 2: Expose GetImage on gfxQuartzSurface without leaking

Things are complicated with the way Thebes does refcounting (that is, by sharing the Cairo refcount).

We have to use gfxASurface::Wrap() because we can't be certain whether the surface has been wrapped before. Wrap always addrefs its return value. However, so does cairo_quartz_surface_get_image, and since we share the refcount, this causes a reference leak.

To fix this leak, we explicitly Release one of the references to this surface. It's ugly, but it seems necessary.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 475429 [details] [diff] [review]
part 0: call CGContextFlush in _cairo_quartz_get_image

Does this make a difference?

Comment 9

[Joe Drew (not getting mail)]

No - just in case.

Comment 10

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

Can we have a new bug to try to use APPLE_client_storage?

Comment 11

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

I'm actually not sure that APPLE_client_storage is what we want; it's described here -- http://www.opengl.org/registry/specs/APPLE/client_storage.txt says "Clients using this extension are agreeing to preserve a texture's image data for the life of the texture."  Do we actually want that?  Would mean that each TextureImageCGL instance would need to permanently keep around a gfxQuartzSurface (and its associated CGBitmapContext) for its lifetime.

Comment 12

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

Yeah, it's a performance memory tradeoff, but we should explore it.

Comment 13

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

Comment on attachment 475432 [details] [diff] [review]
Part 2: Expose GetImage on gfxQuartzSurface without leaking

Is fine, but please add newlines after the return values (and fix GetDefaultContextFlags while you're in it).

Comment 14

[Joe Drew (not getting mail)]

Attachment: part 4: check if the APPLE_client_storage extension is available

This patch makes it easy to tell if we've got the client storage extension.

Comment 15

[Joe Drew (not getting mail)]

Attachment: part 5: use APPLE_client_storage

This patch actually uses APPLE_client_storage. Unfortunately, it has some issues; for example, about 3/4 of the time when I launch Firefox to a blank page, then navigate to a new page, the back button is only partially drawn. Other, similar things can also happen: http://grab.by/6xwJ

I haven't got a lot of ideas as to what I could be doing wrong here. Building on top of using double buffering properly (bug 593342) doesn't seem to make any difference. Any ideas or fixes you fine gentlemen might have would be highly appreciated.

Comment 16

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

Comment on attachment 478184 [details] [diff] [review]
part 5: use APPLE_client_storage

This is beyond my ken

Comment 17

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

Comment on attachment 478184 [details] [diff] [review]
part 5: use APPLE_client_storage

This looks pretty reasonable to me, except for the removal of -fomit-frame-pointer.

Comment 18

[Markus Stange [:mstange]]

The spec at http://www.opengl.org/registry/specs/APPLE/client_storage.txt says:

> Clients using this extension are agreeing to preserve a texture's image data
> for the life of the texture.  The life of the texture is defined, in this
> case, as the time from first issuing the TexImage3D, TexImage2D or
> TexImage1D command, for the specific texture object with the
> UNPACK_CLIENT_STORAGE_APPLE pixel storage parameter set to TRUE, until the
> DeleteTextures command or another TexImage command for that same object. 
Are we honoring that?

Comment 19

[Joe Drew (not getting mail)]

Yes - the only thing that deletes mUpdateSurface is the destructor (and when we need to recreate the texture, but that's fine too).

Comment 20

[Markus Stange [:mstange]]

Comment on attachment 478184 [details] [diff] [review]
part 5: use APPLE_client_storage

OK, I don't have any other ideas unfortunately.

Comment 21

[Joe Drew (not getting mail)]

Attachment: part 1: transfer ownership of image data to image surface on _finish

The quartz surface destroys its image data unconditionally when it gets _finish()ed. That's wrong if we're passing out references to the images. Instead, when we're done, just let the image surface manage the data.

Comment 22

[Joe Drew (not getting mail)]

(Pretend I regenerated the Cairo patch correctly.)

Comment 23

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

Comment on attachment 478322 [details] [diff] [review]
part 1: transfer ownership of image data to image surface on _finish

>diff --git a/gfx/cairo/cairo/src/cairo-quartz-surface.c b/gfx/cairo/cairo/src/cairo-quartz-surface.c
>--- a/gfx/cairo/cairo/src/cairo-quartz-surface.c
>+++ b/gfx/cairo/cairo/src/cairo-quartz-surface.c
>@@ -1880,24 +1880,22 @@ _cairo_quartz_surface_finish (void *abst
>     surface->cgContext = NULL;
> 
>     if (surface->bitmapContextImage) {
>         CGImageRelease (surface->bitmapContextImage);
>         surface->bitmapContextImage = NULL;
>     }
> 
>     if (surface->imageSurfaceEquiv) {
>+	_cairo_image_surface_assume_ownership_of_data (surface->imageSurfaceEquiv);
> 	cairo_surface_destroy (surface->imageSurfaceEquiv);
> 	surface->imageSurfaceEquiv = NULL;
>     }
> 
>-    if (surface->imageData) {
>-	free (surface->imageData);
>-	surface->imageData = NULL;
>-    }
>+    surface->imageData = NULL;

Hmm.. I can't remember the codepaths here, but it seems like you'd want to write this as:

if (surface->imageSurfaceEquiv) {
  ...
} else if (surface->imageData) {
  free(imageData);
}

imageData = NULL;

otherwise you can potentially leak if, for whatever reason, we have imageData but no equivalent image surface.

Comment 24

[Joe Drew (not getting mail)]

Attachment: part 5: use APPLE_client_storage

This is a cleaned-up version of the previous patch, that now works since we transfer ownership of the image data in part 1.

Comment 25

[Joe Drew (not getting mail)]

(In reply to comment #23)

> Hmm.. I can't remember the codepaths here, but it seems like you'd want to
> write this as:
> 
> if (surface->imageSurfaceEquiv) {
>   ...
> } else if (surface->imageData) {
>   free(imageData);
> }
> 
> imageData = NULL;
> 
> otherwise you can potentially leak if, for whatever reason, we have imageData
> but no equivalent image surface.

It doesn't appear to be possible unless cairo_image_surface_create_for_data returns null in cairo_quartz_surface_create (and in that case we're probably in a lot more trouble than just a leak), but it's also harmless to do this.

Comment 26

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

Comment on attachment 478322 [details] [diff] [review]
part 1: transfer ownership of image data to image surface on _finish

Yeah, let's do it for future-proofing (The else-etc.)  r+ with that.

Comment 27

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

Comment on attachment 478350 [details] [diff] [review]
part 5: use APPLE_client_storage

So this is ok, however there's one additional optimization we may be able to make... we're just caching the image surface, which is a one-off obtained from the quartz surface, or is it the actual bits in the quartz surface?  If the latter, we should be able to do just keep around the original Quartz surface to return in CreateUpdateSurface, right?

Comment 28

[Joe Drew (not getting mail)]

Attachment: part 6: reuse gfxQuartzSurface

Like this?

Comment 29

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

Comment on attachment 478474 [details] [diff] [review]
part 6: reuse gfxQuartzSurface

Yeah maybe, though since this uses CreateOffscreenSurface I don't think it actually matters.  The bitmap image that's created is pretty much detached from the context, so we're just saving the cost of creating the context here (at the memory expense of keeping it around)

Comment 30

[Joe Drew (not getting mail)]

The gfxImageSurface returned from cairo_quartz_surface_get_image is the one created by cairo_quartz_surface_create, in case it wasn't obvious. The data it points to backs the context to which we draw.

Comment 31

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

Comment on attachment 478474 [details] [diff] [review]
part 6: reuse gfxQuartzSurface

Oh, I didn't realize we used bitmap contexts always -- then yes, this won't hurt and might help.

Comment 32

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Expose GetAsImageSurface

Updated for bug 605057

Comment 33

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: use GetImage in TextureImageCGL v2

Updated for new part2

Comment 34

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 7: Optimize CanvasLayerOGL using GetAsImageSurface

Comment 35

[Matt Woodrow (:mattwoodrow)]

This canvas patch takes FishIE (with 1 fish) from 10fps -> 18fps for me.

Comment 36

[Oleg Romashin (:romaxa)]

Comment on attachment 484566 [details] [diff] [review]
Part 7: Optimize CanvasLayerOGL using GetAsImageSurface

>     if (mCanvasSurface) {
>-      nsRefPtr<gfxASurface> sourceSurface = mCanvasSurface;
>+      image = mCanvasSurface->GetAsImageSurface();
> 

IIUC this should be inside MAC_OSX ifdef or GetAsImageSurface should be virtual method of gfxASurface

Comment 37

[Matt Woodrow (:mattwoodrow)]

Bug 605057 will add this.

Comment 38

[Joe Drew (not getting mail)]

Comment on attachment 478474 [details] [diff] [review]
part 6: reuse gfxQuartzSurface

I have patches that do a lot more than this in the way of reusing surfaces.

Comment 39

[Joe Drew (not getting mail)]

Attachment: Part 8: add cairo_quartz_surface_create_for_data

Add a new function to create cairo quartz surfaces, cairo_quartz_surface_create_for_data. This takes a pointer to a chunk of memory instead of allocating the memory itself. This will let us avoid allocations when we've already got enough space to hold a particular draw.

Comment 40

[Joe Drew (not getting mail)]

Attachment: Part 9: expose _create_for_data from Thebes

Comment 41

[Joe Drew (not getting mail)]

Attachment: part 10: Alias the bits to avoid allocations/copies

If we have enough room to draw directly into our existing backing store, we should do so, because it avoids copies when we call glTexSubImage2D from a location inside the buffer we initially gave to OpenGL with APPLE_client_storage. Using the new gfxQuartzSurface constructor that uses _create_for_data, we can alias those bits and have draws work properly.

Comment 42

[Joe Drew (not getting mail)]

We need these patches for OpenGL performance.

Comment 43

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 486473 [details] [diff] [review]
part 10: Alias the bits to avoid allocations/copies

It seems like this is trying to emulate clipping by making new surfaces.

Comment 44

[Joe Drew (not getting mail)]

Attachment: Part 8 (mark 2): draw to a clipped context

Rather than creating a new surface, let's just hold on to our old one and clip our drawing to it. We don't need to use a device translation because the surface backing this is already at the origin.

Comment 45

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 489555 [details] [diff] [review]
Part 8 (mark 2): draw to a clipped context


>         }
>     } else {
>+        unsigned char* data = uploadImage->Data() + mClippedRect.x * 4 +
>+                                                    mClippedRect.y * uploadImage->Stride();

Let's use mUpdateOffset or mUpdateOrigin instead of mClippedRect.



> protected:
>+    virtual gfxContext*
>+    BeginUpdate(nsIntRegion& aRegion)
>+    {
>+        ImageFormat format =
>+            (GetContentType() == gfxASurface::CONTENT_COLOR) ?
>+            gfxASurface::ImageFormatRGB24 : gfxASurface::ImageFormatARGB32;
>+        if (!mTextureInited || !mBackingSurface || !mUpdateSurface ||
>+            nsIntSize(mBackingSurface->Width(), mBackingSurface->Height()) < mSize ||
>+            mBackingSurface->Format() != format) {
>+            mUpdateSurface = nsnull;


Fix the indentation here.

>+            mClippedRect = nsIntRect(0, 0, 0, 0);
>+            // We need to (re)create our backing store. Let the base class to that.
>+            return BasicTextureImage::BeginUpdate(aRegion);
>+        }
>+
>+        // the basic impl can't upload updates to disparate regions,

disparate is not the greatest word here. We agreed that saying what it can only do is better.

Comment 46

[Joe Drew (not getting mail)]

Attachment: Part 8 (mark 2): draw to a clipped context, v2

Comment 47

[Joe Drew (not getting mail)]

Attachment: Part 8 (mark 2): draw to a clipped context, v2

Upload the real patch this time.

Comment 48

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/42244cff985a
http://hg.mozilla.org/mozilla-central/rev/1490c9277562
http://hg.mozilla.org/mozilla-central/rev/7cfc5393907d
http://hg.mozilla.org/mozilla-central/rev/6b34818510c1
http://hg.mozilla.org/mozilla-central/rev/3be1fabf50b8
http://hg.mozilla.org/mozilla-central/rev/331f67eda6de
http://hg.mozilla.org/mozilla-central/rev/182eb3c4fc44