Add UMA for droppedframes count

Add UMA for droppedframes count
Adding droppedframecount UMA to monitor videoplayback quality. Metric gets logged only if the pipeline has video & has decoded atleast one video frame.

BUG=
R=dalecurtis

Committed: https://crrev.com/3c9a7d609ff3a9e3654c01fc2b686420192acede
Cr-Commit-Position: refs/heads/master@{#292449}

[prabhur1, 2014-08-22 23:41:02]

PTAL.

Adding UMA for dropped frame count.

[Ilya Sherman, 2014-08-22 23:53:09]

https://codereview.chromium.org/497913002/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/497913002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:10967: +  <summary>Count of dropped
frames between pipeline start and stop.</summary>
For those of us not familiar with the media code, it might be nice to give some
idea of what causes a pipeline to start and stop.

https://codereview.chromium.org/497913002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:10968: +</histogram>
It's often a good idea to compare a failure rate to a success rate, so that you
get a relative failure rate.  Absolute numbers tend to be harder to interpret.

OTOH, if you think that dropped frames ought to be quite rare, then maybe this
histogram on its own could give you a good indication as to whether your
hypothesis is correct.

[prabhur1, 2014-08-23 00:23:07]

Thanks!
PTAL @PS2.

https://codereview.chromium.org/497913002/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/497913002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:10967: +  <summary>Count of dropped
frames between pipeline start and stop.</summary>
On 2014/08/22 23:53:08, Ilya Sherman wrote:
> For those of us not familiar with the media code, it might be nice to give
some
> idea of what causes a pipeline to start and stop.

Done.

https://codereview.chromium.org/497913002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:10968: +</histogram>
On 2014/08/22 23:53:08, Ilya Sherman wrote:
> It's often a good idea to compare a failure rate to a success rate, so that
you
> get a relative failure rate.  Absolute numbers tend to be harder to interpret.
> 
> OTOH, if you think that dropped frames ought to be quite rare, then maybe this
> histogram on its own could give you a good indication as to whether your
> hypothesis is correct.

Acknowledged.

https://codereview.chromium.org/497913002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:10968: +</histogram>
On 2014/08/22 23:53:08, Ilya Sherman wrote:
> It's often a good idea to compare a failure rate to a success rate, so that
you
> get a relative failure rate.  Absolute numbers tend to be harder to interpret.
> 
> OTOH, if you think that dropped frames ought to be quite rare, then maybe this
> histogram on its own could give you a good indication as to whether your
> hypothesis is correct.

Dropped frame count is expected to be close to 0 in most cases. Any huge spikes
need to be investigated. There are exceptions but in general it is expected to
be around 0.

[Ilya Sherman, 2014-08-23 00:37:35]

https://codereview.chromium.org/497913002/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/497913002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10969: +    starts/stops when a HTML5
video is loaded/unloaded respectively  in the
nit: "a HTML5" -> "an HTML5"

https://codereview.chromium.org/497913002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10969: +    starts/stops when a HTML5
video is loaded/unloaded respectively  in the
nit: "respectively  in" -> "respectively in" (too many spaces)

[Ilya Sherman, 2014-08-23 00:37:45]

LGTM % those two nits.

[prabhur1, 2014-08-25 17:45:12]

prabhur@chromium.org changed reviewers:
+ xhwang@chromium.org

[prabhur1, 2014-08-25 17:45:13]

Fixing comments. 

scherkus@, xhwang@ could you take a look ?

https://codereview.chromium.org/497913002/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/497913002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10969: +    starts/stops when a HTML5
video is loaded/unloaded respectively  in the
On 2014/08/23 00:37:35, Ilya Sherman wrote:
> nit: "respectively  in" -> "respectively in" (too many spaces)

Done.

https://codereview.chromium.org/497913002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10969: +    starts/stops when a HTML5
video is loaded/unloaded respectively  in the
On 2014/08/23 00:37:35, Ilya Sherman wrote:
> nit: "respectively  in" -> "respectively in" (too many spaces)

Done.

[scherkus (not reviewing), 2014-08-26 21:41:04]

https://codereview.chromium.org/497913002/diff/40001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/497913002/diff/40001/media/base/pipeline.cc#n...
media/base/pipeline.cc:628: UMA_HISTOGRAM_COUNTS("Media.DroppedFrameCount",
stats.video_frames_dropped);
here's where things get subtle/tricky ... do we want to record this stat even
for Pipelines that don't contain video? (e.g., mp3s)

do we want to record this stat for Pipelines that never even successfully start
playing?

[scherkus (not reviewing), 2014-08-26 22:27:16]

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

[scherkus (not reviewing), 2014-08-26 22:27:16]

dalecurtis: can you review this and help prabhur@ out?

basically ... I want to make sure the two of you have a plan of attack for
adding UMAs that actually measure what we want to measure

for example, here we have some options:
  1) always record dropped frame count
  2) only record dropped frame count when video is present
  3) only record dropped frame count based on some other conditions (e.g.,
pipeline successfully started)

[prabhur1, 2014-08-26 22:30:23]

On 2014/08/26 22:27:16, scherkus wrote:
> dalecurtis: can you review this and help prabhur@ out?
> 
> basically ... I want to make sure the two of you have a plan of attack for
> adding UMAs that actually measure what we want to measure
> 
> for example, here we have some options:
>   1) always record dropped frame count
>   2) only record dropped frame count when video is present
>   3) only record dropped frame count based on some other conditions (e.g.,
> pipeline successfully started)

dalecurtis: I'm thinking to add a conditional to log only when
(stats.video_frames_decoded > 0). Would that take care of 2 & 3 above ?

[DaleCurtis, 2014-08-27 00:13:16]

I think recording when decoded_video_frames > 0 would be reasonable. A better
question might be, do we want to log this when a pipeline doesn't complete
playback successfully. Not doing so many include bad content with outlier
values.

Extending this idea, it'd be great if we could drill down on dropped frames per
content type / decoder / resolution.  I.e. I bet 720p VP9 videos may have tons
of dropped frames on lower end hardware. Ditto for 2160p / "4K" videos w/o
hardware decode.  Transitively, YT or Vimeo rolling out high res codecs would
blow up this number without it signaling any kind of issues on our part.

Blowing out that possibility matrix with UMA is probably a non-starter though
and certainly a PITA, so we should think about what we really want to measure
here and why.

[scherkus (not reviewing), 2014-08-27 01:22:06]

On 2014/08/27 00:13:16, DaleCurtis wrote:
> I think recording when decoded_video_frames > 0 would be reasonable. A better
> question might be, do we want to log this when a pipeline doesn't complete
> playback successfully. Not doing so many include bad content with outlier
> values.

Note that doing so gives us no data on # of Pipelines that didn't drop any video
frames. I believe you'd want to check something media::Renderer::HasVideo():
https://chromium.googlesource.com/chromium/src/+/master/media/base/pipeline.c...

[DaleCurtis, 2014-08-27 17:44:37]

On 2014/08/27 01:22:06, scherkus wrote:
> On 2014/08/27 00:13:16, DaleCurtis wrote:
> > I think recording when decoded_video_frames > 0 would be reasonable. A
better
> > question might be, do we want to log this when a pipeline doesn't complete
> > playback successfully. Not doing so many include bad content with outlier
> > values.
> 
> Note that doing so gives us no data on # of Pipelines that didn't drop any
video
> frames. I believe you'd want to check something media::Renderer::HasVideo():
>
https://chromium.googlesource.com/chromium/src/+/master/media/base/pipeline.c...

Can you elaborate on that? We're not checking dropped_frames > 0, but
decoded_video_frames > 0. IIUC it would give only give us data for videos that
successfully decoded at least one frame.

[scherkus (not reviewing), 2014-08-27 21:26:32]

On 2014/08/27 17:44:37, DaleCurtis wrote:
> On 2014/08/27 01:22:06, scherkus wrote:
> > On 2014/08/27 00:13:16, DaleCurtis wrote:
> > > I think recording when decoded_video_frames > 0 would be reasonable. A
> better
> > > question might be, do we want to log this when a pipeline doesn't complete
> > > playback successfully. Not doing so many include bad content with outlier
> > > values.
> > 
> > Note that doing so gives us no data on # of Pipelines that didn't drop any
> video
> > frames. I believe you'd want to check something media::Renderer::HasVideo():
> >
>
https://chromium.googlesource.com/chromium/src/+/master/media/base/pipeline.c...
> 
> Can you elaborate on that? We're not checking dropped_frames > 0, but
> decoded_video_frames > 0. IIUC it would give only give us data for videos that
> successfully decoded at least one frame.

Ah *decoded* frames -- yeah that'd work I suppose. I'd probably still stick with
checking HasVideo() as it's a more explicit check.

[DaleCurtis, 2014-08-27 22:44:37]

I'm not sure how effective this will be, but it's worth experimenting with.
While redundant I'm fine with the logging condition being:

(HasVideo() && decoded_frames > 0)

[prabhur1, 2014-08-28 01:28:01]

PTAL - Added hasVideo & decoded_frames check before logging.

[DaleCurtis, 2014-08-28 18:25:24]

lgtm % nits and updating the description with some more details.  Did you verify
that you can see the UMA statistic in chrome://histograms/Media. ?

https://codereview.chromium.org/497913002/diff/60001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/497913002/diff/60001/media/base/pipeline.cc#n...
media/base/pipeline.cc:530: if (renderer_ && renderer_->HasVideo() &&
stats.video_frames_decoded > 0) {
No {} required since it's a single line if.

[prabhur1, 2014-08-28 18:46:30]

Thanks for the review! 
Removed the {} & updated the CL description with more details.
Also checked chrome://histograms and verified media.DroppedFrameCount gets
logged as expected.

https://codereview.chromium.org/497913002/diff/60001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/497913002/diff/60001/media/base/pipeline.cc#n...
media/base/pipeline.cc:530: if (renderer_ && renderer_->HasVideo() &&
stats.video_frames_decoded > 0) {
On 2014/08/28 18:25:24, DaleCurtis wrote:
> No {} required since it's a single line if.

Done.

[prabhur1, 2014-08-28 18:46:51]

The CQ bit was checked by prabhur@chromium.org

[commit-bot: I haz the power, 2014-08-28 18:48:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/prabhur@chromium.org/497913002/80001

[commit-bot: I haz the power, 2014-08-28 19:51:16]

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

[commit-bot: I haz the power, 2014-08-28 19:51:17]

Failed to commit the patch.

[prabhur1, 2014-08-28 20:10:34]

The CQ bit was checked by prabhur@chromium.org

[commit-bot: I haz the power, 2014-08-28 20:12:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/prabhur@chromium.org/497913002/80001

[commit-bot: I haz the power, 2014-08-28 20:13:45]

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

[commit-bot: I haz the power, 2014-08-28 20:13:47]

Failed to commit the patch.

[prabhur1, 2014-08-28 20:27:15]

The CQ bit was checked by prabhur@chromium.org

[commit-bot: I haz the power, 2014-08-28 20:27:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/prabhur@chromium.org/497913002/80001

[commit-bot: I haz the power, 2014-08-28 20:29:11]

Committed patchset #5 (id:80001) as ab8dbfddcc21795fd497fd638c16bf3a57041c45

[xhwang, 2014-08-28 23:58:57]

On 2014/08/28 20:29:11, I haz the power (commit-bot) wrote:
> Committed patchset #5 (id:80001) as ab8dbfddcc21795fd497fd638c16bf3a57041c45

drive-by question: how are we going to use this UMA in practice? We are mixing
the number of dropped frames for all playbacks regardless of the length of the
video. Won't it be more effective to report
num_dropped_frames/num_decoded_frames ratio?

[commit-bot: I haz the power, 2014-09-10 03:01:47]

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