Closed Bug 835370 Opened 12 years ago Closed 12 years ago

createOffer/Answer() only support mandatory constraints. Implement optional constraints

Categories

(Core :: WebRTC: Networking, defect, P2)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: jib, Assigned: jib)

References

Details

(Whiteboard: [WebRTC][blocking-webrtc-][qa-])

Attachments

(1 file, 3 obsolete files)

Mandatory constraints are used; optional aren't as we haven't implemented any, so the current implementation walks the list and does nothing with them.
Whiteboard: [getUserMedia]
Priority: -- → P2
Whiteboard: [getUserMedia] → [getUserMedia], [blocking-gum-]
I believe I mis-filed this originally, as I think we were talking about PeerConnection and it's use of constraints in createOffer/Answer(). I've therefore renamed the description. Sorry for any confusion. I'm sitting on a patch for this from before interim, that I held off on. Given that little changed during that meeting, I've refreshed the patch and will upload it next (I know, it is not a blocker, let me know what we want to do).
Summary: getUserMedia() only supports mandatory constraints. Implement optional constraints → createOffer/Answer() only support mandatory constraints. Implement optional constraints
Whiteboard: [getUserMedia], [blocking-gum-] → [WebRTC]
Put the patch up. It would be good to land it
Depends on: 835712
Comment on attachment 713431 [details] [diff] [review] Optional offer/answer constraints work + unsupported mandatory ones fail >+ if (!isObject(constraints) || isArray(constraints)) { Why the "not an array" restriction? >+ if (!isObject(constraints.mandatory) || isArray(constraints.mandatory)) { Likewise. >+ if (!isArray(constraints.optional)) { And this one. Why do you care what kind of arraylike it is? >+ if (!JS_IsArrayObject(aCx, array) || Likewise. Why is that check needed? Note that JS_GetArrayLength is generic, not array-specific; it will work on any object with a .length. Same for JS_GetElement. >+ JSObject* opts = JSVAL_TO_OBJECT(element); JSObject* opts = &element.toObject(); >+ // Expect one property per entry. Hmm. How is the spec written? If I add an enumerable property to Object.prototype on the page, should that really break this code? I would think we want something akin to Object.keys here if possible, not JS_Enumerate. And what we _really_ want is a way to do hashtables in WebIDL. ;) >+ if (JSVAL_IS_BOOLEAN(option)) { if (option.isBoolean()) { > JSVAL_TO_BOOLEAN(option) option.toBoolean() r=me modulo those nits. I really hope this code is not performance-sensitive enough to make us worry about the string copies...
Attachment #713431 - Flags: review?(bzbarsky) → review+
Update with nits fixed. Carrying forward r+ from bz. >>+ if (!isObject(constraints) || isArray(constraints)) { > > Why the "not an array" restriction? To catch malformed input and enforce the spec syntax as written. People will likely enter this by hand, and it's quite complicated IMHO. Confusingly, optional is a single-pair-in-object array while mandatory is a dictionary, both potentially containing the same things, which means the structure itself is overloaded with logical meaning. In essence it's a form of structured expression, which I think benefits from stricter parsing. As a user, I for one would prefer the software to help me out as much as possible with entering this expression, especially since the effects of getting it wrong may not be immediately observable (the effect of mandatory vs. optional for instance may not be observable until I run a different browser). >>+ // Expect one property per entry. > > Hmm. How is the spec written? http://dev.w3.org/2011/webrtc/editor/webrtc.html#constraints uses "MediaConstraints" without defining it. Our code assumes it mirrors the "MediaTrackConstraints" construct used in http://dev.w3.org/2011/webrtc/editor/getusermedia.html#dictionary-mediastreamconstraints-members where it IS defined, though frankly it's overkill in our context (since settings and constraints are not the same thing), and doesn't justify the downstream complexity IMHO (another reason for narrowing its surface as much as possible). > If I add an enumerable property to Object.prototype on the page, should that > really break this code? > > I would think we want something akin to Object.keys here if possible, not > JS_Enumerate. Sorry, I don't understand. I copied the JS_Enumerate from the unchanged mandatory parsing in the same function. But I'm all for improvements if you have them. > I really hope this code is not performance-sensitive enough to make us worry about the string copies... I wouldn't think so.
Attachment #713431 - Attachment is obsolete: true
Attachment #713431 - Flags: review?(rjesup)
Attachment #713677 - Flags: review?(rjesup)
> To catch malformed input and enforce the spec syntax as written. Hmm. Could you link me to the spec, please? In general specs are moving away from special-casing arrays compared to other arraylikes; it turns out that the differences between them are mostly implementation-imposed and web developers don't think of them as distinct very much. This is why all the Array methods in ES5 are generic, for example. > where it IS defined, Mmm... not the part we care about here. :( > I copied the JS_Enumerate from the unchanged mandatory parsing in the same function. Sure. I think we should get a followup filed to sort out what the right behavior here is and then implement it. I strongly doubt JS_Enumerate is it; we may need to add JSAPI to do the right thing.
Actually, looks like JS_Enumerate is just terribly named. It doesn't actually do enumeration, it does already do an Object.keys equivalent. So we're good there.
Updated after chat with bz. - Now uses an isArraylike() function when arrays are desired to avoid over-constricting. - Commented pilot-error validation code with reasoning. - Note: uses isArray() to over-specify when arrays are NOT desired, to avoid over-constricting. - some other nits for safer JS parsing.
Attachment #713677 - Attachment is obsolete: true
Attachment #713677 - Flags: review?(rjesup)
Attachment #713820 - Flags: review?(rjesup)
Attachment #713820 - Flags: review?(bzbarsky)
Comment on attachment 713820 [details] [diff] [review] Optional offer/answer constraints work + unsupported mandatory ones fail. >+ // Testing for pilot error of using [] on mandatory throws nicer msg I think the comment should explain that we'll end up throwing anyway because any array has properties that are not valid constraint names, so we might as well do it now with a nicer error message. >+ for (let constraint in constraints.mandatory) { Looking at this again, this is doing something different from the C++, right? The JS_Enumerate in the C++ only looks at own properties, while this will look at prototype properties too... This should probably use the elements of Object.keys(constraints.mandatory) instead. This stuff desperately needs tests testing the various edge cases. :( >+ if (!JS_IsArrayObject(aCx, array) || Do we still need this? >+ if (!JS_GetPropertyById(aCx, opts, optionalOpts[0], &option) || >+ !JS_IdToValue(aCx, optionalOpts[0], &optionName)) { Please fix indent. r=me with the nits
Attachment #713820 - Flags: review?(bzbarsky) → review+
> https://tbpl.mozilla.org/?tree=Try&rev=44d2473d9f4d I've filed Bug 841404 on the try burn, as I suspect the webidl compiler.
Fixed nits. Carrying forward r+ from bz.
Attachment #713820 - Attachment is obsolete: true
Attachment #713820 - Flags: review?(rjesup)
Attachment #714217 - Flags: review?(rjesup)
blocking-, though I'm sure we'll land this asap
Whiteboard: [WebRTC] → [WebRTC][blocking-webrtc-]
Comment on attachment 714217 [details] [diff] [review] Optional offer/answer constraints work + unsupported mandatory ones fail. r=bz Review of attachment 714217 [details] [diff] [review]: ----------------------------------------------------------------- r- for test ::: dom/media/tests/mochitest/test_peerConnection_bug835370.html @@ +27,5 @@ > + var exception = null; > + try { pconnects.createOffer(step1, failed); } catch (e) { exception = e; } > + ok(!exception, "createOffer(step1, failed) succeeds"); > + exception = null; > + try { pconnect.createOffer(step1, failed, 1); } catch (e) { exception = e; } This is invalid. We've changed createOffer and createAnswer to just return the same string if called multiple times (bug 840344 IIRC). You need a new PeerConnection for each one (which means this will be slow unless you short-circuit the ICE lookup by setting a null STUNserver I think). I'd prefer for this one test not to take minutes to finish... :-)
Attachment #714217 - Flags: review?(rjesup) → review-
That shouldn't be a problem. This test only exercises the synchronous validation code in PeerConnection.js, which is the very first thing to happen in that function, hence shouldn't be affected by bug 840344 (which I don't have access to btw, but I'm guessing). For these tests, unless createOffer throws immediately before it comes back then it's happy, and that's what success means in this context. They don't rely on (nor do they provide sufficient input, nor allow sufficient time for) the calls to actually do something measurable asynchronously. The tests ran very quickly when I tried them FWIW.
Comment on attachment 714217 [details] [diff] [review] Optional offer/answer constraints work + unsupported mandatory ones fail. r=bz Review of attachment 714217 [details] [diff] [review]: ----------------------------------------------------------------- r+ after discussion with abr/jib. ::: dom/media/tests/mochitest/test_peerConnection_bug835370.html @@ +27,5 @@ > + var exception = null; > + try { pconnects.createOffer(step1, failed); } catch (e) { exception = e; } > + ok(!exception, "createOffer(step1, failed) succeeds"); > + exception = null; > + try { pconnect.createOffer(step1, failed, 1); } catch (e) { exception = e; } This is invalid. We've changed createOffer and createAnswer to just return the same string if called multiple times (bug 840344 IIRC). You need a new PeerConnection for each one (which means this will be slow unless you short-circuit the ICE lookup by setting a null STUNserver I think). I'd prefer for this one test not to take minutes to finish... :-)
Attachment #714217 - Flags: review- → review+
Attachment #714217 - Flags: checkin?(rjesup)
Attachment #714217 - Flags: checkin?(rjesup)
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Keywords: verifyme
Component: WebRTC: Audio/Video → WebRTC: Networking
Jan-Ivar - Can you give me some information on how you can verify that optional constraints are working correctly with createOffer, createAnswer, and addStream respectively?
Flags: needinfo?(jib)
You mean beyond http://mxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/test_peerConnection_bug835370.html?force=1 ? I suppose that mochitest only exercises parsing. I'd love tips on how to automate verification of downstream results, plus feel free to develop this test further. Here's how I manually verified that OfferToReceiveVideo:false works: 1) Copy http://mozilla.github.io/webrtc-landing/pc_test.html 2) Add {optional: [{"OfferToReceiveVideo": false }]} to pc1.createOffer() 3) Observe that incoming video is absent (Note: pc1 is on the right!) Here's how I manually verified that OfferToReceiveVideo:true works: 1) Copy http://mozilla.github.io/webrtc-landing/pc_test.html 2) Comment out pc1.addStream(video1); 3) Observe that incoming and outgoing videos are absent. 4) Add {optional: [{"OfferToReceiveVideo": true }]} to pc1.createOffer() 5) Observe that incoming video is present. I didn't test audio. I have no test for MozOfferDataChannel:false but http://www.webrtc.org/interop uses it. We don't support constraints on addStream.
Flags: needinfo?(jib)
Keywords: verifyme
Whiteboard: [WebRTC][blocking-webrtc-] → [WebRTC][blocking-webrtc-][qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: