950903 - Expose backend-specific GLContext subclasses, rather than exposing backend-specific notions in GLContext

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Kelsey Gilbert [:jgilbert]]

We want a good way for external code presented with a GLContext to retrieve the GLContext{EGL,GLX,CGL,WGL}.

From talking with :bjacob we seemed to agree on: (or similar) ('proposal 1')
GLContext has no backend-specific functions
GLContext::GetNativeGLContext() returns a GLContext{EGL,GLX,CGL,WGL}
Backend-specific calls live on backend-specific GLContext implementations:
GLContextCGL::GetCGLContext() returns CGLContext
GLContextCGL::GetNSOpenGLContext() returns NSOpenGLContext

I would actually like to use something similar to SharedSurfaceFoo::Cast(), with the assert that gives us, which would shape the API as: ('proposal 2')

GLContext::GetBackendType returns {EGL, WGL, CGL, GLX}

static GLContextFoo* GLContextFoo::Cast(GLContext* gl) {
  MOZ_ASSERT(gl->GetBackendType() == Foo)
  return (GLContextFoo*)gl;
}

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Taking. The part that I'm interested in, is exposing GLContextCGL and friends directly to remove the need to expose backend-specific things in GLContext itself. I'll adopt the Cast() API that you propose for that. The part that I'm less interested in, is changing the API inside of GLContextCGL and friends. I'll leave that unchanged.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Oh, I see: there simply was no other API than GetNativeData to get the stuff in a GLContextCGL. OK, adding the API that you suggested. Patches coming.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: 1/8 - Expose GLContextEGL publicly

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: 2/8 - Expose GLContextGLX publicly

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: 3/8 - Expose GLContextCGL publicly

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: 4/8 - Expose GLContextWGL publicly

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: 5/8 - Add static Cast methods to allow getting the backend-specific object from a GLContext

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: 6/6 - Remove GLContext::GetNativeData

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: 6/8 - Remove GLContext::GetNativeData

Some build fixes.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: 7/8 - Simplifications around EGLSurface override

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: 8/8 - Make CanvasLayerD3D10 not try to use ANGLE shared handles with WGL

I have no idea how this worked before. Looks like the old code was reinterpreting a WGLContext handle as a EGLContext handle. I had to fix that as the new Cast() methods assert that the context type is the right one. Was hitting this assertion failure on native-gl WebGL reftests on windows.

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Combined diff

As a reviewing helper. 

Also, here's the diff --stat:

$ hg diff --stat -r qparent:no-surfacefactoryangle-on-wgl
 dom/plugins/base/android/ANPOpenGL.cpp           |    4 +-                   
 gfx/gl/GLContext.h                               |    9 -                    
 gfx/gl/GLContextCGL.h                            |   69 ++                   
 gfx/gl/GLContextEGL.h                            |  133 ++++                 
 gfx/gl/GLContextGLX.h                            |   83 ++                   
 gfx/gl/GLContextProviderCGL.mm                   |  174 ++--                 
 gfx/gl/GLContextProviderEGL.cpp                  |  665 +++++++++----------- 
 gfx/gl/GLContextProviderGLX.cpp                  |  371 +++++------          
 gfx/gl/GLContextProviderWGL.cpp                  |  270 +++----
 gfx/gl/GLContextWGL.h                            |   84 ++
 gfx/gl/GLSharedHandleHelpers.cpp                 |    5 +-
 gfx/gl/SharedSurfaceANGLE.cpp                    |   10 +-
 gfx/gl/SharedSurfaceEGL.cpp                      |    6 +-
 gfx/gl/SharedSurfaceIO.cpp                       |    4 +-
 gfx/gl/WGLLibrary.h                              |    2 +
 gfx/gl/moz.build                                 |    8 +-
 gfx/layers/d3d10/CanvasLayerD3D10.cpp            |   12 +-
 gfx/layers/opengl/MacIOSurfaceTextureHostOGL.cpp |    4 +-
 widget/cocoa/nsChildView.mm                      |    9 +-
 19 files changed, 1074 insertions(+), 848 deletions(-)

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

Green on try:
https://tbpl.mozilla.org/?tree=Try&rev=3c9c4a27c843

Comment 14

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8355329 [details] [diff] [review]
5/8 - Add static Cast methods to allow getting the backend-specific object from a GLContext

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

No implied virtual overrides, please!

::: gfx/gl/GLContextCGL.h
@@ +28,5 @@
>                   bool isOffscreen = false);
>  
>      ~GLContextCGL();
>  
> +    GLContextType GetContextType() { return ContextTypeCGL; }

Add `virtual` and `MOZ_OVERRIDE`. ([1])

::: gfx/gl/GLContextEGL.h
@@ +37,5 @@
>                   EGLContext context);
>  
>      ~GLContextEGL();
>  
> +    GLContextType GetContextType() { return ContextTypeEGL; }

[1]

::: gfx/gl/GLContextGLX.h
@@ +30,5 @@
>                      gfxXlibSurface* pixmap = nullptr);
>  
>      ~GLContextGLX();
>  
> +    GLContextType GetContextType() { return ContextTypeGLX; }

[1]

::: gfx/gl/GLContextWGL.h
@@ +38,5 @@
>                   LibType aLibUsed);
>  
>      ~GLContextWGL();
>  
> +    GLContextType GetContextType() { return ContextTypeWGL; }

[1]

Comment 15

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8355613 [details] [diff] [review]
6/8 - Remove GLContext::GetNativeData

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

Woo!

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e51b55aa36de
https://hg.mozilla.org/integration/mozilla-inbound/rev/03baae33872f
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e157aa1a51e
https://hg.mozilla.org/integration/mozilla-inbound/rev/475af8f302c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/b78144fcc95b
https://hg.mozilla.org/integration/mozilla-inbound/rev/3265325c3ad5
https://hg.mozilla.org/integration/mozilla-inbound/rev/3894fdb72320
https://hg.mozilla.org/integration/mozilla-inbound/rev/73a83e2f8751

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e51b55aa36de
https://hg.mozilla.org/mozilla-central/rev/03baae33872f
https://hg.mozilla.org/mozilla-central/rev/4e157aa1a51e
https://hg.mozilla.org/mozilla-central/rev/475af8f302c5
https://hg.mozilla.org/mozilla-central/rev/b78144fcc95b
https://hg.mozilla.org/mozilla-central/rev/3265325c3ad5
https://hg.mozilla.org/mozilla-central/rev/3894fdb72320
https://hg.mozilla.org/mozilla-central/rev/73a83e2f8751