1260599 - Re-enable EXT_disjoint_timer_query on ANGLE

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

It was disabled in bug 1259449 as part of fixing a crash.
We should be able to expose it, but fixing the crash came first.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Can you take a look at this, :ethlin?

Comment 2

[Ethan Lin[:ethlin]]

(In reply to Jeff Gilbert [:jgilbert] from comment #1)
> Can you take a look at this, :ethlin?

Okay, I will take a look at this.

Comment 3

[Ethan Lin[:ethlin]]

Attachment: re-enable XT_disjoint_timer_query

I check the GLFeature::sync to use GetInteger64v or GetIntegerv. There will be truncation problem for GetIntegerv.
Another way is to add flag gl::CreateContextFlags::PREFER_ES3 for webgl1. I tried this and almost all conformance tests in gecko passed. I'm not sure if it's a good way to fix this.

Comment 4

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8739346 [details] [diff] [review]
re-enable XT_disjoint_timer_query

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

There's an outside chance that we can just unconditionally use glGetInteger64v on ANGLE on ES2, even though it's not clear from the extensions it lists that it should support it. Please investigate this first.

Failing that, we want to actually request ES3, as long as it falls back to ES2 gracefully.

Comment 5

[Ethan Lin[:ethlin]]

(In reply to Jeff Gilbert [:jgilbert] from comment #4)
> Comment on attachment 8739346 [details] [diff] [review]
> re-enable XT_disjoint_timer_query
> 
> Review of attachment 8739346 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There's an outside chance that we can just unconditionally use
> glGetInteger64v on ANGLE on ES2, even though it's not clear from the
> extensions it lists that it should support it. Please investigate this first.
> 
> Failing that, we want to actually request ES3, as long as it falls back to
> ES2 gracefully.

I see. I will investigate how the ANGLE handle the glGetInteger64v on ES2.

Comment 6

[Ethan Lin[:ethlin]]

Attachment: re-enable XT_disjoint_timer_query

ANGLE unconditionally exposes glGetInteger64v but it will check version inside and set a INVALID_OPERATION for the version under 3.0[1]. So we could try to use ES3 for webgl1, and check the GLFeature::sync in glGetParameter(GL_TIMESTAMP_EXT).

[1] https://hg.mozilla.org/mozilla-central/annotate/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/gfx/angle/src/libGLESv2/entry_points_gles_3_0.cpp#l2187

Comment 7

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8741308 [details] [diff] [review]
re-enable XT_disjoint_timer_query

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

::: dom/canvas/WebGLContext.cpp
@@ +678,5 @@
>  
>      gl::CreateContextFlags flags = gl::CreateContextFlags::NONE;
>      if (forceEnabled) flags |= gl::CreateContextFlags::FORCE_ENABLE_HARDWARE;
>      if (!IsWebGL2())  flags |= gl::CreateContextFlags::REQUIRE_COMPAT_PROFILE;
> +    flags |= gl::CreateContextFlags::PREFER_ES3;

We should do this in a separate bug.
This bug would be fixed as a side-effect of that bug.
Enabling ES3 by default a big enough step that we should call it out specifically.
This might mean that this bug needs no patch other than depending on the use-ES3 bug.

::: dom/canvas/WebGLContextState.cpp
@@ +283,5 @@
> +              // cast to double (53 bits)
> +              return JS::NumberValue(static_cast<double>(iv));
> +            } else {
> +              GLint iv = 0;
> +              gl->fGetIntegerv(pname, &iv);

I don't think we want this. It's not a proper implementation of the extension.

::: dom/canvas/WebGLExtensionDisjointTimerQuery.cpp
@@ +137,5 @@
>                                     " TIMESTAMP_EXT.", target);
>      return;
>    }
>  
> +  if (query->Target() != target) {

Does this work? It seems like it'll never get below to where query->mTarget is set.
Since our only target is TIMESTAMP, we shouldn't actually need this check.

Comment 8

[Ethan Lin[:ethlin]]

Attachment: re-enable XT_disjoint_timer_query

Move the check of GLFeature::sync to GetParamter from WebGLExtensionDisjointTimerQuery::IsSupported. And make GetParamter(GL_TIMESTAMP_EXT) return zero if not supported.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8763782 [details] [diff] [review]
re-enable XT_disjoint_timer_query

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

Almost there, just need to fix the other entrypoints.

::: dom/canvas/WebGLContextState.cpp
@@ +268,5 @@
>              GLuint64 iv = 0;
> +            if (gl->IsSupported(gl::GLFeature::sync)) {
> +                gl->fGetInteger64v(pname, (GLint64*)&iv);
> +            } else {
> +                GenerateWarning("GetParameter(GL_TIMESTAMP_EXT) doesn't work.");

"QUERY_COUNTER_BITS_EXT for TIMESTAMP_EXT is 0".

Also you need to ensure that zero is returned for `getQueryEXT()`.

I think the best way to do this is to add a `bool WebGLContext::mHasTimestampBits`, and piggy-back all these branches off of this var.

Comment 10

[Ethan Lin[:ethlin]]

Attachment: re-enable XT_disjoint_timer_query

Address jgilbert's comments.

Comment 11

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8765785 [details] [diff] [review]
re-enable XT_disjoint_timer_query

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

Sorry for the delay.

::: dom/canvas/WebGLContext.h
@@ +1511,5 @@
>      bool mNeedsFakeNoDepth;
>      bool mNeedsFakeNoStencil;
>      bool mNeedsEmulatedLoneDepthStencil;
>  
> +    bool mHasTimestampBits;

bool HasTimestampBits() const { return gl->IsSupported(gl::GLFeature::sync); }

::: dom/canvas/WebGLContextState.cpp
@@ +268,5 @@
>              GLuint64 iv = 0;
> +            if (mHasTimestampBits) {
> +                gl->fGetInteger64v(pname, (GLint64*)&iv);
> +            } else {
> +                GenerateWarning("QUERY_COUNTER_BITS_EXT for TIMESTAMP_EXT is 0.");

Warning here is great!

::: dom/canvas/WebGLExtensionDisjointTimerQuery.cpp
@@ +159,5 @@
>        mContext->ErrorInvalidEnumInfo("getQueryEXT: Invalid query target.",
>                                       target);
>        return;
>      }
> +    if (mActiveQuery && mContext->mHasTimestampBits) {

I don't think this conditional should be changed.

@@ +179,5 @@
>      GLint bits = 0;
> +    if (mContext->mHasTimestampBits) {
> +      mContext->GL()->fGetQueryiv(target, pname, &bits);
> +    } else {
> +      mContext->GenerateWarning("QUERY_COUNTER_BITS_EXT for TIMESTAMP_EXT/TIME_ELAPSED_EXT is 0.");

Don't warn here.

Comment 12

[Ethan Lin[:ethlin]]

Attachment: re-enable XT_disjoint_timer_query

Address jgilbert's comments.

Comment 13

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8770798 [details] [diff] [review]
re-enable XT_disjoint_timer_query

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

::: dom/canvas/WebGLContext.cpp
@@ +1607,5 @@
>          ErrorInvalidFramebufferOperation("%s: %s.", funcName, errorText.BeginReading());
>      }
>  }
>  
> +bool WebGLContext::HasTimestampBits() const

bool
WebGLContext::...

@@ +1609,5 @@
>  }
>  
> +bool WebGLContext::HasTimestampBits() const
> +{
> +  return gl->IsSupported(GLFeature::sync);

4-space

::: dom/canvas/WebGLExtensionDisjointTimerQuery.cpp
@@ -243,5 @@
>    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
> -  // 'sync' provides glGetInteger64v either by supporting ARB_sync, GL3+, or GLES3+.

Include this comment in the definition for HasTimestampBits, so it's clear why we ask about GLFeature::sync when we're really wanting to know if we can use timestamps.

Comment 14

[Ethan Lin[:ethlin]]

Attachment: re-enable XT_disjoint_timer_query (carry r+: jgilbert)

Address jgilbert's comment.

Comment 15

[Ethan Lin[:ethlin]]

Attachment: re-enable XT_disjoint_timer_query (carry r+: jgilbert)

It was a wrong patch. Correct it.

Comment 16

[Ethan Lin[:ethlin]]

Attachment: change test status

Change test status. Enable the extension check and disable the the extension test since there are still some problems.

Comment 17

[Ethan Lin[:ethlin]]

try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3aa132525a48&selectedJob=24497947

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d50028181044
Enable EXT_disjoint_timer_query extension on ANGLE. r=jgilbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e066314768c
Change EXT_disjoint_timer_query mochitest status. r=mtseng

Comment 19

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

I had to back this out because mochitest(gl) started permafailing on WinXP after this landed: https://treeherder.mozilla.org/logviewer.html#?job_id=32808550&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3eb53d8f4be

Comment 20

[Ethan Lin[:ethlin]]

(In reply to Wes Kocher (:KWierso) from comment #19)
> I had to back this out because mochitest(gl) started permafailing on WinXP
> after this landed:
> https://treeherder.mozilla.org/logviewer.html#?job_id=32808550&repo=mozilla-
> inbound
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b3eb53d8f4be
I will remove WinXP from the test.

Comment 21

[Ethan Lin[:ethlin]]

Attachment: change test status (carry r+: mtseng)

Change xp platform status.

Comment 22

[Ethan Lin[:ethlin]]

try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cff977273cb7&filter-searchStr=gl

Comment 23

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8776324 [details] [diff] [review]
change test status (carry r+: mtseng)

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

::: dom/canvas/test/webgl-mochitest/mochitest.ini
@@ +83,5 @@
>  [test_webgl_conformance.html]
>  skip-if = toolkit == 'android' #bug 865443- seperate suite - the non_conf* tests pass except for one on armv6 tests
>  [test_webgl_compressed_texture_es3.html]
>  [test_webgl_disjoint_timer_query.html]
> +fail-if = (os == 'win' && os_version != '5.1')

Mark the failing version, not which versions work.

Comment 24

[Ethan Lin[:ethlin]]

(In reply to Jeff Gilbert [:jgilbert] from comment #23)
> Comment on attachment 8776324 [details] [diff] [review]
> change test status (carry r+: mtseng)
> 
> Review of attachment 8776324 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/test/webgl-mochitest/mochitest.ini
> @@ +83,5 @@
> >  [test_webgl_conformance.html]
> >  skip-if = toolkit == 'android' #bug 865443- seperate suite - the non_conf* tests pass except for one on armv6 tests
> >  [test_webgl_compressed_texture_es3.html]
> >  [test_webgl_disjoint_timer_query.html]
> > +fail-if = (os == 'win' && os_version != '5.1')
> 
> Mark the failing version, not which versions work.

okay.

Comment 25

[Ethan Lin[:ethlin]]

Attachment: change test status (carry r+: mtseng)

Change the windows version to failing version.

Comment 26

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8776797 [details] [diff] [review]
change test status (carry r+: mtseng)

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

::: dom/canvas/test/webgl-mochitest/mochitest.ini
@@ +83,5 @@
>  [test_webgl_conformance.html]
>  skip-if = toolkit == 'android' #bug 865443- seperate suite - the non_conf* tests pass except for one on armv6 tests
>  [test_webgl_compressed_texture_es3.html]
>  [test_webgl_disjoint_timer_query.html]
> +fail-if = (os == 'win' && os_version == '6.1') || (os == 'win' && os_version == '6.2')

(os == 'win' && (os_version == '6.1' || os_version == '6.2')) is fine.

Comment 27

[Ethan Lin[:ethlin]]

Attachment: change test status (carry r+: mtseng, jgilbert)

Change the platform.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95b75b253cda
Enable EXT_disjoint_timer_query extension on ANGLE. r=jgilbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/8aa5e7804532
Change EXT_disjoint_timer_query mochitest status. r=mtseng

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/95b75b253cda
https://hg.mozilla.org/mozilla-central/rev/8aa5e7804532

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Attachment: change the fail-if condition for any Windows that isn't XP

I ran into failures for this on Try when running tests on Windows 10. Rather than adding to the already-long version string, let's change the condition so that only XP is expected to pass and all other versions of Windows fail.

Try push confirms that it works as expected:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=522ebd0cbed0&group_state=expanded

Of course, I'm now realizing that there's a whole pile of webgl-conf (os == 'win' && os_version == '6.2') annotations that are likely to also be problematic on Win10 as shown on the above Try push. Most of those can likely be resolved by changing |os_version == '6.2'| to |bits == 64| instead. Guess I'll spin that off into a new bug.

Comment 31

[Ethan Lin[:ethlin]]

Comment on attachment 8783364 [details] [diff] [review]
change the fail-if condition for any Windows that isn't XP

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

::: dom/canvas/test/webgl-mochitest/mochitest.ini
@@ +83,5 @@
>  [test_webgl_conformance.html]
>  skip-if = toolkit == 'android' #bug 865443- seperate suite - the non_conf* tests pass except for one on armv6 tests
>  [test_webgl_compressed_texture_es3.html]
>  [test_webgl_disjoint_timer_query.html]
> +fail-if = (os == 'win' && (os_version != '5.1'))

Per comment 23, we should mark the failing version, not which versions work. So maybe we should add 10.0 here.