Disable background video track behind a feature flag

Disable background video track behind a feature flag

BUG=663999
TEST=unit test + manual testing with the flag on - see chrome://media-internals logging track selection

Committed: https://crrev.com/39c1024093bdd21abd2c2ea3de5c05f7b9fee8ce
Cr-Commit-Position: refs/heads/master@{#434255}

[whywhat, 2016-11-10 03:32:30]

avayvod@chromium.org changed reviewers:
+ mlamouri@chromium.org, servolk@chromium.org

[whywhat, 2016-11-10 03:32:31]

PTaL

[whywhat, 2016-11-10 03:32:34]

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

[commit-bot: I haz the power, 2016-11-10 03:32:45]

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-11-10 04:27:24]

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

[commit-bot: I haz the power, 2016-11-10 04:27:25]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[whywhat, 2016-11-10 16:15:32]

Added the feature flag to histograms.xml

[whywhat, 2016-11-10 16:15:46]

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

[commit-bot: I haz the power, 2016-11-10 16:16:14]

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-11-10 18:32:42]

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

[commit-bot: I haz the power, 2016-11-10 18:32:43]

Dry run: This issue passed the CQ dry run.

[whywhat, 2016-11-11 02:04:21]

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

[whywhat, 2016-11-11 02:04:23]

+Dale

[mlamouri (slow - plz ping), 2016-11-12 23:35:48]

https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left):

https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2556:
DCHECK(RuntimeEnabledFeatures::audioVideoTracksEnabled());
Here and below, did you have to remove the checks? When is it called? At least,
in the case of audio, it sounds odd that it would be called.

https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3188:
webMediaPlayer()->selectedVideoTrackChanged(nullptr);
Here and below, you go from WMPI to HTMLMediaElement to then go back to WMPI.
Any reason why not implementing this directly in WMPI?

https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp (right):

https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp:188: }
tests <3

https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:269:
BackgroundVideoTrackOptimization status=stable
Shouldn't this have no status if it is controlled by the chrome flag?

[whywhat, 2016-11-14 21:50:13]

https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left):

https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2556:
DCHECK(RuntimeEnabledFeatures::audioVideoTracksEnabled());
On 2016/11/12 at 23:35:48, mlamouri (slow) wrote:
> Here and below, did you have to remove the checks? When is it called? At
least, in the case of audio, it sounds odd that it would be called.

When the Media Tracks API is enabled, HTMLMediaElement creates default
placeholder tracks for the media so it's consistent and I want to create a
default video track to be able to unselect it. I'm not sure if creating just the
video track and ignoring the audio one would work and it would probably make the
existing code that creates the default tracks more complex.

See createPlaceholderTracksIfNecessary below.

https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3188:
webMediaPlayer()->selectedVideoTrackChanged(nullptr);
On 2016/11/12 at 23:35:48, mlamouri (slow) wrote:
> Here and below, you go from WMPI to HTMLMediaElement to then go back to WMPI.
Any reason why not implementing this directly in WMPI?

1. I can restore the previously selected video track easily as HTMLMediaElement
has access to those.
2. It works with both WMPI and WMPA (I assume Sergey's implementation works with
both).
3. In WMPI I'd have to change updatePlayState_computePlayState to react to the
backgrounded state change while also playing and so on... It doesn't really deal
with selected tracks at all right now.

https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:269:
BackgroundVideoTrackOptimization status=stable
On 2016/11/12 at 23:35:48, mlamouri (slow) wrote:
> Shouldn't this have no status if it is controlled by the chrome flag?

Yes, a leftover from manual testing :)

[DaleCurtis, 2016-11-14 21:52:48]

dalecurtis@chromium.org changed reviewers:
+ sandersd@chromium.org
- dalecurtis@chromium.org

[DaleCurtis, 2016-11-14 21:52:49]

=>sandersd for review since he's changing some of that media/ code right now.
Overall this is fine with me to land disabled by default.

[sandersd (OOO until July 31), 2016-11-15 00:09:11]

https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3188:
webMediaPlayer()->selectedVideoTrackChanged(nullptr);
On 2016/11/14 21:50:13, whywhat wrote:
> On 2016/11/12 at 23:35:48, mlamouri (slow) wrote:
> > Here and below, you go from WMPI to HTMLMediaElement to then go back to
WMPI.
> Any reason why not implementing this directly in WMPI?
> 
> 1. I can restore the previously selected video track easily as
HTMLMediaElement
> has access to those.
> 2. It works with both WMPI and WMPA (I assume Sergey's implementation works
with
> both).
> 3. In WMPI I'd have to change updatePlayState_computePlayState to react to the
> backgrounded state change while also playing and so on... It doesn't really
deal
> with selected tracks at all right now.

This also looks strange from the WMPI side (which independently checks
IsHidden() to decide what to do). At the very least more comments are necessary
to explain those choices.

[whywhat, 2016-11-15 02:56:02]

On 2016/11/15 at 00:09:11, sandersd wrote:
>
https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):
> 
>
https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3188:
webMediaPlayer()->selectedVideoTrackChanged(nullptr);
> On 2016/11/14 21:50:13, whywhat wrote:
> > On 2016/11/12 at 23:35:48, mlamouri (slow) wrote:
> > > Here and below, you go from WMPI to HTMLMediaElement to then go back to
WMPI.
> > Any reason why not implementing this directly in WMPI?
> > 
> > 1. I can restore the previously selected video track easily as
HTMLMediaElement
> > has access to those.
> > 2. It works with both WMPI and WMPA (I assume Sergey's implementation works
with
> > both).
> > 3. In WMPI I'd have to change updatePlayState_computePlayState to react to
the
> > backgrounded state change while also playing and so on... It doesn't really
deal
> > with selected tracks at all right now.
> 
> This also looks strange from the WMPI side (which independently checks
IsHidden() to decide what to do). At the very least more comments are necessary
to explain those choices.

Hm, would the points above explain enough?

[whywhat, 2016-11-15 03:08:56]

https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3188:
webMediaPlayer()->selectedVideoTrackChanged(nullptr);
On 2016/11/15 at 00:09:11, sandersd wrote:
> On 2016/11/14 21:50:13, whywhat wrote:
> > On 2016/11/12 at 23:35:48, mlamouri (slow) wrote:
> > > Here and below, you go from WMPI to HTMLMediaElement to then go back to
WMPI.
> > Any reason why not implementing this directly in WMPI?
> > 
> > 1. I can restore the previously selected video track easily as
HTMLMediaElement
> > has access to those.
> > 2. It works with both WMPI and WMPA (I assume Sergey's implementation works
with
> > both).
> > 3. In WMPI I'd have to change updatePlayState_computePlayState to react to
the
> > backgrounded state change while also playing and so on... It doesn't really
deal
> > with selected tracks at all right now.
> 
> This also looks strange from the WMPI side (which independently checks
IsHidden() to decide what to do). At the very least more comments are necessary
to explain those choices.

More thoughts:

- WMPA doesn't implement selectedVideoTrackChanged and probably won't so my
argument 2 doesn't apply.

- I check IsHidden() in WMPI::selectedVideoTrackChanged because I didn't want to
add isHidden() to WMP interface or keep a m_isHidden boolean in
HTMLMediaElement. Perhaps the former would keep the code in HTMLMediaElement.

- If I were to implement the whole change in WMPI, I'd have to keep track of the
unselected video track id there which would not keep WMPI stateless. Maybe I
could add a method to WMPClient to get the current video track id?

WDYT?

[mlamouri (slow - plz ping), 2016-11-15 18:21:40]

On 2016/11/15 at 03:08:56, avayvod wrote:
>
https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):
> 
>
https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3188:
webMediaPlayer()->selectedVideoTrackChanged(nullptr);
> On 2016/11/15 at 00:09:11, sandersd wrote:
> > On 2016/11/14 21:50:13, whywhat wrote:
> > > On 2016/11/12 at 23:35:48, mlamouri (slow) wrote:
> > > > Here and below, you go from WMPI to HTMLMediaElement to then go back to
WMPI.
> > > Any reason why not implementing this directly in WMPI?
> > > 
> > > 1. I can restore the previously selected video track easily as
HTMLMediaElement
> > > has access to those.
> > > 2. It works with both WMPI and WMPA (I assume Sergey's implementation
works with
> > > both).
> > > 3. In WMPI I'd have to change updatePlayState_computePlayState to react to
the
> > > backgrounded state change while also playing and so on... It doesn't
really deal
> > > with selected tracks at all right now.
> > 
> > This also looks strange from the WMPI side (which independently checks
IsHidden() to decide what to do). At the very least more comments are necessary
to explain those choices.
> 
> More thoughts:
> 
> - WMPA doesn't implement selectedVideoTrackChanged and probably won't so my
argument 2 doesn't apply.
> 
> - I check IsHidden() in WMPI::selectedVideoTrackChanged because I didn't want
to add isHidden() to WMP interface or keep a m_isHidden boolean in
HTMLMediaElement. Perhaps the former would keep the code in HTMLMediaElement.
> 
> - If I were to implement the whole change in WMPI, I'd have to keep track of
the unselected video track id there which would not keep WMPI stateless. Maybe I
could add a method to WMPClient to get the current video track id?
> 
> WDYT?

Could WMP_MS benefit from having the implementation in HTMLMediaElement?

[sandersd (OOO until July 31), 2016-11-15 18:49:02]

> Maybe I could add a method to WMPClient to get the current video track id?

This sounds like an overall cleaner API to me.

[whywhat, 2016-11-15 20:08:55]

On 2016/11/15 at 18:21:40, mlamouri wrote:
> On 2016/11/15 at 03:08:56, avayvod wrote:
> >
https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):
> > 
> >
https://codereview.chromium.org/2487373003/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3188:
webMediaPlayer()->selectedVideoTrackChanged(nullptr);
> > On 2016/11/15 at 00:09:11, sandersd wrote:
> > > On 2016/11/14 21:50:13, whywhat wrote:
> > > > On 2016/11/12 at 23:35:48, mlamouri (slow) wrote:
> > > > > Here and below, you go from WMPI to HTMLMediaElement to then go back
to WMPI.
> > > > Any reason why not implementing this directly in WMPI?
> > > > 
> > > > 1. I can restore the previously selected video track easily as
HTMLMediaElement
> > > > has access to those.
> > > > 2. It works with both WMPI and WMPA (I assume Sergey's implementation
works with
> > > > both).
> > > > 3. In WMPI I'd have to change updatePlayState_computePlayState to react
to the
> > > > backgrounded state change while also playing and so on... It doesn't
really deal
> > > > with selected tracks at all right now.
> > > 
> > > This also looks strange from the WMPI side (which independently checks
IsHidden() to decide what to do). At the very least more comments are necessary
to explain those choices.
> > 
> > More thoughts:
> > 
> > - WMPA doesn't implement selectedVideoTrackChanged and probably won't so my
argument 2 doesn't apply.
> > 
> > - I check IsHidden() in WMPI::selectedVideoTrackChanged because I didn't
want to add isHidden() to WMP interface or keep a m_isHidden boolean in
HTMLMediaElement. Perhaps the former would keep the code in HTMLMediaElement.
> > 
> > - If I were to implement the whole change in WMPI, I'd have to keep track of
the unselected video track id there which would not keep WMPI stateless. Maybe I
could add a method to WMPClient to get the current video track id?
> > 
> > WDYT?
> 
> Could WMP_MS benefit from having the implementation in HTMLMediaElement?

If they support the MediaTracks API and disabling video track in background
makes sense, maybe.
Currently, they don't. I can sync up with mcasas@ about it.

[whywhat, 2016-11-15 20:09:26]

On 2016/11/15 at 18:49:02, sandersd wrote:
> > Maybe I could add a method to WMPClient to get the current video track id?
> 
> This sounds like an overall cleaner API to me.

Ok, I'll look into it.

[whywhat, 2016-11-21 20:45:38]

Moved the main logic into WMPI

[whywhat, 2016-11-21 20:45:52]

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

[commit-bot: I haz the power, 2016-11-21 20:46:49]

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

[whywhat, 2016-11-21 20:46:57]

The CQ bit was unchecked by avayvod@chromium.org

[whywhat, 2016-11-21 20:47:48]

avayvod@chromium.org changed reviewers:
- servolk@chromium.org

[whywhat, 2016-11-21 20:47:49]

Dan, PTAL

Note: still need to update the tests.

[sandersd (OOO until July 31), 2016-11-21 21:23:25]

WMPI changes LGTM. Can you add a section to your doc that shows what (if any) JS
events get emitted when the track is re-enabled?

[whywhat, 2016-11-22 17:05:53]

avayvod@chromium.org changed reviewers:
+ mkwst@chromium.org, pfeldman@chromium.org
- mlamouri@chromium.org

[whywhat, 2016-11-22 17:05:54]

PTAL

+mkwst for common_param_traits_macros.h and histograms.xml
+pfeldman for the rest of content/ & WebKit plumbing

[whywhat, 2016-11-22 17:06:04]

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

[commit-bot: I haz the power, 2016-11-22 17:06:27]

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

[whywhat, 2016-11-22 17:06:43]

The CQ bit was unchecked by avayvod@chromium.org

[whywhat, 2016-11-22 17:07:44]

Updated test files

[pfeldman, 2016-11-22 19:55:56]

plumbing lgtm

https://codereview.chromium.org/2487373003/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebMediaPlayerClient.h (right):

https://codereview.chromium.org/2487373003/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebMediaPlayerClient.h:106: // Returns the
selected video track id (or an empty id if there's none).
nit:blank line above.

[whywhat, 2016-11-22 23:42:17]

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

[commit-bot: I haz the power, 2016-11-22 23:43:13]

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-11-22 23:48:25]

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

[commit-bot: I haz the power, 2016-11-22 23:48:27]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[whywhat, 2016-11-23 02:22:27]

Rebase + comment fix

[whywhat, 2016-11-23 02:23:26]

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

[whywhat, 2016-11-23 02:23:38]

https://codereview.chromium.org/2487373003/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebMediaPlayerClient.h (right):

https://codereview.chromium.org/2487373003/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebMediaPlayerClient.h:106: // Returns the
selected video track id (or an empty id if there's none).
On 2016/11/22 at 19:55:56, pfeldman wrote:
> nit:blank line above.

Thanks! Done.

[commit-bot: I haz the power, 2016-11-23 02:23:55]

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-11-23 04:24:45]

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

[commit-bot: I haz the power, 2016-11-23 04:24:46]

Dry run: This issue passed the CQ dry run.

[Mike West, 2016-11-23 13:01:14]

adding the item to the IPC struct LGTM.

[whywhat, 2016-11-23 20:41:32]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2016-11-23 20:41:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
sandersd@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2487373003/#ps80001
(title: "Rebase + comment fix")

[commit-bot: I haz the power, 2016-11-23 20:42:06]

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-11-23 21:43:16]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1479933692479740,
"parent_rev": "0a07190364628355d10e0b7cc10a4831a1fcdee8", "commit_rev":
"d77d87db0090a3795a8113494d8103912ace7a82"}

[commit-bot: I haz the power, 2016-11-23 21:43:46]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-23 21:45:46]

Description was changed from

==========
Disable background video track behind a feature flag

BUG=663999
TEST=unit test + manual testing with the flag on - see chrome://media-internals
logging track selection
==========

to

==========
Disable background video track behind a feature flag

BUG=663999
TEST=unit test + manual testing with the flag on - see chrome://media-internals
logging track selection

Committed: https://crrev.com/39c1024093bdd21abd2c2ea3de5c05f7b9fee8ce
Cr-Commit-Position: refs/heads/master@{#434255}
==========

[commit-bot: I haz the power, 2016-11-23 21:45:48]

Patchset 5 (id:??) landed as
https://crrev.com/39c1024093bdd21abd2c2ea3de5c05f7b9fee8ce
Cr-Commit-Position: refs/heads/master@{#434255}

[servolk, 2016-12-05 19:35:40]

On 2016/11/23 21:45:48, commit-bot: I haz the power wrote:
> Patchset 5 (id:??) landed as
> https://crrev.com/39c1024093bdd21abd2c2ea3de5c05f7b9fee8ce
> Cr-Commit-Position: refs/heads/master@{#434255}

Sorry, perhaps I'm missing something, but I can see a unit test in earlier
patchsets of this CL (in HTMLVideoElementTest.cpp), but I can't find anything
similar to that test in the latest patchset #5, which got landed. Has the test
been extracted to a separate CL?

[whywhat, 2016-12-16 15:56:43]

On 2016/12/05 at 19:35:40, servolk wrote:
> On 2016/11/23 21:45:48, commit-bot: I haz the power wrote:
> > Patchset 5 (id:??) landed as
> > https://crrev.com/39c1024093bdd21abd2c2ea3de5c05f7b9fee8ce
> > Cr-Commit-Position: refs/heads/master@{#434255}
> 
> Sorry, perhaps I'm missing something, but I can see a unit test in earlier
patchsets of this CL (in HTMLVideoElementTest.cpp), but I can't find anything
similar to that test in the latest patchset #5, which got landed. Has the test
been extracted to a separate CL?

Ah, the test went away after the implementation moved from HTMLMediaElement to
WMPI as the test was Blink-specific and I didn't find a good way to test this in
WMPI. If you have a suggestion for the test, I'm happy to add it.