Don't underflow for background rendering. Ensure accurate counts.

Don't underflow for background rendering. Ensure accurate counts.

UMA stats are showing a large majority of clients are hitting more
than 100 underflow events; this can occur in the background rendering
case if the process is in low priority. We'll flip in and out of
the underflow state. To fix this, don't report underflow when we
are background rendering. It doesn't matter since the frames aren't
visible anyways.

This also strengthens the state check around reporting an underflow
count to ensure we're only counting the instances that matter.

BUG=641633
TEST=no more spammy underflow

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

[DaleCurtis, 2016-09-08 22:46:36]

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

[chcunningham, 2016-09-09 21:27:06]

LGTM, but not sure about impact

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

https://codereview.chromium.org/2320053004/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1144: ready_state_ ==
WebMediaPlayer::ReadyStateHaveEnoughData) {
I think this is fine, but I'm not sure it will have much impact. Looking at the
VideoRenderer guards, we only fire the event to transition to HAVE_NOTHING when
we are currently in HAVE_ENOUGH. Just want to be sure I'm not missing something.

https://codereview.chromium.org/2320053004/diff/1/media/renderers/video_rende...
File media/renderers/video_renderer_impl.cc (right):

https://codereview.chromium.org/2320053004/diff/1/media/renderers/video_rende...
media/renderers/video_renderer_impl.cc:193: !was_background_rendering_) {
So, IIUC this is just giving us one more tick to get the queue full, right?
Expected to make a big difference?

[DaleCurtis, 2016-09-09 21:51:37]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2016-09-09 21:51:41]

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

https://codereview.chromium.org/2320053004/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1144: ready_state_ ==
WebMediaPlayer::ReadyStateHaveEnoughData) {
On 2016/09/09 at 21:27:05, chcunningham wrote:
> I think this is fine, but I'm not sure it will have much impact. Looking at
the VideoRenderer guards, we only fire the event to transition to HAVE_NOTHING
when we are currently in HAVE_ENOUGH. Just want to be sure I'm not missing
something.

Correct, that was my interpretation, but I wasn't able to verify it 100%.

https://codereview.chromium.org/2320053004/diff/1/media/renderers/video_rende...
File media/renderers/video_renderer_impl.cc (right):

https://codereview.chromium.org/2320053004/diff/1/media/renderers/video_rende...
media/renderers/video_renderer_impl.cc:193: !was_background_rendering_) {
On 2016/09/09 at 21:27:06, chcunningham wrote:
> So, IIUC this is just giving us one more tick to get the queue full, right?
Expected to make a big difference?

Yes, because whenever the previous state was reached this would always be the
underflow state.

[DaleCurtis, 2016-09-09 21:51:54]

(Impact is hopefully way more usable underflow counts)

[commit-bot: I haz the power, 2016-09-09 21:52:04]

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

[chcunningham, 2016-09-09 22:17:47]

https://codereview.chromium.org/2320053004/diff/1/media/renderers/video_rende...
File media/renderers/video_renderer_impl.cc (right):

https://codereview.chromium.org/2320053004/diff/1/media/renderers/video_rende...
media/renderers/video_renderer_impl.cc:193: !was_background_rendering_) {
On 2016/09/09 21:51:41, DaleCurtis wrote:
> On 2016/09/09 at 21:27:06, chcunningham wrote:
> > So, IIUC this is just giving us one more tick to get the queue full, right?
> Expected to make a big difference?
> 
> Yes, because whenever the previous state was reached this would always be the
> underflow state.

You're saying that every time we will *never* have frames decoded between the
transition of Background and Foreground rendering? I expect that this may
sometimes occur, but not always?

[DaleCurtis, 2016-09-09 22:20:46]

https://codereview.chromium.org/2320053004/diff/1/media/renderers/video_rende...
File media/renderers/video_renderer_impl.cc (right):

https://codereview.chromium.org/2320053004/diff/1/media/renderers/video_rende...
media/renderers/video_renderer_impl.cc:193: !was_background_rendering_) {
On 2016/09/09 at 22:17:46, chcunningham wrote:
> On 2016/09/09 21:51:41, DaleCurtis wrote:
> > On 2016/09/09 at 21:27:06, chcunningham wrote:
> > > So, IIUC this is just giving us one more tick to get the queue full,
right?
> > Expected to make a big difference?
> > 
> > Yes, because whenever the previous state was reached this would always be
the
> > underflow state.
> 
> You're saying that every time we will *never* have frames decoded between the
transition of Background and Foreground rendering? I expect that this may
sometimes occur, but not always?

I mean every time we get into a state where we were previously flip flopping on
underflow we would always hit underflow on the first non-background-render tick.

[commit-bot: I haz the power, 2016-09-09 23:23:30]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-09 23:25:55]

Description was changed from

==========
Don't underflow for background rendering. Ensure accurate counts.

UMA stats are showing a large majority of clients are hitting more
than 100 underflow events; this can occur in the background rendering
case if the process is in low priority. We'll flip in and out of
the underflow state. To fix this, don't report underflow when we
are background rendering. It doesn't matter since the frames aren't
visible anyways.

This also strengthens the state check around reporting an underflow
count to ensure we're only counting the instances that matter.

BUG=641633
TEST=no more spammy underflow
==========

to

==========
Don't underflow for background rendering. Ensure accurate counts.

UMA stats are showing a large majority of clients are hitting more
than 100 underflow events; this can occur in the background rendering
case if the process is in low priority. We'll flip in and out of
the underflow state. To fix this, don't report underflow when we
are background rendering. It doesn't matter since the frames aren't
visible anyways.

This also strengthens the state check around reporting an underflow
count to ensure we're only counting the instances that matter.

BUG=641633
TEST=no more spammy underflow

Committed: https://crrev.com/acd77d6a6629bda1bda03c129ab7fc26b6033117
Cr-Commit-Position: refs/heads/master@{#417759}
==========

[commit-bot: I haz the power, 2016-09-09 23:25:56]

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