Closed Bug 1259449 Opened 4 years ago Closed 4 years ago

WebGL EXT_disjoint_timer_query uses GetInteger64v without loading the symbol

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jrmuizel, Assigned: jgilbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

The ES extension https://www.khronos.org/registry/gles/extensions/EXT/EXT_disjoint_timer_query.txt does not add but GetInteger64v also does not say where it comes from. This seems to imply that the existence of the function is optional.
Currently, we crash in Nightly on ANGLE here: https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/dom/canvas/WebGLContextState.cpp#280

ANGLE does have GetInteger64v but we don't load the symbol.

jgilbert, What do you think we should do?
Flags: needinfo?(jgilbert)
ANGLE supports GetInteger64v iff it's GLES3.

We load glGetInteger64v iff we have GLFeature::sync. (GL3.2+, ES3+, ARB/APPLE_sync extension)

EXT_disjoint_timer_query merely says if you happen to have GetInteger64v, you can ask it about TIMESTAMP.

Since we merge GetInteger and GetInteger64 into getParameter in WebGL, we should choose the best available.
Assignee: nobody → jgilbert
Flags: needinfo?(jgilbert)
I checked the WebGL extension spec, and it dictates that getParameter(TIMESTAMP) returns GLuint64EXT, so we should require GetInteger64v. (but I do not see directly if this is actually mandated)

We should ensure that the upstream conformance tests enforce this behavior, or explicitly call out this allowance (for u32 truncation) in the spec.
Comment on attachment 8735043 [details] [diff] [review]
0001-Require-GLFeature-sync-for-webgl-s-EXT_disjoint_time.patch

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

I believe this will disable timer queries on ANGLE as it doesn't support the 'sync' extension. That doesn't seem desirable.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> Comment on attachment 8735043 [details] [diff] [review]
> 0001-Require-GLFeature-sync-for-webgl-s-EXT_disjoint_time.patch
> 
> Review of attachment 8735043 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I believe this will disable timer queries on ANGLE as it doesn't support the
> 'sync' extension. That doesn't seem desirable.

Did you check that it works on ANGLE? ANGLE seems to INVALID_OP in glGetInteger64v on GLES <3.0.
Flags: needinfo?(jmuizelaar)
(In reply to Jeff Gilbert [:jgilbert] from comment #7)
> Try run:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=21b846178034

Yeah, this disables the extension on ANGLE. Chrome is managing to have the extension work we should try to as well.
Flags: needinfo?(jmuizelaar)
(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> (In reply to Jeff Gilbert [:jgilbert] from comment #7)
> > Try run:
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=21b846178034
> 
> Yeah, this disables the extension on ANGLE. Chrome is managing to have the
> extension work we should try to as well.

This is a crash right now. Let's make it first not crash, then fix the extension.

I'm pretty sure Chrome must be using ES3 where available in order to maintain support of this extension.
Comment on attachment 8735043 [details] [diff] [review]
0001-Require-GLFeature-sync-for-webgl-s-EXT_disjoint_time.patch

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

::: dom/canvas/WebGLExtensionDisjointTimerQuery.cpp
@@ +242,5 @@
>    gl::GLContext* gl = webgl->GL();
>    return gl->IsSupported(gl::GLFeature::query_objects) &&
>           gl->IsSupported(gl::GLFeature::get_query_object_i64v) &&
> +         gl->IsSupported(gl::GLFeature::query_counter) && // provides GL_TIMESTAMP
> +         gl->IsSupported(gl::GLFeature::sync); // provides glGetInteger64v

Can you elaborate this comment to something about it not actually requiring sync but this being a proxy for having glGetInteger64v which we currently depend on because of some missing clarity in the webgl spec.

Also, make sure you fix the test failures from the try job.
Attachment #8735043 - Flags: review?(jmuizelaar) → review+
Blocks: 1260599
https://hg.mozilla.org/mozilla-central/rev/26a75fc6f8c7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.