Fixed issue where disjoint timer offsets were incorrectly calculated.

Fixed issue where disjoint timer offsets were incorrectly calculated.

R=vmiura@chromium.org
BUG= https://code.google.com/p/chromium/issues/detail?id=397294
TEST= Trace gpu.service,gpu.device on android device

Committed: https://crrev.com/9e63b3f6d6cbdbdccc5d35bc3809254178e715dd
Cr-Commit-Position: refs/heads/master@{#296006}

[David Yen, 2014-09-18 21:42:24]

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

[David Yen, 2014-09-18 21:42:24]

Adding piman for bindings review.

[piman, 2014-09-18 22:03:43]

lgtm

[David Yen, 2014-09-19 21:13:38]

I've modified the GL_TIMESTAMP query for the DisjointTimer case to only query
when offset_ is not empty, this is only true for the unit test case where we
explicitly set the time. This doesn't seem ideal since we are adding a branch
just so we can unittest it properly, I'm not sure what the best way to do this
is.

The ideal solution would to be able to override
base::TimeTicks::NowFromSystemTraceTime().ToInternalValue(), is there any way to
override that?

[piman, 2014-09-19 22:26:26]

On 2014/09/19 21:13:38, David Yen wrote:
> I've modified the GL_TIMESTAMP query for the DisjointTimer case to only query
> when offset_ is not empty, this is only true for the unit test case where we
> explicitly set the time. This doesn't seem ideal since we are adding a branch
> just so we can unittest it properly, I'm not sure what the best way to do this
> is.

I wouldn't agonize over it, though you may want to add a comment.

> 
> The ideal solution would to be able to override
> base::TimeTicks::NowFromSystemTraceTime().ToInternalValue(), is there any way
to
> override that?

I don't think there is.
The typical way of doing this is to inject a dependency to the time (e.g. define
a TimeProvider interface, a production impl that calls NowFromSystemTraceTime,
and a stub for tests, inject the appropriate one into the GPUTrace). I think it
may be overkill for this.

LGTM

[vmiura, 2014-09-19 22:55:09]

https://codereview.chromium.org/586503003/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer.cc (left):

https://codereview.chromium.org/586503003/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.cc:365: }
Should we clear existing GL_GPU_DISJOINT_EXT values by calling
"glGetIntegerv(GL_GPU_DISJOINT_EXT, &disjoint_value)" somewhere after
*gpu_trace_dev_category is toggled to enabled?

https://codereview.chromium.org/586503003/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/586503003/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.cc:106: // For the disjoint timer, GPU
idle team does not seem to increment the
nit: team -> time?

https://codereview.chromium.org/586503003/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.cc:116: 
nit: Could you add a comment that this intentionally falls through to case
kTracerTypeARBTimer?

[David Yen, 2014-09-19 23:01:18]

https://codereview.chromium.org/586503003/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer.cc (left):

https://codereview.chromium.org/586503003/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.cc:365: }
On 2014/09/19 22:55:09, vmiura wrote:
> Should we clear existing GL_GPU_DISJOINT_EXT values by calling
> "glGetIntegerv(GL_GPU_DISJOINT_EXT, &disjoint_value)" somewhere after
> *gpu_trace_dev_category is toggled to enabled?

It gets cleared under BeginDecoding() as well, this clear used to be for the
TIMESTAMP query specifically.

https://codereview.chromium.org/586503003/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/586503003/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.cc:106: // For the disjoint timer, GPU
idle team does not seem to increment the
On 2014/09/19 22:55:09, vmiura wrote:
> nit: team -> time?

Done.

https://codereview.chromium.org/586503003/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.cc:116: 
On 2014/09/19 22:55:09, vmiura wrote:
> nit: Could you add a comment that this intentionally falls through to case
> kTracerTypeARBTimer?

Done.

[vmiura, 2014-09-19 23:06:03]

On 2014/09/19 23:01:18, David Yen wrote:
>
https://codereview.chromium.org/586503003/diff/40001/gpu/command_buffer/servi...
> File gpu/command_buffer/service/gpu_tracer.cc (left):
> 
>
https://codereview.chromium.org/586503003/diff/40001/gpu/command_buffer/servi...
> gpu/command_buffer/service/gpu_tracer.cc:365: }
> On 2014/09/19 22:55:09, vmiura wrote:
> > Should we clear existing GL_GPU_DISJOINT_EXT values by calling
> > "glGetIntegerv(GL_GPU_DISJOINT_EXT, &disjoint_value)" somewhere after
> > *gpu_trace_dev_category is toggled to enabled?
> 
> It gets cleared under BeginDecoding() as well, this clear used to be for the
> TIMESTAMP query specifically.
> 
>
https://codereview.chromium.org/586503003/diff/40001/gpu/command_buffer/servi...
> File gpu/command_buffer/service/gpu_tracer.cc (right):
> 
>
https://codereview.chromium.org/586503003/diff/40001/gpu/command_buffer/servi...
> gpu/command_buffer/service/gpu_tracer.cc:106: // For the disjoint timer, GPU
> idle team does not seem to increment the
> On 2014/09/19 22:55:09, vmiura wrote:
> > nit: team -> time?
> 
> Done.
> 
>
https://codereview.chromium.org/586503003/diff/40001/gpu/command_buffer/servi...
> gpu/command_buffer/service/gpu_tracer.cc:116: 
> On 2014/09/19 22:55:09, vmiura wrote:
> > nit: Could you add a comment that this intentionally falls through to case
> > kTracerTypeARBTimer?
> 
> Done.

Ah right, lgtm.

[David Yen, 2014-09-19 23:06:38]

The CQ bit was checked by dyen@chromium.org

[commit-bot: I haz the power, 2014-09-19 23:07:58]

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

[commit-bot: I haz the power, 2014-09-20 13:12:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-20 13:12:53]

Failed to apply the patch.

[David Yen, 2014-09-22 16:37:27]

The CQ bit was checked by dyen@chromium.org

[commit-bot: I haz the power, 2014-09-22 16:39:06]

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

[commit-bot: I haz the power, 2014-09-22 17:33:15]

Committed patchset #5 (id:80001) as 4185500132643e32ac49738d14dd83d85a703abe

[commit-bot: I haz the power, 2014-09-22 17:33:46]

Patchset 5 (id:??) landed as
https://crrev.com/9e63b3f6d6cbdbdccc5d35bc3809254178e715dd
Cr-Commit-Position: refs/heads/master@{#296006}