Closed Bug 1011356 Opened 10 years ago Closed 10 years ago

Add support for EXT_timer_query and EXT_disjoint_timer_query

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 974832

People

(Reporter: BenWa, Assigned: BenWa)

Details

Attachments

(2 files, 5 obsolete files)

      No description provided.
I'm not sure how this interact with GLFeature::query_objects. Can you advise?
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #8423587 - Flags: review?(jgilbert)
Remove accidental change
Attachment #8423587 - Attachment is obsolete: true
Attachment #8423587 - Flags: review?(jgilbert)
Attachment #8423588 - Flags: review?(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.
Attachment #8423588 - Flags: review?(jgilbert) → review-
(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.
It looks like these are basically the same extension, though I didn't check very deeply.
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.
Attachment #8423588 - Attachment is obsolete: true
Attachment #8427501 - Flags: review?(jgilbert)
Attached patch Part 1: Expose timer_query (obsolete) — Splinter Review
Fixed datatype
Attachment #8427501 - Attachment is obsolete: true
Attachment #8427501 - Flags: review?(jgilbert)
Attachment #8427502 - Flags: review?(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
Attachment #8427502 - Flags: review?(jgilbert) → review+
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.
Attached patch Part 1: Expose timer_query (obsolete) — Splinter Review
Attachment #8427502 - Attachment is obsolete: true
Attachment #8428185 - Flags: review+
Attachment #8428185 - Attachment is obsolete: true
Attachment #8428192 - Flags: review+
Attached patch patchSplinter Review
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.
Attachment #8428208 - Flags: feedback?(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.
Attachment #8428208 - Flags: feedback?(jgilbert) → feedback+
Bug 974832 is further along. Too bad we didn't dupe this earlier and save duplicate work.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
No longer blocks: 737967
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: