Added support for TimeStamp queries using QueryCounterEXT.

Added support for TimeStamp queries using QueryCounterEXT.

BUG=345227
TEST=trybots

Committed: https://crrev.com/da06bae00bee789d12cd6f4418018d048b04cfd2
Cr-Commit-Position: refs/heads/master@{#339153}

[David Yen, 2015-07-15 23:17:07]

dyen@chromium.org changed reviewers:
+ piman@chromium.org

[piman, 2015-07-16 01:05:53]

https://codereview.chromium.org/1233233002/diff/60001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/1233233002/diff/60001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:4947: // Check capabilities
nit: remove superfluous comment

https://codereview.chromium.org/1233233002/diff/60001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:4963: // id = 0 INV_OP
nit: remove superfluous comment

https://codereview.chromium.org/1233233002/diff/60001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:4969: // if not GENned
INV_OPERATION
nit: remove superfluous comment

https://codereview.chromium.org/1233233002/diff/60001/gpu/command_buffer/clie...
File gpu/command_buffer/client/query_tracker.cc (right):

https://codereview.chromium.org/1233233002/diff/60001/gpu/command_buffer/clie...
gpu/command_buffer/client/query_tracker.cc:357:
current_queries_[query->target()] = query;
According to my reading of the spec, the query never becomes current.

https://codereview.chromium.org/1233233002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1233233002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11718: // TODO(dyen): Also
support GL_TIMESTAMP.
nit: wrong comment?

https://codereview.chromium.org/1233233002/diff/60001/ui/gl/gpu_timing.cc
File ui/gl/gpu_timing.cc (right):

https://codereview.chromium.org/1233233002/diff/60001/ui/gl/gpu_timing.cc#new...
ui/gl/gpu_timing.cc:480: DCHECK(!end_requested_);
We can QueryCounter multiple times on the same query, so I don't think this is
valid.

https://codereview.chromium.org/1233233002/diff/60001/ui/gl/gpu_timing.cc#new...
ui/gl/gpu_timing.cc:482: DCHECK(!time_stamp_result_.get() &&
!elapsed_timer_result_.get());
same here.

https://codereview.chromium.org/1233233002/diff/60001/ui/gl/gpu_timing.cc#new...
ui/gl/gpu_timing.cc:488: DCHECK(!end_requested_);
Is this valid? It looks like for the GL_TIME_ELAPSED queries, we call Start in
BeginBegin and End in EndQuery, so we would hit this the second time?

https://codereview.chromium.org/1233233002/diff/60001/ui/gl/gpu_timing.cc#new...
ui/gl/gpu_timing.cc:490: DCHECK(!time_stamp_result_.get() &&
!elapsed_timer_result_.get());
Same here wrt !elapsed_timer_result_.get()

https://codereview.chromium.org/1233233002/diff/60001/ui/gl/gpu_timing.cc#new...
ui/gl/gpu_timing.cc:516: end_available_ = true;
nit: indent looks wrong

[David Yen, 2015-07-16 20:21:12]

https://codereview.chromium.org/1233233002/diff/60001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/1233233002/diff/60001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:4947: // Check capabilities
On 2015/07/16 01:05:52, piman (Very slow to review) wrote:
> nit: remove superfluous comment

Done.

https://codereview.chromium.org/1233233002/diff/60001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:4963: // id = 0 INV_OP
On 2015/07/16 01:05:52, piman (Very slow to review) wrote:
> nit: remove superfluous comment

Done.

https://codereview.chromium.org/1233233002/diff/60001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:4969: // if not GENned
INV_OPERATION
On 2015/07/16 01:05:52, piman (Very slow to review) wrote:
> nit: remove superfluous comment

Done.

https://codereview.chromium.org/1233233002/diff/60001/gpu/command_buffer/clie...
File gpu/command_buffer/client/query_tracker.cc (right):

https://codereview.chromium.org/1233233002/diff/60001/gpu/command_buffer/clie...
gpu/command_buffer/client/query_tracker.cc:357:
current_queries_[query->target()] = query;
On 2015/07/16 01:05:52, piman (Very slow to review) wrote:
> According to my reading of the spec, the query never becomes current.

Hmm, I will remove it then. Done.

https://codereview.chromium.org/1233233002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1233233002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11718: // TODO(dyen): Also
support GL_TIMESTAMP.
On 2015/07/16 01:05:53, piman (Very slow to review) wrote:
> nit: wrong comment?

Done.

https://codereview.chromium.org/1233233002/diff/60001/ui/gl/gpu_timing.cc
File ui/gl/gpu_timing.cc (right):

https://codereview.chromium.org/1233233002/diff/60001/ui/gl/gpu_timing.cc#new...
ui/gl/gpu_timing.cc:480: DCHECK(!end_requested_);
On 2015/07/16 01:05:53, piman (Very slow to review) wrote:
> We can QueryCounter multiple times on the same query, so I don't think this is
> valid.

Ah yes, GPUTimer has originally always been one use only (For the GPUTracer and
other performance timers). I have restructured it so they can be reused multiple
times and also added unit tests to test for this (verifying it was failing
before).

https://codereview.chromium.org/1233233002/diff/60001/ui/gl/gpu_timing.cc#new...
ui/gl/gpu_timing.cc:482: DCHECK(!time_stamp_result_.get() &&
!elapsed_timer_result_.get());
On 2015/07/16 01:05:53, piman (Very slow to review) wrote:
> same here.

Done.

https://codereview.chromium.org/1233233002/diff/60001/ui/gl/gpu_timing.cc#new...
ui/gl/gpu_timing.cc:488: DCHECK(!end_requested_);
On 2015/07/16 01:05:53, piman (Very slow to review) wrote:
> Is this valid? It looks like for the GL_TIME_ELAPSED queries, we call Start in
> BeginBegin and End in EndQuery, so we would hit this the second time?

Done.

https://codereview.chromium.org/1233233002/diff/60001/ui/gl/gpu_timing.cc#new...
ui/gl/gpu_timing.cc:490: DCHECK(!time_stamp_result_.get() &&
!elapsed_timer_result_.get());
On 2015/07/16 01:05:53, piman (Very slow to review) wrote:
> Same here wrt !elapsed_timer_result_.get()

Done.

https://codereview.chromium.org/1233233002/diff/60001/ui/gl/gpu_timing.cc#new...
ui/gl/gpu_timing.cc:516: end_available_ = true;
On 2015/07/16 01:05:53, piman (Very slow to review) wrote:
> nit: indent looks wrong

No longer relevant as this section has been rewritten.

[piman, 2015-07-16 20:42:26]

lgtm

https://codereview.chromium.org/1233233002/diff/80001/ui/gl/gpu_timing.cc
File ui/gl/gpu_timing.cc (right):

https://codereview.chromium.org/1233233002/diff/80001/ui/gl/gpu_timing.cc#new...
ui/gl/gpu_timing.cc:482: void GPUTimer::Reset() {
I think you can add a DCHECK that we're not in WaitingForEnd state (to keep some
of the original checking logic) - we wouldn't call Reset() between Begin and
End, right?

[David Yen, 2015-07-16 21:19:30]

https://codereview.chromium.org/1233233002/diff/80001/ui/gl/gpu_timing.cc
File ui/gl/gpu_timing.cc (right):

https://codereview.chromium.org/1233233002/diff/80001/ui/gl/gpu_timing.cc#new...
ui/gl/gpu_timing.cc:482: void GPUTimer::Reset() {
On 2015/07/16 20:42:25, piman (Very slow to review) wrote:
> I think you can add a DCHECK that we're not in WaitingForEnd state (to keep
some
> of the original checking logic) - we wouldn't call Reset() between Begin and
> End, right?

Done.

[piman, 2015-07-16 22:17:33]

lgtm

[David Yen, 2015-07-16 22:19:09]

The CQ bit was checked by dyen@chromium.org

[commit-bot: I haz the power, 2015-07-16 22:21:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1233233002/120001

[commit-bot: I haz the power, 2015-07-16 22:59:38]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-07-16 23:01:13]

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