Include playbacks which never underflow in underflow metrics.

Include playbacks which never underflow in underflow metrics.

The histogram is only useful if we include playbacks which never
reach an underflow state.

BUG=641633
TEST=none

Committed: https://crrev.com/821e803f24bdba8524c9e906e558fad9bf2bd174
Cr-Commit-Position: refs/heads/master@{#416007}

[DaleCurtis, 2016-08-31 23:00:10]

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

[chcunningham, 2016-09-01 16:49:06]

lgtm

[DaleCurtis, 2016-09-01 16:51:37]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2016-09-01 16:51:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 18:44:31]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-01 18:47:39]

Description was changed from

==========
Include playbacks which never underflow in underflow metrics.

The histogram is only useful if we include playbacks which never
reach an underflow state.

BUG=641633
TEST=none
==========

to

==========
Include playbacks which never underflow in underflow metrics.

The histogram is only useful if we include playbacks which never
reach an underflow state.

BUG=641633
TEST=none

Committed: https://crrev.com/821e803f24bdba8524c9e906e558fad9bf2bd174
Cr-Commit-Position: refs/heads/master@{#416007}
==========

[commit-bot: I haz the power, 2016-09-01 18:47:40]

Patchset 1 (id:??) landed as
https://crrev.com/821e803f24bdba8524c9e906e558fad9bf2bd174
Cr-Commit-Position: refs/heads/master@{#416007}

[DaleCurtis, 2016-09-06 21:23:58]

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

[DaleCurtis, 2016-09-06 21:23:59]

+holte, the latest histograms are showing massive numbers of out-of-bounds
reports.

https://uma.googleplex.com/histograms/?endDate=latest&dayCount=1&histograms=M...

I just noticed that these histogram macros start from 1 not zero. Do I need to
use a custom histogram counts macro which includes zero now?

[chromium-reviews, 2016-09-07 00:24:07]

Counts for the value 0 will get counted in the underflow bucket (in your
case [0,1) bucket), which is basically just a normal bucket.  Using a
custom histogram won't change that.  It looks like most of your counts are
in the [100, 2147483647) bucket, which implies that most of the values
being recorded are >= 100.  You will probably need to replace your
histogram with one that has a higher max value if want to be able to
distinguish those values.

On Tue, Sep 6, 2016 at 2:24 PM <dalecurtis@chromium.org> wrote:

> +holte, the latest histograms are showing massive numbers of out-of-bounds
> reports.
>
>
>
https://uma.googleplex.com/histograms/?endDate=latest&dayCount=1&histograms=M...
>
> I just noticed that these histogram macros start from 1 not zero. Do I
> need to
> use a custom histogram counts macro which includes zero now?
>
> https://codereview.chromium.org/2294333004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Steven Holte, 2016-09-07 00:24:27]

https://codereview.chromium.org/2294333004/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2294333004/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1092:
UMA_HISTOGRAM_COUNTS_100("Media.UnderflowCount", 0);
You should probably consolidate the macro calls for these histograms into a
single location, using a helper function, since each call will create a local
static variable.

https://codereview.chromium.org/2294333004/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1131:
UMA_HISTOGRAM_COUNTS_100("Media.UnderflowCount", ++underflow_count_);
Generally not a good idea to have side-effecting like ++x in a macro call.  It
should behave correctly in this case, since there is only 1 reference to this
argument, but would be a good thing to fix.

[Steven Holte, 2016-09-07 00:38:21]

On 2016/09/07 00:24:27, Steven Holte wrote:
>
https://codereview.chromium.org/2294333004/diff/1/media/blink/webmediaplayer_...
> File media/blink/webmediaplayer_impl.cc (right):
> 
>
https://codereview.chromium.org/2294333004/diff/1/media/blink/webmediaplayer_...
> media/blink/webmediaplayer_impl.cc:1092:
> UMA_HISTOGRAM_COUNTS_100("Media.UnderflowCount", 0);
> You should probably consolidate the macro calls for these histograms into a
> single location, using a helper function, since each call will create a local
> static variable.
> 
>
https://codereview.chromium.org/2294333004/diff/1/media/blink/webmediaplayer_...
> media/blink/webmediaplayer_impl.cc:1131:
> UMA_HISTOGRAM_COUNTS_100("Media.UnderflowCount", ++underflow_count_);
> Generally not a good idea to have side-effecting like ++x in a macro call.  It
> should behave correctly in this case, since there is only 1 reference to this
> argument, but would be a good thing to fix.

Actually, the fact that you are recording every time you increment this counter
is probably the reason why you are seeing such extreme numbers of counts in the
overflow bucket.  If a user encounters 1000 overflows, they'll record a count of
1 in each of [0,1) - [25,26), a count of 2 in each of [26,28) - [40,42), and so
on to 5 in [95,100), and then 900 counts in the overflow bucket.  So when you
look at total counts across users, this results in massive counts in the
overflow bucket.  If you want a count of how many playback reach some threshold,
you only want to count the final number.