Stop using share groups with CGL

RESOLVED INCOMPLETE

Status

()

RESOLVED INCOMPLETE
5 years ago
2 years ago

People

(Reporter: mattwoodrow, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
We should be able to stop using these now that we using IOSurfaces for sharing texture between WebGL and the window GLContext.

Apparently this will be useful for guillaume's WebGL 2 work.
(Reporter)

Comment 1

5 years ago
Created attachment 778029 [details] [diff] [review]
Improve the common API for drawing using a SharedSurface_GL

Currently we have to have big switch statements in the consumers, separately dealing with each shared surface type.

This makes SharedSurface_GL expose an API that is sufficient to render all the surface types, so we can handle them generically, and stop including all the specific headers and casting.

Not *really* required for this bug, but it makes it easier.

Should make implementing a new SharedSurface_GL subclass in the future much easier too.
Attachment #778029 - Flags: review?(jgilbert)

Comment 2

5 years ago
Add this bug in the WebGL 2 dependencies tree : https://bugzilla.mozilla.org/showdependencygraph.cgi?id=889977
Blocks: 891033
(Reporter)

Comment 3

5 years ago
Created attachment 778030 [details] [diff] [review]
Stop requiring a share group for SharedSurface_IOSurface

Currently we use the same texture for both producer and consumer GLContext.

This switches us to use different one (with the same IOSurface bound to both), so that we no longer need a share group.
Attachment #778030 - Flags: review?(jgilbert)
(Reporter)

Comment 4

5 years ago
Created attachment 778032 [details] [diff] [review]
Remove all context sharing code from CGL

This shouldn't be needed any more!

Passes WebGL tests locally.

Try: https://tbpl.mozilla.org/?tree=Try&rev=3a0e8bf94666
Attachment #778032 - Flags: review?(bjacob)
Comment on attachment 778032 [details] [diff] [review]
Remove all context sharing code from CGL

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

Love that!
Attachment #778032 - Flags: review?(bjacob) → review+
We definitely don't want to remove this code only to add it back for this:
http://www.khronos.org/registry/webgl/extensions/WEBGL_shared_resources/
(Reporter)

Comment 7

5 years ago
Well, we still want the first two patches I think.

I can redo the third one so that we share between offscreen contexts, but not between them and the window.
Comment on attachment 778030 [details] [diff] [review]
Stop requiring a share group for SharedSurface_IOSurface

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

::: gfx/gl/SharedSurfaceIO.h
@@ +57,4 @@
>      RefPtr<MacIOSurface> mSurface;
>      nsRefPtr<gfxImageSurface> mImageSurface;
>      GLuint mTexture;
> +    GLuint mConsTexture;

Where is deletion of this handled?
(Reporter)

Comment 9

5 years ago
(In reply to Jeff Gilbert [:jgilbert] from comment #8)

> 
> Where is deletion of this handled?

Good catch, it isn't.

How would you feel about slightly abusing the MOZ_COUNT_CTOR/MOZ_COUNT_DTOR macros so that they count all texture/framebuffer/renderbuffer etc allocations/deallocations?

It would mean that we'd have to make sure we explicitly delete all our objects (instead of just freeing the context), with the upside that any leaked objects would show up as oranges on tbpl.
(In reply to Jeff Gilbert [:jgilbert] from comment #6)
> We definitely don't want to remove this code only to add it back for this:
> http://www.khronos.org/registry/webgl/extensions/WEBGL_shared_resources/

I don't think that we want to implement this extension in anything but a distant future where current GL drivers performance issues with share groups and multiple threads are resolved.

Having two contexts in the same share group current at the same time on two different threads is (driver developers have explicitly drawn our attention to that) forcing certain/many/most drivers to enable slow locking.

For that reason, WEBGL_shared_resources is conflicting with the far more important, short-term plan to enable WebGL on Web workers.

I wrote this to the mailing list a few times already as I really think that WEBGL_shared_resources is a waste of time, most recently:
https://www.khronos.org/webgl/public-mailing-list/archives/1306/msg00048.html

Really the only thing that people actually share a lot across contexts is textures, and for that, there are much better, specific sharing and locking mechanisms... like the upcoming WEBGL_dynamic_textures.

Besides, if one really want WEBGL_shared_resources, that could be done by GLContext multiplexing, still without actual GL share groups.
(Reporter)

Comment 11

5 years ago
Created attachment 785227 [details] [diff] [review]
Stop requiring a share group for SharedSurface_IOSurface v2
Attachment #778030 - Attachment is obsolete: true
Attachment #778030 - Flags: review?(jgilbert)
Attachment #785227 - Flags: review?(jgilbert)
Comment on attachment 778029 [details] [diff] [review]
Improve the common API for drawing using a SharedSurface_GL

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

I sort of preferred it being explicit, but I can see why that's not great.

::: gfx/gl/SharedSurfaceEGL.cpp
@@ +227,5 @@
>          return mConsTex;
>      }
>  
>      if (!mConsTex) {
> +        mConsGL->fGenTextures(1, &mConsTex);

assert(mConsGL)
It's weird that we didn't do this already.

::: gfx/gl/SharedSurfaceEGL.h
@@ +99,1 @@
>      // Implementation-specific functions below:

GetPixels() isn't exactly implementation-specific anymore. :P

::: gfx/gl/SharedSurfaceGL.h
@@ +69,5 @@
>  
>      virtual void LockProd();
>      virtual void UnlockProd();
>  
> +    virtual void SetConsumerGL(GLContext* consGL) {

Why is this virtual?

@@ +85,5 @@
> +
> +    // Returns 0 if the consumer texture isn't available.
> +    // If 0, use GetPixels below.
> +    virtual GLuint AcquireConsumerTexture() {
> +        return Texture();

Texture() is the wrong thing to use. Texture() is what the content-side is supposed to attach to framebuffers, etc. Particularly with share groups going away, this will give garbage data.

It will also trigger an assertion if we back the surface with a renderbuffer!

@@ +88,5 @@
> +    virtual GLuint AcquireConsumerTexture() {
> +        return Texture();
> +    }
> +
> +    // Will be void if AcquireConsumerTexture returns non-zero.

s/void/null/?
Also, assert this.

::: gfx/layers/CopyableCanvasLayer.cpp
@@ +106,5 @@
>      MOZ_ASSERT(sharedSurf->APIType() == APITypeT::OpenGL);
>      SharedSurface_GL* surfGL = SharedSurface_GL::Cast(sharedSurf);
>  
> +    readSurf = surfGL->GetPixels();
> +    if (!readSurf) {

I don't love this change, since it makes it less explicit what's happening. Maybe leave a comment that this is expected for accelerated surfaces? (Therefore the most likely path?)

::: gfx/layers/d3d10/CanvasLayerD3D10.cpp
@@ +133,4 @@
>  
> +      mSRView = shareSurf->GetSRV();
> +    } else {
> +      SharedSurface_GL* shareSurf = SharedSurface_GL::Cast(surf);

What's the point in this change from SS_Basic to SS_GL? It was restrictive for sanity/safety.

::: gfx/layers/opengl/CanvasLayerOGL.cpp
@@ +202,5 @@
>    bool nothingToShow = false;
>    if (mGLContext) {
>      SharedSurface* surf = mGLContext->RequestFrame();
>      if (surf) {
> +      MOZ_ASSERT(surf->APIType() == APITypeT::OpenGL);

Included in SS_GL::Cast

@@ +205,5 @@
>      if (surf) {
> +      MOZ_ASSERT(surf->APIType() == APITypeT::OpenGL);
> +      SharedSurface_GL* glTexSurf = SharedSurface_GL::Cast(surf);
> +      glTexSurf->SetConsumerGL(mGLContext);
> +      mTextureTarget = glTexSurf->TextureTarget();

Can we make sure this returns 0 for SS_Basic?

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +381,5 @@
>  
>    mSize = IntSize(sharedSurf->Size().width, sharedSurf->Size().height);
>  
>    gfxImageSurface* toUpload = nullptr;
> +  MOZ_ASSERT(sharedSurf->APIType() == APITypeT::OpenGL);

This is asserted by SS_GL::Cast already.

@@ +395,3 @@
>    }
> +  mFormat = sharedSurf->HasAlpha() ? FORMAT_R8G8B8A8
> +                                   : FORMAT_R8G8B8X8;

Don't set this if we just set `toUpload`.
Attachment #778029 - Flags: review?(jgilbert) → review-
Comment on attachment 785227 [details] [diff] [review]
Stop requiring a share group for SharedSurface_IOSurface v2

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

::: gfx/gl/SharedSurfaceIO.cpp
@@ +41,5 @@
> +void
> +SharedSurface_IOSurface::SetConsumerGL(GLContext* consGL)
> +{
> +    MOZ_ASSERT(consGL);
> +    if (consGL != mConsGL) {

When does this happen?
Comment on attachment 785227 [details] [diff] [review]
Stop requiring a share group for SharedSurface_IOSurface v2

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

I'm untagging myself from this, unless you want to revive this discussion.
Attachment #785227 - Flags: review?(jgilbert)
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.