The default bug view has changed. See this FAQ.

Refactor mutual ownership of WebGL objects

RESOLVED FIXED in mozilla11

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

unspecified
mozilla11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 2 obsolete attachments)

7.15 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
3.01 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
2.06 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
4.20 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
7.90 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
2.88 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
5.88 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
13.96 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
44.36 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
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.
(Assignee)

Updated

5 years ago
Blocks: 704827
(Assignee)

Updated

5 years ago
Summary: Refactor mutual ownership of WebGL objects and WebGLObjectRefPtr → Refactor mutual ownership of WebGL objects
(Assignee)

Comment 1

5 years ago
*** 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.
(Assignee)

Comment 2

5 years ago
Created attachment 577265 [details] [diff] [review]
1: implement WebGLRefCountedObject
Attachment #577265 - Flags: review?(jgilbert)
(Assignee)

Updated

5 years ago
Attachment #577265 - Attachment description: implement WebGLRefCountedObject → 1: implement WebGLRefCountedObject
(Assignee)

Comment 3

5 years ago
Created attachment 577266 [details] [diff] [review]
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.
Attachment #577266 - Flags: review?(jgilbert)
(Assignee)

Comment 4

5 years ago
Created attachment 577267 [details] [diff] [review]
3: rename mName to mGLName

That part is cosmetic only. It harmonizes stuff as the same thing was sometimes called "Name" and sometimes "GLName".
Attachment #577267 - Flags: review?(jgilbert)
(Assignee)

Comment 5

5 years ago
Created attachment 577268 [details] [diff] [review]
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.
Attachment #577268 - Flags: review?(jgilbert)
(Assignee)

Comment 6

5 years ago
Created attachment 577269 [details] [diff] [review]
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.
Attachment #577269 - Flags: review?(jgilbert)
(Assignee)

Comment 7

5 years ago
Created attachment 577270 [details] [diff] [review]
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.
Attachment #577270 - Flags: review?(jgilbert)
(Assignee)

Updated

5 years ago
Attachment #577269 - Attachment is patch: true
(Assignee)

Comment 8

5 years ago
Created attachment 577271 [details] [diff] [review]
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.
Attachment #577271 - Flags: review?(jgilbert)
(Assignee)

Comment 9

5 years ago
Created attachment 577272 [details] [diff] [review]
8: move VertexAttribData down

It's out of place where it currently is, near the refcounting helpers.
Attachment #577272 - Flags: review?(jgilbert)
(Assignee)

Updated

5 years ago
Attachment #577272 - Attachment description: move VertexAttribData down → 8: move VertexAttribData down
Created attachment 577276 [details] [diff] [review]
9: remove old classes

This removes the helper classes WebGLObjectRefPtr and WebGLZeroingObject that have been replaced by WebGLRefPtr and WebGLRefCountedObject[Base].
Attachment #577276 - Flags: review?(jgilbert)
Try: https://tbpl.mozilla.org/?tree=Try&rev=655743deed5d
All green!
(Assignee)

Updated

5 years ago
Blocks: 705904

Updated

5 years ago
Assignee: nobody → bjacob
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.
Attachment #577265 - Flags: review?(jgilbert) → review+
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.
Attachment #577266 - Flags: review?(jgilbert) → review+
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.
Attachment #577267 - Flags: review?(jgilbert) → review-
> > +    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.
(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 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.
Attachment #577268 - Flags: review?(jgilbert) → review-
Attachment #577269 - Flags: review?(jgilbert) → review+
Attachment #577270 - Flags: review?(jgilbert) → review+
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 on attachment 577272 [details] [diff] [review]
8: move VertexAttribData down

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

Not even enough to bikeshed. :P
Attachment #577272 - Flags: review?(jgilbert) → review+
Attachment #577276 - Flags: review?(jgilbert) → review+
Created attachment 578379 [details] [diff] [review]
3: rename mName to mGLName
Attachment #577267 - Attachment is obsolete: true
Attachment #578379 - Flags: review?(jgilbert)
Created attachment 578380 [details] [diff] [review]
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.
Attachment #577268 - Attachment is obsolete: true
Attachment #578380 - Flags: review?(jgilbert)
(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 on attachment 577271 [details] [diff] [review]
7: fix framebuffer attachment deletion

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

r+ after discussion.
Attachment #577271 - Flags: review?(jgilbert) → review+
Attachment #578379 - Flags: review?(jgilbert) → review+
Attachment #578380 - Flags: review?(jgilbert) → review+
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
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
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

Updated

4 years ago
Depends on: 822114
You need to log in before you can comment on or make changes to this bug.