1401592 - Bring RTCRtpParameters/setParameters/getParameters up-to-spec

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, enhancement, P2)

Description

[Byron Campen [:bwc]]

There's quite a bit of validation we aren't doing properly in setParameters, including stuff like transaction ids.

Comment 1

[Byron Campen [:bwc]]

So, the spec says that in setParameters, we check whether parameters.encodings has been reordered. But what, exactly, does it mean to reorder a sequence of mutable dictionaries that don't have some unique id in them? Are we supposed to treat codecPayloadType as the unique id?

Comment 2

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

Resolved offline. This changed a couple of times in the spec:

1. from re-ordering encodings to setting codecPayloadType in w3c/webrtc-pc#1592,
2. then from that to setCodecPreferences in w3c/webrtc-pc/pull#2037.

Filed a couple issues around this: #2313, #2314, and #2315.

Comment 3

[rugk]

This here is required for Jitsi Meet to work properly.

As such, as you had some whiteboard for [jitsi-meet], please assign that to this.
More information by a Jitsi dev, here: https://github.com/jitsi/jitsi-meet/issues/4758#issuecomment-672148872

Comment 4

[Óvári]

Will this feature be available in Firefox 80? Thank you

Comment 5

[Dan Minor [:dminor]]

(In reply to Óvári from comment #4)

Will this feature be available in Firefox 80? Thank you

Definitely not in Firefox 80. I believe that Jitsi is looking for the RTCRtpParameters.active field specifically. Perhaps that could be spun out into a separate bug that could be fixed outside of the larger work to bring things up to spec here. I'd have to defer to Nils and Byron on how practical that would be.

Comment 7

[Byron Campen [:bwc]]

Attachment: Bug 1401592: Test cases. r?jib

Comment 8

[Byron Campen [:bwc]]

Attachment: Bug 1401592: JsepSession::AddTransceiver doesn't need to be fallible, so simplify. r?mjf

Depends on D156828

Comment 9

[Byron Campen [:bwc]]

Attachment: Bug 1401592: Remove encoding constraints from JSEP. r?mjf

The spec has settled on not allowing these constraints to be negotiated, so all
that is left is the negotiation of the set of rids.

Depends on D156829

Comment 10

[Byron Campen [:bwc]]

Attachment: Bug 1401592: Teach RTCRtpSender to detect encoding config changes. r?mjf,pehrsons

JsepTrack used to be in charge of this, but because JSEP doesn't negotiate
encoding constraints, it did not make sense to delegate this to the JSEP code.

Depends on D156830

Comment 11

[Byron Campen [:bwc]]

Attachment: Bug 1401592: Common rid validation logic for SDP and JS. r?mjf,ng

Includes a configured max rid length, since it is not very reasonable to assume
that rids of length 255 are supported by the RTP engine, regardless of what
the IETF specs say.

Depends on D156831

Comment 12

[Byron Campen [:bwc]]

Attachment: Bug 1401592: Remove the media.peerconnection.simulcast pref. r?jib

There is not really any reason to support disabling this feature.

Depends on D156832

Comment 13

[Byron Campen [:bwc]]

Attachment: Bug 1401592: Support sendEncodings in addTransceiver, and bring get/setParameters up to spec. r?jib,mjf

Depends on D156833

Comment 14

[Byron Campen [:bwc]]

Attachment: Bug 1401592: Test cases that expose races between sRD/sLD and setParameters. r?jib

Also adds some helper code based on the helper code in wpt.

Depends on D156834

Comment 15

[Byron Campen [:bwc]]

Attachment: Bug 1401592: Race fixes for sRD/sLD vs setParameters r?jib,mjf

This still needs spec work, but this is a proof of concept that works.
Here we adopt the strategy used for sRD(offer) racing against addTrack; when
the race is detected, the sRD is restarted.

Depends on D156835

Comment 16

[Byron Campen [:bwc]]

Attachment: Bug 1401592: Test-cases for duplicate rids, and rid choices, in SDP. r?jib

Depends on D156836

Comment 17

[Byron Campen [:bwc]]

Attachment: Bug 1401592: Ignore duplicate rids in SDP. r?mjf,jib

Depends on D157650

Comment 18

[Byron Campen [:bwc]]

Attachment: Bug 1401592: Simulcast rollback tests. r?jib

Depends on D157651

Comment 19

[Byron Campen [:bwc]]

Attachment: Bug 1401592: Restore old ridless unicast encoding when JSEP rolls back an initial simulcast offer. r?mjf,jib

Depends on D157653

Comment 20

[Byron Campen [:bwc]]

Attachment: Bug 1401592: Test that changes in rid order from JSEP are ignored. r?jib

Depends on D157654

Comment 21

[Byron Campen [:bwc]]

Attachment: Bug 1401592: Test that sRD with a single rid results in [[SendEncodings]] containing that rid. r?jib

Depends on D157655

Comment 22

[Byron Campen [:bwc]]

Attachment: Bug 1401592: Make sure JSEP distinguishes unicast with rid from unicast without rid. r?mjf,jib

Depends on D157656

Comment 25

[Byron Campen [:bwc]]

Attachment: data-review-1401592.md

Comment 26

[Byron Campen [:bwc]]

Attachment: data-review-1401592.md

Comment 27

[Byron Campen [:bwc]]

Attachment: Bug 1401592: Add a config option to imitate the old setParameters behavior. r?jib,chutten

Depends on D157657

Comment 28

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/jobs?repo=try&revision=b55778617397bd903d783dd915492d815b9ff065
https://treeherder.mozilla.org/jobs?repo=try&revision=62cd99a9edb569baab3dae931b280155052a3a6e
https://treeherder.mozilla.org/jobs?repo=try&revision=1d547ffd14846f0afdd029ee4b26b57ac593fbc5

Comment 29

[Byron Campen [:bwc]]

Attachment: data-review-1401592.md

Comment 30

[Chris H-C :chutten]

Comment on attachment 9302711 [details]
data-review-1401592.md

PRELIMINARY NOTES:
Since this data collection is Glean in Firefox Desktop, you can use mach data-review to generate a data review request template for you.

DATA COLLECTION REVIEW RESPONSE:

Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes.

Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection is Telemetry so can be controlled through Firefox's Preferences.

If the request is for permanent data collection, is there someone who will monitor the data over time?

No. This collection will expire in Firefox 116.

Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1, Technical.

Is the data collection request for default-on or default-off?

Default on for all channels.

Does the instrumentation include the addition of any new identifiers?

No.

Is the data collection covered by the existing Firefox privacy notice?

Yes.

Does the data collection use a third-party collection tool?

No.

---

Result: datareview+

Comment 31

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/jobs?repo=try&revision=7916c2f413fb99b2c0098a33f30dda6339c9cd7c
https://treeherder.mozilla.org/jobs?repo=try&revision=a7e24817b89b27c61cbc984e1005d47203a6e798
https://treeherder.mozilla.org/jobs?repo=try&revision=2fc64af79350d5adb0ca39425a7698faf8e7c153

Comment 32

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/jobs?repo=try&revision=9195f1744d9f7f5c510d733bf9eef8397a7eb3c9
https://treeherder.mozilla.org/jobs?repo=try&revision=61f02ff6fcd6b5404b93434a8ecb58f31daf3145

Comment 33

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/jobs?repo=try&revision=32eca636cc09d2bef9235339eece168b49317d25
https://treeherder.mozilla.org/jobs?repo=try&revision=9ff3316bb6d94c0d0840212cec0beef9a8481b56

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b064fbc9a61
Test cases. r=jib
https://hg.mozilla.org/integration/autoland/rev/26a812435ddd
JsepSession::AddTransceiver doesn't need to be fallible, so simplify. r=mjf
https://hg.mozilla.org/integration/autoland/rev/9e11ddaf8b3e
Remove encoding constraints from JSEP. r=mjf
https://hg.mozilla.org/integration/autoland/rev/fffc015a5dd5
Teach RTCRtpSender to detect encoding config changes. r=mjf,pehrsons
https://hg.mozilla.org/integration/autoland/rev/154d08dd3bca
Common rid validation logic for SDP and JS. r=mjf
https://hg.mozilla.org/integration/autoland/rev/01434a8cb2b6
Remove the media.peerconnection.simulcast pref. r=jib
https://hg.mozilla.org/integration/autoland/rev/ce11d420246c
Support sendEncodings in addTransceiver, and bring get/setParameters up to spec. r=mjf,webidl,smaug
https://hg.mozilla.org/integration/autoland/rev/e868d7b3abd8
Test cases that expose races between sRD/sLD and setParameters. r=jib
https://hg.mozilla.org/integration/autoland/rev/b4752eaae108
Race fixes for sRD/sLD vs setParameters r=mjf
https://hg.mozilla.org/integration/autoland/rev/aa6aa10cfd97
Test-cases for duplicate rids, and rid choices, in SDP. r=jib
https://hg.mozilla.org/integration/autoland/rev/a9bfef738d49
Ignore duplicate rids in SDP. r=mjf
https://hg.mozilla.org/integration/autoland/rev/3cb1dde9bbbc
Simulcast rollback tests. r=jib
https://hg.mozilla.org/integration/autoland/rev/f16c0eb92363
Restore old ridless unicast encoding when JSEP rolls back an initial simulcast offer. r=mjf
https://hg.mozilla.org/integration/autoland/rev/bf98bb55e45f
Test that changes in rid order from JSEP are ignored. r=jib
https://hg.mozilla.org/integration/autoland/rev/00409e6f4685
Test that sRD with a single rid results in [[SendEncodings]] containing that rid. r=jib
https://hg.mozilla.org/integration/autoland/rev/ba525fa85b99
Make sure JSEP distinguishes unicast with rid from unicast without rid. r=mjf
https://hg.mozilla.org/integration/autoland/rev/aba56121e546
Add a config option to imitate the old setParameters behavior. r=jib,chutten,webidl,smaug

Comment 35

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/37477 for changes under testing/web-platform/tests

Comment 36

[Atila Butkovits]

Backed out for causing failures at test_peerConnection_scaleResolution_oldSetParameters.html.

Backout link: https://hg.mozilla.org/integration/autoland/rev/d62d1882c5affd1c699ae5147f97af3cc5d40e88

Pushwith failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=47c810ec80601ac59c5dca5d7df027b131e19e8c&selectedTaskRun=Cjgb_1WrQdW9VUAzzAiphQ.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=399526333&repo=autoland&lineNumber=5348

Comment 37

[Byron Campen [:bwc]]

Is this only failing on Android 8.0 Pixel2 AArch64 WebRender debug?

Comment 38

[Atila Butkovits]

Yes, its failing only on Android 8.0 Pixel2 AArch64 WebRender debug.

Comment 39

[Byron Campen [:bwc]]

Ok, let me cancel the try push I just kicked off, and narrow it a bit.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bdaba2c1cf13
Test cases. r=jib
https://hg.mozilla.org/integration/autoland/rev/ca3f32aeb043
JsepSession::AddTransceiver doesn't need to be fallible, so simplify. r=mjf
https://hg.mozilla.org/integration/autoland/rev/157fce0b0691
Remove encoding constraints from JSEP. r=mjf
https://hg.mozilla.org/integration/autoland/rev/e2341923b50f
Teach RTCRtpSender to detect encoding config changes. r=mjf,pehrsons
https://hg.mozilla.org/integration/autoland/rev/6dab2f3adac1
Common rid validation logic for SDP and JS. r=mjf
https://hg.mozilla.org/integration/autoland/rev/4760dbd9857c
Remove the media.peerconnection.simulcast pref. r=jib
https://hg.mozilla.org/integration/autoland/rev/5c37fb3a7a77
Support sendEncodings in addTransceiver, and bring get/setParameters up to spec. r=mjf,webidl,smaug
https://hg.mozilla.org/integration/autoland/rev/a7f2ea3ca266
Test cases that expose races between sRD/sLD and setParameters. r=jib
https://hg.mozilla.org/integration/autoland/rev/7ab95603e050
Race fixes for sRD/sLD vs setParameters r=mjf
https://hg.mozilla.org/integration/autoland/rev/42543938d60d
Test-cases for duplicate rids, and rid choices, in SDP. r=jib
https://hg.mozilla.org/integration/autoland/rev/f27124a55a11
Ignore duplicate rids in SDP. r=mjf
https://hg.mozilla.org/integration/autoland/rev/15e2c79da249
Simulcast rollback tests. r=jib
https://hg.mozilla.org/integration/autoland/rev/a6e4123b8836
Restore old ridless unicast encoding when JSEP rolls back an initial simulcast offer. r=mjf
https://hg.mozilla.org/integration/autoland/rev/bd4500a78ca4
Test that changes in rid order from JSEP are ignored. r=jib
https://hg.mozilla.org/integration/autoland/rev/9c621a081558
Test that sRD with a single rid results in [[SendEncodings]] containing that rid. r=jib
https://hg.mozilla.org/integration/autoland/rev/a5ceeaf7e43b
Make sure JSEP distinguishes unicast with rid from unicast without rid. r=mjf
https://hg.mozilla.org/integration/autoland/rev/92f418bbba4a
Add a config option to imitate the old setParameters behavior. r=jib,chutten,webidl,smaug

Comment 41

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/bdaba2c1cf13
https://hg.mozilla.org/mozilla-central/rev/ca3f32aeb043
https://hg.mozilla.org/mozilla-central/rev/157fce0b0691
https://hg.mozilla.org/mozilla-central/rev/e2341923b50f
https://hg.mozilla.org/mozilla-central/rev/6dab2f3adac1
https://hg.mozilla.org/mozilla-central/rev/4760dbd9857c
https://hg.mozilla.org/mozilla-central/rev/5c37fb3a7a77
https://hg.mozilla.org/mozilla-central/rev/a7f2ea3ca266
https://hg.mozilla.org/mozilla-central/rev/7ab95603e050
https://hg.mozilla.org/mozilla-central/rev/42543938d60d
https://hg.mozilla.org/mozilla-central/rev/f27124a55a11
https://hg.mozilla.org/mozilla-central/rev/15e2c79da249
https://hg.mozilla.org/mozilla-central/rev/a6e4123b8836
https://hg.mozilla.org/mozilla-central/rev/bd4500a78ca4
https://hg.mozilla.org/mozilla-central/rev/9c621a081558
https://hg.mozilla.org/mozilla-central/rev/a5ceeaf7e43b
https://hg.mozilla.org/mozilla-central/rev/92f418bbba4a

Comment 42

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 44

[BugBot [:suhaib / :marco/ :calixte]]

Copying crash signatures from duplicate bugs.

Comment 45

[Ruth John]

Tracking issue for MDN work https://github.com/mdn/content/issues/23677

Comment 47

[Hamish Willee]

I'm trying to update the docs and browser compatibility information for RTCRtpSender.getParameters(), RTCRtpSender.setParameters() in light of these changes.

The methods return an object with: codecs, headerExtensions, rtcp, from RTCRtpParameters, and encodings, transactionId, degradationPreference from RTCRtpSendParameters

• The RTCRtpParameters properties seem to match the spec but be empty in FF110. Do they "work/get populated"? Or are they just objects to conform to the spec? There is a note in the change somewhere that seems to indicate the later.
• In the current compatibility data it says for encodings "Firefox uses RTCRtpParameters.encodings instead. From an end user point of view does it matter? - Note that compatibility data will just record an object being returned - the dictionaries are no longer quoted if at all possible.
• The compatibility data is supposed to show compatibility breaks compared to the spec. For RTCRtpSender.getParameters() I can see that now we return an object that matches the spec. In FF109 and earlier the object that returned was not particularly amenable to inspection - do we know what was "wrong" with the getParameters() and the returned object w.r.t. the spec for a developer? For example, it sounds like codecs, headerExtensions, rtcp are present but not used by firefox, and that was true prior to FF110 - so has any thing changed for developers?
• Similarly, for the MDN release note I can say "Brings RTCRtpParameters/setParameters/getParameters up-to-spec", but that begs the question "what were the main things that were wrong before". On scan some of the things that happened:
  • RTCRtpEncodingParameters.active support implemented (in a different issue, but part of this). Support sendEncodings in addTransceiver
  • RTCRtpEncodingParameters - added default values for .active and .priority. Removed .degradationPreference
  • There also seems to be some kind of warnings if you try and set parameters but haven't got them first.
  • What would you consider important to let developers know?

Comment 48

[Hamish Willee]

EDITED: - confirmed that the there is only one peer associated with each sender. Would appreciate confirmation of how this works still. My guess:

• Encodings that are available are set when you call RTCPeerConnection.addTransceiver()
• When you call RTCRtpSender.getParameters() you'll get this list of encodings. The one that is in use for sending data is the one with the active flag set - right? Given the default for the active flag is true does that mean if you have multiples you will have to explicitly set the ones you don't want to use as false or will this happen for you when you set an encoding as active?
• It looks like you can get a list of codecs in the parameters too. However as these are set using transceiver.setCodecPreferences from a point of view of set/getParameters these would be read-only right?
• In other words, if you're getting and setting parameters, the interesting one in this discussion is the encodings, since you can use those to change how the data is sent. The codecs are more "for information" at this point..

Is that ^^^ in any way correct?