1259449 - WebGL EXT_disjoint_timer_query uses GetInteger64v without loading the symbol

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

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?

Comment 2

[Kelsey Gilbert [: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.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Attachment: 0001-Require-GLFeature-sync-for-webgl-s-EXT_disjoint_time.patch

Comment 4

[Kelsey Gilbert [: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 5

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 6

[Kelsey Gilbert [:jgilbert]]

(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.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=21b846178034

Comment 8

[Jeff Muizelaar [:jrmuizel]]

(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.

Comment 9

[Kelsey Gilbert [:jgilbert]]

(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 10

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26a75fc6f8c7

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/26a75fc6f8c7