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

Status: RESOLVED FIXED

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

Description

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

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.

Comment 1

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

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).

Comment 2

[Eric Rescorla (:ekr)]

Put the patch up. It would be good to land it

Comment 3

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

Attachment: Optional offer/answer constraints work + unsupported mandatory ones fail

Comment 4

[Boris Zbarsky [:bzbarsky]]

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...

Comment 5

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

Attachment: Optional offer/answer constraints work + unsupported mandatory ones fail. r=bz

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

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

Attachment: Optional offer/answer constraints work + unsupported mandatory ones fail.

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.

Comment 9

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

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

Comment 10

[Boris Zbarsky [: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

Comment 11

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

> https://tbpl.mozilla.org/?tree=Try&rev=44d2473d9f4d

I've filed Bug 841404 on the try burn, as I suspect the webidl compiler.

Comment 12

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

Attachment: Optional offer/answer constraints work + unsupported mandatory ones fail. r=bz

Fixed nits. Carrying forward r+ from bz.

Comment 13

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

blocking-, though I'm sure we'll land this asap

Comment 14

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

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... :-)

Comment 15

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

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 16

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

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... :-)

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/f213ceb739aa

Comment 19

[Jason Smith [:jsmith]]

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?

Comment 20

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

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.