Add query objects (GL_ARB_occlusion_query). See https://wiki.mozilla.org/Platform/GFX/WebGL2

Implement only ANY_SAMPLES_PASSED, because as said in the OpenGL ES 3.0 specifications : "If the target of the query is ANY_SAMPLES_PASSED_CONSERVATIVE, an implementa- tion may choose to use a less precise version of the test which can additionally set the samples-boolean state to TRUE in some other implementation-dependent cases."

Comment on attachment 777911 [details] [diff] [review] patch revision 2 Review of attachment 777911 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/WebGLContextAsyncQueries.cpp @@ +34,5 @@ > +{ > + if (!IsContextStable()) > + return; > + > + if (query == nullptr) Style is `!query`. @@ +53,5 @@ > +{ > + if (!IsContextStable()) > + return; > + > + if (target != LOCAL_GL_ANY_SAMPLES_PASSED) { Should support _CONSERVATIVE as well. @@ +58,5 @@ > + ErrorInvalidEnum("beginQuery: target must be ANY_SAMPLES_PASSED"); > + return; > + } > + > + if (query == nullptr) { `!query` @@ +92,5 @@ > + ErrorInvalidOperation("beginQuery: target doesn't match with the query type"); > + return; > + } > + > + if (mActiveOcclusionQuery != nullptr) { Generally we should do just `(foo)` instead of `(foo != nullptr)`. @@ +105,5 @@ > + > + if (gl->IsGLES2()) { > + gl->fBeginQuery(LOCAL_GL_ANY_SAMPLES_PASSED, query->mGLName); > + } > + else { `} else {` @@ +106,5 @@ > + if (gl->IsGLES2()) { > + gl->fBeginQuery(LOCAL_GL_ANY_SAMPLES_PASSED, query->mGLName); > + } > + else { > + gl->fBeginQuery(LOCAL_GL_SAMPLES_PASSED, query->mGLName); We should only do this if we don't have ARB_occlusion_query2. @@ +149,5 @@ > + > + if (gl->IsGLES2()) { > + gl->fEndQuery(LOCAL_GL_ANY_SAMPLES_PASSED); > + } > + else { `} else {` @@ +189,5 @@ > + ErrorInvalidEnum("getQuery: pname must be CURRENT_QUERY"); > + return nullptr; > + } > + > + nsRefPtr<WebGLQuery> tmp = mActiveOcclusionQuery.get(); `.get()` shouldn't be necessary, I think. @@ +246,5 @@ > + gl->fGetQueryObjectuiv(query->mGLName, LOCAL_GL_QUERY_RESULT, &returned); > + > + /* > + * test (returned != 0) is important because desktop drivers return the number of > + * sampler drawed when the OpenGL ES driver return only a boolean if a sampler has been s/sampler/samples/ I'll probably to a clean-up patch for comments at some point. This also isn't necessarily true, just that we emulate ANY_SAMPLES_PASSED with SAMPLES_PASSED when we don't have ARB_occlusion_query2 on desktop. ::: content/canvas/src/WebGLQuery.cpp @@ +33,5 @@ > +} > + > +bool WebGLQuery::IsActive() const > +{ > + return mType == LOCAL_GL_ANY_SAMPLES_PASSED && Why do we bother checking mType here? ::: dom/webidl/WebGL2RenderingContext.webidl @@ +52,5 @@ > const GLenum MIN = 0x8007; > const GLenum MAX = 0x8008; > > + /* query objects */ > + const GLenum ANY_SAMPLES_PASSED = 0x8C2F; Where is `ANY_SAMPLES_PASSED_CONSERVATIVE`? It looks like we'll need to just pass this through as `ANY_SAMPLES_PASSED` on desktop GL, but that's within spec. ::: gfx/gl/GLContext.cpp @@ +94,5 @@ > "GL_EXT_gpu_shader4", > "GL_EXT_blend_minmax", > "GL_ARB_draw_instanced", > "GL_EXT_draw_instanced", > + "GL_ARB_occlusion_query", Don't bother tracking this, since it went core in desktop GL 2.0. @@ +313,5 @@ > { (PRFuncPtr*) &mSymbols.fReadBuffer, { "ReadBuffer", NULL } }, > { (PRFuncPtr*) &mSymbols.fMapBuffer, { "MapBuffer", NULL } }, > { (PRFuncPtr*) &mSymbols.fUnmapBuffer, { "UnmapBuffer", NULL } }, > { (PRFuncPtr*) &mSymbols.fPointParameterf, { "PointParameterf", NULL } }, > + { (PRFuncPtr*) &mSymbols.fBeginQuery, { "BeginQuery", "BeginQueryARB", NULL } }, Since this was core, don't bother with the ARB decorations. @@ +633,5 @@ > > + if (IsGLES2() && > + IsExtensionSupported(EXT_occlusion_query_boolean)) { > + SymLoadStruct queryObjectsSymbols[] = { > + { (PRFuncPtr*) &mSymbols.fBeginQuery, { "BeginQueryEXT", NULL } }, s/NULL/nullptr/ ::: gfx/gl/GLDefs.h @@ +1181,5 @@ > #define LOCAL_GL_CLIENT_ACTIVE_TEXTURE_ARB 0x84E1 > #define LOCAL_GL_MAX_TEXTURE_UNITS_ARB 0x84E2 > #define LOCAL_GL_ARB_occlusion_query 1 > #define LOCAL_GL_QUERY_COUNTER_BITS_ARB 0x8864 > +#define LOCAL_GL_ANY_SAMPLES_PASSED 0x8C2F This wasn't added here, but rather in ARB_occlusion_query2, which looks like it went core in 3.2.

(In reply to Jeff Gilbert [:jgilbert] from comment #3) > Comment on attachment 777911 [details] [diff] [review] > patch revision 2 > > Review of attachment 777911 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/canvas/src/WebGLContextAsyncQueries.cpp > @@ +34,5 @@ > > +{ > > + if (!IsContextStable()) > > + return; > > + > > + if (query == nullptr) > > Style is `!query`. Ok. > > @@ +53,5 @@ > > +{ > > + if (!IsContextStable()) > > + return; > > + > > + if (target != LOCAL_GL_ANY_SAMPLES_PASSED) { > > Should support _CONSERVATIVE as well. I don't think so, because it is said that ANY_SAMPLES_PASSED_CONSERVATIVE can be platform dependent. [1] > @@ +58,5 @@ > > + ErrorInvalidEnum("beginQuery: target must be ANY_SAMPLES_PASSED"); > > + return; > > + } > > + > > + if (query == nullptr) { > > `!query` Ok. > > @@ +92,5 @@ > > + ErrorInvalidOperation("beginQuery: target doesn't match with the query type"); > > + return; > > + } > > + > > + if (mActiveOcclusionQuery != nullptr) { > > Generally we should do just `(foo)` instead of `(foo != nullptr)`. Ok. > > @@ +105,5 @@ > > + > > + if (gl->IsGLES2()) { > > + gl->fBeginQuery(LOCAL_GL_ANY_SAMPLES_PASSED, query->mGLName); > > + } > > + else { > > `} else {` Ok. > > @@ +106,5 @@ > > + if (gl->IsGLES2()) { > > + gl->fBeginQuery(LOCAL_GL_ANY_SAMPLES_PASSED, query->mGLName); > > + } > > + else { > > + gl->fBeginQuery(LOCAL_GL_SAMPLES_PASSED, query->mGLName); > > We should only do this if we don't have ARB_occlusion_query2. It's true, but I don't have ARB_occlusion_query2 on my laptop to test. > > @@ +149,5 @@ > > + > > + if (gl->IsGLES2()) { > > + gl->fEndQuery(LOCAL_GL_ANY_SAMPLES_PASSED); > > + } > > + else { > > `} else {` Ok. > > @@ +189,5 @@ > > + ErrorInvalidEnum("getQuery: pname must be CURRENT_QUERY"); > > + return nullptr; > > + } > > + > > + nsRefPtr<WebGLQuery> tmp = mActiveOcclusionQuery.get(); > > `.get()` shouldn't be necessary, I think. I had a problem here, asked Joe and he gave me this solution that is working perfectly even I agree this dirty. > > @@ +246,5 @@ > > + gl->fGetQueryObjectuiv(query->mGLName, LOCAL_GL_QUERY_RESULT, &returned); > > + > > + /* > > + * test (returned != 0) is important because desktop drivers return the number of > > + * sampler drawed when the OpenGL ES driver return only a boolean if a sampler has been > > s/sampler/samples/ > I'll probably to a clean-up patch for comments at some point. > > This also isn't necessarily true, just that we emulate ANY_SAMPLES_PASSED > with SAMPLES_PASSED when we don't have ARB_occlusion_query2 on desktop. This is already emulating for WebGL where GLContext would not be able to manage ANY_SAMPLES_PASSED and SAMPLES_PASSED at the same time if we want to emulate this in on GLContext. > > ::: content/canvas/src/WebGLQuery.cpp > @@ +33,5 @@ > > +} > > + > > +bool WebGLQuery::IsActive() const > > +{ > > + return mType == LOCAL_GL_ANY_SAMPLES_PASSED && > > Why do we bother checking mType here? Because transform feedback will bring a new type of queries. > > ::: dom/webidl/WebGL2RenderingContext.webidl > @@ +52,5 @@ > > const GLenum MIN = 0x8007; > > const GLenum MAX = 0x8008; > > > > + /* query objects */ > > + const GLenum ANY_SAMPLES_PASSED = 0x8C2F; > > Where is `ANY_SAMPLES_PASSED_CONSERVATIVE`? > It looks like we'll need to just pass this through as `ANY_SAMPLES_PASSED` > on desktop GL, but that's within spec. See [1] > > ::: gfx/gl/GLContext.cpp > @@ +94,5 @@ > > "GL_EXT_gpu_shader4", > > "GL_EXT_blend_minmax", > > "GL_ARB_draw_instanced", > > "GL_EXT_draw_instanced", > > + "GL_ARB_occlusion_query", > > Don't bother tracking this, since it went core in desktop GL 2.0. It's true. > > @@ +313,5 @@ > > { (PRFuncPtr*) &mSymbols.fReadBuffer, { "ReadBuffer", NULL } }, > > { (PRFuncPtr*) &mSymbols.fMapBuffer, { "MapBuffer", NULL } }, > > { (PRFuncPtr*) &mSymbols.fUnmapBuffer, { "UnmapBuffer", NULL } }, > > { (PRFuncPtr*) &mSymbols.fPointParameterf, { "PointParameterf", NULL } }, > > + { (PRFuncPtr*) &mSymbols.fBeginQuery, { "BeginQuery", "BeginQueryARB", NULL } }, > > Since this was core, don't bother with the ARB decorations. It's true. > > @@ +633,5 @@ > > > > + if (IsGLES2() && > > + IsExtensionSupported(EXT_occlusion_query_boolean)) { > > + SymLoadStruct queryObjectsSymbols[] = { > > + { (PRFuncPtr*) &mSymbols.fBeginQuery, { "BeginQueryEXT", NULL } }, > > s/NULL/nullptr/ Oups ... > > ::: gfx/gl/GLDefs.h > @@ +1181,5 @@ > > #define LOCAL_GL_CLIENT_ACTIVE_TEXTURE_ARB 0x84E1 > > #define LOCAL_GL_MAX_TEXTURE_UNITS_ARB 0x84E2 > > #define LOCAL_GL_ARB_occlusion_query 1 > > #define LOCAL_GL_QUERY_COUNTER_BITS_ARB 0x8864 > > +#define LOCAL_GL_ANY_SAMPLES_PASSED 0x8C2F > > This wasn't added here, but rather in ARB_occlusion_query2, which looks like > it went core in 3.2. Yes.

(In reply to Guillaume Abadie from comment #4) > (In reply to Jeff Gilbert [:jgilbert] from comment #3) > > > + if (target != LOCAL_GL_ANY_SAMPLES_PASSED) { > > > > Should support _CONSERVATIVE as well. > I don't think so, because it is said that ANY_SAMPLES_PASSED_CONSERVATIVE > can be platform dependent. [1] AFAIK, the result of _CONSERVATIVE can be platform dependent (e.g. some impls might return true, but others might not), but I'm pretty sure that it's always valid to do a _CONSERVATIVE query. It's just never allowed to be FALSE when ANY_SAMPLES_PASSED is TRUE, but it might be TRUE when the other is FALSE.

(In reply to Guillaume Abadie from comment #4) > (In reply to Jeff Gilbert [:jgilbert] from comment #3) > > @@ +53,5 @@ > > > +{ > > > + if (!IsContextStable()) > > > + return; > > > + > > > + if (target != LOCAL_GL_ANY_SAMPLES_PASSED) { > > > > Should support _CONSERVATIVE as well. > I don't think so, because it is said that ANY_SAMPLES_PASSED_CONSERVATIVE > can be platform dependent. [1] It's platform dependent within well-specified bounds. It just means that it can effectively use a bloom filter, where we can get false-positives, though false-negatives are still disallowed. An implementation without an optimization here will just do the same thing as ANY_SAMPLES_PASSED. This is purely an optimization opportunity. > > > > @@ +106,5 @@ > > > + if (gl->IsGLES2()) { > > > + gl->fBeginQuery(LOCAL_GL_ANY_SAMPLES_PASSED, query->mGLName); > > > + } > > > + else { > > > + gl->fBeginQuery(LOCAL_GL_SAMPLES_PASSED, query->mGLName); > > > > We should only do this if we don't have ARB_occlusion_query2. > It's true, but I don't have ARB_occlusion_query2 on my laptop to test. That's why we have specs. Just follow what the spec says. The overarching goal here is not just to get it working on your machine, but to implement these things in general. That means that these things need to be fully implemented, not just enough for your machine. > > > > @@ +189,5 @@ > > > + ErrorInvalidEnum("getQuery: pname must be CURRENT_QUERY"); > > > + return nullptr; > > > + } > > > + > > > + nsRefPtr<WebGLQuery> tmp = mActiveOcclusionQuery.get(); > > > > `.get()` shouldn't be necessary, I think. > I had a problem here, asked Joe and he gave me this solution that is working > perfectly even I agree this dirty. Alright. > > > > @@ +246,5 @@ > > > + gl->fGetQueryObjectuiv(query->mGLName, LOCAL_GL_QUERY_RESULT, &returned); > > > + > > > + /* > > > + * test (returned != 0) is important because desktop drivers return the number of > > > + * sampler drawed when the OpenGL ES driver return only a boolean if a sampler has been > > > > s/sampler/samples/ > > I'll probably to a clean-up patch for comments at some point. > > > > This also isn't necessarily true, just that we emulate ANY_SAMPLES_PASSED > > with SAMPLES_PASSED when we don't have ARB_occlusion_query2 on desktop. > This is already emulating for WebGL where GLContext would not be able to > manage ANY_SAMPLES_PASSED and SAMPLES_PASSED at the same time if we want to > emulate this in on GLContext. Interesting. I guess we can't emulate this then. ARB_occlusion_query2 should cover this, though. This means that ARB_occlusion_query2 is a hard requirement for WebGL2 on desktop GL, then. > > > > ::: content/canvas/src/WebGLQuery.cpp > > @@ +33,5 @@ > > > +} > > > + > > > +bool WebGLQuery::IsActive() const > > > +{ > > > + return mType == LOCAL_GL_ANY_SAMPLES_PASSED && > > > > Why do we bother checking mType here? > Because transform feedback will bring a new type of queries. Yes, but why would this change in the presence of a new enum? If the question we're asking is 'is this query active', shouldn't we just be able to compare it against the context's active query? This brings up this question: Shouldn't mActiveOcclusionQuery instead be mActiveQuery_AnySamplesPassed, since we're going to have other queries as well?

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #5) > (In reply to Guillaume Abadie from comment #4) > > (In reply to Jeff Gilbert [:jgilbert] from comment #3) > > > > + if (target != LOCAL_GL_ANY_SAMPLES_PASSED) { > > > > > > Should support _CONSERVATIVE as well. > > I don't think so, because it is said that ANY_SAMPLES_PASSED_CONSERVATIVE > > can be platform dependent. [1] > > AFAIK, the result of _CONSERVATIVE can be platform dependent (e.g. some > impls might return true, but others might not), but I'm pretty sure that > it's always valid to do a _CONSERVATIVE query. It's just never allowed to > be FALSE when ANY_SAMPLES_PASSED is TRUE, but it might be TRUE when the > other is FALSE. This is correct. _CONSERVATIVE acts as a bloom filter.

This patch also introduce WebGL2Objects.webidl compiled only if RELEASE_BUILD undefined.

Comment on attachment 781280 [details] [diff] [review] patch revision 3 Review of attachment 781280 [details] [diff] [review]: ----------------------------------------------------------------- Some things other than the missing _CONSERVATIVE option. Also, it would be more ideal if we could design this so it wasn't so hardcoded for just occlusion queries. We can fix this after the fact, but it's always nicer to get it right the first time. Feel free to leave this for later, though. ::: content/canvas/src/WebGLContextAsyncQueries.cpp @@ +139,5 @@ > + * and to contain the query result. If the active query object name for > + * <target> is zero when EndQueryEXT is called, the error INVALID_OPERATION > + * is generated. > + */ > + ErrorInvalidOperation("endQuery: any active query"); "endQuery: There is no active query." would be better. @@ +148,5 @@ > + > + if (gl->IsGLES2()) { > + gl->fEndQuery(LOCAL_GL_ANY_SAMPLES_PASSED); > + } else { > + gl->fEndQuery(LOCAL_GL_SAMPLES_PASSED); I don't like that we're hard-coding this here. ::: content/canvas/src/WebGLQuery.cpp @@ +22,5 @@ > +{ > + SetIsDOMBinding(); > + mContext->MakeContextCurrent(); > + mContext->mQueries.insertBack(this); > + mContext->gl->fGenQueries(1, &mGLName); We actually can't do this at Query creation time, since this triggers INVALID_OPERATION on desktop GL if there's currently an active query of any type. We need to GenQueries lazily at , and assume it's infallible, and just do GenQueries at BeginQuery time. Also, please leave a comment about this, since it's very strange and surprising. @@ +34,5 @@ > + > +bool WebGLQuery::IsActive() const > +{ > + return mType == LOCAL_GL_ANY_SAMPLES_PASSED && > + mContext->mActiveOcclusionQuery == this; I still think this is wrong. If the type is not ANY_SAMPLES_PASSED, but we were still the mActiveOcculsionQuery, why would this Query not be considered active? ::: dom/webidl/WebGL2RenderingContext.webidl @@ +52,5 @@ > const GLenum MIN = 0x8007; > const GLenum MAX = 0x8008; > > + /* query objects */ > + const GLenum ANY_SAMPLES_PASSED = 0x8C2F; GLES3 has ANY_SAMPLES_PASSED_CONSERVATIVE, so we should expect WebGL2 to have it as well. ::: gfx/gl/GLContext.cpp @@ +644,5 @@ > > + if (IsGLES2() && > + IsExtensionSupported(EXT_occlusion_query_boolean)) { > + SymLoadStruct queryObjectsSymbols[] = { > + { (PRFuncPtr*) &mSymbols.fBeginQuery, { "BeginQueryEXT", nullptr } }, We need to include the non-EXT versions too, since that'll be what GLES3 has.

(In reply to Jeff Gilbert [:jgilbert] from comment #9) > Comment on attachment 781280 [details] [diff] [review] > patch revision 3 > > Review of attachment 781280 [details] [diff] [review]: > ----------------------------------------------------------------- > > Some things other than the missing _CONSERVATIVE option. > > Also, it would be more ideal if we could design this so it wasn't so > hardcoded for just occlusion queries. We can fix this after the fact, but > it's always nicer to get it right the first time. Feel free to leave this > for later, though. > > ::: content/canvas/src/WebGLContextAsyncQueries.cpp > @@ +139,5 @@ > > + * and to contain the query result. If the active query object name for > > + * <target> is zero when EndQueryEXT is called, the error INVALID_OPERATION > > + * is generated. > > + */ > > + ErrorInvalidOperation("endQuery: any active query"); > > "endQuery: There is no active query." would be better. Fixed > > @@ +148,5 @@ > > + > > + if (gl->IsGLES2()) { > > + gl->fEndQuery(LOCAL_GL_ANY_SAMPLES_PASSED); > > + } else { > > + gl->fEndQuery(LOCAL_GL_SAMPLES_PASSED); > > I don't like that we're hard-coding this here. > > ::: content/canvas/src/WebGLQuery.cpp > @@ +22,5 @@ > > +{ > > + SetIsDOMBinding(); > > + mContext->MakeContextCurrent(); > > + mContext->mQueries.insertBack(this); > > + mContext->gl->fGenQueries(1, &mGLName); > > We actually can't do this at Query creation time, since this triggers > INVALID_OPERATION on desktop GL if there's currently an active query of any > type. We need to GenQueries lazily at , and assume it's infallible, and just > do GenQueries at BeginQuery time. > Also, please leave a comment about this, since it's very strange and > surprising. Oups ... Going to fix that. > > @@ +34,5 @@ > > + > > +bool WebGLQuery::IsActive() const > > +{ > > + return mType == LOCAL_GL_ANY_SAMPLES_PASSED && > > + mContext->mActiveOcclusionQuery == this; > > I still think this is wrong. If the type is not ANY_SAMPLES_PASSED, but we > were still the mActiveOcculsionQuery, why would this Query not be considered > active? Well from specification, queries are designed to have different targets (for future types of queries). Therefore this function would have to check the WebGLRefPtr depending on its type (assuming it has not been binded on a wrong WebGLRefPtr). > > ::: dom/webidl/WebGL2RenderingContext.webidl > @@ +52,5 @@ > > const GLenum MIN = 0x8007; > > const GLenum MAX = 0x8008; > > > > + /* query objects */ > > + const GLenum ANY_SAMPLES_PASSED = 0x8C2F; > > GLES3 has ANY_SAMPLES_PASSED_CONSERVATIVE, so we should expect WebGL2 to > have it as well. Ok. > > ::: gfx/gl/GLContext.cpp > @@ +644,5 @@ > > > > + if (IsGLES2() && > > + IsExtensionSupported(EXT_occlusion_query_boolean)) { > > + SymLoadStruct queryObjectsSymbols[] = { > > + { (PRFuncPtr*) &mSymbols.fBeginQuery, { "BeginQueryEXT", nullptr } }, > > We need to include the non-EXT versions too, since that'll be what GLES3 has. Fixed.

Add fixes and implement ANY_SAMPLES_PASSED_CONSERVATIVE

Comment on attachment 781393 [details] [diff] [review] patch revision 4 Review of attachment 781393 [details] [diff] [review]: ----------------------------------------------------------------- Almost. If you want to flip it around again tonight, I'll try to review it when I get home. ::: content/canvas/src/WebGLContextAsyncQueries.cpp @@ +30,5 @@ > + * any target is active causes an INVALID_OPERATION error to be > + * generated. > + */ > + ErrorInvalidOperation("createQuery: the WebGL 2 prototype can't create a query" > + "object while one is active."); Make this a warning on Desktop GL. Follow-up bug to make GL3.0 a requirement for creating a WebGL2 context. (Add support for webgl2.force-enable pref) That way, it will generally work, particularly on windows+native-GL and on linux, which have GL4+ drivers. @@ +62,5 @@ > + * any target is active causes an INVALID_OPERATION error to be > + * generated. > + */ > + ErrorInvalidOperation("deleteQuery: the WebGL 2 prototype can't delete a query" > + "object while one is active."); Same as above. @@ +287,5 @@ > + return true; > +} > + > +WebGLRefPtr<WebGLQuery>& > +WebGLContext::GetQueryTarget(WebGLenum target) Call this `GetActiveQueryByTarget`. ::: gfx/gl/GLDefs.h @@ +3050,5 @@ > #define LOCAL_GL_WAIT_FAILED 0x911D > > +// ARB_occlusion_query2 > +#define LOCAL_GL_ANY_SAMPLES_PASSED 0x8C2F > +#define LOCAL_GL_ANY_SAMPLES_PASSED_CONSERVATIVE 0x8D6A _CONSERVATIVE was added in EXT_occlusion_query_boolean (iirc)

Comment on attachment 781408 [details] [diff] [review] patch revision 5 Review of attachment 781408 [details] [diff] [review]: ----------------------------------------------------------------- Check them all, but I've marked the blockers with [r-]. ::: content/canvas/src/WebGLContextAsyncQueries.cpp @@ +30,5 @@ > + * any target is active causes an INVALID_OPERATION error to be > + * generated. > + */ > + GenerateWarning("createQuery: the WebGL 2 prototype might generate INVALID_OPERATION" > + "when creating a query object while one other is active."); [r-] Please file a follow-up bug, and add the bug number to a comment here. We *need* to lock webgl2 to GL>=3.0 on desktop, but we don't have a good mechanism to do this yet. I have to r- if we stand a decent chance at forgetting about this. @@ +112,5 @@ > + ErrorInvalidOperation("beginQuery: target doesn't match with the query type"); > + return; > + } > + > + if (GetActiveQueryByTarget(target)) { [r-] This branch needs to run whether or not the query has ever been active, otherwise we'll allow all first-run queries to run while another query is already active. @@ +126,5 @@ > + > + MakeContextCurrent(); > + > + if (!gl->IsGLES2()) { > + gl->fBeginQuery(LOCAL_GL_SAMPLES_PASSED, query->mGLName); If we have occlusion_query2 (I forget the name), desktop will have ANY_SAMPLES_PASSED as well. Can do as a follow-up. @@ +144,5 @@ > + if (!ValidateTargetParameter(target, "endQuery")) { > + return; > + } > + > + if (!GetActiveQueryByTarget(target) || target != mActiveOcclusionQuery->mType) { s/mActiveOcclusionQuery/GetActiveQueryByTarget(target)/ Though the error for that case shouldn't be "There is no active query", but rather "There is no active query of type `target`." @@ +169,5 @@ > + } else { > + gl->fEndQuery(target); > + } > + > + mActiveOcclusionQuery = nullptr; We should make GetActiveQueryByTarget return a reference, so we can set it to null here. Alternatively, make a setter. @@ +245,5 @@ > + ErrorInvalidOperation("getQueryObject: query has never been active"); > + return JS::NullValue(); > + } > + > + if (pname == LOCAL_GL_QUERY_RESULT_AVAILABLE) { This should probably be a switch. @@ +278,5 @@ > + if (target != LOCAL_GL_ANY_SAMPLES_PASSED && > + target != LOCAL_GL_ANY_SAMPLES_PASSED_CONSERVATIVE) > + { > + ErrorInvalidEnum("%s: target must be ANY_SAMPLES_PASSED{_CONSERVATIVE}", infos); > + return nullptr; s/nullptr/false/ ::: content/canvas/src/WebGLContextValidate.cpp @@ +1063,5 @@ > !IsExtensionSupported(WEBGL_draw_buffers) || > !gl->IsExtensionSupported(gl::GLContext::EXT_gpu_shader4) || > !gl->IsExtensionSupported(gl::GLContext::EXT_blend_minmax) || > + !gl->IsExtensionSupported(gl::GLContext::XXX_draw_instanced) || > + (gl->IsGLES2() && !gl->IsExtensionSupported(gl::GLContext::EXT_occlusion_query_boolean)) [r-] Add this: // Todo: Bug XXXXXX: Only allow WebGL2 on GL>=3.0 on desktop GL. (But, well, an actual bug number :)) ::: content/canvas/src/WebGLQuery.h @@ +36,5 @@ > + } > + > + bool IsActive() const > + { > + return mContext->GetActiveQueryByTarget(mType) == this; This belongs in the CPP, so we can remove the dep of WebGLContext.h, unless 'friend' makes this required.

(In reply to Jeff Gilbert [:jgilbert] from comment #14) > Comment on attachment 781408 [details] [diff] [review] > patch revision 5 > > Review of attachment 781408 [details] [diff] [review]: > ----------------------------------------------------------------- > > Check them all, but I've marked the blockers with [r-]. > > ::: content/canvas/src/WebGLContextAsyncQueries.cpp > @@ +30,5 @@ > > + * any target is active causes an INVALID_OPERATION error to be > > + * generated. > > + */ > > + GenerateWarning("createQuery: the WebGL 2 prototype might generate INVALID_OPERATION" > > + "when creating a query object while one other is active."); > > [r-] Please file a follow-up bug, and add the bug number to a comment here. > We *need* to lock webgl2 to GL>=3.0 on desktop, but we don't have a good > mechanism to do this yet. I have to r- if we stand a decent chance at > forgetting about this. See 898404. > > @@ +112,5 @@ > > + ErrorInvalidOperation("beginQuery: target doesn't match with the query type"); > > + return; > > + } > > + > > + if (GetActiveQueryByTarget(target)) { > > [r-] This branch needs to run whether or not the query has ever been active, > otherwise we'll allow all first-run queries to run while another query is > already active. Oups, artefact about trying to add genQueries here. Fixed. > > @@ +126,5 @@ > > + > > + MakeContextCurrent(); > > + > > + if (!gl->IsGLES2()) { > > + gl->fBeginQuery(LOCAL_GL_SAMPLES_PASSED, query->mGLName); > > If we have occlusion_query2 (I forget the name), desktop will have > ANY_SAMPLES_PASSED as well. Can do as a follow-up. Yes. > > @@ +144,5 @@ > > + if (!ValidateTargetParameter(target, "endQuery")) { > > + return; > > + } > > + > > + if (!GetActiveQueryByTarget(target) || target != mActiveOcclusionQuery->mType) { > > s/mActiveOcclusionQuery/GetActiveQueryByTarget(target)/ > Though the error for that case shouldn't be "There is no active query", but > rather "There is no active query of type `target`." Fixed. > > @@ +169,5 @@ > > + } else { > > + gl->fEndQuery(target); > > + } > > + > > + mActiveOcclusionQuery = nullptr; > > We should make GetActiveQueryByTarget return a reference, so we can set it > to null here. > Alternatively, make a setter. GetActiveQueryByTarget already return a reference, I just forget to replace it here. > > @@ +245,5 @@ > > + ErrorInvalidOperation("getQueryObject: query has never been active"); > > + return JS::NullValue(); > > + } > > + > > + if (pname == LOCAL_GL_QUERY_RESULT_AVAILABLE) { > > This should probably be a switch. Fixed. > > @@ +278,5 @@ > > + if (target != LOCAL_GL_ANY_SAMPLES_PASSED && > > + target != LOCAL_GL_ANY_SAMPLES_PASSED_CONSERVATIVE) > > + { > > + ErrorInvalidEnum("%s: target must be ANY_SAMPLES_PASSED{_CONSERVATIVE}", infos); > > + return nullptr; > > s/nullptr/false/ Oups ... > > ::: content/canvas/src/WebGLContextValidate.cpp > @@ +1063,5 @@ > > !IsExtensionSupported(WEBGL_draw_buffers) || > > !gl->IsExtensionSupported(gl::GLContext::EXT_gpu_shader4) || > > !gl->IsExtensionSupported(gl::GLContext::EXT_blend_minmax) || > > + !gl->IsExtensionSupported(gl::GLContext::XXX_draw_instanced) || > > + (gl->IsGLES2() && !gl->IsExtensionSupported(gl::GLContext::EXT_occlusion_query_boolean)) > > [r-] Add this: > // Todo: Bug XXXXXX: Only allow WebGL2 on GL>=3.0 on desktop GL. > > (But, well, an actual bug number :)) Added. > > ::: content/canvas/src/WebGLQuery.h > @@ +36,5 @@ > > + } > > + > > + bool IsActive() const > > + { > > + return mContext->GetActiveQueryByTarget(mType) == this; > > This belongs in the CPP, so we can remove the dep of WebGLContext.h, unless > 'friend' makes this required. No we can't, because the WebGLQuery's WebIDL interface is in WebGL2Objects, that only have this for now, and we are having a build-time error the generated .cpp don't include WebGLContext.h And all include of WebGLQuery.h include WebGLContext.h anyway.

Comment on attachment 781685 [details] [diff] [review] patch revision 7 Review of attachment 781685 [details] [diff] [review]: ----------------------------------------------------------------- Fix the nits and this should be good. ::: content/canvas/src/WebGLContext.h @@ +793,5 @@ > + > +private: > + bool ValidateTargetParameter(WebGLenum target, const char* infos); > + WebGLRefPtr<WebGLQuery>& GetActiveQueryByTarget(WebGLenum target); > + static const char* GetQueryTargetEnumString(WebGLenum target); Why does this need to be on WebGLContext, and not just in the caller's CPP file? ::: content/canvas/src/WebGLContextAsyncQueries.cpp @@ +165,5 @@ > + ErrorInvalidOperation("endQuery: There is no active query of type %s.", > + GetQueryTargetEnumString(target)); > + /* > + * We *need* to lock webgl2 to GL>=3.0 on desktop, but we don't have a good > + * mechanism to do this yet. See bug 898404. Not sure this comment is in a great place. Shouldn't it be up in Create/DeleteQuery? @@ +323,5 @@ > + default: > + break; > + } > + > + MOZ_ASSERT(); MOZ_ASSERT(false, "Unknown query `target`.");

Thanks Jeff ! https://hg.mozilla.org/integration/mozilla-inbound/rev/6ba07a3e0c80

Backed out to fix test failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/a4535d7fce37 https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=6ba07a3e0c80 https://tbpl.mozilla.org/php/getParsedLog.php?id=25836449&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=25836344&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=25836420&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=25836292&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=25836472&tree=Mozilla-Inbound

Don't forget to touch CLOBBER when this re-lands too or Windows builds will intermittently die due to bugs in the WebIDL build system.

GetQueryObject seems to only return null or booleans; why did it not use 'boolean?'?

(In reply to :Ms2ger from comment #24) > GetQueryObject seems to only return null or booleans; why did it not use > 'boolean?'? because we could have anytime to implement queries that might return something else that a boolean, like SAMPLE_PASSED for example. So we make this function generic.