[Video] Collect average keyframe distance for video

[Video] Collect average keyframe distance for video

BUG=670150
TEST=None

Committed: https://crrev.com/25bc24a5a449737281cd63e7deae7ea9777ba425
Cr-Commit-Position: refs/heads/master@{#437287}

[whywhat, 2016-12-02 18:31:31]

Added histogram logging

[whywhat, 2016-12-02 18:42:50]

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

[whywhat, 2016-12-02 18:42:51]

Hey Dale, could you take a first look to confirm I'm digging in the right
direction.

I tested it locally on Linux and couldn't trigger RendererWrapper::Stop to be
called by closing the tab or waiting until the video finishes and so on :/ How
is it supposed to work?

Would it be the right way to accumulate the stats or should I rather collect the
total time delta and the key frames count and divide when recording?

I suspect I could also only keep the last key frame timestamp as it's in media
time so it would be more or less equal to the total distance between the key
frames (assuming there's always a keyframe with distance 0).

Not sure I take stopping the decoding into account though.

[DaleCurtis, 2016-12-02 18:52:06]

I wouldn't worry about trying to record at Stop, just continously record in a
way that makes sense. Renderers are killed so you can't rely on UMA being
finalized at that point unfortunately. The way we generally work around this, if
we need a single data point, is to report it to the media event log and let it
record it.

In this case I think you could keep a running average and report it every few 10
seconds or so; perhaps choosing the mean value based on watch time. You might
also just record it when the max value changes.

Decoder stream seems like a fine place to do this.

[whywhat, 2016-12-02 23:39:12]

Removed spammy logs

[whywhat, 2016-12-02 23:41:06]

PTAL

Would UMA macros work without performance impact directly from the
decoder_stream_traits (which is IIUC on the media thread). The docs for
histograms say not to worry about performance but then it's a video decoding
pipeline :)

[DaleCurtis, 2016-12-03 01:24:57]

UMA folk should way in on the best stat to report here.

https://codereview.chromium.org/2545523005/diff/40001/media/filters/decoder_s...
File media/filters/decoder_stream_traits.cc (right):

https://codereview.chromium.org/2545523005/diff/40001/media/filters/decoder_s...
media/filters/decoder_stream_traits.cc:81: namespace {
Just inline this if it's only used in one spot. Add a comment there about why
you chose this value.

https://codereview.chromium.org/2545523005/diff/40001/media/filters/decoder_s...
media/filters/decoder_stream_traits.cc:132: ResetMetrics();
Why bother if we only care about the max value? This is all still within the
same stream.

https://codereview.chromium.org/2545523005/diff/40001/media/filters/decoder_s...
media/filters/decoder_stream_traits.cc:138: if (!buffer.get())
Drop .get()

https://codereview.chromium.org/2545523005/diff/40001/media/filters/decoder_s...
media/filters/decoder_stream_traits.cc:162:
UMA_HISTOGRAM_MEDIUM_TIMES("Media.Video.KeyFrameDistanceAverage",
I wonder if instead you just want to record every delta and let the UMA sampling
process take care of the actual averaging. The question we're trying to answer
is if a given stream has keyframes near enough for us to enable the
disable-track optimization.

[whywhat, 2016-12-03 01:55:52]

avayvod@chromium.org changed reviewers:
+ isherman@chromium.org

[whywhat, 2016-12-03 01:55:52]

+Ilya for the histograms.xml and opinion on UMA

[whywhat, 2016-12-05 19:20:20]

https://codereview.chromium.org/2545523005/diff/40001/media/filters/decoder_s...
File media/filters/decoder_stream_traits.cc (right):

https://codereview.chromium.org/2545523005/diff/40001/media/filters/decoder_s...
media/filters/decoder_stream_traits.cc:81: namespace {
On 2016/12/03 at 01:24:57, DaleCurtis wrote:
> Just inline this if it's only used in one spot. Add a comment there about why
you chose this value.

I just guessed :) How would you choose the number here?

https://codereview.chromium.org/2545523005/diff/40001/media/filters/decoder_s...
media/filters/decoder_stream_traits.cc:132: ResetMetrics();
On 2016/12/03 at 01:24:57, DaleCurtis wrote:
> Why bother if we only care about the max value? This is all still within the
same stream.

I'm not entirely sure when OnStreamReset is called.
I'm worried about the case when we disable the video track for a while - it
seems like the next decoded frame timestamp might be far from the last one which
would bump the max distance up by the time the video was not decoded. At least,
that's what I saw while testing my change -- although it seems like the reset in
end_of_stream() case in OnDecode took care of that.

Perhaps, I don't need to reset |max| at least.

https://codereview.chromium.org/2545523005/diff/40001/media/filters/decoder_s...
media/filters/decoder_stream_traits.cc:162:
UMA_HISTOGRAM_MEDIUM_TIMES("Media.Video.KeyFrameDistanceAverage",
On 2016/12/03 at 01:24:57, DaleCurtis wrote:
> I wonder if instead you just want to record every delta and let the UMA
sampling process take care of the actual averaging. The question we're trying to
answer is if a given stream has keyframes near enough for us to enable the
disable-track optimization.

For UMA, perhaps, but when we try to decide whether to disable the video track
or not, won't we need to calculate the average across a few frames to decide?
I'm okay re-adding it in some next patch.

[Ilya Sherman, 2016-12-05 23:48:04]

https://codereview.chromium.org/2545523005/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2545523005/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:25335: +    every time the average
increases from the previous one.
Recording every time the average increases, but not when it decreases, seems
like an odd choice; I'd expect it to make the data hard to interpret.  If you
just want to record the max seen over the lifetime of the video, could you
record it after the video finishes playing?

[whywhat, 2016-12-06 05:16:28]

https://codereview.chromium.org/2545523005/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2545523005/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:25335: +    every time the average
increases from the previous one.
On 2016/12/05 at 23:48:04, Ilya Sherman wrote:
> Recording every time the average increases, but not when it decreases, seems
like an odd choice; I'd expect it to make the data hard to interpret.  If you
just want to record the max seen over the lifetime of the video, could you
record it after the video finishes playing?

I think the main reason to report it continuously is because I suspect the
majority of videos are not watched till the end (renderer gets killed, the user
doesn't watch until the titles finish and so on).

We care about the average frame distance to avoid excluding videos with a few
frames being rare from the optimization (the max could be much bigger than
average) we'd like to enable, but reporting it only when max changes presumably
makes the reporting less frequent - average frame distance may change a bit with
every key frame but max seems to stabilize over time in my local testing.

Dale suggested reporting the actual distance for each keyframe (as an example:
every 5s for YouTube, every 0.5s for Netflix) and have histograms take care of
accumulating. WDYT?

[whywhat, 2016-12-06 16:44:12]

Addressed some Dale's comments

[DaleCurtis, 2016-12-06 21:31:05]

Code lgtm, but defer on histogram usage. The question we're trying to answer is
what percentage of playbacks would we allow background-track-disable on at a
given keyframe distance.

https://codereview.chromium.org/2545523005/diff/60001/media/filters/decoder_s...
File media/filters/decoder_stream_traits.cc (right):

https://codereview.chromium.org/2545523005/diff/60001/media/filters/decoder_s...
media/filters/decoder_stream_traits.cc:107: // The samples count for keyframe
distance average is picked arbitrarily.
Generally just put over the variable name.

[whywhat, 2016-12-06 21:45:09]

Moved the comment

[Ilya Sherman, 2016-12-06 22:03:55]

https://codereview.chromium.org/2545523005/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2545523005/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:25335: +    every time the average
increases from the previous one.
On 2016/12/06 05:16:28, whywhat_OOO_till_Mon_Nov_28 wrote:
> On 2016/12/05 at 23:48:04, Ilya Sherman wrote:
> > Recording every time the average increases, but not when it decreases, seems
> like an odd choice; I'd expect it to make the data hard to interpret.  If you
> just want to record the max seen over the lifetime of the video, could you
> record it after the video finishes playing?
> 
> I think the main reason to report it continuously is because I suspect the
> majority of videos are not watched till the end (renderer gets killed, the
user
> doesn't watch until the titles finish and so on).
> 
> We care about the average frame distance to avoid excluding videos with a few
> frames being rare from the optimization (the max could be much bigger than
> average) we'd like to enable, but reporting it only when max changes
presumably
> makes the reporting less frequent - average frame distance may change a bit
with
> every key frame but max seems to stabilize over time in my local testing.
> 
> Dale suggested reporting the actual distance for each keyframe (as an example:
> every 5s for YouTube, every 0.5s for Netflix) and have histograms take care of
> accumulating. WDYT?

I think recording once every keyframe makes more sense, in that it'll be easier
to understand the data.  Otherwise, I'd expect that you'd have a very odd bias:
videos where the max distance changes more often get more votes; and videos
where the max is quite different from the typical average will report unhelpful
values.  The downside of recording for every keyframe is that it doesn't give
you a great way to get an average per video, since data recorded to histograms
lacks timestamps.

Here's a question then: When you're deciding whether or not to apply the
optimization, how will you make that decision?  Presumably it'll be based on
some heuristic, since you won't have the full video stream available
immediately.  Would it make sense to record that heuristic prediction (or if
you're trying to calibrate that heuristic, the values that go into making the
prediction)?

[whywhat, 2016-12-07 21:02:46]

On 2016/12/06 at 22:03:55, isherman wrote:
>
https://codereview.chromium.org/2545523005/diff/40001/tools/metrics/histogram...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/2545523005/diff/40001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:25335: +    every time the average
increases from the previous one.
> On 2016/12/06 05:16:28, whywhat_OOO_till_Mon_Nov_28 wrote:
> > On 2016/12/05 at 23:48:04, Ilya Sherman wrote:
> > > Recording every time the average increases, but not when it decreases,
seems
> > like an odd choice; I'd expect it to make the data hard to interpret.  If
you
> > just want to record the max seen over the lifetime of the video, could you
> > record it after the video finishes playing?
> > 
> > I think the main reason to report it continuously is because I suspect the
> > majority of videos are not watched till the end (renderer gets killed, the
user
> > doesn't watch until the titles finish and so on).
> > 
> > We care about the average frame distance to avoid excluding videos with a
few
> > frames being rare from the optimization (the max could be much bigger than
> > average) we'd like to enable, but reporting it only when max changes
presumably
> > makes the reporting less frequent - average frame distance may change a bit
with
> > every key frame but max seems to stabilize over time in my local testing.
> > 
> > Dale suggested reporting the actual distance for each keyframe (as an
example:
> > every 5s for YouTube, every 0.5s for Netflix) and have histograms take care
of
> > accumulating. WDYT?
> 
> I think recording once every keyframe makes more sense, in that it'll be
easier to understand the data.  Otherwise, I'd expect that you'd have a very odd
bias: videos where the max distance changes more often get more votes; and
videos where the max is quite different from the typical average will report
unhelpful values.  The downside of recording for every keyframe is that it
doesn't give you a great way to get an average per video, since data recorded to
histograms lacks timestamps.
> 
> Here's a question then: When you're deciding whether or not to apply the
optimization, how will you make that decision?  Presumably it'll be based on
some heuristic, since you won't have the full video stream available
immediately.  Would it make sense to record that heuristic prediction (or if
you're trying to calibrate that heuristic, the values that go into making the
prediction)?

I think we'll just accumulate the moving average of the key frame distance over
few key frames and have some threshold (e.g. if it's above 10s, we don't apply
it). We were planning to run experiments with different threshold values once we
implement the success experiment metric (that metric is being implemented in
https://codereview.chromium.org/2552493002).

I think Dale's idea is that we don't have to _record_ the moving average since
histograms will average the data anyway.

[whywhat, 2016-12-07 22:54:35]

Record only the distance between frames

[whywhat, 2016-12-07 22:58:38]

The CQ bit was checked by avayvod@chromium.org to run a CQ dry run

[whywhat, 2016-12-07 22:59:08]

I went ahead and simplified this to simply record distance between each key
frame pair.

[commit-bot: I haz the power, 2016-12-07 22:59:24]

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

[DaleCurtis, 2016-12-07 23:02:43]

lgtm % nit.

https://codereview.chromium.org/2545523005/diff/100001/media/filters/decoder_...
File media/filters/decoder_stream_traits.cc (right):

https://codereview.chromium.org/2545523005/diff/100001/media/filters/decoder_...
media/filters/decoder_stream_traits.cc:128: if (!buffer)
This can't ever be true or you've already deref'd a nullptr :)

[whywhat, 2016-12-08 00:23:18]

Fixed early exit conditions

[whywhat, 2016-12-08 00:23:30]

https://codereview.chromium.org/2545523005/diff/100001/media/filters/decoder_...
File media/filters/decoder_stream_traits.cc (right):

https://codereview.chromium.org/2545523005/diff/100001/media/filters/decoder_...
media/filters/decoder_stream_traits.cc:128: if (!buffer)
On 2016/12/07 at 23:02:43, DaleCurtis wrote:
> This can't ever be true or you've already deref'd a nullptr :)

Oops, weird auto formatting obscuring a missing return I guess...

[whywhat, 2016-12-08 01:00:45]

The CQ bit was checked by avayvod@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 01:01:28]

Dry run: 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-12-08 03:58:54]

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

[commit-bot: I haz the power, 2016-12-08 03:58:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ilya Sherman, 2016-12-08 04:55:27]

Metrics LGTM, thanks.

[whywhat, 2016-12-08 16:54:41]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2016-12-08 16:54:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/2545523005/#ps120001
(title: "Fixed early exit conditions")

[commit-bot: I haz the power, 2016-12-08 16:54:56]

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-12-08 18:21:16]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481216081926850,
"parent_rev": "c8f65081e453bfed0230f139ae34f5a8a8dec4d4", "commit_rev":
"d48cdfdba8681c3edea5734713ba5ef10f8b6fa3"}

[commit-bot: I haz the power, 2016-12-08 18:21:42]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-12-08 18:25:50]

Description was changed from

==========
[Video] Collect average keyframe distance for video

BUG=670150
TEST=None
==========

to

==========
[Video] Collect average keyframe distance for video

BUG=670150
TEST=None

Committed: https://crrev.com/25bc24a5a449737281cd63e7deae7ea9777ba425
Cr-Commit-Position: refs/heads/master@{#437287}
==========

[commit-bot: I haz the power, 2016-12-08 18:25:51]

Patchset 7 (id:??) landed as
https://crrev.com/25bc24a5a449737281cd63e7deae7ea9777ba425
Cr-Commit-Position: refs/heads/master@{#437287}