Implement functions and types to support WebGLSampler object.

Query GL functions for Samplers.

Comment on attachment 8493656 [details] [diff] [review] WebGL2 - GL symbols for Samplers. Review of attachment 8493656 [details] [diff] [review]: ----------------------------------------------------------------- I'll just grab this review, since it's simple.

/r/457 - Bug 1048720 - [WebGL2] Implement WebGL2Sampler. r=jgiblert /r/459 - Bug 1048741 - [WebGL2] Partner code doesn't specify all mipmaps for compressed textures allocated via glTexStorage. r=jgilbert /r/461 - Bug 1097413 - Symbolic constants kGLESVersion2 and kGLESVersion3. r=jgilbert /r/463 - Bug 1097411 - Extend error reporting. r=jgilbert /r/465 - Bug 1097416 - [WebGL2] enabled if all GLES 3 level features are available. r=jgilbert /r/467 - Bug 1048745 - [WebGL2] Extend WebGLUniformInfo with GLES 3 types. r=jgilbert /r/469 - Bug 1048743 - [WebGL2] No support for binary shaders. r=jgilbert /r/471 - Bug 1048747 - [WebGL2] Query active uniform blocks. r=jgilbert /r/473 - Bug 1048743 - [WebGL2] Split shader compilation into ANGLE and Bypass options. r=jgilbert /r/475 - Bug 1048724 - [WebGL2] Transform Feedback. r=jgilbert /r/477 - Bug 1048719 - [WebGL2] Query objects. r=jgilbert /r/479 - Bug 1048745 - [WebGL2] Integer vertex attributes. r=jgilbert /r/481 - Bug 1048747 - [WebGL2] Implement uniform block/buffer. r=jgilbert /r/483 - Bug 1048747 - [WebGL2] Implement GetTexParameter for all valid WebGL2 pnames. r=jgilbert /r/485 - Bug 1048731 - [WebGL2] Implement new buffer binding targets. r=jgilbert /r/487 - Bug 1048745 - [WebGL2] Extend UniformInfo with WebGL2/GLES3 types. r=jgilbert /r/489 - Bug 1048741 - [WebGL2] texParameter: TEXTURE_COMPARE_MODE and TEXTURE_COMPARE_FUNC support. r=jgilbert Pull down these commits: hg pull review -r 417423811da0284fd6f8866d40cc04c636905c55

https://reviewboard.mozilla.org/r/457/#review211 ::: dom/canvas/WebGL2ContextSamplers.cpp (Diff revision 1) > + // TODO(djg): If active handle unbinding NS_WARNING, maybe? ::: dom/canvas/WebGLContextUnchecked.h (Diff revision 1) > +namespace gl { class GLContext; } Use newlines. ::: dom/canvas/WebGLContextUnchecked.h (Diff revision 1) > + explicit WebGLContextUnchecked(gl::GLContext* gl); I think this is a good move. Right now, we keep the same WebGLContext around even though `gl` might change due to context loss. It would be easier to make sure context loss recovery works well to have `gl` elsewhere. (as you have here) ::: dom/canvas/WebGLContextUnchecked.cpp (Diff revision 1) > +WebGLContextUnchecked::BindSampler(WebGLSampler* sampler, GLuint unit) It seems like the order should be (unit, sampler), since that's what GL has. ::: dom/canvas/WebGLContextValidate.cpp (Diff revision 1) > + pname == LOCAL_GL_TEXTURE_COMPARE_FUNC); Use a switch. ::: dom/canvas/WebGLContextUnchecked.cpp (Diff revision 1) > +#include "GLContext.h" Alphabetize includes other than the include for the parent header file. ::: dom/canvas/WebGL2ContextSamplers.cpp (Diff revision 1) > + GLint value; > + WebGLContextUnchecked::GetSamplerParameteriv(sampler, pname, &value); If GetSamplerParameteriv is infallible, then have it return GLint instead of having an out-param. If it's not infallible, we need to initialize value to something reasonable. ::: dom/canvas/WebGL2ContextSamplers.cpp (Diff revision 1) > + return (ValidateObjectAllowDeleted("isSampler", sampler) && > + !sampler->IsDeleted() && > + sampler->HasEverBeenBound()); I would rather you do this in parts, rather than as a big concatenanted line. ::: dom/canvas/WebGLContext.h (Diff revision 1) > +class WebGLIntOrFloat { > + enum { > + Int, > + Float > + } mType; > + union { > + GLint i; > + GLfloat f; > + } mValue; > + > +public: > + explicit WebGLIntOrFloat(GLint i) : mType(Int) { mValue.i = i; } > + explicit WebGLIntOrFloat(GLfloat f) : mType(Float) { mValue.f = f; } > + > + GLint AsInt() const { return (mType == Int) ? mValue.i : NS_lroundf(mValue.f); } > + GLfloat AsFloat() const { return (mType == Float) ? mValue.f : GLfloat(mValue.i); } > +}; > + This is nearly dangerously clever. ::: dom/canvas/WebGLContext.h (Diff revision 1) > + // TODO(djg): Does this need a rethink? Should it be WebGL2Context? > + LinkedList<WebGLSampler> mSamplers; I think it probably should. ::: dom/canvas/WebGLContextValidate.cpp (Diff revision 1) > + bool valid = (pname == LOCAL_GL_TEXTURE_MIN_FILTER || > + pname == LOCAL_GL_TEXTURE_MAG_FILTER || > + pname == LOCAL_GL_TEXTURE_WRAP_S || > + pname == LOCAL_GL_TEXTURE_WRAP_T || > + pname == LOCAL_GL_TEXTURE_WRAP_R || > + pname == LOCAL_GL_TEXTURE_MIN_LOD || > + pname == LOCAL_GL_TEXTURE_MAX_LOD || > + pname == LOCAL_GL_TEXTURE_COMPARE_MODE || > + pname == LOCAL_GL_TEXTURE_COMPARE_FUNC); > + if (!valid) > + ErrorInvalidEnum("%s: invalid pname.", info); Use a switch. ::: dom/canvas/WebGLContextValidate.cpp (Diff revision 1) > + bool valid = (p == LOCAL_GL_NEAREST || > + p == LOCAL_GL_LINEAR || > + p == LOCAL_GL_NEAREST_MIPMAP_NEAREST || > + p == LOCAL_GL_NEAREST_MIPMAP_LINEAR || > + p == LOCAL_GL_LINEAR_MIPMAP_NEAREST || > + p == LOCAL_GL_LINEAR_MIPMAP_LINEAR); > + if (!valid) > + ErrorInvalidEnum("%s: invalid param", info); Use a switch. ::: dom/canvas/WebGLContextValidate.cpp (Diff revision 1) > + bool valid = (p == LOCAL_GL_LEQUAL || > + p == LOCAL_GL_GEQUAL || > + p == LOCAL_GL_LESS || > + p == LOCAL_GL_GREATER || > + p == LOCAL_GL_EQUAL || > + p == LOCAL_GL_NOTEQUAL || > + p == LOCAL_GL_ALWAYS || > + p == LOCAL_GL_NEVER); Use a switch. ::: dom/canvas/WebGLContextValidate.cpp (Diff revision 1) > + } I think the format for switches should be like: switch (foo) { case bar: { int x; break; } } Otherwise we can get: switch (foo) { case bar: { int x; break; } } ::: dom/canvas/WebGLSampler.cpp (Diff revision 1) > - MOZ_CRASH("Not Implemented."); > + mContext->MakeContextCurrent(); > + mContext->gl->fGenSamplers(1, &mGLName); Really MakeCurrent and GenSamplers should be pulled out into a helper function, so that mGLName can be const.

https://reviewboard.mozilla.org/r/457/#review213 > NS_WARNING, maybe? I checked and we don't track bound samplers at the moment. I think I can remove the comment. > If GetSamplerParameteriv is infallible, then have it return GLint instead of having an out-param. If it's not infallible, we need to initialize value to something reasonable. GetSamplerParameteriv is a very thin wrapper over GL/GL ES. I think it would be appropriate to have it return value. > This is nearly dangerously clever. It's not clever, but dodgy discriminated union in C++. Rust/ML enum would be so much nicer. Also, the GL spec is a bit fuzzy around these conversions. I found some language about it in Sec. 2.3.1, "Data Conversion For State-Setting Commands", es_spec_3.0.4.pdf, pg 14 > I think it probably should. The issue then becomes how to order calls that clean up mSamplers, eg. WebGLContext::DestroyResourcesAndContext(). I've been erring on adding tracking like this into WebGLContext to make it a little easier and I would like to leave it here. > Really MakeCurrent and GenSamplers should be pulled out into a helper function, so that mGLName can be const. I've implemented this in another patch. You made the same comment previous on Bug 1002281.

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a93db41f97cc

What's all this stuff it thinks I requested?!