Closed Bug 575521 Opened 9 years ago Closed 9 years ago

Make TextureImageCGL fast

Categories

(Core :: Graphics, enhancement)

x86
macOS
enhancement
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: cjones, Assigned: joe)

References

Details

Attachments

(10 files, 9 obsolete files)

4.21 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
905 bytes, patch
jrmuizel
: review+
Details | Diff | Splinter Review
1.78 KB, patch
vlad
: review+
Details | Diff | Splinter Review
1.20 KB, patch
vlad
: review+
Details | Diff | Splinter Review
8.91 KB, patch
vlad
: review+
bas.schouten
: review+
Details | Diff | Splinter Review
7.05 KB, patch
vlad
: review+
Details | Diff | Splinter Review
1.79 KB, patch
joe
: review+
Details | Diff | Splinter Review
4.17 KB, patch
joe
: review+
Details | Diff | Splinter Review
4.33 KB, patch
joe
: review+
romaxa
: feedback-
Details | Diff | Splinter Review
6.74 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
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.
We can get direct access to the image data backing the quartz surfaces in a lot of cases. Make that accessible to users.
Assignee: nobody → joe
Attachment #475317 - Flags: review?(jmuizelaar)
Attachment #475318 - Flags: review?(jmuizelaar)
Attachment #475317 - Attachment description: Add cairo_quartz_surface_get_image → Part 1: Add cairo_quartz_surface_get_image
Now that we have GetImage, we should just use it to avoid a copy and make textures faster.
Attachment #475319 - Flags: review?(jmuizelaar)
Attachment #475317 - Flags: review?(jmuizelaar) → review+
Attachment #475318 - Flags: review?(jmuizelaar) → review+
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;
ok, that doesn't handle the case where 'surface' is already wrapped.
We should really call CGContextFlush in get_image, so we're sure we have the latest contents.
Attachment #475429 - Flags: review?(jmuizelaar)
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.
Attachment #475318 - Attachment is obsolete: true
Attachment #475432 - Flags: review?(jmuizelaar)
Comment on attachment 475429 [details] [diff] [review]
part 0: call CGContextFlush in _cairo_quartz_get_image

Does this make a difference?
No - just in case.
Attachment #475429 - Flags: review?(jmuizelaar) → review+
Attachment #475319 - Flags: review?(jmuizelaar) → review+
Can we have a new bug to try to use APPLE_client_storage?
Whiteboard: [needs landing]
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.
Yeah, it's a performance memory tradeoff, but we should explore it.
Whiteboard: [needs landing] → [needs review jeff]
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).
Attachment #475432 - Flags: review?(jmuizelaar) → review+
Whiteboard: [needs review jeff]
This patch makes it easy to tell if we've got the client storage extension.
Attachment #478182 - Flags: review?(vladimir)
Attached patch part 5: use APPLE_client_storage (obsolete) — Splinter Review
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.
Attachment #478184 - Flags: feedback?(vladimir)
Attachment #478184 - Flags: feedback?(roc)
Attachment #478184 - Flags: feedback?(mstange)
Attachment #478184 - Flags: feedback?(matt.woodrow+bugzilla)
Comment on attachment 478184 [details] [diff] [review]
part 5: use APPLE_client_storage

This is beyond my ken
Attachment #478184 - Flags: feedback?(roc)
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.
Attachment #478184 - Flags: feedback?(vladimir) → feedback+
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?
Yes - the only thing that deletes mUpdateSurface is the destructor (and when we need to recreate the texture, but that's fine too).
Comment on attachment 478184 [details] [diff] [review]
part 5: use APPLE_client_storage

OK, I don't have any other ideas unfortunately.
Attachment #478184 - Flags: feedback?(mstange)
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.
Attachment #475317 - Attachment is obsolete: true
Attachment #478322 - Flags: review?(vladimir)
Attachment #478322 - Flags: review?(bas.schouten)
(Pretend I regenerated the Cairo patch correctly.)
Attachment #478322 - Flags: review?(bas.schouten) → review+
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.
This is a cleaned-up version of the previous patch, that now works since we transfer ownership of the image data in part 1.
Attachment #478184 - Attachment is obsolete: true
Attachment #478350 - Flags: review?(vladimir)
Attachment #478184 - Flags: feedback?(matt.woodrow+bugzilla)
(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 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.
Attachment #478322 - Flags: review?(vladimir) → review+
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?
Attachment #478350 - Flags: review?(vladimir) → review+
Attached patch part 6: reuse gfxQuartzSurface (obsolete) — Splinter Review
Like this?
Attachment #478474 - Flags: review?(vladimir)
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)
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 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.
Attachment #478474 - Flags: review?(vladimir) → review+
Depends on: 605057
Updated for new part2
Attachment #484564 - Flags: review?(joe)
This canvas patch takes FishIE (with 1 fish) from 10fps -> 18fps for me.
Attachment #484561 - Flags: review?(joe) → review+
Attachment #484564 - Flags: review?(joe) → review+
Attachment #484566 - Flags: review?(joe) → review+
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
Attachment #484566 - Flags: feedback-
Bug 605057 will add this.
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.
Attachment #478474 - Attachment is obsolete: true
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.
Attachment #486471 - Flags: review?(jmuizelaar)
Attachment #486472 - Flags: review?(jmuizelaar)
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.
Attachment #486473 - Flags: review?(jmuizelaar)
We need these patches for OpenGL performance.
blocking2.0: --- → betaN+
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.
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.
Attachment #486471 - Attachment is obsolete: true
Attachment #486472 - Attachment is obsolete: true
Attachment #486473 - Attachment is obsolete: true
Attachment #489555 - Flags: review?(jmuizelaar)
Attachment #486471 - Flags: review?(jmuizelaar)
Attachment #486472 - Flags: review?(jmuizelaar)
Attachment #486473 - Flags: review?(jmuizelaar)
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.
Attachment #489555 - Flags: review?(jmuizelaar) → review-
Attachment #489597 - Flags: review?(jmuizelaar)
Upload the real patch this time.
Attachment #489555 - Attachment is obsolete: true
Attachment #489597 - Attachment is obsolete: true
Attachment #489602 - Flags: review?(jmuizelaar)
Attachment #489597 - Flags: review?(jmuizelaar)
Attachment #489602 - Flags: review?(jmuizelaar) → review+
Depends on: 611498
Depends on: 611564
You need to log in before you can comment on or make changes to this bug.