704839 - Refactor mutual ownership of WebGL objects

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

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

WebGL objects are different from most other JS objects in that they can be explicitly deleted by webgl.deleteXxx() functions. For example, webgl.deleteTexture(texture).

When such a function is called, here's what happens:
 - The object is put in "deleted" OpenGL state immediately i.e. querying its DELETE_STATUS will return true, but that doesn't mean that its resources are freed. If it's referenced by other WebGL objects (like WebGLProgram referencing its WebGLShaders) then it's kept alive and objects that area already using it keep working, despite its "deleted" status.
 - If no other WebGL object is internally referencing that object (like how a WebGLProgram references its WebGLShaders), the object's resources may then be freed, regardless of any references that the JS engine may still have to this object. Anyway, WebGL won't allow new references to be created to an already deleted object.

So far we are only partly compliant with this (see bug 704827). We have a WebGLObjectRefPtr class that implements different semantics than that: it basically implements _weak_ references between WebGL objects whereas these should be _strong_ references. So it tries to clear references to WebGL objects that get deleted, which is the opposite of what we now want. Something complicated has been implemented in bug 682496 to work around this and pass the conformance test around shader/program attachment, and rather than doing the same for bug 704827, I think we should refactor WebGLObjectRefPtr to directly implement the ownership semantics mandated by the WebGL spec, and do so without relying on the underlying OpenGL implementation to implement them correctly.

In addition to the XPCOM refcount, we need a separate refcount that's internal to the WebGL implementation and not exposed to the JS engine. WebGLObjectRefPtr should inc/dec BOTH refcounts, while the JS engine would keep only inc/dec'ing the XPCOM refcount. WebGLObjectRefPtr is that WebGL objects would use to manage the references that they own to each other.

Comment 1

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

*** Prerequisite patches for the patches here ***

Before applying the patches here, apply the patches from bug 705663, 705665, 705668, 705673.

Bug 705663 is a security bug, it doesn't matter too much if you don't apply it, you'll just get one webgl test error.

Comment 2

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

Attachment: 1: implement WebGLRefCountedObject

Comment 3

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

Attachment: 2: implement WebGLRefPtr

Note that I'm leaving the old classes, WebGLObjectRefPtr and WebGLZeroingObject, in for now, to get a patch series that builds at each stage. They will be removed at the end.

Comment 4

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

Attachment: 3: rename mName to mGLName

That part is cosmetic only. It harmonizes stuff as the same thing was sometimes called "Name" and sometimes "GLName".

Comment 5

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

Attachment: 4: the big changes

This patch is where we switch to the new approach. It introduces some test regressions that we fix over the next patches.

Comment 6

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

Attachment: 5: fix GL_DELETE_STATUS queries

This fixes test regressions introduced by the above changes. We don't call glDeleteXxx immediately anymore in webgl.deleteXxx, so OpenGL's GL_DELETE_STATUS queries do no longer return the right result for WebGL, but that's alright as we really don't need to call them at all.

Comment 7

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

Attachment: 6: fix IsObject functions

I don't think that there was a test failure here, but this patch rewrites these functions in a much simpler way, and the test passes.

Comment 8

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

Attachment: 7: fix framebuffer attachment deletion

The GLES2 spec says, and the WebGL tests enforce, that when one deletes a texture or renderbuffer that is attached to the currently bound framebuffer, it gets detached from it. This regressed in the above changes, as we are no longer calling glDeleteXxx immediately in webgl.deleteXxx.

Comment 9

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

Attachment: 8: move VertexAttribData down

It's out of place where it currently is, near the refcounting helpers.

Comment 10

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

Attachment: 9: remove old classes

This removes the helper classes WebGLObjectRefPtr and WebGLZeroingObject that have been replaced by WebGLRefPtr and WebGLRefCountedObject[Base].

Comment 11

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=655743deed5d

Comment 12

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

All green!

Comment 13

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 577265 [details] [diff] [review]
1: implement WebGLRefCountedObject

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

::: content/canvas/src/WebGLContext.h
@@ +266,5 @@
> + *
> + * This WebGLRefCountedObject class take the Derived type
> + * as template parameter, as a means to allow DeleteOnce to call Delete()
> + * on the Derived class, without either method being virtual. This is a common
> + * C++ pattern known as the "curiously recursive template pattern (CRTP)".

Google seems to show this is moderately more commonly known as "Curiously Recurring Template Pattern", as opposed to 'recursive', but both are used, and google actually has them as synonyms, so it's easy to find either way.

If you feel like changing that, it's probably best to either move the right-hand double-quotes between 'pattern' and '(CRTP)', or just remove the pair of quotes. (See wikipedia's formatting in the article)

Also, the bikeshed should be red.

Comment 14

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 577266 [details] [diff] [review]
2: implement WebGLRefPtr

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

::: content/canvas/src/WebGLContext.h
@@ +348,5 @@
> +        return get();
> +    }
> +
> +    T* operator->() const {
> +

Is this extra newline (and the other one below) a style thing? I'd personally put the newline between the NS_ABORT_IF_FALSE and return statements.

Comment 15

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 577267 [details] [diff] [review]
3: rename mName to mGLName

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

::: content/canvas/src/WebGLContext.h
@@ +2388,5 @@
>  protected:
>      bool mDeleted;
>      WebGLint mSize;
>      WebGLenum mType;
> +    nsString mGLName;

I disagree with this change. Elsewhere, mGLName is the int handle for the GL object, whereas here it's a string. I think leaving it as mName is ideal, especially since it's the name of an attrib or uniform, not a GL name as created by glGenXxx.

Comment 16

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

> > +    nsString mGLName;
> 
> I disagree with this change. Elsewhere, mGLName is the int handle for the GL
> object, whereas here it's a string. I think leaving it as mName is ideal,
> especially since it's the name of an attrib or uniform, not a GL name as
> created by glGenXxx.

Oops. Automatic search and replace fail.

Comment 17

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

(In reply to Jeff Gilbert [:jgilbert] from comment #14)
> Comment on attachment 577266 [details] [diff] [review] [diff] [details] [review]
> 2: implement WebGLRefPtr
> 
> Review of attachment 577266 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContext.h
> @@ +348,5 @@
> > +        return get();
> > +    }
> > +
> > +    T* operator->() const {
> > +
> 
> Is this extra newline (and the other one below) a style thing? I'd
> personally put the newline between the NS_ABORT_IF_FALSE and return
> statements.

No, it's just a mistake on part. I used to have a debug printf there.

Comment 18

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 577268 [details] [diff] [review]
4: the big changes

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

I like how the changes here are routinely simplifying the code.

::: content/canvas/src/WebGLContext.h
@@ -2463,5 @@
>                                    "passed as argument", info);
>          return false;
>      }
>  
> -    if ((*aConcreteObject)->Deleted()) {

Since we're already touching this code, could we change:
*aConcreteObject = static_cast<ConcreteObjectType*>(aInterface);

To:
ConcreteObjectType* foo = *aConcreteObject = static_cast<ConcreteObjectType*>(aInterface);

And reuse 'foo->' instead of '(*aConcreteObject)->'?

::: content/canvas/src/WebGLContextGL.cpp
@@ +1220,5 @@
>          return NS_OK;
>  
> +    for (int i = 0; i < mGLMaxTextureUnits; i++) {
> +        if ((tex->Target() == LOCAL_GL_TEXTURE_2D && mBound2DTextures[i] == tex) ||
> +            (tex->Target() == LOCAL_GL_TEXTURE_CUBE_MAP && mBoundCubeMapTextures[mActiveTexture] == tex))

It looks like you forgot to index through 'mBoundCubeMapTextures' by 'i' instead of 'mActiveTexture'.

@@ +1226,5 @@
> +            ActiveTexture(LOCAL_GL_TEXTURE0 + i);
> +            BindTexture(tex->Target(), nsnull);
> +            ActiveTexture(LOCAL_GL_TEXTURE0 + mActiveTexture);
> +        }
> +    }

Can we not bring the second ActiveTexture() outside the loop here?

@@ +2572,5 @@
>  
>      *retval = nsnull;
>  
>      WebGLuint progname;
> +    WebGLProgram *prog;

Is this unused? It looks that way from Splinter.

Comment 19

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 577271 [details] [diff] [review]
7: fix framebuffer attachment deletion

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

::: content/canvas/src/WebGLContext.h
@@ +2239,5 @@
>          }
> +
> +        if (int(mDepthAttachment.IsDefined()) +
> +            int(mStencilAttachment.IsDefined()) +
> +            int(mDepthStencilAttachment.IsDefined()) >= 2)

Isn't this saying that if we have at least two of depth, stencil, or depth_stencil, we have bad attachments? Are we only allowed to have zero or one attachments at a time?

@@ +2291,5 @@
> +            FramebufferTexture2D(LOCAL_GL_FRAMEBUFFER, LOCAL_GL_DEPTH_ATTACHMENT, LOCAL_GL_TEXTURE_2D, nsnull, 0);
> +        if (mStencilAttachment.Texture() == tex)
> +            FramebufferTexture2D(LOCAL_GL_FRAMEBUFFER, LOCAL_GL_STENCIL_ATTACHMENT, LOCAL_GL_TEXTURE_2D, nsnull, 0);
> +        if (mDepthStencilAttachment.Texture() == tex)
> +            FramebufferTexture2D(LOCAL_GL_FRAMEBUFFER, LOCAL_GL_DEPTH_STENCIL_ATTACHMENT, LOCAL_GL_TEXTURE_2D, nsnull, 0);

Can these not be else-ifs?

Are we sure tex is non-null? It looks like we can, but it might be best to clarify this with a comment or a debug assert.

@@ +2302,5 @@
> +            FramebufferRenderbuffer(LOCAL_GL_FRAMEBUFFER, LOCAL_GL_DEPTH_ATTACHMENT, LOCAL_GL_RENDERBUFFER, nsnull);
> +        if (mStencilAttachment.Renderbuffer() == rb)
> +            FramebufferRenderbuffer(LOCAL_GL_FRAMEBUFFER, LOCAL_GL_STENCIL_ATTACHMENT, LOCAL_GL_RENDERBUFFER, nsnull);
> +        if (mDepthStencilAttachment.Renderbuffer() == rb)
> +            FramebufferRenderbuffer(LOCAL_GL_FRAMEBUFFER, LOCAL_GL_DEPTH_STENCIL_ATTACHMENT, LOCAL_GL_RENDERBUFFER, nsnull);

Again here.

Comment 20

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 577272 [details] [diff] [review]
8: move VertexAttribData down

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

Not even enough to bikeshed. :P

Comment 21

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

Attachment: 3: rename mName to mGLName

Comment 22

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

Attachment: 4: the big changes

(In reply to Jeff Gilbert [:jgilbert] from comment #18)
> ::: content/canvas/src/WebGLContext.h
> @@ -2463,5 @@
> >                                    "passed as argument", info);
> >          return false;
> >      }
> >  
> > -    if ((*aConcreteObject)->Deleted()) {
> 
> Since we're already touching this code, could we change:
> *aConcreteObject = static_cast<ConcreteObjectType*>(aInterface);
> 
> To:
> ConcreteObjectType* foo = *aConcreteObject =
> static_cast<ConcreteObjectType*>(aInterface);
> 
> And reuse 'foo->' instead of '(*aConcreteObject)->'?

Done.

> 
> ::: content/canvas/src/WebGLContextGL.cpp
> @@ +1220,5 @@
> >          return NS_OK;
> >  
> > +    for (int i = 0; i < mGLMaxTextureUnits; i++) {
> > +        if ((tex->Target() == LOCAL_GL_TEXTURE_2D && mBound2DTextures[i] == tex) ||
> > +            (tex->Target() == LOCAL_GL_TEXTURE_CUBE_MAP && mBoundCubeMapTextures[mActiveTexture] == tex))
> 
> It looks like you forgot to index through 'mBoundCubeMapTextures' by 'i'
> instead of 'mActiveTexture'.

Oops! Good catch.

> 
> @@ +1226,5 @@
> > +            ActiveTexture(LOCAL_GL_TEXTURE0 + i);
> > +            BindTexture(tex->Target(), nsnull);
> > +            ActiveTexture(LOCAL_GL_TEXTURE0 + mActiveTexture);
> > +        }
> > +    }
> 
> Can we not bring the second ActiveTexture() outside the loop here?

Done.


> 
> @@ +2572,5 @@
> >  
> >      *retval = nsnull;
> >  
> >      WebGLuint progname;
> > +    WebGLProgram *prog;
> 
> Is this unused? It looks that way from Splinter.

In this patch, yes. That was meant to go in the next one.

Comment 23

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

(In reply to Jeff Gilbert [:jgilbert] from comment #19)
> Comment on attachment 577271 [details] [diff] [review] [diff] [details] [review]
> 7: fix framebuffer attachment deletion
> 
> Review of attachment 577271 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContext.h
> @@ +2239,5 @@
> >          }
> > +
> > +        if (int(mDepthAttachment.IsDefined()) +
> > +            int(mStencilAttachment.IsDefined()) +
> > +            int(mDepthStencilAttachment.IsDefined()) >= 2)
> 
> Isn't this saying that if we have at least two of depth, stencil, or
> depth_stencil, we have bad attachments? Are we only allowed to have zero or
> one attachments at a time?

*among depth, stencil, and depth_stencil*, yes.

So if you want both a depth and a stencil buffer, the only way to get that is to use a DEPTH_STENCIL buffer. See: http://www.khronos.org/registry/webgl/specs/latest/#6.5

> 
> @@ +2291,5 @@
> > +            FramebufferTexture2D(LOCAL_GL_FRAMEBUFFER, LOCAL_GL_DEPTH_ATTACHMENT, LOCAL_GL_TEXTURE_2D, nsnull, 0);
> > +        if (mStencilAttachment.Texture() == tex)
> > +            FramebufferTexture2D(LOCAL_GL_FRAMEBUFFER, LOCAL_GL_STENCIL_ATTACHMENT, LOCAL_GL_TEXTURE_2D, nsnull, 0);
> > +        if (mDepthStencilAttachment.Texture() == tex)
> > +            FramebufferTexture2D(LOCAL_GL_FRAMEBUFFER, LOCAL_GL_DEPTH_STENCIL_ATTACHMENT, LOCAL_GL_TEXTURE_2D, nsnull, 0);
> 
> Can these not be else-ifs?

No, because the same texture or renderbuffer could be attached at more than one attachment point. Of course this isn't possible in the current state of the spec while still giving a complete framebuffer, but it would already be possible to write a valid conformance test that would fail if we added 'else' here:
 1. attach a texture to color attachment point.
 2. attach a DEPTH_STENCIL renderbuffer to both depth and stencil attachment points, so that it's incorrect for both.
 3. delete the renderbuffer.
 4. check that the framebuffer is complete.
This test will only succeed if the renderbuffer has been detached from both attachment points.

> 
> Are we sure tex is non-null? It looks like we can, but it might be best to
> clarify this with a comment or a debug assert.

First of all it shouldn't matter whether tex is null here, as we're not dereferencing it, only comparing it to other pointers.

But yes, tex is non-null because these functions are only called by deleteTexture and deleteRenderbuffer who already take care of the null case earlier.

Comment 24

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 577271 [details] [diff] [review]
7: fix framebuffer attachment deletion

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

r+ after discussion.

Comment 25

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/4d13dff07f89
http://hg.mozilla.org/integration/mozilla-inbound/rev/cba3b08c4bac
http://hg.mozilla.org/integration/mozilla-inbound/rev/539c05fd263c
http://hg.mozilla.org/integration/mozilla-inbound/rev/e8de56f450c9
http://hg.mozilla.org/integration/mozilla-inbound/rev/9d44f155bdbb
http://hg.mozilla.org/integration/mozilla-inbound/rev/c1de92ddbedf
http://hg.mozilla.org/integration/mozilla-inbound/rev/1a4f061c4f2d
http://hg.mozilla.org/integration/mozilla-inbound/rev/2facd7e6fd7c
http://hg.mozilla.org/integration/mozilla-inbound/rev/3a7aa2475d3c

Comment 26

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/4d13dff07f89
http://hg.mozilla.org/integration/mozilla-inbound/rev/cba3b08c4bac
http://hg.mozilla.org/integration/mozilla-inbound/rev/539c05fd263c
http://hg.mozilla.org/integration/mozilla-inbound/rev/e8de56f450c9
http://hg.mozilla.org/integration/mozilla-inbound/rev/9d44f155bdbb
http://hg.mozilla.org/integration/mozilla-inbound/rev/c1de92ddbedf
http://hg.mozilla.org/integration/mozilla-inbound/rev/1a4f061c4f2d
http://hg.mozilla.org/integration/mozilla-inbound/rev/2facd7e6fd7c
http://hg.mozilla.org/integration/mozilla-inbound/rev/3a7aa2475d3c

Comment 27

[Matt Brubeck (:mbrubeck)]

Of course I mean
http://hg.mozilla.org/mozilla-central/rev/4d13dff07f89
http://hg.mozilla.org/mozilla-central/rev/cba3b08c4bac
http://hg.mozilla.org/mozilla-central/rev/539c05fd263c
http://hg.mozilla.org/mozilla-central/rev/e8de56f450c9
http://hg.mozilla.org/mozilla-central/rev/9d44f155bdbb
http://hg.mozilla.org/mozilla-central/rev/c1de92ddbedf
http://hg.mozilla.org/mozilla-central/rev/1a4f061c4f2d
http://hg.mozilla.org/mozilla-central/rev/2facd7e6fd7c
http://hg.mozilla.org/mozilla-central/rev/3a7aa2475d3c