942343 - Pref off media.peerconnection.enabled on Firefox OS

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Jason Smith [:jsmith]]

With bug 941985, QC is currently blocked on their CS testing. We need to unblock them by preffing off Peer Connection support on Firefox OS until the root cause of bug 941985 is resolved.

Comment 1

[bhavana bajaj [:bajaj]]

:jesup, passing this onto you for now as we are trying to avoid un-owned blocker bugs, please re-assign if needed.

Comment 2

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

Attachment: pref off RTCPeerConnection for B2G until it's functional

Also dropped a few prefs we'll want to have be B2G-specific into the #ifdef, and took a first-cut at complexity defaults for B2G (may want to modify per-device for shipping builds).

Comment 3

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

Attachment: pref off RTCPeerConnection for B2G until it's functional

for now, put the default gum capture size back to 640x480 - it's more an issue with constraining the size we want to send (until CPU monitoring is in and stable)

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 8337333 [details] [diff] [review]
pref off RTCPeerConnection for B2G until it's functional

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

I think I wouldn't change max_fs and max_fr, but if you still think it's best
after reading my comment below, then go ahead.

::: modules/libpref/src/init/all.js
@@ +237,5 @@
>  pref("media.navigator.video.default_minfps",10);
> +#ifdef MOZ_WIDGET_GONK
> +pref("media.peerconnection.enabled", false);
> +pref("media.navigator.video.max_fs", 1200); // 640x480 == 1200mb
> +pref("media.navigator.video.max_fr", 30);

Why restrict these here? Based on the comments here, these settings more or less line up with
640x480 which is what you are using for video input, so this is just going to affect
decoding negotiation. Is that something we really care about right now?

Comment 5

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

(In reply to Eric Rescorla (:ekr) from comment #4)
> Comment on attachment 8337333 [details] [diff] [review]
> pref off RTCPeerConnection for B2G until it's functional
> 
> Review of attachment 8337333 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think I wouldn't change max_fs and max_fr, but if you still think it's best
> after reading my comment below, then go ahead.

I think it is best, and I think we want them in there - and it's much easier to adjust the value later, than to uplift a new, un-tested pref never used before.  This gets the basic pathways for max-fs/max-fr to get invoked and tested.

> 
> ::: modules/libpref/src/init/all.js
> @@ +237,5 @@
> >  pref("media.navigator.video.default_minfps",10);
> > +#ifdef MOZ_WIDGET_GONK
> > +pref("media.peerconnection.enabled", false);
> > +pref("media.navigator.video.max_fs", 1200); // 640x480 == 1200mb
> > +pref("media.navigator.video.max_fr", 30);
> 
> Why restrict these here? Based on the comments here, these settings more or
> less line up with
> 640x480 which is what you are using for video input, so this is just going
> to affect
> decoding negotiation. Is that something we really care about right now?

If we want to avoid Chrome deciding to send us HD video on B2G (which was why we implemented them in the first place), yes.  These are things that likely should be tuned on a per-device basis - sending us data >> screen resolution is likely not a great idea, for example.  For some devices, we may want low FPS or lower max_fs than this, and this (the defaults) isn't something the app should be expected to figure out.

Thanks

Comment 6

[Eric Rescorla (:ekr)]

(In reply to Randell Jesup [:jesup] from comment #5)
> (In reply to Eric Rescorla (:ekr) from comment #4)
> > Comment on attachment 8337333 [details] [diff] [review]
> > pref off RTCPeerConnection for B2G until it's functional
> > 
> > Review of attachment 8337333 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I think I wouldn't change max_fs and max_fr, but if you still think it's best
> > after reading my comment below, then go ahead.
> 
> I think it is best, and I think we want them in there - and it's much easier
> to adjust the value later, than to uplift a new, un-tested pref never used
> before.  This gets the basic pathways for max-fs/max-fr to get invoked and
> tested.

I just want to note that this pref was here before, it was just set
to 0.


> > ::: modules/libpref/src/init/all.js
> > @@ +237,5 @@
> > >  pref("media.navigator.video.default_minfps",10);
> > > +#ifdef MOZ_WIDGET_GONK
> > > +pref("media.peerconnection.enabled", false);
> > > +pref("media.navigator.video.max_fs", 1200); // 640x480 == 1200mb
> > > +pref("media.navigator.video.max_fr", 30);
> > 
> > Why restrict these here? Based on the comments here, these settings more or
> > less line up with
> > 640x480 which is what you are using for video input, so this is just going
> > to affect
> > decoding negotiation. Is that something we really care about right now?
> 
> If we want to avoid Chrome deciding to send us HD video on B2G (which was
> why we implemented them in the first place), yes.  These are things that
> likely should be tuned on a per-device basis - sending us data >> screen
> resolution is likely not a great idea, for example.  For some devices, we
> may want low FPS or lower max_fs than this, and this (the defaults) isn't
> something the app should be expected to figure out.
> 
> Thanks

Comment 7

[Jason Smith [:jsmith]]

Do we need to land the pref off on b2g 26 as well? I'm seeing peer connection preffed on in the 26 branch:

http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/file/64e1acc29811/modules/libpref/src/init/all.js#l220

Comment 8

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

> > > I think I wouldn't change max_fs and max_fr, but if you still think it's best
> > > after reading my comment below, then go ahead.
> > 
> > I think it is best, and I think we want them in there - and it's much easier
> > to adjust the value later, than to uplift a new, un-tested pref never used
> > before.  This gets the basic pathways for max-fs/max-fr to get invoked and
> > tested.
> 
> I just want to note that this pref was here before, it was just set
> to 0.

right, but that means the logic to make use of the value is effectively bypassed:

gdm_sdp.c:            if (max_fs || max_fr) {

etc

Comment 9

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

https://hg.mozilla.org/integration/b2g-inbound/rev/2cae126450b2

Comment 10

[Carsten Book [:Tomcat]]

(In reply to Randell Jesup [:jesup] from comment #9)
> https://hg.mozilla.org/integration/b2g-inbound/rev/2cae126450b2

Hey Randell, sorry had to backout this change in https://hg.mozilla.org/integration/b2g-inbound/rev/12ace9aab045 because i think it was causing perma-orange test failure  in mochitest 4 and 8 like https://tbpl.mozilla.org/php/getParsedLog.php?id=31032865&tree=B2g-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=31032777&tree=B2g-Inbound

Comment 11

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

Turns out one test didn't include head.js (which forces peerconnection.enabled=true), and the interface tests didn't check the pref that turns on and off peerconnection (which it should have done, since it's preffable).

Ran both locally in mochitest (and I modified test_interfaces.html locally to allow me to pref on and off peerconnection, and tried it both ways, plus with forcing an error to make sure the test worked correctly).

https://tbpl.mozilla.org/?tree=Try&rev=b34a91b359b3

Comment 12

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

Attachment: pref off RTCPeerConnection for B2G until it's functional

Comment 13

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

Comment on attachment 8337702 [details] [diff] [review]
pref off RTCPeerConnection for B2G until it's functional

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

Green try; needs DOM review for test_interfaces changes, jsmith for test change

Comment 14

[Jason Smith [:jsmith]]

Comment on attachment 8337702 [details] [diff] [review]
pref off RTCPeerConnection for B2G until it's functional

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

lgtm

::: dom/media/tests/mochitest/test_peerConnection_toJSON.html
@@ +14,5 @@
> +<pre id="test">
> +<script type="application/javascript">
> +  createHTML({
> +    bug: "872377",
> +    title: "fixme"

Nit - fix the title here to something descriptive

Comment 15

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

https://hg.mozilla.org/integration/b2g-inbound/rev/5de30b683df2

Comment 16

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #7)
> Do we need to land the pref off on b2g 26 as well? I'm seeing peer
> connection preffed on in the 26 branch:
> 
> http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/file/64e1acc29811/modules/
> libpref/src/init/all.js#l220

Randell - Can you answer this question? I need to know if this needs to be in 1.2 as well.

Comment 17

[Eric Rescorla (:ekr)]

I am not Randell, but I believe it shoudl be off for 1.2

Comment 18

[Jason Smith [:jsmith]]

(In reply to Eric Rescorla (:ekr) from comment #17)
> I am not Randell, but I believe it shoudl be off for 1.2

Okay, thanks for the confirmation. Switching to koi+ then - blocking because we need to turn off a feature that's not ready for ship on 1.2.

Comment 19

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

Careful: I thought PeerConnection was never enabled on 1.2 (though via a different mechanism than the patch I landed here).  Please verify  with an actual 1.2 build (a simple "var pc = new mozRTCPeerConnection(); if (pc) complain;" should do); there are a number of ways you can disable peerconnection.  If it really is enabled, we should disable it as it won't work.

Comment 20

[Jason Smith [:jsmith]]

(In reply to Randell Jesup [:jesup] from comment #19)
> Careful: I thought PeerConnection was never enabled on 1.2 (though via a
> different mechanism than the patch I landed here).  Please verify  with an
> actual 1.2 build (a simple "var pc = new mozRTCPeerConnection(); if (pc)
> complain;" should do); there are a number of ways you can disable
> peerconnection.  If it really is enabled, we should disable it as it won't
> work.

It looks like it's enabled, but we throw an exception immediately upon creation of the object. Using the pc_test page, I tried to do an audio to audio call and got:

11-25 15:20:51.120: E/GeckoConsole(482): [JavaScript Error: "NS_ERROR_UNEXPECTED: " {file: "http://mozilla.github.io/webrtc-landing/pc_test.html" line: 142}]

Line 142 is the point where the peer connection is created:

pc1 = new mozRTCPeerConnection();

HTML 5 Test strangely does report that WebRTC is enabled behind a prefix though.

So sounds like creating the peer connection object is failing, so sounds like we're okay here.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5de30b683df2