New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 641633 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

Add rebuffering (underflow) metrics to src= playback.

Project Member Reported by dalecur...@chromium.org, Aug 27 2016

Issue description

It's crazy that we don't already have these. I think they're worth merging to get early data for improvements around cancelled reads and demuxer buffering we'll experiment with in M55.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9d638a1ee2933e62dccaed16eef64e64cbe2ded9

commit 9d638a1ee2933e62dccaed16eef64e64cbe2ded9
Author: dalecurtis <dalecurtis@chromium.org>
Date: Tue Aug 30 06:20:55 2016

Add metrics for tracking underflow during src= playback.

I'm surprised we didn't already have these metrics, they are pretty
key to evaluating improvements to our pipeline. Possibly they were
harder to add prior to chcunningham@'s recent work to expose the
BUFFERING_HAVE_NOTHING state to WMPI.

BUG= 641633 
TEST=manual.

Review-Url: https://codereview.chromium.org/2286843003
Cr-Commit-Position: refs/heads/master@{#415215}

[modify] https://crrev.com/9d638a1ee2933e62dccaed16eef64e64cbe2ded9/media/blink/webmediaplayer_impl.cc
[modify] https://crrev.com/9d638a1ee2933e62dccaed16eef64e64cbe2ded9/media/blink/webmediaplayer_impl.h
[modify] https://crrev.com/9d638a1ee2933e62dccaed16eef64e64cbe2ded9/tools/metrics/histograms/histograms.xml

Project Member

Comment 2 by bugdroid1@chromium.org, Sep 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/821e803f24bdba8524c9e906e558fad9bf2bd174

commit 821e803f24bdba8524c9e906e558fad9bf2bd174
Author: dalecurtis <dalecurtis@chromium.org>
Date: Thu Sep 01 18:44:15 2016

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

Review-Url: https://codereview.chromium.org/2294333004
Cr-Commit-Position: refs/heads/master@{#416007}

[modify] https://crrev.com/821e803f24bdba8524c9e906e558fad9bf2bd174/media/blink/webmediaplayer_impl.cc

Cc: brajkumar@chromium.org
Labels: Needs-Feedback
dalecurtis@ - Do we have any manual repro steps for this issue to verify it from Chrome-TE end? If yes, Could you please let us know.


Labels: -Needs-Feedback
Had an offline chat with dalecurtis@, he is still working on this issue.

Thank you!
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 9 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/acd77d6a6629bda1bda03c129ab7fc26b6033117

commit acd77d6a6629bda1bda03c129ab7fc26b6033117
Author: dalecurtis <dalecurtis@chromium.org>
Date: Fri Sep 09 23:23:14 2016

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

Review-Url: https://codereview.chromium.org/2320053004
Cr-Commit-Position: refs/heads/master@{#417759}

[modify] https://crrev.com/acd77d6a6629bda1bda03c129ab7fc26b6033117/media/blink/webmediaplayer_impl.cc
[modify] https://crrev.com/acd77d6a6629bda1bda03c129ab7fc26b6033117/media/renderers/video_renderer_impl.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Sep 20 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/af34d871739e277c5807de7d646de6b1ef49c07a

commit af34d871739e277c5807de7d646de6b1ef49c07a
Author: dalecurtis <dalecurtis@chromium.org>
Date: Tue Sep 20 04:32:26 2016

Reset underflow count upon seek event.

This is inline with what we do for watch time and prevents looping
videos from skewing underflow counts.

BUG= 641633 

Review-Url: https://codereview.chromium.org/2353803002
Cr-Commit-Position: refs/heads/master@{#419679}

[modify] https://crrev.com/af34d871739e277c5807de7d646de6b1ef49c07a/media/blink/webmediaplayer_impl.cc

Cc: chcunningham@chromium.org
Labels: -Pri-1 -M-54 M-55 Pri-2
These are still not great sadly. There are massive counts in the overflow bucket, which implies many users are seeing more than 100 underflow events today... +chcunningham to help investigate this since I'll be OOO for the next two weeks. Since these are not looking great, I don't think they're worth trying to get into M54.
Project Member

Comment 8 by bugdroid1@chromium.org, Feb 22 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4f6d14dbe9482803495eaee9377c0f2816f8f2d9

commit 4f6d14dbe9482803495eaee9377c0f2816f8f2d9
Author: dalecurtis <dalecurtis@chromium.org>
Date: Wed Feb 22 17:42:22 2017

Remove Media.UnderflowCount, add Media.UnderflowDuration for MSE.

The counts can be inferred by the sample volume for UnderflowDuration,
so there's no need to track both.

BUG= 641633 
TEST=manually checked histograms in chrome://histograms.

Review-Url: https://codereview.chromium.org/2707973005
Cr-Commit-Position: refs/heads/master@{#452113}

[modify] https://crrev.com/4f6d14dbe9482803495eaee9377c0f2816f8f2d9/media/blink/webmediaplayer_impl.cc
[modify] https://crrev.com/4f6d14dbe9482803495eaee9377c0f2816f8f2d9/media/blink/webmediaplayer_impl.h
[modify] https://crrev.com/4f6d14dbe9482803495eaee9377c0f2816f8f2d9/tools/metrics/histograms/histograms.xml

Labels: Merge-Request-57
Merge-Request for adding UMA to pick up some metrics with M57 stable.
Labels: -M-55 M-57
(Specifically for 4f6d14dbe9482803495eaee9377c0f2816f8f2d9)
Project Member

Comment 11 by sheriffbot@chromium.org, Feb 23 2017

Labels: -Merge-Request-57 Hotlist-Merge-Approved Merge-Approved-57
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 12 by bugdroid1@chromium.org, Feb 23 2017

Labels: -merge-approved-57 merge-merged-2987
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f828655ca154a1724302a7a840bdad39f348f129

commit f828655ca154a1724302a7a840bdad39f348f129
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Thu Feb 23 19:30:26 2017

Merge M57: "Remove Media.UnderflowCount, add Media.UnderflowDuration for MSE."

The counts can be inferred by the sample volume for UnderflowDuration,
so there's no need to track both.

BUG= 641633 
TEST=manually checked histograms in chrome://histograms.

Review-Url: https://codereview.chromium.org/2707973005
Cr-Commit-Position: refs/heads/master@{#452113}
(cherry picked from commit 4f6d14dbe9482803495eaee9377c0f2816f8f2d9)

Review-Url: https://codereview.chromium.org/2713883003 .
Cr-Commit-Position: refs/branch-heads/2987@{#663}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/f828655ca154a1724302a7a840bdad39f348f129/media/blink/webmediaplayer_impl.cc
[modify] https://crrev.com/f828655ca154a1724302a7a840bdad39f348f129/media/blink/webmediaplayer_impl.h
[modify] https://crrev.com/f828655ca154a1724302a7a840bdad39f348f129/tools/metrics/histograms/histograms.xml

Status: Fixed (was: Started)
Project Member

Comment 14 by bugdroid1@chromium.org, Feb 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d06eabc40101635e329c9e27e5008b123fdd815a

commit d06eabc40101635e329c9e27e5008b123fdd815a
Author: dalecurtis <dalecurtis@chromium.org>
Date: Fri Feb 24 23:43:29 2017

Fix underflow duration calculation for MSE.

http://crrev.com/452113 extended underflow duration tracking for MSE,
but did not actually enable the timer :(

BUG= 641633 
TEST=manual

Review-Url: https://codereview.chromium.org/2713323002
Cr-Commit-Position: refs/heads/master@{#453001}

[modify] https://crrev.com/d06eabc40101635e329c9e27e5008b123fdd815a/media/blink/webmediaplayer_impl.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Feb 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8aba48f58c9572e943340c94065adb27d5ee7f3a

commit 8aba48f58c9572e943340c94065adb27d5ee7f3a
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Fri Feb 24 23:52:07 2017

Merge M57: "Fix underflow duration calculation for MSE."

http://crrev.com/452113 extended underflow duration tracking for MSE,
but did not actually enable the timer :(

BUG= 641633 
TEST=manual

Review-Url: https://codereview.chromium.org/2713323002
Cr-Commit-Position: refs/heads/master@{#453001}
(cherry picked from commit d06eabc40101635e329c9e27e5008b123fdd815a)

Review-Url: https://codereview.chromium.org/2716813003 .
Cr-Commit-Position: refs/branch-heads/2987@{#685}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/8aba48f58c9572e943340c94065adb27d5ee7f3a/media/blink/webmediaplayer_impl.cc

Sign in to add a comment