974832 - WebGL EXT_disjoint_timer_query may now be implemented

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Florian Bösch]

The WebGL extension EXT_disjoint_timer_query was moved to draft: http://www.khronos.org/registry/webgl/extensions/EXT_disjoint_timer_query/

Vendors are encouraged to consider implementation of this extension.

Comment 1

[Florian Bösch]

Chrome ticket: https://code.google.com/p/chromium/issues/detail?id=345227
Webkit ticket: https://bugs.webkit.org/show_bug.cgi?id=129090

Comment 2

[Florian Bösch]

This extension can be implemented by a UA if either any one of these conditions is met:

- The OpenGL 3.3 core profile is supported: https://www.opengl.org/registry/doc/glspec33.core.20100311.pdf
- ARB_timer_query is supported: https://www.opengl.org/registry/specs/ARB/timer_query.txt
- Direct3D 9 is supported: http://msdn.microsoft.com/en-us/library/windows/desktop/bb147308(v=vs.85).aspx
- The OpenGL ES EXT_disjoint_timer_query is supported: http://www.khronos.org/registry/gles/extensions/EXT/EXT_disjoint_timer_query.txt

The disjoint behavior lacking from ARB_timer_query and OpenGL 3.3 core can be emulated by the UA.

Comment 3

[Florian Bösch]

The ANGLE project ticket possible used for tracking this feature: https://code.google.com/p/angleproject/issues/detail?id=142

Comment 4

[Benoit Girard (:BenWa)]

I'm interested in doing part of the work here since I'll be looking at bug 737967 but I may not implement everything listed in Comment 2. If others are interested in this as well let me know.

Comment 5

[Guillaume Abadie]

(In reply to Benoit Girard (:BenWa) from comment #4)
> I'm interested in doing part of the work here since I'll be looking at bug
> 737967 but I may not implement everything listed in Comment 2. If others are
> interested in this as well let me know.

The implementation shouldn't be very complicated for everything listed in Comment 2. Especially ANGLE will provide EXT_disjoint_timer_query or not for the Direct3D thing. So all of these support requirement is pretty much just a GLFeature::disjoint_timer_query (GLContextFeatures.cpp).

The strange thing of this extension is they are introducing WebGLTimerQuery instead of a general purpose one like WebGLQuery in our WebGL 2 implementation. But I think we should set WebGLTimerQuery as a typedef or dummy inherited class of WebGLQuery (in the WebIDL specification). Then the implementation is just within WebGLContextAsyncQuery.cpp.

Comment 6

[Guillaume Abadie]

Let me know if you are still interested in implementing this extension! =D

Comment 7

[Guillaume Abadie]

(In reply to Florian Bösch from comment #2)
> This extension can be implemented by a UA if either any one of these
> conditions is met:
> 
> - The OpenGL 3.3 core profile is supported:
> https://www.opengl.org/registry/doc/glspec33.core.20100311.pdf
> - ARB_timer_query is supported:
> https://www.opengl.org/registry/specs/ARB/timer_query.txt
> - Direct3D 9 is supported:
> http://msdn.microsoft.com/en-us/library/windows/desktop/bb147308(v=vs.85).
> aspx
> - The OpenGL ES EXT_disjoint_timer_query is supported:
> http://www.khronos.org/registry/gles/extensions/EXT/EXT_disjoint_timer_query.
> txt
> 
> The disjoint behavior lacking from ARB_timer_query and OpenGL 3.3 core can
> be emulated by the UA.

Wait, GPU_DISJOINT_EXT is only supported by GL_EXT_disjoint_timer_query. Nothing else. Should we let gl.getParameter(glext.GPU_DISJOINT_EXT) return false to simulate on desktops this WebGL extension with ARB_timer_query or a GL 3.3 profile?

Comment 8

[Florian Bösch]

The disjoint functionality means that the context has a way to inform the programmer when a timing result cannot be valid anymore. Such as when the computer went to sleep, the context was lost and restored or on a mobile when the application was suspended due to screen lock or being shuffled for another application.

This behavior can be emulated by the UA for WebGL if it isn't natively offered by a given backend.

Comment 9

[Guillaume Abadie]

(In reply to Florian Bösch from comment #8)
> The disjoint functionality means that the context has a way to inform the
> programmer when a timing result cannot be valid anymore. Such as when the
> computer went to sleep, the context was lost and restored or on a mobile
> when the application was suspended due to screen lock or being shuffled for
> another application.
> 
> This behavior can be emulated by the UA for WebGL if it isn't natively
> offered by a given backend.

Oh yeah that is true. My bad!

Comment 10

[Florian Bösch]

EXT_disjoint_timer query is desperately needed. Measuring performance by way of FPS is completely unusable, as it mostly just boils down to 3 answers, 15fps, 30fps or 60fps. Programmers can't measure their WebGL programs, and hence they cannot optimize them. This is extremely bad.

Comment 11

[Guillaume Abadie]

They could by measuring across several frames instead. And I'm still ok to take it!

Comment 12

[Guillaume Abadie]

(In reply to Guillaume Abadie from comment #11)
> They could by measuring across several frames instead. And I'm still ok to
> take it!

My bad no they can't ^^

Comment 13

[Benoit Girard (:BenWa)]

Ahh, feel free to take it if you have. I'll get back to implementing platform side profiling so if this is ready it would help me out.

Comment 14

[Guillaume Abadie]

(In reply to Benoit Girard (:BenWa) from comment #13)
> Ahh, feel free to take it if you have. I'll get back to implementing
> platform side profiling so if this is ready it would help me out.

I'm taking it then! Thanks! =)

Comment 15

[Guillaume Abadie]

Attachment: GLContext - revision 1

This patch adds support for GL_{ARB_timer,EXT_timer,EXT_disjoint_timer}_query extensions in the OpenGL context...

Comment 16

[Guillaume Abadie]

Attachment: Implements WebGLTimerQuery - revision 1

This patch is implementing WebGLTimerQuery WebIDL interface as a typedef of WebGL 2's WebGLQuery. Still have to deferred the query's result to the next frame that I chose to implement in a following on.

But I think it is just not a good idea at all to do it as the WebGL extension is proposing. Especially if at some point we are going to expose conditional rendering as a WebGL extension. This WebGL nit would then become one of the biggest pain in the... =)

Comment 17

[Guillaume Abadie]

Also will have to implement GPU_DISJOINT_EXT faking in its own patch for an easier review.

Comment 18

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8459122 [details] [diff] [review]
GLContext - revision 1

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

::: gfx/gl/GLContext.cpp
@@ +998,5 @@
> +                { (PRFuncPtr*) &mSymbols.fGetQueryObjectui64v, { "GetQueryObjectui64vEXT", nullptr } },
> +                END_SYMBOLS
> +            };
> +
> +            bool useCore = IsFeatureProvidedByCoreSymbols(GLFeature::get_query_object_64);

Why are we doing this instead of the usual fallback system of `{"Foo", "FooEXT", nullptr}`?
This seems more complicated without benefit.

::: gfx/gl/GLContextSymbols.h
@@ +500,5 @@
> +
> +    // timer_query
> +    typedef void (GLAPIENTRY * PFNGLQUERYCOUNTER) (GLuint id, GLenum target);
> +    PFNGLQUERYCOUNTER fQueryCounter;
> +    typedef void (GLAPIENTRY * PFNGLGETQUERYOBJECTI64V) (GLuint id, GLenum pname, GLint64 *params);

Star to left.

Comment 19

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8459247 [details] [diff] [review]
Implements WebGLTimerQuery - revision 1

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

::: content/canvas/src/WebGLContextAsyncQueries.cpp
@@ +45,5 @@
> +static uint32_t
> +GetQueryTargetBits(GLenum target)
> +{
> +    switch (target)
> +    {

switch (foo) {

@@ +293,5 @@
>  
> +    switch(pname)
> +    {
> +        case LOCAL_GL_CURRENT_QUERY:
> +        {

switch (foo): {
    case BAR: {

@@ +397,5 @@
> +                 * ARB_occlusion_query_boolean return only a boolean if a sample has been drawed.
> +                 */
> +                return JS::BooleanValue(result != LOCAL_GL_FALSE);
> +            }
> +            else if (query_bits == 64) {

} else if (foo) {

If you need more whitespace, do:

if (foo) {
  code
  code

} else if (bar) {

@@ +424,5 @@
>      return JS::NullValue();
>  }
>  
> +void
> +WebGLContext::QueryCounter(WebGLQuery *query, GLenum target)

Star to left.

@@ +429,5 @@
>  {
> +    /*
> +     * This entry point must only be accessible throught EXT_disjoint_timer_query
> +     */
> +    MOZ_ASSERT(IsExtensionSupported(WebGLExtensionID::EXT_disjoint_timer_query));

Use IsExtensionEnabled, not Supported.

@@ +474,5 @@
> +}
> +
> +bool
> +WebGLContext::ValidateQueryTarget(GLenum target, GLbitfield validationMask,
> +                                  WebGLRefPtr<WebGLQuery>** outTargetSlot,

I don't think it's worth making this optional. I think it would be better to just require wasting a junkvar in the case where it's not wanted.

@@ +475,5 @@
> +
> +bool
> +WebGLContext::ValidateQueryTarget(GLenum target, GLbitfield validationMask,
> +                                  WebGLRefPtr<WebGLQuery>** outTargetSlot,
> +                                  const char* infos)

Outvars go last.

Also this function is really quite noisy for what it looks like it's doing. Why is it better to have this big function instead of a couple little ones?

@@ +490,5 @@
> +    // if we are only validating query counter target, asking a target slot has no sense
> +    MOZ_ASSERT((validationMask & kQueryValidateTarget) != 0x0 ||
> +               out_target_slot == nullptr);
> +
> +    // if are requesting the query target slot, make sur it has been inited to nullptr

"sure"

@@ +494,5 @@
> +    // if are requesting the query target slot, make sur it has been inited to nullptr
> +    MOZ_ASSERT(out_target_slot == nullptr || *out_target_slot == nullptr);
> +
> +    WebGLRefPtr<WebGLQuery>* targetSlot = nullptr;
> +    const char * errorMsg = nullptr;

const char* foo = bar;

::: content/canvas/src/WebGLContextState.cpp
@@ +167,5 @@
> +            return JS::BooleanValue(false); // TODO
> +        } else if (pname == LOCAL_GL_TIMESTAMP) {
> +            GLuint64 timestamp = 0;
> +
> +            gl->fGetInteger64v(pname, (GLint64 *) &timestamp);

(GLint64*)&timestamp

::: content/canvas/src/WebGLExtensionDisjointTimerQuery.cpp
@@ +14,5 @@
> +
> +WebGLExtensionDisjointTimerQuery::WebGLExtensionDisjointTimerQuery(WebGLContext* context)
> +  : WebGLExtensionBase(context)
> +{
> +    MOZ_ASSERT(IsSupported(context), "should not construct WebGLExtensionDisjointTimerQuery :"

No space before ":".

@@ +15,5 @@
> +WebGLExtensionDisjointTimerQuery::WebGLExtensionDisjointTimerQuery(WebGLContext* context)
> +  : WebGLExtensionBase(context)
> +{
> +    MOZ_ASSERT(IsSupported(context), "should not construct WebGLExtensionDisjointTimerQuery :"
> +                                     "EXT_disjoint_timer_query unsuported.");

"unsupported"

@@ +26,5 @@
> +already_AddRefed<WebGLQuery>
> +WebGLExtensionDisjointTimerQuery::CreateQueryEXT()
> +{
> +    if (mIsLost) {
> +        mContext->ErrorInvalidOperation("createQueryEXT: Extension is lost. Returning NULL.");

`null` is lowercase in JS.

@@ +34,5 @@
> +    return mContext->CreateQuery();
> +}
> +
> +void
> +WebGLExtensionDisjointTimerQuery::DeleteQueryEXT(WebGLQuery *query)

Star to left.

@@ +43,5 @@
> +    mContext->DeleteQuery(query);
> +}
> +
> +void
> +WebGLExtensionDisjointTimerQuery::BeginQueryEXT(GLenum target, WebGLQuery *query)

Star to left.

@@ +61,5 @@
> +    mContext->EndQuery(target);
> +}
> +
> +bool
> +WebGLExtensionDisjointTimerQuery::IsQueryEXT(WebGLQuery *query)

Star to left.

@@ +73,5 @@
> +}
> +
> +void
> +WebGLExtensionDisjointTimerQuery::GetQueryEXT(JSContext* cx, GLenum target, GLenum pname,
> +                    JS::MutableHandle<JS::Value> retval, ErrorResult& rv)

Bad indent.

@@ +85,5 @@
> +    return mContext->GetQuery(cx, target, pname, retval, rv);
> +}
> +
> +void
> +WebGLExtensionDisjointTimerQuery::GetQueryObjectEXT(JSContext* cx, WebGLQuery *query, GLenum pname,

Star to left.

@@ +86,5 @@
> +}
> +
> +void
> +WebGLExtensionDisjointTimerQuery::GetQueryObjectEXT(JSContext* cx, WebGLQuery *query, GLenum pname,
> +                    JS::MutableHandle<JS::Value> retval)

Bad indent.

@@ +98,5 @@
> +    return mContext->GetQueryObject(cx, query, pname, retval);
> +}
> +
> +void
> +WebGLExtensionDisjointTimerQuery::QueryCounterEXT(WebGLQuery *query, GLenum target)

Star to left.

@@ +111,5 @@
> +WebGLExtensionDisjointTimerQuery::IsSupported(const WebGLContext* context)
> +{
> +    gl::GLContext* gl = context->GL();
> +
> +    return false; // TODO remainings

r-.

::: content/canvas/src/WebGLExtensions.h
@@ +330,5 @@
> +    WebGLExtensionDisjointTimerQuery(WebGLContext*);
> +    virtual ~WebGLExtensionDisjointTimerQuery();
> +
> +    already_AddRefed<WebGLQuery> CreateQueryEXT();
> +    void DeleteQueryEXT(WebGLQuery *query);

Star to the left.

::: content/canvas/src/WebGLQuery.cpp
@@ +44,5 @@
>  
> +    mContext->ValidateQueryTarget(mType, WebGLContext::kQueryValidateAllTarget,
> +                                  &targetSlot);
> +
> +    return targetSlot != nullptr && *targetSlot == this;

`foo != nullptr` is unnecessary. Prefer `foo`.

::: content/canvas/src/WebGLTypes.h
@@ +146,5 @@
>  // Please keep extensions in alphabetic order.
>  MOZ_BEGIN_ENUM_CLASS(WebGLExtensionID, uint8_t)
>      ANGLE_instanced_arrays,
>      EXT_blend_minmax,
> +    EXT_disjoint_timer_query,

EXT_disjoint should be after EXT_color, but before EXT_frag.

::: dom/webidl/WebGLRenderingContext.webidl
@@ +64,5 @@
>  
>  interface WebGLTexture {
>  };
>  
> +typedef WebGLQuery WebGLTimerQuery;

I don't think this should be moved here?

Comment 20

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #19)
> Comment on attachment 8459247 [details] [diff] [review]
> Implements WebGLTimerQuery - revision 1
> 
> Review of attachment 8459247 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +293,5 @@
> > +    switch(pname)
> > +    {
> > +        case LOCAL_GL_CURRENT_QUERY:
> > +        {
> 
> switch (foo): {
>     case BAR: {

Shouldn't this be:

    switch (foo) {
    case BAR: {

???

> @@ +397,5 @@
> > +                 * ARB_occlusion_query_boolean return only a boolean if a sample has been drawed.

"drawn"

> > +bool
> > +WebGLContext::ValidateQueryTarget(GLenum target, GLbitfield validationMask,
> > +                                  WebGLRefPtr<WebGLQuery>** outTargetSlot,
> > +                                  const char* infos)
> 
> Outvars go last.

That may have been me, or I copied from local style. ie. Lots of the validate functions take the info (ie function name) for ErrorInvalidXXX as the last parameter.

Comment 21

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #18)
> Why are we doing this instead of the usual fallback system of `{"Foo",
> "FooEXT", nullptr}`?
> This seems more complicated without benefit.

This seems to be a new idiom introduced by Vlad via Bug 1009965.

Comment 22

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #20)
> (In reply to Jeff Gilbert [:jgilbert] from comment #19)
> > Comment on attachment 8459247 [details] [diff] [review]
> > Implements WebGLTimerQuery - revision 1
> > 
> > Review of attachment 8459247 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > @@ +293,5 @@
> > > +    switch(pname)
> > > +    {
> > > +        case LOCAL_GL_CURRENT_QUERY:
> > > +        {
> > 
> > switch (foo): {
> >     case BAR: {
> 
> Shouldn't this be:
> 
>     switch (foo) {
>     case BAR: {
> 
> ???

In this case, I don't think so, as you end up with:
> switch (foo) {
> case BAR: {
>   break;
> }
> } // double } at the same indent level?!

I think we should do:
> switch (foo) {
> case BAR:
>     break;
> }

And:
> switch (foo) {
>     case BAR: {
>         break;
>     }
> }

> 
> > @@ +397,5 @@
> > > +                 * ARB_occlusion_query_boolean return only a boolean if a sample has been drawed.
> 
> "drawn"
> 
> > > +bool
> > > +WebGLContext::ValidateQueryTarget(GLenum target, GLbitfield validationMask,
> > > +                                  WebGLRefPtr<WebGLQuery>** outTargetSlot,
> > > +                                  const char* infos)
> > 
> > Outvars go last.
> 
> That may have been me, or I copied from local style. ie. Lots of the
> validate functions take the info (ie function name) for ErrorInvalidXXX as
> the last parameter.
I think we should stick with outvars being last, giving us an approximation of a syntax like `fn Foo(inarg0, inarg1)->(outarg0, outarg1)`. I'm open to discussion on this though. (I think it's nicer)

I think `info` should go last of the regular args, but before any outvars.

Comment 23

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #21)
> (In reply to Jeff Gilbert [:jgilbert] from comment #18)
> > Why are we doing this instead of the usual fallback system of `{"Foo",
> > "FooEXT", nullptr}`?
> > This seems more complicated without benefit.
> 
> This seems to be a new idiom introduced by Vlad via Bug 1009965.

I think this was for a very specific reason. It looks like we didn't end up with a comment about it, though. I'll check.

Comment 24

[Kelsey Gilbert [:jgilbert]]

(In reply to Jeff Gilbert [:jgilbert] from comment #23)
> (In reply to Dan Glastonbury :djg :kamidphish from comment #21)
> > (In reply to Jeff Gilbert [:jgilbert] from comment #18)
> > > Why are we doing this instead of the usual fallback system of `{"Foo",
> > > "FooEXT", nullptr}`?
> > > This seems more complicated without benefit.
> > 
> > This seems to be a new idiom introduced by Vlad via Bug 1009965.
> 
> I think this was for a very specific reason. It looks like we didn't end up
> with a comment about it, though. I'll check.

Oh, ok, interesting/sad. It sounds like we need to do this because in a GLES2 ANGLE context, it's still forbidden to call things which would be suffixless in GLES3, but exist from extensions in GLES2. That is, calling glBlitFramebuffer is illegal in GLES2, whereas glBlitFramebufferEXT is valid.

Comment 25

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #22)
> (In reply to Dan Glastonbury :djg :kamidphish from comment #20)
> > (In reply to Jeff Gilbert [:jgilbert] from comment #19)
> > > Comment on attachment 8459247 [details] [diff] [review]
> > > Implements WebGLTimerQuery - revision 1
> > > 
> > > Review of attachment 8459247 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > @@ +293,5 @@
> > > > +    switch(pname)
> > > > +    {
> > > > +        case LOCAL_GL_CURRENT_QUERY:
> > > > +        {
> > > 
> > > switch (foo): {
> > >     case BAR: {
> > 
> > Shouldn't this be:
> > 
> >     switch (foo) {
> >     case BAR: {
> > 
> > ???
> 
> In this case, I don't think so, as you end up with:
> > switch (foo) {
> > case BAR: {
> >   break;
> > }
> > } // double } at the same indent level?!
> 
> I think we should do:
> > switch (foo) {
> > case BAR:
> >     break;
> > }
> 
> And:
> > switch (foo) {
> >     case BAR: {
> >         break;
> >     }
> > }
No way! That is completely disgusting. You might have a switch with statements that requires a scope (for instancing variables) and other who might not need them. But you should keep the alignment of the case keywords. And so if you add the first statement that requires a scope to instantiate a variable, you would have to indent all other, clobbering the mercurial blame...

Also, 
case BAR: {
        break;
    } 

is dirty since you might have
case FOO:
case BAR: {
        break;
    } 

Also the fall through become not obvious because of the miss-alignement of { and }:
case FOO: {
        // fall through
    } 
case BAR: {
        break;
    } 

And since the scope is purely optional, that would have a complete sense to keep it like we would add one anywhere in another one

void main()
{
    int a;
    
    { // note that { and } are aligned in this case, and is purely optional (only for life expectency, as same as in a switch statement).
        int b;
    }
}
> 
> > 
> > > @@ +397,5 @@
> > > > +                 * ARB_occlusion_query_boolean return only a boolean if a sample has been drawed.
> > 
> > "drawn"
> > 
> > > > +bool
> > > > +WebGLContext::ValidateQueryTarget(GLenum target, GLbitfield validationMask,
> > > > +                                  WebGLRefPtr<WebGLQuery>** outTargetSlot,
> > > > +                                  const char* infos)
> > > 
> > > Outvars go last.
> > 
> > That may have been me, or I copied from local style. ie. Lots of the
> > validate functions take the info (ie function name) for ErrorInvalidXXX as
> > the last parameter.
> I think we should stick with outvars being last, giving us an approximation
> of a syntax like `fn Foo(inarg0, inarg1)->(outarg0, outarg1)`. I'm open to
> discussion on this though. (I think it's nicer)
> 
> I think `info` should go last of the regular args, but before any outvars.
That makes sence...

Comment 26

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #18)
> Comment on attachment 8459122 [details] [diff] [review]
> GLContext - revision 1
> 
> Review of attachment 8459122 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLContext.cpp
> @@ +998,5 @@
> > +                { (PRFuncPtr*) &mSymbols.fGetQueryObjectui64v, { "GetQueryObjectui64vEXT", nullptr } },
> > +                END_SYMBOLS
> > +            };
> > +
> > +            bool useCore = IsFeatureProvidedByCoreSymbols(GLFeature::get_query_object_64);
> 
> Why are we doing this instead of the usual fallback system of `{"Foo",
> "FooEXT", nullptr}`?
> This seems more complicated without benefit.
> 
> ::: gfx/gl/GLContextSymbols.h
> @@ +500,5 @@
> > +
> > +    // timer_query
> > +    typedef void (GLAPIENTRY * PFNGLQUERYCOUNTER) (GLuint id, GLenum target);
> > +    PFNGLQUERYCOUNTER fQueryCounter;
> > +    typedef void (GLAPIENTRY * PFNGLGETQUERYOBJECTI64V) (GLuint id, GLenum pname, GLint64 *params);
> 
> Star to left.
I think the best way would just be to generates source code of entry points and load their loading with a python script parsing gl.xml (https://cvs.khronos.org/svn/repos/ogl/trunk/doc/registry/public/api/) as we have already done for enums... GL Features could be generated this way to by intersecting entry points from differents source (profiles, extensions and so on) and generates two non cyclic directional dependencies graphs (one for features and one for extensions). And they we don't touch it anymore.

Comment 27

[Guillaume Abadie]

(In reply to Guillaume Abadie from comment #26)
> (In reply to Jeff Gilbert [:jgilbert] from comment #18)
> > Comment on attachment 8459122 [details] [diff] [review]
> > GLContext - revision 1
> > 
> > Review of attachment 8459122 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/gl/GLContext.cpp
> > @@ +998,5 @@
> > > +                { (PRFuncPtr*) &mSymbols.fGetQueryObjectui64v, { "GetQueryObjectui64vEXT", nullptr } },
> > > +                END_SYMBOLS
> > > +            };
> > > +
> > > +            bool useCore = IsFeatureProvidedByCoreSymbols(GLFeature::get_query_object_64);
> > 
> > Why are we doing this instead of the usual fallback system of `{"Foo",
> > "FooEXT", nullptr}`?
> > This seems more complicated without benefit.
> > 
> > ::: gfx/gl/GLContextSymbols.h
> > @@ +500,5 @@
> > > +
> > > +    // timer_query
> > > +    typedef void (GLAPIENTRY * PFNGLQUERYCOUNTER) (GLuint id, GLenum target);
> > > +    PFNGLQUERYCOUNTER fQueryCounter;
> > > +    typedef void (GLAPIENTRY * PFNGLGETQUERYOBJECTI64V) (GLuint id, GLenum pname, GLint64 *params);
> > 
> > Star to left.
> I think the best way would just be to generates source code of entry points
> and load their loading with a python script parsing gl.xml
> (https://cvs.khronos.org/svn/repos/ogl/trunk/doc/registry/public/api/) as we
> have already done for enums... GL Features could be generated this way to by
> intersecting entry points from differents source (profiles, extensions and
> so on) and generates two non cyclic directional dependencies graphs (one for
> features and one for extensions). And they we don't touch it anymore.
Oups my english was very bad on this one...

I think the best way would just be to generate source code of entry points and their loading mechanism with a python script parsing gl.xml (https://cvs.khronos.org/svn/repos/ogl/trunk/doc/registry/public/api/) as we have already done for enums... GL Features could be generated this way to by intersecting entry points from differents sources (profiles, extensions and so on) and then generates two non cyclic directional dependencies graphs (one for features and one for extensions), that we can disable extensions/features and their dependencies correctly when blacklisting driver because of bugs. And so we wouldn't take care of this job of adding the code to pull the missing entry point anymore.

Comment 29

[Andrew Comminos [:acomminos]]

I'll be working on a patch for this for my intern project.

Comment 30

[Andrew Comminos [:acomminos]]

Attachment: Part 1 - Add necessary features to GLContext.

Comment 31

[Andrew Comminos [:acomminos]]

Attachment: Part 2 - Add support for WebGL's EXT_disjoint_timer_query.

This patch avoids using WebGL2's timer queries in favor of a more isolated approach.

Comment 32

[Andrew Comminos [:acomminos]]

Attachment: Part 3 - Add tests for WebGL's EXT_disjoint_timer_query.

Lastly, here are some tests for the implemented functionality. Thanks in advance for taking a look :jgilbert!

Comment 33

[Andrew Comminos [:acomminos]]

Attachment: Part 2 - Add support for WebGL's EXT_disjoint_timer_query.

Forgot to include some important files for this one.

Comment 34

[u480271]

Comment on attachment 8607551 [details] [diff] [review]
Part 1 - Add necessary features to GLContext.

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

Wow. OpenGL is really messy dealing with queries. <insert sad panda>

Comment 35

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8607555 [details] [diff] [review]
Part 2 - Add support for WebGL's EXT_disjoint_timer_query.

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

In general, don't include EXT_ and similar prefix/suffixes if they are unnecessary.

::: dom/bindings/Bindings.conf
@@ +1508,5 @@
>  },
>  
> +'WebGLTimerQueryEXT': {
> +    'nativeType': 'mozilla::WebGLTimerQueryEXT',
> +    'headerFile': 'WebGLTimerQueryEXT.h'

Drop the EXT everywhere here. Note that other extensions do not have it.

::: dom/canvas/WebGLContextExtensions.cpp
@@ +35,5 @@
>          WEBGL_EXTENSION_IDENTIFIER(EXT_frag_depth)
>          WEBGL_EXTENSION_IDENTIFIER(EXT_shader_texture_lod)
>          WEBGL_EXTENSION_IDENTIFIER(EXT_sRGB)
>          WEBGL_EXTENSION_IDENTIFIER(EXT_texture_filter_anisotropic)
> +        WEBGL_EXTENSION_IDENTIFIER(MOZ_EXT_disjoint_timer_query)

Remove the MOZ_ prefix.

@@ +183,2 @@
>          switch (ext) {
> +        case WebGLExtensionID::MOZ_EXT_disjoint_timer_query:

Remove the MOZ_ prefix.

@@ +326,5 @@
>          obj = new WebGLExtensionTextureFilterAnisotropic(this);
>          break;
>  
> +    // MOZ_ (draft)
> +    case WebGLExtensionID::MOZ_EXT_disjoint_timer_query:

Remove the MOZ_ prefix.

::: dom/canvas/WebGLContextState.cpp
@@ +183,5 @@
> +            gl->fGetInteger64v(pname, (GLint64*) &iv);
> +            return JS::NumberValue(uint64_t(iv));
> +        } else if (pname == LOCAL_GL_GPU_DISJOINT_EXT) {
> +            // Default is to indicate that the query was not disjoint when
> +            // unsupported.

"When disjoint isn't supported, leave as false."

::: dom/canvas/WebGLExtensionDisjointTimerQuery.cpp
@@ +84,5 @@
> +                                        " active.");
> +        return;
> +    }
> +
> +    gl::GLContext *gl = mContext->GL();

Star to left, against type.

@@ +141,5 @@
> +        return;
> +
> +    switch (pname) {
> +    case LOCAL_GL_CURRENT_QUERY_EXT:
> +    {

{ for case on same line.

@@ +171,5 @@
> +        break;
> +    }
> +    default:
> +        mContext->ErrorInvalidEnumInfo("getQueryEXT: Invalid query property.",
> +                                       pname);

Include the break here for consistency.

::: dom/webidl/WebGLRenderingContext.webidl
@@ +29,5 @@
>  typedef unsigned short GLushort;
>  typedef unsigned long  GLuint;
>  typedef unrestricted float GLfloat;
>  typedef unrestricted float GLclampf;
> +typedef unsigned long long GLuint64EXT;

Drop the EXT.

Comment 36

[u480271]

Comment on attachment 8607555 [details] [diff] [review]
Part 2 - Add support for WebGL's EXT_disjoint_timer_query.

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

Overall, I'm not sure putting much of the 'heavy lifting' code into the extension class itself. The intention has always been that extensions classes are thin wrappers that call into WebGLContext, so that functionality can be shared. That said, trying to merge this with WebGL 2 queries might be pretty messy. Maybe it's best to land both and then try to find the intersection of each and factor out the commonality in a follow up bug.

I'm going to request Jeff Gilbert to provide feedback too.

Also, changes to .webidl files require review from a DOM peer before the patch can be landed. You can request :smaug to review the webidl changes.

::: dom/canvas/WebGLContextExtensions.cpp
@@ +35,5 @@
>          WEBGL_EXTENSION_IDENTIFIER(EXT_frag_depth)
>          WEBGL_EXTENSION_IDENTIFIER(EXT_shader_texture_lod)
>          WEBGL_EXTENSION_IDENTIFIER(EXT_sRGB)
>          WEBGL_EXTENSION_IDENTIFIER(EXT_texture_filter_anisotropic)
> +        WEBGL_EXTENSION_IDENTIFIER(MOZ_EXT_disjoint_timer_query)

No MOZ_ prefix. Instead of using prefix, toggling `webgl.enable-draft-extensions` pref is required.

::: dom/canvas/WebGLExtensionDisjointTimerQuery.cpp
@@ +53,5 @@
> +    if (!query)
> +        return false;
> +
> +    return (mContext->ValidateObjectAllowDeleted("isQueryEXT", query) &&
> +            !query->IsDeleted());

Jeff Gilbert prefers to break these out in separate if stmts. I have a patch in the works that moves to this style, where we finally call the GL driver.

if (!mContext->ValidateObjectAllowDeleted("isQueryEXT", query))
    return false;

if (query->IsDeleted())
    return false;

mContext->MakeContextCurrent();
return mContext->GL()->fIsQuery(query->GLName());

@@ +95,5 @@
> +WebGLExtensionDisjointTimerQuery::EndQueryEXT(GLenum target)
> +{
> +    if (mIsLost)
> +        return;
> +    

Please setup editor to delete trailing white space.

@@ +127,5 @@
> +                                       " TIMESTAMP_EXT.", target);
> +        return;
> +    }
> +
> +    mContext->GL()->fQueryCounter(query->GLName(), target);

Because of the way our GL context handling works, you'll need to call mContext->MakeContextCurrent() before calling any GL functions, since it's not guaranteed that the GL context will be the active one. If you search for MakeContextCurrent() you'll see the idiom throughout dom/canvas/WebGL* files.

@@ +195,5 @@
> +
> +    // XXX: Note that the query result *may change* within the same task!
> +    // This does not follow the specification, which states that all calls
> +    // checking query results must return the same value until the event loop
> +    // is empty.

Why are we out of spec? Can the code be changed to meet the spec?

@@ +198,5 @@
> +    // checking query results must return the same value until the event loop
> +    // is empty.
> +    switch (pname) {
> +    case LOCAL_GL_QUERY_RESULT_EXT:
> +    {

Move { to end of previous line.

@@ +207,5 @@
> +        retval.set(JS::NumberValue(result));
> +        break;
> +    }
> +    case LOCAL_GL_QUERY_RESULT_AVAILABLE_EXT:
> +    {

Move { to end of previous line.

::: dom/canvas/WebGLTypes.h
@@ +150,5 @@
>      EXT_frag_depth,
>      EXT_sRGB,
>      EXT_shader_texture_lod,
>      EXT_texture_filter_anisotropic,
> +    MOZ_EXT_disjoint_timer_query,

No MOZ_ prefix.

Comment 37

[u480271]

Comment on attachment 8607553 [details] [diff] [review]
Part 3 - Add tests for WebGL's EXT_disjoint_timer_query.

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

Jeff is "Mr. WebGL Mochitest". Asked for feedback from him to check the on the mochitestness.

::: dom/canvas/test/webgl-mochitest/test_webgl_disjoint_timer_query.html
@@ +20,5 @@
> +    return;
> +  }
> +
> +  ok(!ext.getQueryEXT(ext.TIME_ELAPSED_EXT, ext.CURRENT_QUERY_EXT),
> +     "Ensures that no query is active initially.");

I'd drop the "Ensures that" part for all the messages.

@@ +38,5 @@
> +  ok(ext.getQueryObjectEXT(elapsedQuery, ext.QUERY_RESULT_AVAILABLE_EXT),
> +     "Time elapsed query should be available immediately after flush.");
> +
> +  ext.deleteQueryEXT(elapsedQuery);
> +  ok(!ext.isQueryEXT(elapsedQuery), "Checks that the query is no longer valid after deletion.");

This sounds like your stating what the code is doing. I would prefer "Query is no longer valid after deletion."

Comment 38

[Andrew Comminos [:acomminos]]

Thanks for the reviews guys, I'll get to work on revisions.

(In reply to Dan Glastonbury :djg :kamidphish from comment #36)
> @@ +195,5 @@
> > +
> > +    // XXX: Note that the query result *may change* within the same task!
> > +    // This does not follow the specification, which states that all calls
> > +    // checking query results must return the same value until the event loop
> > +    // is empty.
> 
> Why are we out of spec? Can the code be changed to meet the spec?

I consulted with :jrmuizel on this, and we agreed that the spec behaviour (waiting until the event loop is idle before updating the query result) wouldn't be ideal to implement. The 'Issues' section of https://www.khronos.org/registry/webgl/extensions/EXT_disjoint_timer_query explains some of the motivation behind why.

Comment 39

[Andrew Comminos [:acomminos]]

Attachment: Part 2 - Add support for WebGL's EXT_disjoint_timer_query.

Comment 40

[Andrew Comminos [:acomminos]]

Attachment: Part 3 - Add tests for WebGL's EXT_disjoint_timer_query.

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8609411 [details] [diff] [review]
Part 2 - Add support for WebGL's EXT_disjoint_timer_query.

At some point would be really nice to move WebGL to mozilla::dom namespace and then get rid of the stuff in Bindings.conf

Not reviewing  dom/canvas/* but
could you follow mozilla coding style in new files, pretty please.
Otherwise poiru will just go through that code later and fix its coding style
issues ;)
So, 2 spaces for indentation, aFoo naming for arguments, always {} with if, etc.

The spec doesn't have interface WebGLExtensionDisjointTimerQuery.
We try to have our .webidl files as close to the specs as possible.
EXT_disjoint_timer_query is odd name, but that is what the spec has. Why would we have something else?

Comment 42

[Andrew Comminos [:acomminos]]

(In reply to Olli Pettay [:smaug] (review overload. No new review requests before May 24, please) from comment #41)
> Not reviewing  dom/canvas/* but
> could you follow mozilla coding style in new files, pretty please.
> Otherwise poiru will just go through that code later and fix its coding style
> issues ;)
> So, 2 spaces for indentation, aFoo naming for arguments, always {} with if,
> etc.

Understood, thanks! Thought code style was maintained on a per-module basis.

> The spec doesn't have interface WebGLExtensionDisjointTimerQuery.
> We try to have our .webidl files as close to the specs as possible.
> EXT_disjoint_timer_query is odd name, but that is what the spec has. Why
> would we have something else?

For the other WebGL extensions, it seems we drop the prefix (e.g. OES_texture_float -> WebGLExtensionTextureFloat). This change is reflected both in IDL and C++. I'm not sure why this is the case, but it seems to be consistent across all WebGL extensions.

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

I see. Could you at least add a comment to the .webidl indicating what the interface name is in the spec.

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

Hmm, looking how the new thing is used...

What kind of thing is
gl.getExtension('EXT_disjoint_timer_query').__proto__ with your patch?

Comment 45

[Andrew Comminos [:acomminos]]

(In reply to Olli Pettay [:smaug] (review overload. No new review requests before May 24, please) from comment #44)
> Hmm, looking how the new thing is used...
> 
> What kind of thing is
> gl.getExtension('EXT_disjoint_timer_query').__proto__ with your patch?

It's a WebGLExtensionDisjointTimerQueryPrototype.

Comment 46

[Olli Pettay [:smaug][[e-mail]]]

and gl.getExtension('EXT_disjoint_timer_query').toString(); is then
[object WebGLExtensionDisjointTimerQuery], right?

That is wrong per spec. Those should be 
"EXT_disjoint_timer_queryPrototype" and
"[object EXT_disjoint_timer_query]"

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

(sounds like we need to fix also some other interfaces then, but in a different bug)

Comment 48

[u480271]

Comment on attachment 8609411 [details] [diff] [review]
Part 2 - Add support for WebGL's EXT_disjoint_timer_query.

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

::: dom/canvas/WebGLTimerQuery.h
@@ +1,4 @@
> +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Since it's been asked by :smaug that new files follow the Mozilla coding style, please update all new files to 2 space indentation.
The correct file header can be found here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Mode_Line

Comment 49

[Andrew Comminos [:acomminos]]

Attachment: Part 2 - Add support for WebGL's EXT_disjoint_timer_query. Carry r=dglastonbury

Here are the changes to indentation and a comment in WebIDL about interface naming. I think we can address the rest of the style issues in 1168244, and the interface naming in a separate bug.

Comment 50

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8611201 [details] [diff] [review]
Part 2 - Add support for WebGL's EXT_disjoint_timer_query. Carry r=dglastonbury

Ok, fine, but land this around the same time as bug 1168244 so that we don't end up shipping more broken interfaces.
So, r+ in case you don't land this for different release than the fix for bug 1168244.

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

er, wait, I didn't mean bug 1168244. I meant some to-be-filed bug to update the interface names.

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

Filed Bug 1168845. The patch in this bug should land around the time that gets fixed.

Comment 53

[Andrew Comminos [:acomminos]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f5994b801a5

Comment 54

[Andrew Comminos [:acomminos]]

Attachment: Part 3 - Add tests for WebGL's EXT_disjoint_timer_query. Carry r=dglastonbury

Fixed assertion if extension is not supported.

Comment 55

[Andrew Comminos [:acomminos]]

Attachment: Part 3 - Add tests for WebGL's EXT_disjoint_timer_query. Carry r=dglastonbury

Added SimpleTest.finish() in case where extension is unsupported.

Comment 56

[Andrew Comminos [:acomminos]]

Flagging checkin-needed as the WebGL test failure on the try server is almost certainly fixed by the change to part 3 (missing SimpleTest.finish() when extension is not supported).

I can push again to try if necessary, though.

Comment 57

[Carsten Book [:Tomcat]]

Hi,

part 1 failed to apply :

(eg '1-3,5', or 's' to toggle the sort order between id & patch description) 3
adding 974832 to series file
renamed 974832 -> glcontext-timer-queries-feature
applying glcontext-timer-queries-feature
patching file gfx/gl/GLContext.cpp
Hunk #2 FAILED at 91
1 out of 4 hunks FAILED -- saving rejects to file gfx/gl/GLContext.cpp.rej
patching file gfx/gl/GLContext.h
Hunk #3 FAILED at 387
1 out of 6 hunks FAILED -- saving rejects to file gfx/gl/GLContext.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh glcontext-timer-queries-feature

could you take a look, thanks!

Comment 58

[Andrew Comminos [:acomminos]]

Attachment: Part 1 - Implement necessary GL features to provide timer queries. Carry r=dglastonbury

Comment 59

[Andrew Comminos [:acomminos]]

(In reply to Carsten Book [:Tomcat] from comment #57)
> Hi,
> 
> part 1 failed to apply :
> 
> (eg '1-3,5', or 's' to toggle the sort order between id & patch description)
> 3
> adding 974832 to series file
> renamed 974832 -> glcontext-timer-queries-feature
> applying glcontext-timer-queries-feature
> patching file gfx/gl/GLContext.cpp
> Hunk #2 FAILED at 91
> 1 out of 4 hunks FAILED -- saving rejects to file gfx/gl/GLContext.cpp.rej
> patching file gfx/gl/GLContext.h
> Hunk #3 FAILED at 387
> 1 out of 6 hunks FAILED -- saving rejects to file gfx/gl/GLContext.h.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working directory
> errors during apply, please fix and refresh glcontext-timer-queries-feature
> 
> could you take a look, thanks!

The updated patch should apply cleanly now. Thanks!

Comment 60

[Ryan VanderMeulen [:RyanVM]]

Part 1 still doesn't apply.

Comment 61

[Andrew Comminos [:acomminos]]

Attachment: Part 1 - Implement necessary GL features to provide timer queries. Carry r=dglastonbury

Sorry, my git mirror was a bit behind. Conflicts should be resolved.

Comment 62

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/44ebf04352b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/a817e0e2a86b
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fa23f765209

Comment 63

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/44ebf04352b7
https://hg.mozilla.org/mozilla-central/rev/a817e0e2a86b
https://hg.mozilla.org/mozilla-central/rev/3fa23f765209