Closed Bug 1002281 Opened 10 years ago Closed 10 years ago

WebGL2 - Refactor common functions into WebGLBindableName.

Categories

(Core :: Graphics: CanvasWebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: u480271, Assigned: u480271)

References

Details

Attachments

(3 files, 5 obsolete files)

WebGLBindableName represents a GL 'name' (GLuint) that can be bound to
part of the GL state machine. Similar code appeared in a number of
classes that represent GL bindable names, such as WebGLBuffer,
WebGLTexture, WebGLFramebuffer, etc.

Cleanup to reduce copy-n-paste code that's needed for creating new
objects for WebGL 2.
Blocks: webgl2
Extract common code into WebGLBindableName class.
Attachment #8413468 - Flags: review?(jgilbert)
Attachment #8413468 - Flags: review?(bjacob)
WebGLBindableName represents a GL 'name' (GLuint) that can be bound to
part of the GL state machine. Similar code appeared in a number of
classes that represent GL bindable names, such as WebGLBuffer,
WebGLTexture, WebGLFramebuffer, etc.

Cleanup to reduce copy-n-paste code that's needed for creating new
objects for WebGL 2.
Attachment #8416366 - Flags: review?(jgilbert)
Attachment #8413468 - Attachment is obsolete: true
Attachment #8413468 - Flags: review?(jgilbert)
Attachment #8413468 - Flags: review?(bjacob)
Attachment #8416366 - Flags: review?(bjacob)
(In reply to Dan Glastonbury :djg :kamidphish from comment #2)
> Created attachment 8416366 [details] [diff] [review]
> WebGL2 - Refactor common functions into WebGLBindableName.
> 
> WebGLBindableName represents a GL 'name' (GLuint) that can be bound to
> part of the GL state machine. Similar code appeared in a number of
> classes that represent GL bindable names, such as WebGLBuffer,
> WebGLTexture, WebGLFramebuffer, etc.
> 
> Cleanup to reduce copy-n-paste code that's needed for creating new
> objects for WebGL 2.

Refresh patch and fix non-unified builds.
Comment on attachment 8416366 [details] [diff] [review]
WebGL2 - Refactor common functions into WebGLBindableName.

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

::: content/canvas/src/WebGLBindableName.h
@@ +18,5 @@
> +public:
> +    WebGLBindableName() : mGLName(0), mTarget(0) {}
> +    void BindTo(GLenum target) { mTarget = target; DoBind(); }
> +
> +    bool HasEverBeenBound() const { return mTarget != 0; }

This suggests that once bound to a non-zero target, can't be bound back to the 0 target. That should, then, be MOZ_ASSERTed in BindTo.

Also, shouldn't BindTo also assert that once bound to a target, one doesn't ever try to bind to any other target? That should be validated against before calling this internal code, as it probably requires generating an INVALID_OPERATION.

@@ +24,5 @@
> +    GLenum Target() const { return mTarget; }
> +
> +protected:
> +
> +    virtual void DoBind() {}

The names, BindTo and DoBind, are misleading as reading them I took it for granted that DoBind would call glBindFoo(), which it doesn't. I think that needs to be addressed somehow (possibly just by renaming) before errplussing.

::: content/canvas/src/WebGLContextBuffers.cpp
@@ +31,5 @@
>      }
>  
>      if (buffer) {
> +        if (!buffer->HasEverBeenBound())
> +            buffer->BindTo(target);

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures

"Always brace controlled statements, even a single-line consequent of an if else else. This is redundant typically, but it avoids dangling else bugs, so it's safer at scale than fine-tuning."

::: content/canvas/src/WebGLRenderbuffer.h
@@ +32,5 @@
>  
>      void Delete();
>  
> +    bool HasEverBeenBound() const { return mTarget != LOCAL_GL_NONE; }
> +    void BindTo(GLenum target) { mTarget = target; }

Why is WebGLRenderbuffer not inheriting from WebGLBindableName ?
Attachment #8416366 - Flags: review?(bjacob) → review-
(In reply to Benoit Jacob [:bjacob] from comment #5)
> This suggests that once bound to a non-zero target, can't be bound back to
> the 0 target. That should, then, be MOZ_ASSERTed in BindTo.
> 
> Also, shouldn't BindTo also assert that once bound to a target, one doesn't
> ever try to bind to any other target? That should be validated against
> before calling this internal code, as it probably requires generating an
> INVALID_OPERATION.

Bindable name is a formulation of a pattern that already existed in the code base. Visual inspection of the caller sites shows this happening, but I agree that it can't hurt to have the extra protection and will add the requested MOZ_ASSERTs.

> 
> @@ +24,5 @@
> > +    GLenum Target() const { return mTarget; }
> > +
> > +protected:
> > +
> > +    virtual void DoBind() {}
> 
> The names, BindTo and DoBind, are misleading as reading them I took it for
> granted that DoBind would call glBindFoo(), which it doesn't. I think that
> needs to be addressed somehow (possibly just by renaming) before errplussing.

how about 'OnBindingChange()' or 'OnBind()' - This virtual is an event handling point so sub-classes can perform some action on the event of binding.

> ::: content/canvas/src/WebGLRenderbuffer.h
> @@ +32,5 @@
> >  
> >      void Delete();
> >  
> > +    bool HasEverBeenBound() const { return mTarget != LOCAL_GL_NONE; }
> > +    void BindTo(GLenum target) { mTarget = target; }
> 
> Why is WebGLRenderbuffer not inheriting from WebGLBindableName ?

I don't remember, exactly. There was a reason at the time I wrote the patch. I'll revisit.
Attached patch Address bjacob review comments. (obsolete) — Splinter Review
Attachment #8417181 - Flags: review?(bjacob)
Comment on attachment 8417181 [details] [diff] [review]
Address bjacob review comments.

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

::: content/canvas/src/WebGLFramebuffer.cpp
@@ -66,5 @@
>  {
>      const WebGLTexture* tex = Texture();
>      if (tex) {
>          if (tex->HasImageInfoAt(mTexImageTarget, mTexImageLevel)) {
> -            GLenum type = tex->ImageInfoAt(mTexImageTarget, mTexImageLevel).TyWebGLpe();

This appears to be from a rebasing error for previous patch onto central. When I land, I'll flatten this and previous patch into one.
With fixes for bjacob: https://tbpl.mozilla.org/?tree=Try&rev=306bd800a6f0
Comment on attachment 8417181 [details] [diff] [review]
Address bjacob review comments.

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

::: content/canvas/src/WebGLBindableName.cpp
@@ +19,5 @@
> +{
> +    MOZ_ASSERT(target != LOCAL_GL_NONE, "Can't bind to GL_NONE.");
> +    MOZ_ASSERT(mTarget == LOCAL_GL_NONE || mTarget == target, "Rebinding is illegal.");
> +    mTarget = target;
> +    OnBindingChange();

Is it intentional that OnBindingChange() is called even if mTarget == target? Doesn't the name, OnBindingChange, suggest that we already know there is a change happening?
Attachment #8417181 - Flags: review?(bjacob) → review+
Comment on attachment 8416366 [details] [diff] [review]
WebGL2 - Refactor common functions into WebGLBindableName.

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

::: content/canvas/src/WebGLBindableName.h
@@ +24,5 @@
> +    GLenum Target() const { return mTarget; }
> +
> +protected:
> +
> +    virtual void DoBind() {}

Perhaps OnTargetChange?

::: content/canvas/src/WebGLContextBuffers.cpp
@@ +38,1 @@
>              return ErrorInvalidOperation("bindBuffer: buffer already bound to a different target");

This ordering is wrong. If we generate an error, we can't have changed anything. It shouldn't be possible to fall-through the previous if to this one. (We can't do `buffer->BindTo(target)` and then generate INVALID_OP)

::: content/canvas/src/WebGLTexture.cpp
@@ +139,5 @@
>  WebGLTexture::SetImageInfo(GLenum aTarget, GLint aLevel,
>                    GLsizei aWidth, GLsizei aHeight,
>                    GLenum aFormat, GLenum aType, WebGLImageDataStatus aStatus)
>  {
> +    // TODO(djg): This expression is trying to express more than it's saying.

Huh?
Attachment #8416366 - Flags: review?(jgilbert) → review-
(In reply to Jeff Gilbert [:jgilbert] from comment #11)
> Comment on attachment 8416366 [details] [diff] [review]
> WebGL2 - Refactor common functions into WebGLBindableName.
> 
> Review of attachment 8416366 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLBindableName.h
> @@ +24,5 @@
> > +    GLenum Target() const { return mTarget; }
> > +
> > +protected:
> > +
> > +    virtual void DoBind() {}
> 
> Perhaps OnTargetChange?

Will take OnTargetChanged, it's called -after- mTarget changes.

> ::: content/canvas/src/WebGLContextBuffers.cpp
> @@ +38,1 @@
> >              return ErrorInvalidOperation("bindBuffer: buffer already bound to a different target");
> 
> This ordering is wrong. If we generate an error, we can't have changed
> anything. It shouldn't be possible to fall-through the previous if to this
> one. (We can't do `buffer->BindTo(target)` and then generate INVALID_OP)

I ran through the logic in my head and I don't think it is wrong, but I'll put the else back in to return it to the same structure as before.
 
> ::: content/canvas/src/WebGLTexture.cpp
> @@ +139,5 @@
> >  WebGLTexture::SetImageInfo(GLenum aTarget, GLint aLevel,
> >                    GLsizei aWidth, GLsizei aHeight,
> >                    GLenum aFormat, GLenum aType, WebGLImageDataStatus aStatus)
> >  {
> > +    // TODO(djg): This expression is trying to express more than it's saying.
> 
> Huh?

Will expand the comment to clarify. I was confused by what the logic in the ASSERT was trying to encode.
Attachment #8416366 - Attachment is obsolete: true
Attachment #8417181 - Attachment is obsolete: true
WebGLBindableName represents a GL 'name' (GLuint) that can be bound to
part of the GL state machine. Similar code appeared in a number of
classes that represent GL bindable names, such as WebGLBuffer,
WebGLTexture, WebGLFramebuffer, etc.

Cleanup to reduce copy-n-paste code that's needed for creating new
objects for WebGL 2.
Comment on attachment 8467475 [details] [diff] [review]
WebGL2 - Refactor common functions into WebGLBindableName.

Refreshed patch including:
* address bjacob's comments,
* address jgilbert's comments,
* rebase upon move of WebGL to dom/canvas from content/canvas/src.
Attachment #8467475 - Flags: review?(jgilbert)
remote: You can view the progress of your build at the following URL:
remote:   https://tbpl.mozilla.org/?tree=Try&rev=fc0bcc16e01e
remote: Alternatively, view them on Treeherder (experimental):
remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fc0bcc16e01e
Comment on attachment 8467475 [details] [diff] [review]
WebGL2 - Refactor common functions into WebGLBindableName.

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

::: dom/canvas/WebGLBindableName.cpp
@@ +22,5 @@
> +
> +    bool targetChanged = (target != mTarget);
> +    mTarget = target;
> +    if (targetChanged)
> +        OnTargetChanged();

Is it not useful to know what the target changed from? (pass the old target to this func?)
There's no such case in this patch, so I guess we'll punt because of YAGNI for now.

::: dom/canvas/WebGLBindableName.h
@@ +7,5 @@
> +#define WEBGLBINDABLENAME_H_
> +
> +#include "WebGLTypes.h"
> +
> +namespace mozilla

Our namespaces generally start with:
namespace foo {

And end with:
} // namespace foo

...since they're often so far apart.

@@ +27,5 @@
> +
> +    //! Called after mTarget has been changed by BindTo(target).
> +    virtual void OnTargetChanged() {}
> +
> +    GLuint mGLName;

Surely the ctor should take a `GLuint name` and this should be `const GLuint mGLName`?

I suppose this would basically require us to use static constructor helpers for all these classes. We should consider this later, but it's fine for now.

::: dom/canvas/WebGLTexture.cpp
@@ +143,5 @@
>  {
> +    // TODO(djg): I suspected the following ASSERT and check are
> +    //            trying to express more than they're saying, probably
> +    //            to do with cubemap targets. We should do this
> +    //            properly. https://bugzilla.mozilla.org/show_bug.cgi?id=1006908

This is correct.
Attachment #8467475 - Flags: review?(jgilbert) → review+
(In reply to Jeff Gilbert [:jgilbert] from comment #16)
> There's no such case in this patch, so I guess we'll punt because of YAGNI
> for now.

That's what I was going for. Since I've had patches rejected for have unused params that may have been useful in the future.

> Our namespaces generally start with:
> namespace foo {
> 
> And end with:
> } // namespace foo
> 
> ...since they're often so far apart.

Done.

> > +    //! Called after mTarget has been changed by BindTo(target).
> > +    virtual void OnTargetChanged() {}
> > +
> > +    GLuint mGLName;
> 
> Surely the ctor should take a `GLuint name` and this should be `const GLuint
> mGLName`?

Nope. GL objects start in an unbound state. Once there bound to a target the binding is set. (According to my reading of the GL specs)
(In reply to Dan Glastonbury :djg :kamidphish from comment #17)
> > > +    //! Called after mTarget has been changed by BindTo(target).
> > > +    virtual void OnTargetChanged() {}
> > > +
> > > +    GLuint mGLName;
> > 
> > Surely the ctor should take a `GLuint name` and this should be `const GLuint
> > mGLName`?
> 
> Nope. GL objects start in an unbound state. Once there bound to a target the
> binding is set. (According to my reading of the GL specs)

Sure, but I'm asking about mGLName, not mTarget. mTarget is unbound directly after glGenTextures (for instance), but mGLName is already set, and never changes.
(In reply to Jeff Gilbert [:jgilbert] from comment #18)
> Sure, but I'm asking about mGLName, not mTarget. mTarget is unbound directly
> after glGenTextures (for instance), but mGLName is already set, and never
> changes.

Oh I misread. Follow up patch.
Keywords: leave-open
(In reply to Dan Glastonbury :djg :kamidphish from comment #19)
> (In reply to Jeff Gilbert [:jgilbert] from comment #18)
> > Sure, but I'm asking about mGLName, not mTarget. mTarget is unbound directly
> > after glGenTextures (for instance), but mGLName is already set, and never
> > changes.
> 
> Oh I misread. Follow up patch.

Unless there's some way to do what you asked, mGLName is set in the body of the constructor and isn't available to WebGLBindableName() constructor. Eg:

WebGL2Sampler::WebGL2Sampler(WebGLContext* context)
    : WebGLBindableName()
    , WebGLContextBoundObject(context)
{
    SetIsDOMBinding();
    mContext->MakeContextCurrent();
    mContext->gl->fGenSamplers(1, &mGLName);
    mContext->mSamplers.insertBack(this);
}
Flags: needinfo?(jgilbert)
Keywords: leave-open
(In reply to Dan Glastonbury :djg :kamidphish from comment #22)
> (In reply to Dan Glastonbury :djg :kamidphish from comment #19)
> > (In reply to Jeff Gilbert [:jgilbert] from comment #18)
> > > Sure, but I'm asking about mGLName, not mTarget. mTarget is unbound directly
> > > after glGenTextures (for instance), but mGLName is already set, and never
> > > changes.
> > 
> > Oh I misread. Follow up patch.
> 
> Unless there's some way to do what you asked, mGLName is set in the body of
> the constructor and isn't available to WebGLBindableName() constructor. Eg:
> 
> WebGL2Sampler::WebGL2Sampler(WebGLContext* context)
>     : WebGLBindableName()
>     , WebGLContextBoundObject(context)
> {
>     SetIsDOMBinding();
>     mContext->MakeContextCurrent();
>     mContext->gl->fGenSamplers(1, &mGLName);
>     mContext->mSamplers.insertBack(this);
> }

We can just make the constructor here take a GLuint name, and fGenSamplers the name in WebGLContext::CreateSampler.

I think it would be cleaner this way, but I don't know as we should go out of our way to fix this right now.
Flags: needinfo?(jgilbert)
Attachment #8521930 - Flags: review?(jgilbert)
/r/569 - Bug 1002281 - Change WebGLBindableName to make mGLName const.

Pull down this commit:

hg pull review -r 86d1aeaf40cf3f18ad8f9338d62bc423072019cd
https://reviewboard.mozilla.org/r/569/#review261

::: dom/canvas/WebGLContextGL.cpp
(Diff revision 1)
> +    if (!gl->IsExtensionSupported(GLContext::EXT_packed_depth_stencil) &&
> +        !gl->IsExtensionSupported(GLContext::OES_packed_depth_stencil)) {
> +        gl->fGenRenderbuffers(1, &secondaryRB);
> +    }

I would actually let the ctor handle this, or just always allocate one. This is too much API leakage, IMO.

::: dom/canvas/WebGLFramebuffer.h
(Diff revision 1)
> -    explicit WebGLFramebuffer(WebGLContext* context);
> +        explicit WebGLFramebuffer(WebGLContext* context, GLuint fbo);

indentation

::: dom/canvas/WebGLBindableName.h
(Diff revision 1)
> +    WebGLBindableName(GLuint name)
> +        : WebGLBindable<T>()
> +        , mGLName(name)
> +    { }

Assert that `name` is non-zero.

::: dom/canvas/WebGLBuffer.h
(Diff revision 1)
> -    explicit WebGLBuffer(WebGLContext* aContext);
> +    explicit WebGLBuffer(WebGLContext* context, GLuint buf);

I should just remove all the aFoo warts from our code.
Split the two functions into two classes. WebGLRenderBuffer and
WebGLVertexArray are special cases.
Attachment #8522640 - Flags: review?(jgilbert)
Attachment #8522640 - Flags: review?(jgilbert)
(In reply to Jeff Gilbert [:jgilbert] from comment #26)

> ::: dom/canvas/WebGLContextGL.cpp
> I would actually let the ctor handle this, or just always allocate one. This
> is too much API leakage, IMO.

OK. 

> ::: dom/canvas/WebGLFramebuffer.h
> (Diff revision 1)
> > -    explicit WebGLFramebuffer(WebGLContext* context);
> > +        explicit WebGLFramebuffer(WebGLContext* context, GLuint fbo);
> 
> indentation

Fixed.

> ::: dom/canvas/WebGLBindableName.h
> Assert that `name` is non-zero.

We can't because in a later patch we have a default transform feedback buffer (and vertex array object) that are bound to name of 0.
 
> ::: dom/canvas/WebGLBuffer.h
> I should just remove all the aFoo warts from our code.

I agree. Is "our code" just WebGL?
The indentation error was fixed in previous base patch I uploaded to the bug.
Attachment #8522660 - Flags: review?(jgilbert)
Attachment #8522660 - Flags: review?(jgilbert) → review+
Comment on attachment 8521930 [details]
MozReview Request: bz://1002281/kamidphish

This is r+ with the fixup patch.
Attachment #8521930 - Flags: review?(jgilbert) → review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/dbaf57dec1a3 because b2g is why we can't have nice things.
https://hg.mozilla.org/mozilla-central/rev/f5f0ee65aa5b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Attachment #8521930 - Attachment is obsolete: true
Attachment #8618150 - Flags: review+
Attachment #8618150 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.