Closed Bug 942343 Opened 6 years ago Closed 6 years ago

Pref off media.peerconnection.enabled on Firefox OS

Categories

(Core :: WebRTC, defect)

26 Branch
ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28
blocking-b2g 1.3+
Tracking Status
firefox28 --- fixed

People

(Reporter: jsmith, Assigned: jesup)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
OS: All → Gonk (Firefox OS)
Hardware: All → ARM
Blocks: 930299
Summary: Pref media.peerconnection.enabled on Firefox OS → Pref off media.peerconnection.enabled on Firefox OS
blocking-b2g: --- → 1.3?
:jesup, passing this onto you for now as we are trying to avoid un-owned blocker bugs, please re-assign if needed.
Assignee: nobody → rjesup
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).
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)
Attachment #8337326 - Attachment is obsolete: true
Attachment #8337333 - Flags: review?(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?
Attachment #8337333 - Flags: review?(ekr) → review+
(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
(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
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
blocking-b2g: 1.3? → 1.3+
> > > 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
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 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
Attachment #8337702 - Flags: review?(jsmith)
Attachment #8337702 - Flags: review?(bugs)
Attachment #8337702 - Flags: review?(bugs) → review+
Attachment #8337702 - Flags: review?(jsmith) → review?(hskupin)
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
Attachment #8337702 - Flags: review?(hskupin) → review+
(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.
Flags: needinfo?(rjesup)
I am not Randell, but I believe it shoudl be off for 1.2
(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.
blocking-b2g: 1.3+ → koi+
Flags: needinfo?(rjesup)
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.
Flags: needinfo?(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.
blocking-b2g: koi+ → 1.3+
Flags: needinfo?(jsmith)
https://hg.mozilla.org/mozilla-central/rev/5de30b683df2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.