Closed
Bug 1002281
Opened 10 years ago
Closed 10 years ago
WebGL2 - Refactor common functions into WebGLBindableName.
Categories
(Core :: Graphics: CanvasWebGL, defect)
Core
Graphics: CanvasWebGL
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: u480271, Assigned: u480271)
References
Details
Attachments
(3 files, 5 obsolete files)
28.75 KB,
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
16.31 KB,
patch
|
Details | Diff | Splinter Review | |
3.47 KB,
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
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.
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 5•10 years ago
|
||
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.
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 10•10 years ago
|
||
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 11•10 years ago
|
||
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-
Assignee | ||
Comment 12•10 years ago
|
||
(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
Assignee | ||
Comment 13•10 years ago
|
||
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.
Assignee | ||
Comment 14•10 years ago
|
||
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)
Assignee | ||
Comment 15•10 years ago
|
||
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
Updated•10 years ago
|
Keywords: dev-doc-needed
Comment 16•10 years ago
|
||
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+
Assignee | ||
Comment 17•10 years ago
|
||
(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)
Comment 18•10 years ago
|
||
(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.
Assignee | ||
Comment 19•10 years ago
|
||
(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.
Assignee | ||
Comment 20•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/baa68b41e879
Keywords: leave-open
Assignee | ||
Comment 22•10 years ago
|
||
(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
Comment 23•10 years ago
|
||
(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)
Assignee | ||
Comment 24•10 years ago
|
||
Attachment #8521930 -
Flags: review?(jgilbert)
Assignee | ||
Comment 25•10 years ago
|
||
/r/569 - Bug 1002281 - Change WebGLBindableName to make mGLName const. Pull down this commit: hg pull review -r 86d1aeaf40cf3f18ad8f9338d62bc423072019cd
Comment 26•10 years ago
|
||
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.
Assignee | ||
Comment 27•10 years ago
|
||
Split the two functions into two classes. WebGLRenderBuffer and WebGLVertexArray are special cases.
Attachment #8522640 -
Flags: review?(jgilbert)
Attachment #8522640 -
Flags: review?(jgilbert)
Assignee | ||
Comment 28•10 years ago
|
||
(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?
Assignee | ||
Comment 29•10 years ago
|
||
The indentation error was fixed in previous base patch I uploaded to the bug.
Attachment #8522660 -
Flags: review?(jgilbert)
Updated•10 years ago
|
Attachment #8522660 -
Flags: review?(jgilbert) → review+
Comment 30•10 years ago
|
||
Comment on attachment 8521930 [details]
MozReview Request: bz://1002281/kamidphish
This is r+ with the fixup patch.
Attachment #8521930 -
Flags: review?(jgilbert) → review+
Comment 31•10 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/dbaf57dec1a3 because b2g is why we can't have nice things.
Comment 32•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f5f0ee65aa5b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Updated•9 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 33•9 years ago
|
||
Attachment #8521930 -
Attachment is obsolete: true
Attachment #8618150 -
Flags: review+
Assignee | ||
Comment 34•9 years ago
|
||
Attachment #8618150 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•