947429 - Provide prefs to allow disabling of video gUM and video PeerConnections

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

Attachment: prefs.patch

This is so we can safely enable/disable these features at any point with minimal risk.

Starts out with them disabled on B2G

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8343983 [details] [diff] [review]
prefs.patch

Review of attachment 8343983 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm. One question.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
@@ +73,5 @@
>    memset(c, 0, sizeof(*c));
>  #ifdef MOZILLA_INTERNAL_API
>    Apply(aSrc.mMandatory.mOfferToReceiveAudio, &c->offer_to_receive_audio, true);
>    Apply(aSrc.mMandatory.mOfferToReceiveVideo, &c->offer_to_receive_video, true);
> +  if (!Preferences::GetBool("media.peerconnection.video.enabled", true)) {

Don't you need #ifdef MOZ_WIDGET_GONK here to make it default to false for gonk?

It's not clear to me it matters, since I'm not sure in what circumstances the default arg is used when the presence of the pref is guaranteed (which I assume it is). But I wanted to mention it.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +245,5 @@
>  
>    // Adding tracks here based on nsDOMMediaStream expectation settings
>    uint32_t hints = stream->GetHintContents();
> +#ifdef MOZILLA_INTERNAL_API
> +  if (!Preferences::GetBool("media.peerconnection.video.enabled", true)) {

Same here.

Comment 2

[Eric Rescorla (:ekr)]

Comment on attachment 8343983 [details] [diff] [review]
prefs.patch

Review of attachment 8343983 [details] [diff] [review]:
-----------------------------------------------------------------

Jesup, doesn't this patch disable gUM video?

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8343983 [details] [diff] [review]
prefs.patch

Review of attachment 8343983 [details] [diff] [review]:
-----------------------------------------------------------------

still lgtm.

::: dom/media/MediaManager.cpp
@@ +1310,5 @@
>      aPrivileged = true;
>    }
> +  if (!Preferences::GetBool("media.navigator.video.enabled", true)) {
> +    c.mVideo = false;
> +  }

It only disables gUM video if media.navigator.video.enabled is present and set to false.

::: modules/libpref/src/init/all.js
@@ +229,5 @@
>  pref("media.apple.mp3.enabled", true);
>  #endif
>  #ifdef MOZ_WEBRTC
>  pref("media.navigator.enabled", true);
> +pref("media.navigator.video.enabled", true);

It defaults to true.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #1)
> Comment on attachment 8343983 [details] [diff] [review]
> prefs.patch
> 
> Review of attachment 8343983 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm. One question.
> 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
> @@ +73,5 @@
> >    memset(c, 0, sizeof(*c));
> >  #ifdef MOZILLA_INTERNAL_API
> >    Apply(aSrc.mMandatory.mOfferToReceiveAudio, &c->offer_to_receive_audio, true);
> >    Apply(aSrc.mMandatory.mOfferToReceiveVideo, &c->offer_to_receive_video, true);
> > +  if (!Preferences::GetBool("media.peerconnection.video.enabled", true)) {
> 
> Don't you need #ifdef MOZ_WIDGET_GONK here to make it default to false for
> gonk?

No, since the real default for gonk is in all.js, so I'm not doing that here.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5d23d937ffc

Comment 6

[Jason Smith [:jsmith]]

blocking+ - needed to ensure safe pref off peer connection & gUM for 1.3 if need be.

Comment 7

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/84cb9cf72c02 on suspicion of breaking gaia-ui-test, which in its opaque way says https://tbpl.mozilla.org/php/getParsedLog.php?id=31600298&tree=Mozilla-Inbound, but says it consistently.

Comment 8

[Jason Smith [:jsmith]]

(In reply to Phil Ringnalda (:philor) from comment #7)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/84cb9cf72c02 on
> suspicion of breaking gaia-ui-test, which in its opaque way says
> https://tbpl.mozilla.org/php/getParsedLog.php?id=31600298&tree=Mozilla-
> Inbound, but says it consistently.

FWIW - This looks like a general failure to shutdown a content process.

Comment 9

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #8)
> (In reply to Phil Ringnalda (:philor) from comment #7)
> > Backed out in
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/84cb9cf72c02 on
> > suspicion of breaking gaia-ui-test, which in its opaque way says
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=31600298&tree=Mozilla-
> > Inbound, but says it consistently.
> 
> FWIW - This looks like a general failure to shutdown a content process.

However - I don't see how this could be causing the gaia ui test failure - none of our gaia ui tests touch UI code that hits getUserMedia right now.

Comment 10

[Phil Ringnalda (:philor)]

Yeah, me neither, since the push before this was what got merged to m-c, and Gu failed there too. Eventually, and perhaps this time, I'll learn that when Gu breaks, it is never ever going to be because of a Gecko push, it's always going to be from something invisible and external, and I'll just hide it when it breaks.

Relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/33e4d9281b5d

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/33e4d9281b5d