Added GL_CHROMIUM_trace_marker feature as well as gpu_toplevel markers.

Added GL_CHROMIUM_trace_marker feature as well as gpu_toplevel markers.

Added a GL_CHROMIUM_trace_marker extension which allows us to execute
GPU traces with an associated category and trace name. This feature
has been documented here and is also used for "gpu_toplevel" traces
so we can display traces for top level graphics contexts.

blink side: https://codereview.chromium.org/797273003/

R=vmiura@chromium.org
BUG=None
TEST=trybots

Committed: https://crrev.com/52554adb44477b802d66105d8e345f1bead3810c
Cr-Commit-Position: refs/heads/master@{#310399}

[David Yen, 2014-12-08 22:37:50]

This CL is to be merged and tested after: 

https://codereview.chromium.org/776603003/

[vmiura, 2014-12-09 00:18:11]

https://codereview.chromium.org/780653007/diff/20001/base/debug/trace_event.h
File base/debug/trace_event.h (right):

https://codereview.chromium.org/780653007/diff/20001/base/debug/trace_event.h...
base/debug/trace_event.h:794: context->TraceBeginCHROMIUM(category_group, name);
\
I think we should check if the category is enabled here, and in the END0.

if (INTERNAL_TRACE_EVENT_CATEGORY_GROUP_ENABLED_FOR_RECORDING_MODE()) {
 ...
}

https://codereview.chromium.org/780653007/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/780653007/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:143: if
(context_ && *gpu_trace_top_level_category_) {
Check if the category is enabled inside the macros instead of here.  You'll need
to pass the category to END0 as well.

https://codereview.chromium.org/780653007/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:202: if
(*gpu_trace_top_level_category_) {
Same, check if the category is enabled inside the macros instead of here.

https://codereview.chromium.org/780653007/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/780653007/diff/20001/content/common/gpu/clien...
content/common/gpu/client/context_provider_command_buffer.cc:101: if
(*gpu_trace_top_level_category_) {
As in render_widget_host_view_android.cc, move the category checks into the
macros.

https://codereview.chromium.org/780653007/diff/20001/webkit/common/gpu/contex...
File webkit/common/gpu/context_provider_in_process.cc (right):

https://codereview.chromium.org/780653007/diff/20001/webkit/common/gpu/contex...
webkit/common/gpu/context_provider_in_process.cc:105: if
(*gpu_trace_top_level_category_) {
Same with these.

[vmiura, 2014-12-09 00:25:23]

BTW, I think it will be better to put the macros in GPU client code rather than
in base.

e.g. gpu/command_buffer/client/gles2_trace_event.h

[David Yen, 2014-12-17 22:50:23]

dyen@chromium.org changed reviewers:
+ kbr@chromium.org, sievers@chromium.org

[David Yen, 2014-12-17 22:50:24]

I've removed the macros for tracing GPUs from this CL because all the
gpu_toplevel traces are within blink contexts. I will back in the macros in a
separate CL.

This CL mainly documents the CHROMIUM_trace_marker extension as well as
implementing it within Chromium. The blink side changes (empty base class
definitions) can be found here: https://codereview.chromium.org/797273003/

I am still waiting for the following CL to fix some issues with the actual GPU
Tracer: https://codereview.chromium.org/813573003/

Once that CL gets in, I will also add unit tests to test that "gpu_toplevel" is
plumbed properly all the way through.

- Added sievers to review "render_widget_host_view_android.cc".
- Added kbr for webkit gpu changes.

[Ken Russell (switch to Gerrit), 2014-12-17 23:56:10]

I'm not familiar enough with the semantics of these calls to review this
effectively.

rslgtm once vmiura and sievers are happy.

https://codereview.chromium.org/780653007/diff/60001/webkit/common/gpu/contex...
File webkit/common/gpu/context_provider_in_process.cc (right):

https://codereview.chromium.org/780653007/diff/60001/webkit/common/gpu/contex...
webkit/common/gpu/context_provider_in_process.cc:106:
unique_context_name.c_str());
I'll have to defer to Victor's review on whether it's correct that this will be
called once per context.

https://codereview.chromium.org/780653007/diff/60001/webkit/common/gpu/contex...
webkit/common/gpu/context_provider_in_process.cc:198:
context3d_->traceEndCHROMIUM();
Similarly it's not obvious to me that calling this once at the end of the
context's lifetime is correct.

[Ken Russell (switch to Gerrit), 2014-12-17 23:56:22]

er, rs lgtm.

[vmiura, 2014-12-18 02:38:52]

https://codereview.chromium.org/780653007/diff/60001/gpu/GLES2/extensions/CHR...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_trace_marker.txt (right):

https://codereview.chromium.org/780653007/diff/60001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_trace_marker.txt:25: the trace will
automatically end with the commands of the traced graphics
nit: This is not so clear.

Should it say something like "the trace will automatically end when the context
is destroyed?"

https://codereview.chromium.org/780653007/diff/60001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_trace_marker.txt:55: None.
I think these can cause GL_INVALID_OPERATION.

https://codereview.chromium.org/780653007/diff/60001/webkit/common/gpu/contex...
File webkit/common/gpu/context_provider_in_process.cc (right):

https://codereview.chromium.org/780653007/diff/60001/webkit/common/gpu/contex...
webkit/common/gpu/context_provider_in_process.cc:106:
unique_context_name.c_str());
On 2014/12/17 23:56:10, Ken Russell wrote:
> I'll have to defer to Victor's review on whether it's correct that this will
be
> called once per context.

I think this is fine.

https://codereview.chromium.org/780653007/diff/60001/webkit/common/gpu/contex...
webkit/common/gpu/context_provider_in_process.cc:198:
context3d_->traceEndCHROMIUM();
On 2014/12/17 23:56:10, Ken Russell wrote:
> Similarly it's not obvious to me that calling this once at the end of the
> context's lifetime is correct.

It is not necessarily needed, as the End is implied after the last command
executed on a context.

Ken is it safe to call context3d_->* from this method?

[Ken Russell (switch to Gerrit), 2014-12-18 19:03:56]

https://codereview.chromium.org/780653007/diff/60001/webkit/common/gpu/contex...
File webkit/common/gpu/context_provider_in_process.cc (right):

https://codereview.chromium.org/780653007/diff/60001/webkit/common/gpu/contex...
webkit/common/gpu/context_provider_in_process.cc:198:
context3d_->traceEndCHROMIUM();
On 2014/12/18 02:38:52, vmiura wrote:
> On 2014/12/17 23:56:10, Ken Russell wrote:
> > Similarly it's not obvious to me that calling this once at the end of the
> > context's lifetime is correct.
> 
> It is not necessarily needed, as the End is implied after the last command
> executed on a context.
> 
> Ken is it safe to call context3d_->* from this method?

Wait a second. Looking more closely, this method is a query, not a notification
that the context was destroyed on the main thread. I think calling
traceEndCHROMIUM is semantically incorrect here.

[vmiura, 2014-12-18 19:30:27]

https://codereview.chromium.org/780653007/diff/60001/content/common/gpu/clien...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/780653007/diff/60001/content/common/gpu/clien...
content/common/gpu/client/context_provider_command_buffer.cc:202:
context3d_->traceEndCHROMIUM();
Likewise, can we just remove this call, as GL_CHROMIUM_trace_marker spec implies
that this is not needed.

https://codereview.chromium.org/780653007/diff/60001/webkit/common/gpu/contex...
File webkit/common/gpu/context_provider_in_process.cc (right):

https://codereview.chromium.org/780653007/diff/60001/webkit/common/gpu/contex...
webkit/common/gpu/context_provider_in_process.cc:198:
context3d_->traceEndCHROMIUM();
On 2014/12/18 19:03:56, Ken Russell wrote:
> On 2014/12/18 02:38:52, vmiura wrote:
> > On 2014/12/17 23:56:10, Ken Russell wrote:
> > > Similarly it's not obvious to me that calling this once at the end of the
> > > context's lifetime is correct.
> > 
> > It is not necessarily needed, as the End is implied after the last command
> > executed on a context.
> > 
> > Ken is it safe to call context3d_->* from this method?
> 
> Wait a second. Looking more closely, this method is a query, not a
notification
> that the context was destroyed on the main thread. I think calling
> traceEndCHROMIUM is semantically incorrect here.

Can we just remove this call David, as GL_CHROMIUM_trace_marker spec implies
that this is not needed.

[David Yen, 2014-12-18 20:32:36]

On 2014/12/18 19:30:27, vmiura wrote:
>
https://codereview.chromium.org/780653007/diff/60001/content/common/gpu/clien...
> File content/common/gpu/client/context_provider_command_buffer.cc (right):
> 
>
https://codereview.chromium.org/780653007/diff/60001/content/common/gpu/clien...
> content/common/gpu/client/context_provider_command_buffer.cc:202:
> context3d_->traceEndCHROMIUM();
> Likewise, can we just remove this call, as GL_CHROMIUM_trace_marker spec
implies
> that this is not needed.
> 
>
https://codereview.chromium.org/780653007/diff/60001/webkit/common/gpu/contex...
> File webkit/common/gpu/context_provider_in_process.cc (right):
> 
>
https://codereview.chromium.org/780653007/diff/60001/webkit/common/gpu/contex...
> webkit/common/gpu/context_provider_in_process.cc:198:
> context3d_->traceEndCHROMIUM();
> On 2014/12/18 19:03:56, Ken Russell wrote:
> > On 2014/12/18 02:38:52, vmiura wrote:
> > > On 2014/12/17 23:56:10, Ken Russell wrote:
> > > > Similarly it's not obvious to me that calling this once at the end of
the
> > > > context's lifetime is correct.
> > > 
> > > It is not necessarily needed, as the End is implied after the last command
> > > executed on a context.
> > > 
> > > Ken is it safe to call context3d_->* from this method?
> > 
> > Wait a second. Looking more closely, this method is a query, not a
> notification
> > that the context was destroyed on the main thread. I think calling
> > traceEndCHROMIUM is semantically incorrect here.
> 
> Can we just remove this call David, as GL_CHROMIUM_trace_marker spec implies
> that this is not needed.

After looking at the code a little more closely, I think I wrote that part of
the specification wrong. I will edit the specification.

I originally thought that EndDecoding() removed the active traces, but after
reviewing the code a bit more it looks like it is more of a "pause" and does not
actually remove any traces. In other words, active traces stop tracing when
EndDecoding() is called, but are resurrected when BeginDecoding() is called
again.

Having said that, it would seem that all traces should have a begin/end or else
there will be a memory leak and/or mismatched trace begin/end markers. Where
should the context mark the end of its trace?

[vmiura, 2014-12-18 20:49:49]

On 2014/12/18 20:32:36, David Yen wrote:
> On 2014/12/18 19:30:27, vmiura wrote:
> >
>
https://codereview.chromium.org/780653007/diff/60001/content/common/gpu/clien...
> > File content/common/gpu/client/context_provider_command_buffer.cc (right):
> > 
> >
>
https://codereview.chromium.org/780653007/diff/60001/content/common/gpu/clien...
> > content/common/gpu/client/context_provider_command_buffer.cc:202:
> > context3d_->traceEndCHROMIUM();
> > Likewise, can we just remove this call, as GL_CHROMIUM_trace_marker spec
> implies
> > that this is not needed.
> > 
> >
>
https://codereview.chromium.org/780653007/diff/60001/webkit/common/gpu/contex...
> > File webkit/common/gpu/context_provider_in_process.cc (right):
> > 
> >
>
https://codereview.chromium.org/780653007/diff/60001/webkit/common/gpu/contex...
> > webkit/common/gpu/context_provider_in_process.cc:198:
> > context3d_->traceEndCHROMIUM();
> > On 2014/12/18 19:03:56, Ken Russell wrote:
> > > On 2014/12/18 02:38:52, vmiura wrote:
> > > > On 2014/12/17 23:56:10, Ken Russell wrote:
> > > > > Similarly it's not obvious to me that calling this once at the end of
> the
> > > > > context's lifetime is correct.
> > > > 
> > > > It is not necessarily needed, as the End is implied after the last
command
> > > > executed on a context.
> > > > 
> > > > Ken is it safe to call context3d_->* from this method?
> > > 
> > > Wait a second. Looking more closely, this method is a query, not a
> > notification
> > > that the context was destroyed on the main thread. I think calling
> > > traceEndCHROMIUM is semantically incorrect here.
> > 
> > Can we just remove this call David, as GL_CHROMIUM_trace_marker spec implies
> > that this is not needed.
> 
> After looking at the code a little more closely, I think I wrote that part of
> the specification wrong. I will edit the specification.
> 
> I originally thought that EndDecoding() removed the active traces, but after
> reviewing the code a bit more it looks like it is more of a "pause" and does
not
> actually remove any traces. In other words, active traces stop tracing when
> EndDecoding() is called, but are resurrected when BeginDecoding() is called
> again.
> 
> Having said that, it would seem that all traces should have a begin/end or
else
> there will be a memory leak and/or mismatched trace begin/end markers. Where
> should the context mark the end of its trace?

So, I think traces are always resumed & paused in BeginDecoding()/EndDecoding().

Any active traces left when the context is destroyed will be deleted along with
the context.

[David Yen, 2014-12-18 21:26:00]

Ok, I removed the end traces for the contexts.

https://codereview.chromium.org/780653007/diff/60001/content/common/gpu/clien...
File content/common/gpu/client/context_provider_command_buffer.cc (right):

https://codereview.chromium.org/780653007/diff/60001/content/common/gpu/clien...
content/common/gpu/client/context_provider_command_buffer.cc:202:
context3d_->traceEndCHROMIUM();
On 2014/12/18 19:30:27, vmiura wrote:
> Likewise, can we just remove this call, as GL_CHROMIUM_trace_marker spec
implies
> that this is not needed.

Done.

https://codereview.chromium.org/780653007/diff/60001/webkit/common/gpu/contex...
File webkit/common/gpu/context_provider_in_process.cc (right):

https://codereview.chromium.org/780653007/diff/60001/webkit/common/gpu/contex...
webkit/common/gpu/context_provider_in_process.cc:198:
context3d_->traceEndCHROMIUM();
On 2014/12/18 19:30:27, vmiura wrote:
> On 2014/12/18 19:03:56, Ken Russell wrote:
> > On 2014/12/18 02:38:52, vmiura wrote:
> > > On 2014/12/17 23:56:10, Ken Russell wrote:
> > > > Similarly it's not obvious to me that calling this once at the end of
the
> > > > context's lifetime is correct.
> > > 
> > > It is not necessarily needed, as the End is implied after the last command
> > > executed on a context.
> > > 
> > > Ken is it safe to call context3d_->* from this method?
> > 
> > Wait a second. Looking more closely, this method is a query, not a
> notification
> > that the context was destroyed on the main thread. I think calling
> > traceEndCHROMIUM is semantically incorrect here.
> 
> Can we just remove this call David, as GL_CHROMIUM_trace_marker spec implies
> that this is not needed.

Done.

https://codereview.chromium.org/780653007/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/780653007/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:139:
context_->traceEndCHROMIUM();
Should this one be removed as well?

[vmiura, 2014-12-18 21:49:39]

LGTM modulo comments.

https://codereview.chromium.org/780653007/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/780653007/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:139:
context_->traceEndCHROMIUM();
On 2014/12/18 21:26:00, David Yen wrote:
> Should this one be removed as well?

Yeah, let's remove this one as well.

https://codereview.chromium.org/780653007/diff/80001/gpu/GLES2/extensions/CHR...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_trace_marker.txt (right):

https://codereview.chromium.org/780653007/diff/80001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_trace_marker.txt:25: the trace will
automatically end with the commands of the traced graphics
nit: "the trace will automatically end when the context is destroyed".

I'm not sure we should go into detail about pausing & resuming when the context
is Flushed here.

[David Yen, 2014-12-18 21:54:27]

https://codereview.chromium.org/780653007/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/780653007/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:139:
context_->traceEndCHROMIUM();
On 2014/12/18 21:49:39, vmiura wrote:
> On 2014/12/18 21:26:00, David Yen wrote:
> > Should this one be removed as well?
> 
> Yeah, let's remove this one as well.

Done.

https://codereview.chromium.org/780653007/diff/80001/gpu/GLES2/extensions/CHR...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_trace_marker.txt (right):

https://codereview.chromium.org/780653007/diff/80001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_trace_marker.txt:25: the trace will
automatically end with the commands of the traced graphics
On 2014/12/18 21:49:39, vmiura wrote:
> nit: "the trace will automatically end when the context is destroyed".
> 
> I'm not sure we should go into detail about pausing & resuming when the
context
> is Flushed here.

Done.

[David Yen, 2015-01-05 17:28:05]

On 2014/12/18 21:54:27, David Yen wrote:
>
https://codereview.chromium.org/780653007/diff/80001/content/browser/renderer...
> File content/browser/renderer_host/render_widget_host_view_android.cc (right):
> 
>
https://codereview.chromium.org/780653007/diff/80001/content/browser/renderer...
> content/browser/renderer_host/render_widget_host_view_android.cc:139:
> context_->traceEndCHROMIUM();
> On 2014/12/18 21:49:39, vmiura wrote:
> > On 2014/12/18 21:26:00, David Yen wrote:
> > > Should this one be removed as well?
> > 
> > Yeah, let's remove this one as well.
> 
> Done.
> 
>
https://codereview.chromium.org/780653007/diff/80001/gpu/GLES2/extensions/CHR...
> File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_trace_marker.txt (right):
> 
>
https://codereview.chromium.org/780653007/diff/80001/gpu/GLES2/extensions/CHR...
> gpu/GLES2/extensions/CHROMIUM/CHROMIUM_trace_marker.txt:25: the trace will
> automatically end with the commands of the traced graphics
> On 2014/12/18 21:49:39, vmiura wrote:
> > nit: "the trace will automatically end when the context is destroyed".
> > 
> > I'm not sure we should go into detail about pausing & resuming when the
> context
> > is Flushed here.
> 
> Done.

ping sievers to review "render_widget_host_view_android.cc"

[no sievers, 2015-01-05 19:08:07]

On 2015/01/05 17:28:05, David Yen wrote:
> On 2014/12/18 21:54:27, David Yen wrote:
> >
>
https://codereview.chromium.org/780653007/diff/80001/content/browser/renderer...
> > File content/browser/renderer_host/render_widget_host_view_android.cc
(right):
> > 
> >
>
https://codereview.chromium.org/780653007/diff/80001/content/browser/renderer...
> > content/browser/renderer_host/render_widget_host_view_android.cc:139:
> > context_->traceEndCHROMIUM();
> > On 2014/12/18 21:49:39, vmiura wrote:
> > > On 2014/12/18 21:26:00, David Yen wrote:
> > > > Should this one be removed as well?
> > > 
> > > Yeah, let's remove this one as well.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/780653007/diff/80001/gpu/GLES2/extensions/CHR...
> > File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_trace_marker.txt (right):
> > 
> >
>
https://codereview.chromium.org/780653007/diff/80001/gpu/GLES2/extensions/CHR...
> > gpu/GLES2/extensions/CHROMIUM/CHROMIUM_trace_marker.txt:25: the trace will
> > automatically end with the commands of the traced graphics
> > On 2014/12/18 21:49:39, vmiura wrote:
> > > nit: "the trace will automatically end when the context is destroyed".
> > > 
> > > I'm not sure we should go into detail about pausing & resuming when the
> > context
> > > is Flushed here.
> > 
> > Done.
> 
> ping sievers to review "render_widget_host_view_android.cc"

lgtm

[David Yen, 2015-01-07 18:53:12]

The CQ bit was checked by dyen@chromium.org

[commit-bot: I haz the power, 2015-01-07 18:54:14]

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

[commit-bot: I haz the power, 2015-01-07 19:02:37]

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

[commit-bot: I haz the power, 2015-01-07 19:02:38]

Try jobs failed on following builders:
  android_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[David Yen, 2015-01-07 22:54:48]

The CQ bit was checked by dyen@chromium.org

[commit-bot: I haz the power, 2015-01-07 22:55:47]

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

[commit-bot: I haz the power, 2015-01-07 22:57:03]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-01-07 22:58:05]

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

[no sievers, 2015-04-28 21:27:01]

Why did this remove the group markers?
I just noticed that I don't get the info about the context anymore when the
decoder is logging.