1011356 - Add support for EXT_timer_query and EXT_disjoint_timer_query

Status: RESOLVED DUPLICATE of bug 974832

(Core :: Graphics, defect)

Comment 1

[Benoit Girard (:BenWa)]

Attachment: Part 1: Expose GL_EXT_timer_query

I'm not sure how this interact with GLFeature::query_objects. Can you advise?

Comment 2

[Benoit Girard (:BenWa)]

Attachment: Part 1: Expose GL_EXT_timer_query

Remove accidental change

Comment 3

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8423588 [details] [diff] [review]
Part 1: Expose GL_EXT_timer_query

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

This is the right direction, but you need to add entrypoints to GLContext, so we can actually call this.

::: gfx/gl/GLContextSymbols.h
@@ +137,5 @@
>      PFNGLGETQUERYOBJECTIVPROC fGetQueryObjectiv;
>      typedef void (GLAPIENTRY * PFNGLGETQUERYOBJECTUIVPROC) (GLuint id, GLenum pname, GLuint* params);
>      PFNGLGETQUERYOBJECTUIVPROC fGetQueryObjectuiv;
> +    typedef void (GLAPIENTRY * PFNGLGETQUERYOBJECTI64VEXTPROC) (GLuint id, GLenum pname, GLint* params);
> +    PFNGLGETQUERYOBJECTIVPROC fGetQueryObjecti64vEXT;

Remove the EXT bit, unless there's a reason we should keep it separate from the inevitable non-EXT version.

Comment 4

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Girard (:BenWa) from comment #1)
> Created attachment 8423587 [details] [diff] [review]
> Part 1: Expose GL_EXT_timer_query
> 
> I'm not sure how this interact with GLFeature::query_objects. Can you advise?

query_objects is actually for occlusion queries, and we should probably rename it to match.
We should make a new Feature here, which includes both the extensions in this bug, as long as there is functionality overlap between the two.

Comment 5

[Kelsey Gilbert [:jgilbert]]

It looks like these are basically the same extension, though I didn't check very deeply.

Comment 6

[Kelsey Gilbert [:jgilbert]]

From the GLES extension:

    (14) How does this extension differ from ARB_timer_query?

    This extension contains most ARB_timer_query behavior unchanged as well
    as adds the ability to detect GPU issues using GPU_DISJOINT_EXT.

So we should call the feature timer_query.

Comment 7

[Benoit Girard (:BenWa)]

Attachment: Part 1: Expose GL_EXT_timer_query

Comment 8

[Benoit Girard (:BenWa)]

Attachment: Part 1: Expose timer_query

Fixed datatype

Comment 9

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8427502 [details] [diff] [review]
Part 1: Expose timer_query

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

::: gfx/gl/GLContextFeatures.cpp
@@ +404,5 @@
>          }
> +    },
> +    {
> +        "timer_query",
> +        0, // OpenGL version

Core as of GL 330

Comment 10

[Benoit Girard (:BenWa)]

Thanks. I'm going to land this as a first pass but I expect more follows up will be needed to adjust to disjoint query and that mac doesn't support GL_TIMESTAMP.

Comment 11

[Benoit Girard (:BenWa)]

Attachment: Part 1: Expose timer_query

Comment 12

[Benoit Girard (:BenWa)]

Attachment: Part 1: Expose timer_query

Comment 13

[Benoit Girard (:BenWa)]

Attachment: patch

Needed to get hacky. Even upstream libGLESv2.so doesn't support these functions call so its going to take awhile for them to get wrapped.

Comment 14

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8428208 [details] [diff] [review]
patch

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

This looks OK, but I don't think you should do this on non-adreno devices.

::: gfx/gl/GLContext.cpp
@@ +825,5 @@
> +                // The extension was reported but it's not exposes from libGLESv2.so so lets
> +                // try harder to load it directly.
> +                PRLibSpec lspec;
> +                lspec.type = PR_LibSpec_Pathname;
> +                lspec.value.pathname = "libGLESv2_adreno.so";

Sounds like this should only happen for Adrenos, yes?

@@ +832,5 @@
> +                mSymbols.fQueryCounter = (mozilla::gl::GLContextSymbols::PFNGLQUERYCOUNTERPROC)PR_FindFunctionSymbol(library, "glQueryCounterEXT");
> +                mSymbols.fGetQueryObjecti64v = (mozilla::gl::GLContextSymbols::PFNGLGETQUERYOBJECTI64VPROC)PR_FindFunctionSymbol(library, "glGetQueryObjecti64vEXT");
> +                mSymbols.fGetQueryObjectui64v = (mozilla::gl::GLContextSymbols::PFNGLGETQUERYOBJECTUI64VPROC)PR_FindFunctionSymbol(library, "glGetQueryObjectui64vEXT");
> +#endif
> +                if (!mSymbols.fQueryCounter || !mSymbols.fGetQueryObjecti64v || !mSymbols.fGetQueryObjectui64v) {

Long line.

@@ +833,5 @@
> +                mSymbols.fGetQueryObjecti64v = (mozilla::gl::GLContextSymbols::PFNGLGETQUERYOBJECTI64VPROC)PR_FindFunctionSymbol(library, "glGetQueryObjecti64vEXT");
> +                mSymbols.fGetQueryObjectui64v = (mozilla::gl::GLContextSymbols::PFNGLGETQUERYOBJECTUI64VPROC)PR_FindFunctionSymbol(library, "glGetQueryObjectui64vEXT");
> +#endif
> +                if (!mSymbols.fQueryCounter || !mSymbols.fGetQueryObjecti64v || !mSymbols.fGetQueryObjectui64v) {
> +                    MarkUnsupported(GLFeature::timer_query);

Warn here.

Comment 15

[Benoit Girard (:BenWa)]

Bug 974832 is further along. Too bad we didn't dupe this earlier and save duplicate work.