1117175 - Stop using MacIOSurfaceTextureSourceOGL for WebGL

Status: RESOLVED INCOMPLETE

(Core :: Graphics: CanvasWebGL, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: Stop using MacIOSurfaceTextureSourceOGL for WebGL

MacIOSurfaceTextureSourceOGL is needlessly inefficient (it calls CGLTexImageIOSurface2D every draw) and hopefully going away soon. This makes gl::SharedSurfaceType::IOSurface uses a regular GLTextureSource instead.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8543347 [details] [diff] [review]
Stop using MacIOSurfaceTextureSourceOGL for WebGL

Review of attachment 8543347 [details] [diff] [review]:
-----------------------------------------------------------------

Lock things that can race.

::: gfx/gl/SharedSurfaceIO.cpp
@@ +131,5 @@
>  
> +void
> +SharedSurface_IOSurface::AcquireConsumerTexture(GLContext* consGL, GLuint* out_texture, GLuint* out_target)
> +{
> +    // XXX: does this need to lock?

Does it happen on a different thread? Lock it.

@@ +157,4 @@
>          MOZ_ASSERT(success);
>          mGL->fDeleteTextures(1, &mProdTex);
>          mGL->fDeleteTextures(1, &mConsTex); // This will work if we're shared.
> +    } else if (mConsTex) {

Why?
Just move the mConsTex deletion out of the `if (mProdTex)` branch.

::: gfx/layers/composite/TextureHost.cpp
@@ +926,5 @@
> +      surf->AcquireConsumerTexture(gl, &tex, &target);
> +
> +      texSource = new GLTextureSource(compositorOGL, tex, target,
> +                                      surf->mSize, format,
> +                                      true/*externally owned*/);

Pull `true` out into a local, and comment on it there. (This will also at-a-glance tell readers what `true` is for)

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8543347 [details] [diff] [review]
Stop using MacIOSurfaceTextureSourceOGL for WebGL

Review of attachment 8543347 [details] [diff] [review]:
-----------------------------------------------------------------

Oh, this was f?. f+ then.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: Stop using MacIOSurfaceTextureSourceOGL for WebGL

The semantics of this version should be clearer now that some of the old stuff has been cleaned up.

The externally_owned stuff I kept to match the other callers in the same file. If we want to change it to use a local bool we can do that in a follow up.

Comment 4

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8545630 [details] [diff] [review]
Stop using MacIOSurfaceTextureSourceOGL for WebGL

Review of attachment 8545630 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/SharedSurfaceIO.cpp
@@ +136,4 @@
>          DebugOnly<bool> success = mGL->MakeCurrent();
>          MOZ_ASSERT(success);
>          mGL->fDeleteTextures(1, &mProdTex);
> +    } else if (mConsTex) {

Woah, why is this `else if`, and not just `if`?

If we have both an `mProdTex` and an `mConsTex`, only Prod will be deleted.

Unless there's a good reason to keep this `else if`, this should be `if`, even if having both at the same time isn't currently possible.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8545630 [details] [diff] [review]
Stop using MacIOSurfaceTextureSourceOGL for WebGL

Review of attachment 8545630 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/SharedSurfaceIO.cpp
@@ +136,4 @@
>          DebugOnly<bool> success = mGL->MakeCurrent();
>          MOZ_ASSERT(success);
>          mGL->fDeleteTextures(1, &mProdTex);
> +    } else if (mConsTex) {

Nope, the 'else if' was an unintentional leftover, I'll change it to an 'if'.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Attachment: Stop using MacIOSurfaceTextureSourceOGL for WebGL

Comment 7

[Bas Schouten (:bas.schouten)]

Comment on attachment 8545941 [details] [diff] [review]
Stop using MacIOSurfaceTextureSourceOGL for WebGL

Review of attachment 8545941 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/SharedSurfaceIO.cpp
@@ +124,5 @@
> +
> +        mConsGL = consGL;
> +    }
> +
> +    MOZ_ASSERT(consGL == mConsGL);

I might argue this assert is a little excessive.. You've asserted this before and in the alternative control flow just done the assignment. But I guess it isn't harmful.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

So MakeCurrent is failing on destruction presumably because MarkDestroyed is being called on the compositor's gl context. I'm not yet sure how to deal with this...

Comment 9

[Jeff Muizelaar [:jrmuizel]]

It looks like this approach is ultimately going to have the problem of using gl contexts on different threads than they were intended to. We probably shouldn't do this.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

It may just make sense to switch directly to TextureClient/TextureHost like gralloc is doing.

Comment 11

[u279076]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #10)
> It may just make sense to switch directly to TextureClient/TextureHost like
> gralloc is doing.

It's been over a year, is there any update on this?