Add UMA reporting for RTCVideoDecoder.

Add UMA reporting for RTCVideoDecoder.

Media.RTCVideoDecoderInitDecodeStatus: the results of InitDecode
Media.RTCVideoDecoderError: errors reported from VDA

BUG=170345
TEST=Run apprtc.appspot.com on Chromebook Daisy.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223288

[wuchengli, 2013-09-11 07:36:52]

Media.GpuVideoDecoderInitializeStatus is also in
tools/metrics/histograms/histograms.xml.
<histogram name="Media.GpuVideoDecoderInitializeStatus" enum="PipelineStatus">
  <summary>Results of attempts to GpuVideoDecoder::Initialize().</summary>
</histogram>

Where will it be displayed? I guess RTCVideoDecoder doesn't need it now.

[wuchengli, 2013-09-11 07:38:52]

Also, what's the difference between HISTOGRAM_BOOLEAN and UMA_HISTOGRAM_BOOLEAN?
The flag kUmaTargetedHistogramFlag says "Histogram should be UMA uploaded."
Where can we see uploaded UMA?

[wuchengli, 2013-09-11 07:58:56]

Pawel answered my questions. Please ignore them.

[Ami GONE FROM CHROMIUM, 2013-09-11 17:27:35]

https://codereview.chromium.org/23967010/diff/1/content/renderer/media/rtc_vi...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/23967010/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder.cc:175:
UMA_HISTOGRAM_BOOLEAN("Media.RTCVideoDecoderInitDecode", true);
Do you want also want a false log somewhere to indicate attempt+failure for HW
decode?

https://codereview.chromium.org/23967010/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder.cc:175:
UMA_HISTOGRAM_BOOLEAN("Media.RTCVideoDecoderInitDecode", true);
You'll also need to edit histograms.xml if you want to see this on the UMA
dashboard.

[wuchengli, 2013-09-13 07:38:23]

asvitkine@ owner review for histograms.xml. Thanks.

https://codereview.chromium.org/23967010/diff/1/content/renderer/media/rtc_vi...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/23967010/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder.cc:175:
UMA_HISTOGRAM_BOOLEAN("Media.RTCVideoDecoderInitDecode", true);
On 2013/09/11 17:27:35, Ami Fischman wrote:
> You'll also need to edit histograms.xml if you want to see this on the UMA
> dashboard.

Done.

https://codereview.chromium.org/23967010/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder.cc:175:
UMA_HISTOGRAM_BOOLEAN("Media.RTCVideoDecoderInitDecode", true);
On 2013/09/11 17:27:35, Ami Fischman wrote:
> Do you want also want a false log somewhere to indicate attempt+failure for HW
> decode?
Done. I didn't use enumeration for InitDecode because success/fail is enough.

I also added Media.RTCVideoDecoderError for errors from VDA.

https://codereview.chromium.org/23967010/diff/8001/tools/metrics/histograms/h...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/23967010/diff/8001/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:5739: +<histogram
name="Media.RTCVideoDecoderError" enum="PepperVideoDecodeError">
VideoDecodeAccelerator::Error has the same enum as PP_VideoDecodeError_Dev.

[Alexei Svitkine (slow), 2013-09-13 14:53:24]

https://codereview.chromium.org/23967010/diff/8001/content/renderer/media/rtc...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/23967010/diff/8001/content/renderer/media/rtc...
content/renderer/media/rtc_video_decoder.cc:755: bool sample = (status ==
WEBRTC_VIDEO_CODEC_OK) ? true : false;
Why not log the actual status as an enum rather than just whether it's an error
or not?

[Ami GONE FROM CHROMIUM, 2013-09-13 17:44:37]

LGTM

[wuchengli, 2013-09-14 01:40:16]

https://codereview.chromium.org/23967010/diff/8001/content/renderer/media/rtc...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/23967010/diff/8001/content/renderer/media/rtc...
content/renderer/media/rtc_video_decoder.cc:755: bool sample = (status ==
WEBRTC_VIDEO_CODEC_OK) ? true : false;
On 2013/09/13 14:53:25, Alexei Svitkine wrote:
> Why not log the actual status as an enum rather than just whether it's an
error
> or not?
Success case is more important than error. Most of the time people want to know
if HW or SW has been used for decoding. They can go to chrome://histograms to
check it. Also, InitDecode implementation is very light weight. It's less likely
to report error there. I logged the false case for consistency. Another use case
is that people can see how many times HW decoding is used during a period of
time.

Several ways I've considered:
- Log only the success case of InitDecode.
- Log only when RTCVideoDecoder is successfully created. Fail case is not very
important because most time it fails because HW doesn't support the codec.
- Log the enum of InitDecode. This is fine and just needs more code.
Let me know if you think the current patchset is ok or you prefer another way.

What will happen if one change the type of histogram from boolean to enum after
it runs for some time? Will UMA dashboard be a mess?

[Alexei Svitkine (slow), 2013-09-14 02:37:27]

lgtm

https://codereview.chromium.org/23967010/diff/8001/content/renderer/media/rtc...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/23967010/diff/8001/content/renderer/media/rtc...
content/renderer/media/rtc_video_decoder.cc:755: bool sample = (status ==
WEBRTC_VIDEO_CODEC_OK) ? true : false;
Okay, that sounds reasonable. Might be worth adding a short comment to that
extent about it.

On 2013/09/14 01:40:17, wuchengli wrote:
> What will happen if one change the type of histogram from boolean to enum
after
> it runs for some time? Will UMA dashboard be a mess?

A boolean is simply an enum with values 0 and 1. If you keep the same values and
change it to an enum, adding let's say a value 2, it will be compatible with the
old version.

[wuchengli, 2013-09-15 06:42:21]

On 2013/09/14 02:37:27, Alexei Svitkine wrote:
> lgtm
> 
>
https://codereview.chromium.org/23967010/diff/8001/content/renderer/media/rtc...
> File content/renderer/media/rtc_video_decoder.cc (right):
> 
>
https://codereview.chromium.org/23967010/diff/8001/content/renderer/media/rtc...
> content/renderer/media/rtc_video_decoder.cc:755: bool sample = (status ==
> WEBRTC_VIDEO_CODEC_OK) ? true : false;
> Okay, that sounds reasonable. Might be worth adding a short comment to that
> extent about it.
> 
> On 2013/09/14 01:40:17, wuchengli wrote:
> > What will happen if one change the type of histogram from boolean to enum
> after
> > it runs for some time? Will UMA dashboard be a mess?
> 
> A boolean is simply an enum with values 0 and 1. If you keep the same values
and
> change it to an enum, adding let's say a value 2, it will be compatible with
the
> old version.
Added a comment. Submitting now.

[commit-bot: I haz the power, 2013-09-15 06:42:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wuchengli@chromium.org/23967010/21001

[commit-bot: I haz the power, 2013-09-15 15:24:12]

Change committed as 223288