Use proper tracing category in image/tab capture trace logs

Use proper tracing category in image/tab capture trace logs

Trivial change to use a proper trace category 'gpu' instead of random
category 'mirror' in the GLHelper class trace statements. 'mirror'
category doesn't give any clue as to which group of statements execution
would be recorded with that.

BUG=None
TESTS=None

Committed: https://crrev.com/638e624a0639793183ec76ae949e6b4d9a897be7
Cr-Commit-Position: refs/heads/master@{#322984}

[r.kasibhatla, 2015-03-24 09:24:24]

r.kasibhatla@samsung.com changed reviewers:
+ danakj@chromium.org, kbr@chromium.org

[r.kasibhatla, 2015-03-24 09:24:25]

Trivial change to use proper category for trace statements. PTAL!

[Ken Russell (switch to Gerrit), 2015-03-24 22:10:57]

kbr@chromium.org changed reviewers:
+ hubbe@chromium.org

[Ken Russell (switch to Gerrit), 2015-03-24 22:10:57]

I don't know whether the preferred direction here would be "gpu" or "mirroring".
src/chrome/browser/extensions/api/tab_capture/tab_capture_performancetest.cc in
particular enables categories "gpu" and "mirroring". +hubbe

[r.kasibhatla, 2015-03-25 04:22:30]

On 2015/03/24 at 22:10:57, kbr wrote:
> I don't know whether the preferred direction here would be "gpu" or
"mirroring".
src/chrome/browser/extensions/api/tab_capture/tab_capture_performancetest.cc in
particular enables categories "gpu" and "mirroring". +hubbe

I was thinking about changing even "mirroring" to something more meaningful in
this patch itself. If we see the locations [1] where we record these traces, all
are related to capture or read the composited frame into buffer. So, if not for
"gpu", I was thinking something on the lines of "gpu.capture" or "gpu.readback".

[1] TRACE_EVENT1("mirroring", "FrameSubscriber::ShouldCaptureFrame",
TRACE_EVENT_ASYNC_STEP_INTO0("mirroring", "Capture", output.get(), "Scale");
TRACE_EVENT_ASYNC_STEP_INTO0("mirroring", "Capture", output.get(), "YUV");
TRACE_EVENT_ASYNC_BEGIN2("mirroring", "Capture", output_buffer.get()

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...

[Ken Russell (switch to Gerrit), 2015-03-25 18:27:21]

On 2015/03/25 04:22:30, r.kasibhatla wrote:
> On 2015/03/24 at 22:10:57, kbr wrote:
> > I don't know whether the preferred direction here would be "gpu" or
> "mirroring".
> src/chrome/browser/extensions/api/tab_capture/tab_capture_performancetest.cc
in
> particular enables categories "gpu" and "mirroring". +hubbe
> 
> I was thinking about changing even "mirroring" to something more meaningful in
> this patch itself. If we see the locations [1] where we record these traces,
all
> are related to capture or read the composited frame into buffer. So, if not
for
> "gpu", I was thinking something on the lines of "gpu.capture" or
"gpu.readback".
> 
> [1] TRACE_EVENT1("mirroring", "FrameSubscriber::ShouldCaptureFrame",
> TRACE_EVENT_ASYNC_STEP_INTO0("mirroring", "Capture", output.get(), "Scale");
> TRACE_EVENT_ASYNC_STEP_INTO0("mirroring", "Capture", output.get(), "YUV");
> TRACE_EVENT_ASYNC_BEGIN2("mirroring", "Capture", output_buffer.get()
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...

hubbe@ needs to comment on this change. The tab_capture_performancetest will
need to be updated to use the new categories -- and possibly the
tab_capture_end2end_test as well.

[hubbe, 2015-03-25 18:41:34]

On 2015/03/25 18:27:21, Ken Russell wrote:
> On 2015/03/25 04:22:30, r.kasibhatla wrote:
> > On 2015/03/24 at 22:10:57, kbr wrote:
> > > I don't know whether the preferred direction here would be "gpu" or
> > "mirroring".
> > src/chrome/browser/extensions/api/tab_capture/tab_capture_performancetest.cc
> in
> > particular enables categories "gpu" and "mirroring". +hubbe
> > 
> > I was thinking about changing even "mirroring" to something more meaningful
in
> > this patch itself. If we see the locations [1] where we record these traces,
> all
> > are related to capture or read the composited frame into buffer. So, if not
> for
> > "gpu", I was thinking something on the lines of "gpu.capture" or
> "gpu.readback".
> > 
> > [1] TRACE_EVENT1("mirroring", "FrameSubscriber::ShouldCaptureFrame",
> > TRACE_EVENT_ASYNC_STEP_INTO0("mirroring", "Capture", output.get(), "Scale");
> > TRACE_EVENT_ASYNC_STEP_INTO0("mirroring", "Capture", output.get(), "YUV");
> > TRACE_EVENT_ASYNC_BEGIN2("mirroring", "Capture", output_buffer.get()
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...
> 
> hubbe@ needs to comment on this change. The tab_capture_performancetest will
> need to be updated to use the new categories -- and possibly the
> tab_capture_end2end_test as well.

AFAIK, no tests uses the traces changed in this CL.
I have not checked if that applies to the traces mentioned in the review
comments too.

    /Hubbe

[Ken Russell (switch to Gerrit), 2015-03-25 18:55:53]

On 2015/03/25 18:41:34, hubbe wrote:
> On 2015/03/25 18:27:21, Ken Russell wrote:
> > On 2015/03/25 04:22:30, r.kasibhatla wrote:
> > > On 2015/03/24 at 22:10:57, kbr wrote:
> > > > I don't know whether the preferred direction here would be "gpu" or
> > > "mirroring".
> > >
src/chrome/browser/extensions/api/tab_capture/tab_capture_performancetest.cc
> > in
> > > particular enables categories "gpu" and "mirroring". +hubbe
> > > 
> > > I was thinking about changing even "mirroring" to something more
meaningful
> in
> > > this patch itself. If we see the locations [1] where we record these
traces,
> > all
> > > are related to capture or read the composited frame into buffer. So, if
not
> > for
> > > "gpu", I was thinking something on the lines of "gpu.capture" or
> > "gpu.readback".
> > > 
> > > [1] TRACE_EVENT1("mirroring", "FrameSubscriber::ShouldCaptureFrame",
> > > TRACE_EVENT_ASYNC_STEP_INTO0("mirroring", "Capture", output.get(),
"Scale");
> > > TRACE_EVENT_ASYNC_STEP_INTO0("mirroring", "Capture", output.get(), "YUV");
> > > TRACE_EVENT_ASYNC_BEGIN2("mirroring", "Capture", output_buffer.get()
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...
> > 
> > hubbe@ needs to comment on this change. The tab_capture_performancetest will
> > need to be updated to use the new categories -- and possibly the
> > tab_capture_end2end_test as well.
> 
> AFAIK, no tests uses the traces changed in this CL.
> I have not checked if that applies to the traces mentioned in the review
> comments too.
> 
>     /Hubbe

OK. Thanks for the feedback.

@r.kasibhatla: in that case I think a more specific category like gpu.capture
would be better, to avoid overloading the "gpu" category. Would you make this
change?

[r.kasibhatla, 2015-03-26 04:30:08]

On 2015/03/25 at 18:55:53, kbr wrote:
> On 2015/03/25 18:41:34, hubbe wrote:
> > On 2015/03/25 18:27:21, Ken Russell wrote:
> > > On 2015/03/25 04:22:30, r.kasibhatla wrote:
> > > > On 2015/03/24 at 22:10:57, kbr wrote:
> > > > > I don't know whether the preferred direction here would be "gpu" or
> > > > "mirroring".
> > > >
src/chrome/browser/extensions/api/tab_capture/tab_capture_performancetest.cc
> > > in
> > > > particular enables categories "gpu" and "mirroring". +hubbe
> > > > 
> > > > I was thinking about changing even "mirroring" to something more
meaningful
> > in
> > > > this patch itself. If we see the locations [1] where we record these
traces,
> > > all
> > > > are related to capture or read the composited frame into buffer. So, if
not
> > > for
> > > > "gpu", I was thinking something on the lines of "gpu.capture" or
> > > "gpu.readback".
> > > > 
> > > > [1] TRACE_EVENT1("mirroring", "FrameSubscriber::ShouldCaptureFrame",
> > > > TRACE_EVENT_ASYNC_STEP_INTO0("mirroring", "Capture", output.get(),
"Scale");
> > > > TRACE_EVENT_ASYNC_STEP_INTO0("mirroring", "Capture", output.get(),
"YUV");
> > > > TRACE_EVENT_ASYNC_BEGIN2("mirroring", "Capture", output_buffer.get()
> > > > 
> > > >
> > >
> >
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...
> > > >
> > >
> >
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...
> > > 
> > > hubbe@ needs to comment on this change. The tab_capture_performancetest
will
> > > need to be updated to use the new categories -- and possibly the
> > > tab_capture_end2end_test as well.
> > 
> > AFAIK, no tests uses the traces changed in this CL.
> > I have not checked if that applies to the traces mentioned in the review
> > comments too.
> > 
> >     /Hubbe
> 
> OK. Thanks for the feedback.
> 
> @r.kasibhatla: in that case I think a more specific category like gpu.capture
would be better, to avoid overloading the "gpu" category. Would you make this
change?

Sure. I will update all the 3 files (including the ones I mentioned in the
previous review comments) to gpu.capture and update the effected tests also.

[r.kasibhatla, 2015-03-26 07:28:32]

Changed as per our discussion to gpu.capture for all files. PTAL!

[Ken Russell (switch to Gerrit), 2015-03-28 00:18:42]

On 2015/03/26 07:28:32, r.kasibhatla wrote:
> Changed as per our discussion to gpu.capture for all files. PTAL!

I see "error: old chunk mismatch" when viewing the side-by-side diffs. Could you
please re-upload?

Also, please check the directories of the files you've changed (and their
parents) and make sure you have owners in the reviewer list. For example you're
missing owners for src/content/browser/media/capture/ .

[r.kasibhatla, 2015-03-30 06:39:56]

r.kasibhatla@samsung.com changed reviewers:
+ finnur@chromium.org, wez@chromium.org

[r.kasibhatla, 2015-03-30 06:39:56]

Added owners for chrome/browser/extensions/ and content/browser/media/capture
folders.
PTAL!!

[r.kasibhatla, 2015-03-30 06:44:18]

r.kasibhatla@samsung.com changed reviewers:
+ miu@chromium.org
- finnur@chromium.org, wez@chromium.org

[r.kasibhatla, 2015-03-30 06:44:18]

+ miu@ who is OWNER for both chrome/browser/extensions/api/tab_capture and
content/browser/media/capture folders.

[Ken Russell (switch to Gerrit), 2015-03-30 18:32:06]

lgtm from my standpoint.

[miu, 2015-03-31 01:21:50]

lgtm  Thanks!  :)

[r.kasibhatla, 2015-03-31 04:08:03]

The CQ bit was checked by r.kasibhatla@samsung.com

[commit-bot: I haz the power, 2015-03-31 04:08:15]

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

[commit-bot: I haz the power, 2015-03-31 05:19:15]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-31 05:19:47]

Patchset 3 (id:??) landed as
https://crrev.com/638e624a0639793183ec76ae949e6b4d9a897be7
Cr-Commit-Position: refs/heads/master@{#322984}