media: Add tracing around VPx decoding.

media: Add tracing around VPx decoding.

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

[Tom Finegan, 2014-12-02 23:40:29]

tomfinegan@chromium.org changed reviewers:
+ fgalligan@chromium.org

[Tom Finegan, 2014-12-02 23:40:52]

This appears to do what you want...

[Tom Finegan, 2014-12-03 18:07:15]

tomfinegan@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[Tom Finegan, 2014-12-03 18:08:26]

On 2014/12/02 23:40:52, Tom Finegan wrote:
> This appears to do what you want...

+Dale

Hi Dale,

We need to add some tracing for libvpx decode performance tracking. Does this
look all right? Should I be using "media" instead of "video"?

PTAL/Thanks!

[fgalligan1, 2014-12-03 18:29:56]

https://codereview.chromium.org/773053002/diff/1/media/filters/vpx_video_deco...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/773053002/diff/1/media/filters/vpx_video_deco...
media/filters/vpx_video_decoder.cc:365: TRACE_EVENT0("video", "VpxDecode");
Is this needed?

https://codereview.chromium.org/773053002/diff/1/media/filters/vpx_video_deco...
media/filters/vpx_video_decoder.cc:373: TRACE_EVENT_BEGIN0("video",
"vpx_codec_decode");
Maybe add the timestamp so we could distinguish the decode calls in the trace?

https://codereview.chromium.org/773053002/diff/1/media/filters/vpx_video_deco...
media/filters/vpx_video_decoder.cc:387: TRACE_EVENT_BEGIN0("video",
"vpx_codec_get_frame");
I think we can get rid of these. They should pretty much just return a pointer
at this point.

[Tom Finegan, 2014-12-03 18:51:39]

https://codereview.chromium.org/773053002/diff/1/media/filters/vpx_video_deco...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/773053002/diff/1/media/filters/vpx_video_deco...
media/filters/vpx_video_decoder.cc:365: TRACE_EVENT0("video", "VpxDecode");
On 2014/12/03 18:29:56, fgalligan1 wrote:
> Is this needed?

This tracked the entire VpxDecode() call. Removed.

https://codereview.chromium.org/773053002/diff/1/media/filters/vpx_video_deco...
media/filters/vpx_video_decoder.cc:373: TRACE_EVENT_BEGIN0("video",
"vpx_codec_decode");
On 2014/12/03 18:29:56, fgalligan1 wrote:
> Maybe add the timestamp so we could distinguish the decode calls in the trace?

Done.

https://codereview.chromium.org/773053002/diff/1/media/filters/vpx_video_deco...
media/filters/vpx_video_decoder.cc:387: TRACE_EVENT_BEGIN0("video",
"vpx_codec_get_frame");
On 2014/12/03 18:29:56, fgalligan1 wrote:
> I think we can get rid of these. They should pretty much just return a pointer
> at this point.

Done.

[DaleCurtis, 2014-12-03 19:12:16]

https://codereview.chromium.org/773053002/diff/20001/media/filters/vpx_video_...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/773053002/diff/20001/media/filters/vpx_video_...
media/filters/vpx_video_decoder.cc:372: TRACE_EVENT_BEGIN1("video",
"vpx_codec_decode", "timestamp", timestamp);
You can just use TRACE_EVENT_1() and scope it around this call with {} to avoid
the begin/end business.

https://codereview.chromium.org/773053002/diff/20001/media/filters/vpx_video_...
media/filters/vpx_video_decoder.cc:407: TRACE_EVENT_BEGIN1("video",
"vpx_codec_decode_alpha",
Ditto.

[Tom Finegan, 2014-12-03 19:33:10]

https://codereview.chromium.org/773053002/diff/20001/media/filters/vpx_video_...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/773053002/diff/20001/media/filters/vpx_video_...
media/filters/vpx_video_decoder.cc:372: TRACE_EVENT_BEGIN1("video",
"vpx_codec_decode", "timestamp", timestamp);
On 2014/12/03 19:12:16, DaleCurtis wrote:
> You can just use TRACE_EVENT_1() and scope it around this call with {} to
avoid
> the begin/end business. 

I thought the extra scoping was uglier. :)

Done.

[DaleCurtis, 2014-12-03 20:47:17]

lgtm

[Tom Finegan, 2014-12-03 20:54:32]

The CQ bit was checked by tomfinegan@chromium.org

[commit-bot: I haz the power, 2014-12-03 20:55:41]

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

[commit-bot: I haz the power, 2014-12-03 22:35:08]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-03 22:35:53]

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