Closed Bug 1260599 Opened 4 years ago Closed 4 years ago

Re-enable EXT_disjoint_timer_query on ANGLE

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: jgilbert, Assigned: ethlin)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 9 obsolete files)

5.23 KB, patch
Details | Diff | Splinter Review
2.43 KB, patch
Details | Diff | Splinter Review
1.53 KB, patch
Details | Diff | Splinter Review
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.
Can you take a look at this, :ethlin?
Flags: needinfo?(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.
Assignee: nobody → ethlin
Flags: needinfo?(ethlin)
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.
Attachment #8739346 - Flags: feedback?(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.
Attachment #8739346 - Flags: feedback?(jgilbert) → feedback-
(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.
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
Attachment #8739346 - Attachment is obsolete: true
Attachment #8741308 - Flags: review?(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.
Attachment #8741308 - Flags: review?(jgilbert) → review-
Depends on: 1265668
Move the check of GLFeature::sync to GetParamter from WebGLExtensionDisjointTimerQuery::IsSupported. And make GetParamter(GL_TIMESTAMP_EXT) return zero if not supported.
Attachment #8741308 - Attachment is obsolete: true
Attachment #8763782 - Flags: review?(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.
Attachment #8763782 - Flags: review?(jgilbert) → review-
Address jgilbert's comments.
Attachment #8763782 - Attachment is obsolete: true
Attachment #8765785 - Flags: review?(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.
Attachment #8765785 - Flags: review?(jgilbert) → review-
Address jgilbert's comments.
Attachment #8765785 - Attachment is obsolete: true
Attachment #8770798 - Flags: review?(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.
Attachment #8770798 - Flags: review?(jgilbert) → review+
Address jgilbert's comment.
Attachment #8770798 - Attachment is obsolete: true
It was a wrong patch. Correct it.
Attachment #8770871 - Attachment is obsolete: true
Attached patch change test status (obsolete) — Splinter Review
Change test status. Enable the extension check and disable the the extension test since there are still some problems.
Attachment #8775051 - Flags: review?(mtseng)
Attachment #8775051 - Flags: review?(mtseng) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
(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.
Flags: needinfo?(ethlin)
Change xp platform status.
Attachment #8775051 - Attachment is obsolete: true
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.
(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.
Change the windows version to failing version.
Attachment #8776324 - Attachment is obsolete: true
Attachment #8776797 - Flags: review?(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.
Attachment #8776797 - Flags: review?(jgilbert) → review+
Change the platform.
Attachment #8776797 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/95b75b253cda
https://hg.mozilla.org/mozilla-central/rev/8aa5e7804532
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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.
Attachment #8783364 - Flags: review?(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.
Attachment #8783364 - Flags: review?(ethlin)
You need to log in before you can comment on or make changes to this bug.