[Media] Plumb a feature parameter for the background video keyframe distance.

[Video] Set the max keyframe distance for background optimization via FieldTrials.

Get a field trial variations param as the time in milliseconds. Pass it from
RenderViewHostImpl in the browser process to the renderer's RenderFrameImpl via
WebPreferences. Set the value in WebMediaPlayerParams when WebMediaPlayerImpl is
created.

BUG=663999
TEST=manually with chrome --enable-features="BackgroundVideoTrackOptimization<Study" --force-fieldtrials=Study/Group1 --force-fieldtrial-params=Study.Group1:max_keyframe_distance_ms/1000

Review-Url: https://codereview.chromium.org/2628313002
Cr-Commit-Position: refs/heads/master@{#444946}
Committed: https://chromium.googlesource.com/chromium/src/+/590011e3f3b68faf7f68592b3b931181e0cb4672

[whywhat, 2017-01-13 02:19:31]

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

[commit-bot: I haz the power, 2017-01-13 02:20:03]

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

[whywhat, 2017-01-13 02:22:17]

avayvod@chromium.org changed reviewers:
+ asvitkine@chromium.org, dalecurtis@chromium.org, nasko@chromium.org

[whywhat, 2017-01-13 02:22:18]

Dale: media/
Nasko: content/
Alexei: to check I'm using variations correctly and for testing (manual and/or
automated) suggestions.

[commit-bot: I haz the power, 2017-01-13 04:35:36]

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

[commit-bot: I haz the power, 2017-01-13 04:35:36]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2017-01-13 16:16:20]

code lgtm

In the CL description, usually the BUG & TEST lines go last.

Re: Testing, we do have support for manual and automated testing of this.

For manual testing, you can look at go/variations-params and check the same
command line that specifies --enable-features.

For automated testing, there's some helper classes you can use, such as
VariationParamsManager.

[nasko, 2017-01-13 16:30:04]

The code itself looks good. 

However, it will be best if variations are actually available in the renderer
process. Can you follow up to check why this isn't the case? If we have to plumb
each variation through the WebPreferences and content/ this will get out of
control.

https://codereview.chromium.org/2628313002/diff/1/content/public/common/web_p...
File content/public/common/web_preferences.h (right):

https://codereview.chromium.org/2628313002/diff/1/content/public/common/web_p...
content/public/common/web_preferences.h:266: // If background video track
optimization is enabled, don't disable video
nit: Keep the the style of the file and put an empty line above the comment.

[DaleCurtis, 2017-01-13 20:34:13]

https://codereview.chromium.org/2628313002/diff/1/content/public/common/web_p...
File content/public/common/web_preferences.h (right):

https://codereview.chromium.org/2628313002/diff/1/content/public/common/web_p...
content/public/common/web_preferences.h:269: int
max_keyframe_distance_to_disable_background_video_sec;
base::TimeDelta?

https://codereview.chromium.org/2628313002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2628313002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:2910:
media_player->SetMaxKeyframeDistanceToDisableBackgroundVideo(
Should this instead be part of WebMediaPlayerParams?

[Alexei Svitkine (slow), 2017-01-13 20:41:44]

Re: variations params in renderer processes:

This is coming and is on my OKRs this quarter, but not yet ready yet.

On Fri, Jan 13, 2017 at 3:34 PM, <dalecurtis@chromium.org> wrote:

>
> https://codereview.chromium.org/2628313002/diff/1/content/
> public/common/web_preferences.h
> File content/public/common/web_preferences.h (right):
>
> https://codereview.chromium.org/2628313002/diff/1/content/
> public/common/web_preferences.h#newcode269
> content/public/common/web_preferences.h:269: int
> max_keyframe_distance_to_disable_background_video_sec;
> base::TimeDelta?
>
> https://codereview.chromium.org/2628313002/diff/1/content/
> renderer/render_frame_impl.cc
> File content/renderer/render_frame_impl.cc (right):
>
> https://codereview.chromium.org/2628313002/diff/1/content/
> renderer/render_frame_impl.cc#newcode2910
> content/renderer/render_frame_impl.cc:2910:
> media_player->SetMaxKeyframeDistanceToDisableBackgroundVideo(
> Should this instead be part of WebMediaPlayerParams?
>
> https://codereview.chromium.org/2628313002/
>

-- 
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.

[whywhat, 2017-01-13 21:10:54]

Thanks, will look into tests.
Yeah, I checked with Alexei before hand and it seems like the support for the
renderer process is coming but after we want to run the experiment (M57).

Alexei, do you have a tracking bug I could put near a TODO to remove the
plumbing?

https://codereview.chromium.org/2628313002/diff/1/content/public/common/web_p...
File content/public/common/web_preferences.h (right):

https://codereview.chromium.org/2628313002/diff/1/content/public/common/web_p...
content/public/common/web_preferences.h:266: // If background video track
optimization is enabled, don't disable video
On 2017/01/13 at 16:30:04, nasko wrote:
> nit: Keep the the style of the file and put an empty line above the comment.

Done.

https://codereview.chromium.org/2628313002/diff/1/content/public/common/web_p...
content/public/common/web_preferences.h:269: int
max_keyframe_distance_to_disable_background_video_sec;
On 2017/01/13 at 20:34:13, DaleCurtis wrote:
> base::TimeDelta?

Hm, I've no idea if the IPC macros will handle anything that's not a primitive
type or a string. Will try :)

https://codereview.chromium.org/2628313002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2628313002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:2910:
media_player->SetMaxKeyframeDistanceToDisableBackgroundVideo(
On 2017/01/13 at 20:34:13, DaleCurtis wrote:
> Should this instead be part of WebMediaPlayerParams?

That seemed like a smaller change, given the temporary nature of this code: it's
an experiment and we might be able to query the params directly in WMPI by EOQ.

I don't know when WMPParams should be used? Dan?

[Alexei Svitkine (slow), 2017-01-13 21:17:22]

Just filed one here: crbug.com/681160

On Fri, Jan 13, 2017 at 4:10 PM, <avayvod@chromium.org> wrote:

> Thanks, will look into tests.
> Yeah, I checked with Alexei before hand and it seems like the support for
> the
> renderer process is coming but after we want to run the experiment (M57).
>
> Alexei, do you have a tracking bug I could put near a TODO to remove the
> plumbing?
>
>
> https://codereview.chromium.org/2628313002/diff/1/content/
> public/common/web_preferences.h
> File content/public/common/web_preferences.h (right):
>
> https://codereview.chromium.org/2628313002/diff/1/content/
> public/common/web_preferences.h#newcode266
> content/public/common/web_preferences.h:266: // If background video
> track optimization is enabled, don't disable video
> On 2017/01/13 at 16:30:04, nasko wrote:
> > nit: Keep the the style of the file and put an empty line above the
> comment.
>
> Done.
>
> https://codereview.chromium.org/2628313002/diff/1/content/
> public/common/web_preferences.h#newcode269
> content/public/common/web_preferences.h:269: int
> max_keyframe_distance_to_disable_background_video_sec;
> On 2017/01/13 at 20:34:13, DaleCurtis wrote:
> > base::TimeDelta?
>
> Hm, I've no idea if the IPC macros will handle anything that's not a
> primitive type or a string. Will try :)
>
> https://codereview.chromium.org/2628313002/diff/1/content/
> renderer/render_frame_impl.cc
> File content/renderer/render_frame_impl.cc (right):
>
> https://codereview.chromium.org/2628313002/diff/1/content/
> renderer/render_frame_impl.cc#newcode2910
> content/renderer/render_frame_impl.cc:2910:
> media_player->SetMaxKeyframeDistanceToDisableBackgroundVideo(
> On 2017/01/13 at 20:34:13, DaleCurtis wrote:
> > Should this instead be part of WebMediaPlayerParams?
>
> That seemed like a smaller change, given the temporary nature of this
> code: it's an experiment and we might be able to query the params
> directly in WMPI by EOQ.
>
> I don't know when WMPParams should be used? Dan?
>
> https://codereview.chromium.org/2628313002/
>

-- 
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.

[whywhat, 2017-01-13 21:53:45]

Description was changed from

==========
[Media] Plumb a feature parameter for the background video keyframe distance.

BUG=663999
TEST=???

Use WebPreferences to pass the value from the browser process where
variations are supported to the renderer. Set the value later when
WebMediaPlayerImpl is created in RenderFrameImpl.
==========

to

==========
[Media] Plumb a feature parameter for the background video keyframe distance.

Use WebPreferences to pass the value from the browser process where
variations are supported to the renderer. Set the value later when
WebMediaPlayerImpl is created in RenderFrameImpl.

BUG=663999
TEST=???
==========

[whywhat, 2017-01-13 22:23:41]

Addressed comments

[DaleCurtis, 2017-01-14 00:28:04]

https://codereview.chromium.org/2628313002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2628313002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:2910:
media_player->SetMaxKeyframeDistanceToDisableBackgroundVideo(
On 2017/01/13 at 21:10:54, whywhat wrote:
> On 2017/01/13 at 20:34:13, DaleCurtis wrote:
> > Should this instead be part of WebMediaPlayerParams?
> 
> That seemed like a smaller change, given the temporary nature of this code:
it's an experiment and we might be able to query the params directly in WMPI by
EOQ.
> 
> I don't know when WMPParams should be used? Dan?

It should always be used for one time values that are never changed from
construction onward.

[whywhat, 2017-01-18 19:42:32]

https://codereview.chromium.org/2628313002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2628313002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:2910:
media_player->SetMaxKeyframeDistanceToDisableBackgroundVideo(
On 2017/01/14 at 00:28:04, DaleCurtis wrote:
> On 2017/01/13 at 21:10:54, whywhat wrote:
> > On 2017/01/13 at 20:34:13, DaleCurtis wrote:
> > > Should this instead be part of WebMediaPlayerParams?
> > 
> > That seemed like a smaller change, given the temporary nature of this code:
it's an experiment and we might be able to query the params directly in WMPI by
EOQ.
> > 
> > I don't know when WMPParams should be used? Dan?
> 
> It should always be used for one time values that are never changed from
construction onward.

Ok, let me see how bigger the change will be due to modifying the params ctor.

[whywhat, 2017-01-18 19:48:38]

Set the experiment value via params

[whywhat, 2017-01-18 20:43:06]

Fix compile

[whywhat, 2017-01-18 21:54:04]

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

[commit-bot: I haz the power, 2017-01-18 21:54:38]

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-18 22:19:21]

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

[commit-bot: I haz the power, 2017-01-18 22:19:22]

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...)

[whywhat, 2017-01-18 22:30:50]

Fixed unittests

[whywhat, 2017-01-18 22:31:31]

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

[whywhat, 2017-01-18 22:31:51]

Changed to WMPParams. PTAL.

[commit-bot: I haz the power, 2017-01-18 22:32:22]

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-18 23:54:39]

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

[commit-bot: I haz the power, 2017-01-18 23:54:40]

Dry run: This issue passed the CQ dry run.

[whywhat, 2017-01-19 00:17:43]

Removed unnecessary changes.

[whywhat, 2017-01-19 05:17:15]

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

[commit-bot: I haz the power, 2017-01-19 05:17:42]

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-19 06:24:05]

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

[commit-bot: I haz the power, 2017-01-19 06:24:06]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[DaleCurtis, 2017-01-19 17:33:13]

lgtm

[whywhat, 2017-01-19 18:22:49]

avayvod@chromium.org changed reviewers:
+ dcheng@chromium.org, jam@chromium.org
- nasko@chromium.org

[whywhat, 2017-01-19 18:22:49]

+dcheng for IPC
+jam for the remaining OWNERS

nasko seems to be OOO today

[whywhat, 2017-01-19 20:46:01]

avayvod@chromium.org changed reviewers:
+ avi@chromium.org, rsesek@chromium.org
- dcheng@chromium.org, jam@chromium.org

[Avi (use Gerrit), 2017-01-19 20:58:52]

lgtm

stamp

[Robert Sesek, 2017-01-19 21:11:43]

lgtm

[whywhat, 2017-01-19 22:14:22]

Changed the param to milliseconds

[whywhat, 2017-01-19 22:18:53]

Description was changed from

==========
[Media] Plumb a feature parameter for the background video keyframe distance.

Use WebPreferences to pass the value from the browser process where
variations are supported to the renderer. Set the value later when
WebMediaPlayerImpl is created in RenderFrameImpl.

BUG=663999
TEST=???
==========

to

==========
[Video] Set the max keyframe distance for background optimization via
FieldTrials.

Get a field trial variations param as the time in milliseconds. Pass it from
RenderViewHostImpl in the browser process to the renderer's RenderFrameImpl via
WebPreferences. Set the value in WebMediaPlayerParams when WebMediaPlayerImpl is
created.

BUG=663999
TEST=manually with chrome
--enable-features="BackgroundVideoTrackOptimization<Study"
--force-fieldtrials=Study/Group1
--force-fieldtrial-params=Study.Group1:max_keyframe_distance_ms/1000
==========

[whywhat, 2017-01-19 22:18:58]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2017-01-19 22:18:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, rsesek@chromium.org, avi@chromium.org,
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/2628313002/#ps120001
(title: "Changed the param to milliseconds")

[commit-bot: I haz the power, 2017-01-19 22:19:31]

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-20 02:01:04]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484864338890870,
"parent_rev": "bcfa0007f638e46826cf0803746dfd90c53d6fc4", "commit_rev":
"590011e3f3b68faf7f68592b3b931181e0cb4672"}

[commit-bot: I haz the power, 2017-01-20 02:01:40]

Description was changed from

==========
[Video] Set the max keyframe distance for background optimization via
FieldTrials.

Get a field trial variations param as the time in milliseconds. Pass it from
RenderViewHostImpl in the browser process to the renderer's RenderFrameImpl via
WebPreferences. Set the value in WebMediaPlayerParams when WebMediaPlayerImpl is
created.

BUG=663999
TEST=manually with chrome
--enable-features="BackgroundVideoTrackOptimization<Study"
--force-fieldtrials=Study/Group1
--force-fieldtrial-params=Study.Group1:max_keyframe_distance_ms/1000
==========

to

==========
[Video] Set the max keyframe distance for background optimization via
FieldTrials.

Get a field trial variations param as the time in milliseconds. Pass it from
RenderViewHostImpl in the browser process to the renderer's RenderFrameImpl via
WebPreferences. Set the value in WebMediaPlayerParams when WebMediaPlayerImpl is
created.

BUG=663999
TEST=manually with chrome
--enable-features="BackgroundVideoTrackOptimization<Study"
--force-fieldtrials=Study/Group1
--force-fieldtrial-params=Study.Group1:max_keyframe_distance_ms/1000

Review-Url: https://codereview.chromium.org/2628313002
Cr-Commit-Position: refs/heads/master@{#444946}
Committed:
https://chromium.googlesource.com/chromium/src/+/590011e3f3b68faf7f68592b3b93...
==========

[commit-bot: I haz the power, 2017-01-20 02:01:41]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/590011e3f3b68faf7f68592b3b93...

[dcheng, 2017-01-20 10:50:17]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2017-01-20 10:50:18]

IPC lgtm