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

Comment 29

[Hamish Willee]

This issue added support for some properties of encodings set by RTCRtpSender: setParameters() and returned by getParameters. However these are not in the standard: rtx, fec, srcc. Is there any way to get information about what they do/are for. The doc PR for these is here https://github.com/mdn/content/pull/30080 and includes comments for the places I need information.

IN addition, the active is documented in spec but is defined as "encoding is being sent" while our docs state "encoding is being used". I THINK this means that you can set the value to offer it as an option for the encoder/decoder, or you can read the option to find out if it is actively being used in an encoder.
Presumably if you set false this would tell the encoder you want to stop using the value, but setting true wouldn't make an encoder use the encoding, just make it available?

I realize this is very old. I'm hoping someone here is still around!

Comment 30

[Byron Campen [:bwc]]

(In reply to Hamish Willee from comment #29)

This issue added support for some properties of encodings set by RTCRtpSender: setParameters() and returned by getParameters. However these are not in the standard: rtx, fec, srcc. Is there any way to get information about what they do/are for. The doc PR for these is here https://github.com/mdn/content/pull/30080 and includes comments for the places I need information.

I'm guessing that at one point, the spec allowed JS to choose the ssrcs for the stream, the RTX stream, and the FEC stream? I do not think we have ever allowed those things to be set, and we certainly don't pay them any attention now. We should probably just remove them. I'll file a bug.

IN addition, the active is documented in spec but is defined as "encoding is being sent" while our docs state "encoding is being used". I THINK this means that you can set the value to offer it as an option for the encoder/decoder, or you can read the option to find out if it is actively being used in an encoder.
Presumably if you set false this would tell the encoder you want to stop using the value, but setting true wouldn't make an encoder use the encoding, just make it available?

The spec doesn't completely spell this out, but I believe the intent was that active is a "What JS wants you to do" property; if the encoder is not sending the stream due to some internal reason, that would not be reflected here. @jib, does that sound right? Does the spec need a clarification here?

Comment 31

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

See bug 1863534 comment 3 for the members we actually expose, which is a subset of what's in our WebIDL and does NOT include rtx, fec, or srcc.

The active bit is a control surface allowing webapps to temporarily turn off individual simulcast layers to (encode and) send, using setParameters.

But also note Chrome's non-standard "legacy SVC mode" in VP9/AV1, where the active bit instead controls SVC layers (because encodings themselves were abused to control SVC instead of simulcast).

Comment 32

[Hamish Willee]

Thank you both very much. I'm trying to remove those properties from docs and compatibility. I have improved the wording around active.