[Video] Disable bg optimization if avg keyframe distance is >10s

[Video] Disable bg optimization if avg keyframe distance is >10s

Just picked 10s arbitratily before we have a way to set this value via
Finch. Sending this out to see if collecting the average keyframe
distance and passing it via PipelineStatistics sounds reasonable.

BUG=663999
TEST=manual

Review-Url: https://codereview.chromium.org/2611333003
Cr-Commit-Position: refs/heads/master@{#443424}
Committed: https://chromium.googlesource.com/chromium/src/+/c4bfb0e6139a447f0b4eeb31d34064b7ebf0fdcc

[whywhat, 2017-01-07 03:10:47]

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

[whywhat, 2017-01-07 03:10:48]

PTaL

[DaleCurtis, 2017-01-09 20:29:54]

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

https://codereview.chromium.org/2611333003/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:2085: bool
WebMediaPlayerImpl::ShouldDecodeFastWhenShown() const {
This name is very strange. Something like IsVideoSuitableForBackgroundDisable?
would be better. I'd probably just roll it into the
IsBackgroundVideoTrackOptimizationEnabled() function so folk don't need to worry
about two functions when checking.

[whywhat, 2017-01-10 01:06:04]

On 2017/01/09 at 20:29:54, dalecurtis wrote:
>
https://codereview.chromium.org/2611333003/diff/1/media/blink/webmediaplayer_...
> File media/blink/webmediaplayer_impl.cc (right):
> 
>
https://codereview.chromium.org/2611333003/diff/1/media/blink/webmediaplayer_...
> media/blink/webmediaplayer_impl.cc:2085: bool
WebMediaPlayerImpl::ShouldDecodeFastWhenShown() const {
> This name is very strange. Something like IsVideoSuitableForBackgroundDisable?
would be better. I'd probably just roll it into the
IsBackgroundVideoTrackOptimizationEnabled() function so folk don't need to worry
about two functions when checking.

Agree, I would probably leave the name as IsVideoSuitableForBackgroundDisable()
at this point.

[whywhat, 2017-01-10 04:11:53]

Changed the name and set a minimum number of samples

[whywhat, 2017-01-10 06:27:56]

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

[commit-bot: I haz the power, 2017-01-10 06:28:17]

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, 2017-01-10 08:05:03]

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

[commit-bot: I haz the power, 2017-01-10 08:05:04]

Dry run: This issue passed the CQ dry run.

[DaleCurtis, 2017-01-10 21:11:40]

https://codereview.chromium.org/2611333003/diff/20001/media/base/pipeline_sta...
File media/base/pipeline_status.h (right):

https://codereview.chromium.org/2611333003/diff/20001/media/base/pipeline_sta...
media/base/pipeline_status.h:62: int64_t video_keyframe_distance_average_ms = 0;
Just use a base::TimeDelta?

https://codereview.chromium.org/2611333003/diff/20001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2611333003/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:2121: return
IsBackgroundVideoTrackOptimizationEnabled() && hasVideo() &&
Early return before grabbing statistics if the rest of this is false.

https://codereview.chromium.org/2611333003/diff/20001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2611333003/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.h:626: // 10 seconds by default but can be
overridden by a Finch experiment.
This statement isn't true at the moment? Are you sure you can override the value
via Finch config? I thought you could only select from predefined values.

[whywhat, 2017-01-10 21:51:46]

Changed to TimeDelta

[whywhat, 2017-01-10 21:51:55]

https://codereview.chromium.org/2611333003/diff/20001/media/base/pipeline_sta...
File media/base/pipeline_status.h (right):

https://codereview.chromium.org/2611333003/diff/20001/media/base/pipeline_sta...
media/base/pipeline_status.h:62: int64_t video_keyframe_distance_average_ms = 0;
On 2017/01/10 at 21:11:39, DaleCurtis wrote:
> Just use a base::TimeDelta?

Done.

https://codereview.chromium.org/2611333003/diff/20001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2611333003/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:2121: return
IsBackgroundVideoTrackOptimizationEnabled() && hasVideo() &&
On 2017/01/10 at 21:11:40, DaleCurtis wrote:
> Early return before grabbing statistics if the rest of this is false.

Done.

https://codereview.chromium.org/2611333003/diff/20001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2611333003/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.h:626: // 10 seconds by default but can be
overridden by a Finch experiment.
On 2017/01/10 at 21:11:40, DaleCurtis wrote:
> This statement isn't true at the moment? Are you sure you can override the
value via Finch config? I thought you could only select from predefined values.

I was advised to use GetVariationParamValueByFeature()
(https://cs.chromium.org/chromium/src/components/variations/variations_associa...)
by the Finch team.

Not true at the moment though.

[DaleCurtis, 2017-01-11 01:14:51]

lg, can you add a WMPI test?

https://codereview.chromium.org/2611333003/diff/40001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2611333003/diff/40001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:2125: return false;
Multiline if needs {}

https://codereview.chromium.org/2611333003/diff/40001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2611333003/diff/40001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.h:627: base::TimeDelta
max_keyframe_distance_for_fast_decode_ =
Replace fast_decode_ terminology like before.

[whywhat, 2017-01-11 17:14:32]

Pass the value to WMPI and fix comments

[whywhat, 2017-01-11 17:17:33]

Do you have any suggestions in mind for testing?

I realized I could easily unittest other conditions of
ShouldDisableVideoWhenHidden except for the one added in this CL, but changing
the statistics from the test is hardly possible.

I could add SetPipelineStatisticsForTesting to WMPI and check if it was set in
GetStatistics() before returning the actual thing?

https://codereview.chromium.org/2611333003/diff/40001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2611333003/diff/40001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:2125: return false;
On 2017/01/11 at 01:14:51, DaleCurtis wrote:
> Multiline if needs {}

Done.

https://codereview.chromium.org/2611333003/diff/40001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2611333003/diff/40001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.h:627: base::TimeDelta
max_keyframe_distance_for_fast_decode_ =
On 2017/01/11 at 01:14:51, DaleCurtis wrote:
> Replace fast_decode_ terminology like before.

Done.

[whywhat, 2017-01-11 20:00:09]

Unittest

[DaleCurtis, 2017-01-11 23:44:48]

lgtm, thanks!

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

https://codereview.chromium.org/2611333003/diff/80001/media/filters/decoder_s...
media/filters/decoder_stream_traits.cc:113:
statistics.video_keyframe_distance_average = base::TimeDelta::Max();
We typically use kNoTimestamp() in media for this.

[whywhat, 2017-01-12 00:16:32]

Thanks for the reviews!

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

https://codereview.chromium.org/2611333003/diff/80001/media/filters/decoder_s...
media/filters/decoder_stream_traits.cc:113:
statistics.video_keyframe_distance_average = base::TimeDelta::Max();
On 2017/01/11 at 23:44:48, DaleCurtis wrote:
> We typically use kNoTimestamp() in media for this.

That would be a minimum value though so we will enable the optimization (unless
we special case kNoTimestamp in ShouldDisableVideoIfHidden) -- my intention was
to put the max value so we don't disable the video track in this case. WDYT?

[whywhat, 2017-01-12 00:16:39]

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

[commit-bot: I haz the power, 2017-01-12 00:18:11]

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, 2017-01-12 01:00:55]

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

[commit-bot: I haz the power, 2017-01-12 01:00:56]

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

[DaleCurtis, 2017-01-12 01:13:38]

lgtm either way

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

https://codereview.chromium.org/2611333003/diff/80001/media/filters/decoder_s...
media/filters/decoder_stream_traits.cc:113:
statistics.video_keyframe_distance_average = base::TimeDelta::Max();
On 2017/01/12 at 00:16:32, whywhat wrote:
> On 2017/01/11 at 23:44:48, DaleCurtis wrote:
> > We typically use kNoTimestamp() in media for this.
> 
> That would be a minimum value though so we will enable the optimization
(unless we special case kNoTimestamp in ShouldDisableVideoIfHidden) -- my
intention was to put the max value so we don't disable the video track in this
case. WDYT?

Ah, I guess that works, but anyone looking at statistics may be a bit surprised.

[whywhat, 2017-01-12 16:58:35]

Rebase the test

[whywhat, 2017-01-12 17:05:24]

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

[commit-bot: I haz the power, 2017-01-12 17:06:08]

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, 2017-01-12 21:01:47]

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

[commit-bot: I haz the power, 2017-01-12 21:01:48]

Dry run: This issue passed the CQ dry run.

[whywhat, 2017-01-12 23:43:26]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2017-01-12 23:43:26]

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/2611333003/#ps100001
(title: "Rebase the test")

[commit-bot: I haz the power, 2017-01-12 23:44:56]

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

[commit-bot: I haz the power, 2017-01-12 23:44:57]

This CL has an open dependency (Issue 2618883002 Patch 160001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[whywhat, 2017-01-13 00:19:52]

The CQ bit was checked by avayvod@chromium.org

[commit-bot: I haz the power, 2017-01-13 00:21:42]

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, 2017-01-13 01:07:05]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484266792094040,
"parent_rev": "c805e12955f1f3b6030f604a6ff5fd3f909a458a", "commit_rev":
"c4bfb0e6139a447f0b4eeb31d34064b7ebf0fdcc"}

[commit-bot: I haz the power, 2017-01-13 01:08:51]

Description was changed from

==========
[Video] Disable bg optimization if avg keyframe distance is >10s

Just picked 10s arbitratily before we have a way to set this value via
Finch. Sending this out to see if collecting the average keyframe
distance and passing it via PipelineStatistics sounds reasonable.

BUG=663999
TEST=manual
==========

to

==========
[Video] Disable bg optimization if avg keyframe distance is >10s

Just picked 10s arbitratily before we have a way to set this value via
Finch. Sending this out to see if collecting the average keyframe
distance and passing it via PipelineStatistics sounds reasonable.

BUG=663999
TEST=manual

Review-Url: https://codereview.chromium.org/2611333003
Cr-Commit-Position: refs/heads/master@{#443424}
Committed:
https://chromium.googlesource.com/chromium/src/+/c4bfb0e6139a447f0b4eeb31d340...
==========

[commit-bot: I haz the power, 2017-01-13 01:08:54]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/c4bfb0e6139a447f0b4eeb31d340...