1230184 - SetParameters support for PeerConnection

Status: RESOLVED FIXED

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

Description

[Byron Campen [:bwc]]

This covers the JS API for SetParameters, and the plumbing to JsepTrack.

Comment 1

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

Attachment: MozReview Request: Bug 1230184 - setParameters webidl.

Bug 1230184 - setParameters webidl.

Comment 2

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

Attachment: MozReview Request: Bug 1230184 - plumb setParameters down to JsepTrack.

Bug 1230184 - plumb setParameters down to JsepTrack.

Comment 3

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

rid is a recent addition.

Comment 4

[Byron Campen [:bwc]]

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.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 6

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

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.

Comment 7

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

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 8

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

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 9

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

Attachment: MozReview Request: Bug 1230184 - add input parameter validation to setParameters.

Bug 1230184 - add input parameter validation to setParameters.

Comment 10

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

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/

Comment 11

[Byron Campen [:bwc]]

Comment on attachment 8696886 [details]
MozReview Request: Bug 1230184 - plumb setParameters down to JsepTrack.

https://reviewboard.mozilla.org/r/27461/#review24825

Comment 12

[Byron Campen [:bwc]]

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.

Comment 13

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

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 14

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

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 15

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

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 16

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

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/

Comment 17

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

Rebased and incorporated feedback.

Comment 18

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

Attachment: MozReview Request: Bug 1230184 - add media.peerconnection.simulcast pref.

Bug 1230184 - add media.peerconnection.simulcast pref.

Comment 19

[Byron Campen [:bwc]]

Comment on attachment 8697428 [details]
MozReview Request: Bug 1230184 - add media.peerconnection.simulcast pref.

https://reviewboard.mozilla.org/r/27611/#review24871

Comment 20

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=121b2a7158b2

Comment 21

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1230184 - Disable setParameters mochitest on the same platforms that basicVideo is disabled on. r?jesup

Review commit: https://reviewboard.mozilla.org/r/29321/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29321/

Comment 22

[Byron Campen [:bwc]]

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 23

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

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

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8696886 [details]
MozReview Request: Bug 1230184 - plumb setParameters down to JsepTrack.

https://reviewboard.mozilla.org/r/27461/#review26091

Comment 25

[Byron Campen [:bwc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59cefb0904c409757f446ef319f7f81834d400f1
Bug 1230184 - setParameters webidl. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/9da04c2984fdaf2c77e0e489759be963573fd915
Bug 1230184 - plumb setParameters down to JsepTrack. r=bwc, r=mrbkap

https://hg.mozilla.org/integration/mozilla-inbound/rev/6418df768fe4dd193a5c20bc672f45dbc4b91a52
Bug 1230184 - add input parameter validation to setParameters. r=bwc

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d2a98ad114f849b51723c9f17faf5de5be53e47
Bug 1230184 - add media.peerconnection.simulcast pref. r=bwc

https://hg.mozilla.org/integration/mozilla-inbound/rev/e23c280f0d3a31cf8b8cb0e335d1b97f39a96603
Bug 1230184 - Disable setParameters mochitest on the same platforms that basicVideo is disabled on. r=jesup

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/59cefb0904c4
https://hg.mozilla.org/mozilla-central/rev/9da04c2984fd
https://hg.mozilla.org/mozilla-central/rev/6418df768fe4
https://hg.mozilla.org/mozilla-central/rev/9d2a98ad114f
https://hg.mozilla.org/mozilla-central/rev/e23c280f0d3a

Comment 27

[Eric Shepherd [:sheppy]]

This was never DDN and got missed back in the day but has now been documented and added to Firefox 46 for developers.

Comment 28

[Eric Shepherd [:sheppy]]

The following documents have been added to MDN:

https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpSendParameters
https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpSendParameters/encodings
https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpEncodingParameters
https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpEncodingParameters/maxBitrate
https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpEncodingParameters/scaleResolutionDownBy

Updated:

https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpSender/setParameters