860615 - Support Mac plugins efficiently with OMTC

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: Add CreateSharedHandle support to GLContextCGL

I think the best approach here is to create a GLContext::SharedTextureHandle for the IOSurface, since we already support that with the layers refactoring.

This will also let us get rid of the MacIOSurfaceImage code entirely.

I know jgilbert wants to get rid of this API, but I think it still makes sense to share code with android plugins. We can replace both users with a new API fairly easily.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Add support for SharedTextureHandles in ImageLayerOGL

Hopefully this code can go away too, once we enable OMTC everywhere (or use Compositor in-thread).

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Create SharedTextureHandles for OSX plugins

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Kill MacIOSurfaceImage

Nothing will use this any more, hooray!

Comment 4

[Matt Woodrow (:mattwoodrow)]

As a follow-up, it would be really nice if we could skip involving the main thread at all and have plugins just notify the compositor.

Not entirely sure where to start on that though.

Comment 5

[Nicolas Silva [:nical]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> As a follow-up, it would be really nice if we could skip involving the main
> thread at all and have plugins just notify the compositor.
> 
> Not entirely sure where to start on that though.

Wait for the layers refactoring to stabilize, then use the same mechanism as Async-video. ImageBridge at the moment only does video but there's nothing that is specific to video in ImageBridge: frames are just given to an ImageClient that is off the main thread rather than using one that is on the main thread.

Comment 6

[Benoit Girard (:BenWa)]

Comment on attachment 736138 [details] [diff] [review]
Add CreateSharedHandle support to GLContextCGL

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

::: gfx/gl/GLContextProviderCGL.mm
@@ +101,5 @@
>      {
> +        if (MakeCurrent()) {
> +            if (mTemporaryIOSurfaceTexture != 0) {
> +                fDeleteTextures(1, &mTemporaryIOSurfaceTexture);
> +                mTemporaryIOSurfaceTexture = 0;

Remove unless we have a good reason to be worried of use-after-free.

@@ +197,5 @@
> +
> +        MacIOSurface* surf = static_cast<MacIOSurface*>(buffer);
> +        surf->AddRef();
> +
> +        return (SharedTextureHandle)surf;

reinterpret_cast

Comment 7

[Benoit Girard (:BenWa)]

Comment on attachment 736141 [details] [diff] [review]
Add support for SharedTextureHandles in ImageLayerOGL

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

LGTM with these cleanups.

::: dom/plugins/ipc/PluginInstanceParent.cpp
@@ +29,5 @@
>  #include "nsNPAPIPluginInstance.h"
>  #include "Layers.h"
> +#include "SharedTextureImage.h"
> +#include "GLContext.h"
> +#include "GLContextProvider.h"

Doesn't belong in this patch.

::: dom/plugins/ipc/PluginInstanceParent.h
@@ +349,5 @@
>      uint16_t               mShHeight;
>      CGColorSpaceRef        mShColorSpace;
>      RefPtr<MacIOSurface> mIOSurface;
>      RefPtr<MacIOSurface> mFrontIOSurface;
> +    nsRefPtr<gl::GLContext>  mGLContext;

Doesn't belong in this patch.

::: gfx/layers/opengl/ImageLayerOGL.cpp
@@ +154,5 @@
>    aOutTexture->TakeFrom(&mRecycledTextures[aType].ElementAt(last));
>    mRecycledTextures[aType].RemoveElementAt(last);
>  }
>  
> +struct THEBES_API ImageOGLBackendData : public ImageBackendData

This doesn't really belong either

@@ +200,5 @@
> +  aGL->fTexParameteri(aTarget, LOCAL_GL_TEXTURE_MIN_FILTER, LOCAL_GL_LINEAR);
> +  aGL->fTexParameteri(aTarget, LOCAL_GL_TEXTURE_MAG_FILTER, LOCAL_GL_LINEAR);
> +  aGL->fTexParameteri(aTarget, LOCAL_GL_TEXTURE_WRAP_S, LOCAL_GL_CLAMP_TO_EDGE);
> +  aGL->fTexParameteri(aTarget, LOCAL_GL_TEXTURE_WRAP_T, LOCAL_GL_CLAMP_TO_EDGE);
> +  

space nit

@@ +411,5 @@
> +      return;
> +    }
> +
> +    ShaderProgramOGL* program = mOGLManager->GetProgram(handleDetails.mProgramType, GetMaskLayer());
> +   

space nit

@@ +432,5 @@
> +    ImageOGLBackendData *backendData =
> +      static_cast<ImageOGLBackendData*>(texImage->GetBackendData(LAYERS_OPENGL));
> +    gl()->fActiveTexture(LOCAL_GL_TEXTURE0);
> +    gl()->fBindTexture(handleDetails.mTarget, backendData->mTexture.GetTextureID());
> +    

space nit

Comment 8

[Benoit Girard (:BenWa)]

Comment on attachment 736142 [details] [diff] [review]
Create SharedTextureHandles for OSX plugins

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

::: dom/plugins/ipc/PluginInstanceParent.cpp
@@ +750,5 @@
>  
> +        NS_ASSERTION(image->GetFormat() == SHARED_TEXTURE, "Wrong format?");
> +        if (!mGLContext) {
> +          gl::GLContext::SurfaceCaps dummyCaps = gl::GLContext::SurfaceCaps::ForRGB();
> +          mGLContext = gl::GLContextProvider::CreateOffscreen(gfxIntSize(16, 16), dummyCaps);

Something like this deserve a big fat comment. I don't think it's acceptable to have a dummy GLContext per plugin either.

Comment 9

[Benoit Girard (:BenWa)]

Comment on attachment 736143 [details] [diff] [review]
Kill MacIOSurfaceImage

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

Nice

Comment 10

[Benoit Girard (:BenWa)]

It would be nice to avoid the extra CGLTexImageIOSurface2D but since we're double buffering we'll be swapping the image every frame so there not much point to it yet. We should really optimize this later because IOSurface lookup by id+CGLTexImageIOSurface2D  shows up quite a bit in profiles.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Create SharedTextureHandles for OSX plugins v2

Added comment, and made GLContext shared between plugin instances.

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: Add a way to create a shared handle without a GLContext

I think we should take this for the moment, since I don't see how switching to the SurfaceStream API will actually improve things.

SurfaceStream doesn't abstract over things that we want, like texture target (IOSurface requires GL_TEXTURE_RECTANGLE), texture transform, shader program, so we'd have to hard code all this into all the users of SurfaceStream_IOSurf. I don't see how this any different to just using a typed union.

The SurfaceStream API also expects SurfaceStream objects to be able to create their own textures for rendering, which requires a GLContext. This is what we are trying to avoid with the current patches.

It's also a fair chunk of work, since it requires implementing StreamStream_IOSurface, SurfaceStreamImage, SurfaceStream_IOSurf handling in TextureHostSurfaceStream, and SurfaceStreamImage handling in ImageLayerOGL.

I really don't have time for that, and I definitely don't think we should block Mac OMTC on having it done. This patch shouldn't add much debt should we decide to remove it again later.

Comment 13

[Matt Woodrow (:mattwoodrow)]

Attachment: Create SharedTextureHandles for OSX plugins v3

GLContext creation all gone!

Comment 14

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 737764 [details] [diff] [review]
Add a way to create a shared handle without a GLContext

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

FWIW, I wasn't suggesting switching to SurfaceStream, only to SharedSurface_*.

Comment 15

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0dc8a672b31
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0bede12278b
https://hg.mozilla.org/integration/mozilla-inbound/rev/16c463ff28dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/61c218d62004
https://hg.mozilla.org/integration/mozilla-inbound/rev/6494a1f1dbfd

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d0dc8a672b31
https://hg.mozilla.org/mozilla-central/rev/b0bede12278b
https://hg.mozilla.org/mozilla-central/rev/16c463ff28dd
https://hg.mozilla.org/mozilla-central/rev/61c218d62004
https://hg.mozilla.org/mozilla-central/rev/6494a1f1dbfd