Closed Bug 1230184 Opened 5 years ago Closed 5 years ago

SetParameters support for PeerConnection

Categories

(Core :: WebRTC: Signaling, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox45 --- affected
firefox46 --- fixed
Blocking Flags:

People

(Reporter: bwc, Assigned: jib)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(5 files)

This covers the JS API for SetParameters, and the plumbing to JsepTrack.
Assignee: nobody → jib
backlog: --- → webrtc/webaudio+
Rank: 15
Priority: -- → P1
Depends on: 1192390
Bug 1230184 - setParameters webidl.
Attachment #8696885 - Flags: review?(bugs)
Bug 1230184 - plumb setParameters down to JsepTrack.
Attachment #8696886 - Flags: review?(docfaraday)
https://reviewboard.mozilla.org/r/27457/#review24753

::: dom/media/PeerConnection.js:1507
(Diff revision 1)
> +  setParameters: function(parameters) {
> +    return this._pc._setParameters(this, parameters);
> +  },

We probably want at least a TODO here for validation code that covers:

1. No duplicate rids
2. If there is more than one element in the array, all elements need to have a rid (since we cannot mux without rids)

On further thought, _changing_ a rid is going to be interpreted as either changing the bitrate on a pre-existing rid, the introduction of a new rid, or the removal of an old one, so that's not something we need to worry about I think.

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:91
(Diff revision 1)
> +    // Test sender parameters.
> +    test.chain.append([
> +      function PC_LOCAL_SET_PARAMETERS(test) {
> +        return parameterstest(test.pcLocal);
> +      }
> +    ]);

I think I'd prefer there to be a separate test-case for this.

::: media/webrtc/signaling/src/jsep/JsepTrack.h:188
(Diff revision 1)
> +  void GetJsConstraints(std::vector<JsConstraints>& outConstraintsList)

This can be const.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2329
(Diff revision 1)
> +  if (aParameters.mEncodings.WasPassed()) {

What does the spec say should happen if no parameters are passed? Does that mean no encodings, or does it mean we revert back to a single encoding with no params on it?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2341
(Diff revision 1)
> +  nsresult rv = mJsepSession->SetParameters(streamId, trackId, constraints);
> +  if (NS_FAILED(rv)) {
> +    return NS_ERROR_FAILURE;
> +  }

I think I would like to see this part in a separate function, so signaling_unittests can call it, although I'm not totally sure I want to test this there.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2359
(Diff revision 1)
> +  nsresult rv = mJsepSession->GetParameters(streamId, trackId, constraints);
> +  if (NS_FAILED(rv)) {
> +    return NS_ERROR_FAILURE;
> +  }

Same thing here.
Blocks: 1231507
Comment on attachment 8696885 [details]
MozReview Request: Bug 1230184 - setParameters webidl.

https://reviewboard.mozilla.org/r/27459/#review24787

::: dom/webidl/RTCRtpSender.webidl:40
(Diff revision 1)
> +  float                    scaleResolutionDownBy = 1.0;

I'm not seeing rid or scaleResolutionDownBy in the spec, but
https://rawgit.com/w3c/webrtc-pc/8bbc42e71da1245a4e09f55e2b8e91f52d81c6f0/webrtc.html#idl-def-RTCRtpEncodingParameters has them.
I assume the spec will get them too.
Attachment #8696885 - Flags: review?(bugs) → review+
https://reviewboard.mozilla.org/r/27459/#review24787

> I'm not seeing rid or scaleResolutionDownBy in the spec, but
> https://rawgit.com/w3c/webrtc-pc/8bbc42e71da1245a4e09f55e2b8e91f52d81c6f0/webrtc.html#idl-def-RTCRtpEncodingParameters has them.
> I assume the spec will get them too.

Yes it will.
https://reviewboard.mozilla.org/r/27457/#review24797

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2329
(Diff revision 1)
> +  if (aParameters.mEncodings.WasPassed()) {

I would naively assume it returns to the default?
Comment on attachment 8696886 [details]
MozReview Request: Bug 1230184 - plumb setParameters down to JsepTrack.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27461/diff/1-2/
Comment on attachment 8697175 [details]
MozReview Request: Bug 1230184 - add input parameter validation to setParameters.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27525/diff/1-2/
Attachment #8697175 - Flags: review?(docfaraday)
Comment on attachment 8696886 [details]
MozReview Request: Bug 1230184 - plumb setParameters down to JsepTrack.

https://reviewboard.mozilla.org/r/27461/#review24825
Attachment #8696886 - Flags: review?(docfaraday) → review+
Comment on attachment 8697175 [details]
MozReview Request: Bug 1230184 - add input parameter validation to setParameters.

https://reviewboard.mozilla.org/r/27525/#review24827

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h:457
(Diff revision 1)
> -                               mozilla::dom::RTCRtpParameters& aOutParameters)
> +                               dom::RTCRtpParameters& aOutParameters)

I missed this last time; convention around here is for outparams to be pointers.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h:469
(Diff revision 1)
> +                std::vector<JsepTrack::JsConstraints>& aOutConstraints);

Same here.
Attachment #8697175 - Flags: review?(docfaraday) → review+
https://reviewboard.mozilla.org/r/27525/#review24849

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h:457
(Diff revision 1)
> -                               mozilla::dom::RTCRtpParameters& aOutParameters)
> +                               dom::RTCRtpParameters& aOutParameters)

This one is dictated by the binding code which does not follow that convention.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h:469
(Diff revision 1)
> +                std::vector<JsepTrack::JsConstraints>& aOutConstraints);

But this one I can fix.
Comment on attachment 8696885 [details]
MozReview Request: Bug 1230184 - setParameters webidl.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27459/diff/1-2/
Comment on attachment 8696886 [details]
MozReview Request: Bug 1230184 - plumb setParameters down to JsepTrack.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27461/diff/2-3/
Comment on attachment 8697175 [details]
MozReview Request: Bug 1230184 - add input parameter validation to setParameters.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27525/diff/1-2/
Rebased and incorporated feedback.
Bug 1230184 - add media.peerconnection.simulcast pref.
Attachment #8697428 - Flags: review?(docfaraday)
Comment on attachment 8697428 [details]
MozReview Request: Bug 1230184 - add media.peerconnection.simulcast pref.

https://reviewboard.mozilla.org/r/27611/#review24871
Attachment #8697428 - Flags: review?(docfaraday) → review+
Depends on: 1232082
Attachment #8703204 - Attachment description: MozReview Request: Bug 1230184 - Disable setParameters mochitest on the same platforms that basicVideo is disabled on. → MozReview Request: Bug 1230184 - Disable setParameters mochitest on the same platforms that basicVideo is disabled on. r?jesup
Attachment #8703204 - Flags: review?(rjesup)
Comment on attachment 8703204 [details]
MozReview Request: Bug 1230184 - Disable setParameters mochitest on the same platforms that basicVideo is disabled on. r?jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29321/diff/1-2/
Comment on attachment 8703204 [details]
MozReview Request: Bug 1230184 - Disable setParameters mochitest on the same platforms that basicVideo is disabled on. r?jesup

https://reviewboard.mozilla.org/r/29321/#review26087
Attachment #8703204 - Flags: review?(rjesup) → review+
Attachment #8696886 - Flags: review?(mrbkap)
Attachment #8696886 - Flags: review?(mrbkap) → review+
Comment on attachment 8696886 [details]
MozReview Request: Bug 1230184 - plumb setParameters down to JsepTrack.

https://reviewboard.mozilla.org/r/27461/#review26091
This was never DDN and got missed back in the day but has now been documented and added to Firefox 46 for developers.
You need to log in before you can comment on or make changes to this bug.