Pass along constraints from PC JS module to PCImpl

RESOLVED FIXED in mozilla19

Status

()

Core
WebRTC
P1
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: anant, Assigned: anant)

Tracking

unspecified
mozilla19
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
According to the spec, addStream, createOffer, createAnswer should all support the constraints parameter. Currently, PCImpl supports them for createOffer and createAnswer, but we don't pass them along from the DOM JS.

Updated

6 years ago
Priority: -- → P1
Whiteboard: [WebRTC], [blocking-webrtc+]
(Assignee)

Comment 1

6 years ago
Created attachment 673002 [details] [diff] [review]
Add constraints support

We only seem to support boolean constraints in PeerConnectionImpl.cpp at the moment. This patch hooks the JS up with constraints.

Test case at: http://people.mozilla.com/~anarayanan/constraints_test.html
Attachment #673002 - Flags: review?(rjesup)
Attachment #673002 - Flags: review?(ekr)
Attachment #673002 - Flags: feedback?(emannion)

Comment 2

6 years ago
Comment on attachment 673002 [details] [diff] [review]
Add constraints support

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

I feel like this embodies too much knowledge of the constraints rules.

::: dom/media/PeerConnection.js
@@ +235,5 @@
> +   * }
> +   *
> +   * We convert the above into two property bags, one each for mandatory
> +   * and optional constraints. We do not check the validity of them, that
> +   * is left to PeerConnectionImpl.cpp

I'd much rather have these be strings. That way we don't need to commit to typing here.

E.g., adding non-bool types is going to require changing this code.

@@ +253,5 @@
> +        mandatory.setPropertyAsBool(keys[i], constraints.mandatory[keys[i]]);
> +      }
> +    }
> +
> +    if (constraints && constraints.optional) {

This seems to destroy the ordering.

::: dom/media/bridge/IPeerConnection.idl
@@ +75,5 @@
>  
>    /* JSEP calls */
> +  void createOffer(in nsIPropertyBag2 mandatory, in nsIPropertyBag2 optional);
> +  void createAnswer(
> +    in nsIPropertyBag2 mandatory, in nsIPropertyBag2 optional, in string offer);

I'm not excited about splitting this up? Is there a way to have these grouped?

Also, how does the ordering work for optional properties?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +733,5 @@
>   */
> +void
> +PeerConnectionImpl::AddConstraints(
> +  nsIPropertyBag2* bag, MediaConstraints* constraints, bool mandatory)
> +{

Can this really not fail?
Attachment #673002 - Flags: review?(ekr) → review-
Comment on attachment 673002 [details] [diff] [review]
Add constraints support

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

::: dom/media/bridge/IPeerConnection.idl
@@ +75,5 @@
>  
>    /* JSEP calls */
> +  void createOffer(in nsIPropertyBag2 mandatory, in nsIPropertyBag2 optional);
> +  void createAnswer(
> +    in nsIPropertyBag2 mandatory, in nsIPropertyBag2 optional, in string offer);

Maybe its for a separate patch but the offer needs to be removed from createAnswer, I did it in a separate patch so maybe so maybe do same here.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +783,4 @@
>    cc_media_constraints_t* cc_constraints = nullptr;
>    constraints.buildArray(&cc_constraints);
>  
>    mCall->createOffer(cc_constraints);

Maybe I should have freed constraints at this point,  cc_constraints will be freed internally in SIPCC.  Same applies to CreateAnswer.  If you agree with this comment can you free the mem here for me?
Attachment #673002 - Flags: feedback?(emannion) → feedback+
Comment on attachment 673002 [details] [diff] [review]
Add constraints support

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +762,5 @@
> +PeerConnectionImpl::CreateOffer(
> +  nsIPropertyBag2* mandatory, nsIPropertyBag2* optional)
> +{
> +  MOZ_ASSERT(mandatory);
> +  MOZ_ASSERT(optional);

I assume the PeerConnection.js code guarantees this currently with the bag stuff.
Attachment #673002 - Flags: review?(rjesup)
(Assignee)

Comment 5

6 years ago
Thanks for the feedback, all! Working on a new version of the patch to address all of it.
(Assignee)

Comment 6

6 years ago
Created attachment 674051 [details] [diff] [review]
Add constraints support - v2
Attachment #673002 - Attachment is obsolete: true
Attachment #674051 - Flags: review?(rjesup)
Attachment #674051 - Flags: review?(emannion)
Attachment #674051 - Flags: review?(ekr)
Comment on attachment 674051 [details] [diff] [review]
Add constraints support - v2

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +784,5 @@
> +          jsval val;
> +          JS_GetElement(aCx, opts, i, &val);
> +          if (JSVAL_IS_PRIMITIVE(val)) {
> +            // Extract name & value and store.
> +            // FIXME: MediaConstraints does not support optional constraints?

The third option to MediaConstraints::setBooleanConstraint is a boolean that specifies whether the constraints in mandatory(true) or optional(false), this info is then passed through into SIPCC. Maybe a boolean is not sufficient, let me know of not.
(Assignee)

Comment 8

6 years ago
(In reply to enda mannion (:emannion) from comment #7)
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +784,5 @@
> > +          jsval val;
> > +          JS_GetElement(aCx, opts, i, &val);
> > +          if (JSVAL_IS_PRIMITIVE(val)) {
> > +            // Extract name & value and store.
> > +            // FIXME: MediaConstraints does not support optional constraints?
> 
> The third option to MediaConstraints::setBooleanConstraint is a boolean that
> specifies whether the constraints in mandatory(true) or optional(false),
> this info is then passed through into SIPCC. Maybe a boolean is not
> sufficient, let me know of not.

The comment there was more along the lines of ordering not being supported. Is the order in which setBooleanConstraint is called important?
setBooleanConstraint is not order dependent, pop them in in any order, SIPCC will interate through regardless.  Unless there is an ordering requirement.
Comment on attachment 674051 [details] [diff] [review]
Add constraints support - v2

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

lgtm once you wire up optional constraints.

Note: I did not really check the JSval handling code in PCImpl since I am unqualified to do so.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +759,5 @@
> +        jsval option, optionName;
> +        if (JS_GetPropertyById(aCx, opts, mandatoryOpts[i], &option)) {
> +          // We only support boolean constraints for now.
> +          if (JSVAL_IS_BOOLEAN(option)) {
> +            if (JS_IdToValue(aCx, mandatoryOpts[i], &optionName)) {

nit: would it make sense to re-order these tests since you presumably always want the option name?

@@ +803,3 @@
>  
> +  MediaConstraints* cs = new MediaConstraints();
> +  nsresult rv = ValidateConstraints(aConstraints, cs, aCx);

Maybe rename "validate" to convert?

@@ +803,4 @@
>  
> +  MediaConstraints* cs = new MediaConstraints();
> +  nsresult rv = ValidateConstraints(aConstraints, cs, aCx);
> +  if (rv != NS_OK) {

Don't the cool kids use NS_FAILED?

@@ +827,5 @@
>    CheckIceState();
>    mRole = kRoleAnswerer;  // TODO(ekr@rtfm.com): Interrogate SIPCC here?
> +
> +  MediaConstraints* cs = new MediaConstraints();
> +  nsresult rv = ValidateConstraints(aConstraints, cs, aCx);

Same comments as above.
Attachment #674051 - Flags: review?(ekr) → review+
(Assignee)

Comment 11

6 years ago
(In reply to enda mannion (:emannion) from comment #9)
> setBooleanConstraint is not order dependent, pop them in in any order, SIPCC
> will interate through regardless.  Unless there is an ordering requirement.

The spec requires that optional constraints be processed in order, which is why they're represented as an array in JS.
Comment on attachment 674051 [details] [diff] [review]
Add constraints support - v2

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

Overall I like it much better

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +803,3 @@
>  
> +  MediaConstraints* cs = new MediaConstraints();
> +  nsresult rv = ValidateConstraints(aConstraints, cs, aCx);

I agree, rename
Attachment #674051 - Flags: review?(rjesup) → review+
Attachment #674051 - Flags: review?(emannion) → review+
https://hg.mozilla.org/mozilla-central/rev/8af544208892
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

Updated

6 years ago
Keywords: verifyme

Updated

6 years ago
Duplicate of this bug: 784513

Updated

5 years ago
Keywords: verifyme

Updated

5 years ago
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc+][qa-]
You need to log in before you can comment on or make changes to this bug.