Only make timer queries' results available when control returns to browser.

Only make timer queries' results available when control returns to browser.

This implements the specified semantics for WebGL's
EXT_disjoint_timer_query extension. Once this is reviewed, the same
semantics will be implemented for the WebGL 2.0 context's query objects.

Tested with conformance/extensions/ext-disjoint-timer-query.html from
https://github.com/KhronosGroup/WebGL (with the command line argument
--enable-webgl-draft-extensions), which has been revised to test this
behavior. Test is available at
https://www.khronos.org/registry/webgl/sdk/tests/conformance/extensions/ext-disjoint-timer-query.html .

BUG=345227
Committed: https://crrev.com/e99ac4a274307be415e397b5f3a8370bb6069f1d
Cr-Commit-Position: refs/heads/master@{#352506}

[Ken Russell (switch to Gerrit), 2015-10-03 05:26:11]

kbr@chromium.org changed reviewers:
+ bajones@chromium.org, dyen@chromium.org, zmo@chromium.org

[Ken Russell (switch to Gerrit), 2015-10-03 05:26:11]

Please review. Thanks.

[bajones, 2015-10-04 04:34:39]

Very nice update, thanks for doing the research to figure out the observer
mechanism! (I'll need to do something similar for WebVR, so you just saved me
some time.)

LGTM!

https://codereview.chromium.org/1385793002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/EXTDisjointTimerQuery.h (left):

https://codereview.chromium.org/1385793002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/EXTDisjointTimerQuery.h:43: void
OnQueryDeleted(GLuint);
Hm, odd that this didn't have an implementation. Nice catch.

[David Yen, 2015-10-05 17:23:16]

https://codereview.chromium.org/1385793002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/EXTDisjointTimerQuery.cpp (right):

https://codereview.chromium.org/1385793002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/EXTDisjointTimerQuery.cpp:100:
m_currentElapsedQuery = query;
We also need to reset the cache results here so we don't return stale query
data.

https://codereview.chromium.org/1385793002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLTimerQueryEXT.cpp (right):

https://codereview.chromium.org/1385793002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLTimerQueryEXT.cpp:39: void
WebGLTimerQueryEXT::resetCachedResult()
We also need to reset the cache results when beginning a query but not register
the task observer, perhaps we should make register task observer function
public? We can rename the function to something more API friendly such as
"BeginTrackingQuery" or something.

[Ken Russell (switch to Gerrit), 2015-10-05 23:17:59]

https://codereview.chromium.org/1385793002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/EXTDisjointTimerQuery.cpp (right):

https://codereview.chromium.org/1385793002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/EXTDisjointTimerQuery.cpp:100:
m_currentElapsedQuery = query;
On 2015/10/05 17:23:16, David Yen wrote:
> We also need to reset the cache results here so we don't return stale query
> data.

I'm not sure about that. The ES 3.0 spec says about EndQuery: "The active query
object for |target| is updated to indicate that query results are not available,
...". This implies to me that any previous results can still be fetched even
after BeginQuery is called.

[David Yen, 2015-10-05 23:36:30]

https://codereview.chromium.org/1385793002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/EXTDisjointTimerQuery.cpp (right):

https://codereview.chromium.org/1385793002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/EXTDisjointTimerQuery.cpp:100:
m_currentElapsedQuery = query;
On 2015/10/05 23:17:59, Ken Russell wrote:
> On 2015/10/05 17:23:16, David Yen wrote:
> > We also need to reset the cache results here so we don't return stale query
> > data.
> 
> I'm not sure about that. The ES 3.0 spec says about EndQuery: "The active
query
> object for |target| is updated to indicate that query results are not
available,
> ...". This implies to me that any previous results can still be fetched even
> after BeginQuery is called.

I think you are right about when the query result should be marked as
unavailable. However looking at the spec there is another caveat here:

The error INVALID_OPERATION is generated if GetQueryObjectuivEXT is called where
<id> is the name of a currently active query object.

So the query result cannot actually be queried between a begin and end query
call. That is probably my fault when I first implemented this. We need to add a
check for "if (m_currentElapsedQuery)" in getQueryEXT() and return
GL_INVALID_OPERATION.

https://codereview.chromium.org/1385793002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/EXTDisjointTimerQuery.cpp:182: if
(!query || query->isDeleted() || !query->validate(0, scoped.context())) {
It looks like we can just add the m_currentElapsedQuery check in here.

[Ken Russell (switch to Gerrit), 2015-10-06 00:08:17]

https://codereview.chromium.org/1385793002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/EXTDisjointTimerQuery.cpp (right):

https://codereview.chromium.org/1385793002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/EXTDisjointTimerQuery.cpp:182: if
(!query || query->isDeleted() || !query->validate(0, scoped.context())) {
On 2015/10/05 23:36:30, David Yen wrote:
> It looks like we can just add the m_currentElapsedQuery check in here.

Thanks. Fixed in the current patch set. Tested with
https://github.com/KhronosGroup/WebGL/pull/1255 .

[David Yen, 2015-10-06 00:10:18]

lgtm

[Ken Russell (switch to Gerrit), 2015-10-06 00:15:15]

The CQ bit was checked by kbr@chromium.org

[Ken Russell (switch to Gerrit), 2015-10-06 00:15:15]

The patchset sent to the CQ was uploaded after l-g-t-m from bajones@chromium.org
Link to the patchset: https://codereview.chromium.org/1385793002/#ps20001
(title: "Addressed review feedback from dyen.")

[commit-bot: I haz the power, 2015-10-06 00:17:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1385793002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1385793002/20001

[commit-bot: I haz the power, 2015-10-06 01:34:54]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-06 01:35:57]

Patchset 2 (id:??) landed as
https://crrev.com/e99ac4a274307be415e397b5f3a8370bb6069f1d
Cr-Commit-Position: refs/heads/master@{#352506}