Closed Bug 1401592 Opened 7 years ago Closed 1 year ago

Bring RTCRtpParameters/setParameters/getParameters up-to-spec

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
110 Branch
Tracking Status
firefox110 --- fixed

People

(Reporter: bwc, Assigned: bwc)

References

(Blocks 6 open bugs)

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [jitsi-meet], [wptsync upstream])

Crash Data

Attachments

(18 files, 2 obsolete files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
5.37 KB, text/plain
chutten
: data-review+
Details
There's quite a bit of validation we aren't doing properly in setParameters, including stuff like transaction ids.
Rank: 25
Priority: -- → P3
Rank: 25 → 19
Priority: P3 → P2
Summary: Bring setParameters up-to-spec → Bring RTCRtpParameters/setParameters/getParameters up-to-spec
Depends on: 1531458
Assignee: nobody → docfaraday
Blocks: 1573726

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?

Flags: needinfo?(jib)
Depends on: 1470568

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.

Flags: needinfo?(jib)
Depends on: 1586109
Blocks: 1611957
Keywords: site-compat

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

Whiteboard: [jitsi-meet]

Will this feature be available in Firefox 80? Thank you

Flags: needinfo?(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.

Flags: needinfo?(dminor)
Blocks: 1688342
See Also: → 1787474

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

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

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

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

Depends on D156832

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

Depends on D156834

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

Blocks: 1790703
Attachment #9293700 - Attachment description: WIP: Bug 1401592: (WIP) Test cases. → WIP: Bug 1401592: Test cases.
Attachment #9293707 - Attachment description: WIP: Bug 1401592: Test case that exposes a race between sRD(unicast answer) and setParameters(simulcast). → WIP: Bug 1401592: Test cases that expose a race between sRD(unicast offer) and setParameters(simulcast).

Depends on D157650

Depends on D157651

Attachment #9293700 - Attachment description: WIP: Bug 1401592: Test cases. → Bug 1401592: Test cases.
Attachment #9293700 - Attachment description: Bug 1401592: Test cases. → Bug 1401592: Test cases. r?jib
Attachment #9293701 - Attachment description: WIP: Bug 1401592: JsepSession::AddTransceiver doesn't need to be fallible, so simplify. → Bug 1401592: JsepSession::AddTransceiver doesn't need to be fallible, so simplify. r?mjf
Attachment #9293702 - Attachment description: WIP: Bug 1401592: Remove encoding constraints from JSEP. → Bug 1401592: Remove encoding constraints from JSEP. r?mjf
Attachment #9293703 - Attachment description: WIP: Bug 1401592: Teach RTCRtpSender to detect encoding config changes. → Bug 1401592: Teach RTCRtpSender to detect encoding config changes. r?mjf,pehrsons
Attachment #9293704 - Attachment description: WIP: Bug 1401592: Common rid validation logic for SDP and JS. → Bug 1401592: Common rid validation logic for SDP and JS. r?mjf,ng
Attachment #9293705 - Attachment description: WIP: Bug 1401592: Remove the media.peerconnection.simulcast pref. → Bug 1401592: Remove the media.peerconnection.simulcast pref. r?jib
Attachment #9293706 - Attachment description: WIP: Bug 1401592: Support sendEncodings in addTransceiver, and bring get/setParameters up to spec. → Bug 1401592: Support sendEncodings in addTransceiver, and bring get/setParameters up to spec. r?jib,mjf
Attachment #9293707 - Attachment description: WIP: Bug 1401592: Test cases that expose a race between sRD(unicast offer) and setParameters(simulcast). → Bug 1401592: Test cases that expose a race between sRD(unicast offer) and setParameters(simulcast). r?jib
Attachment #9293708 - Attachment description: WIP: Bug 1401592: Race fix for sRD(unicast offer) vs setParameters(simulcast) → Bug 1401592: Race fix for sRD(unicast offer) vs setParameters(simulcast) r?jib,mjf
Attachment #9295279 - Attachment description: WIP: Bug 1401592: Test-cases for duplicate rids, and rid choices, in SDP. → Bug 1401592: Test-cases for duplicate rids, and rid choices, in SDP. r?jib
Attachment #9295280 - Attachment description: WIP: Bug 1401592: Ignore duplicate rids in SDP. → Bug 1401592: Ignore duplicate rids in SDP. r?mjf,jib
Attachment #9295282 - Attachment description: WIP: Bug 1401592: Simulcast rollback tests. → Bug 1401592: Simulcast rollback tests. r?jib
Attachment #9295285 - Attachment description: WIP: Bug 1401592: Restore old [[SendEncodings]] when rollback occurs. → Bug 1401592: Restore old [[SendEncodings]] when rollback occurs. r?mjf,jib
Attachment #9295286 - Attachment description: WIP: Bug 1401592: Test that changes in rid order from JSEP are ignored. → Bug 1401592: Test that changes in rid order from JSEP are ignored. r?jib
Attachment #9295287 - Attachment description: WIP: Bug 1401592: Test that sRD with a single rid results in [[SendEncodings]] containing that rid. → Bug 1401592: Test that sRD with a single rid results in [[SendEncodings]] containing that rid. r?jib
Attachment #9295288 - Attachment description: WIP: Bug 1401592: Make sure JSEP distinguishes unicast with rid from unicast without rid. → Bug 1401592: Make sure JSEP distinguishes unicast with rid from unicast without rid. r?mjf,jib
Severity: normal → S3
Attachment #9295285 - Attachment description: Bug 1401592: Restore old [[SendEncodings]] when rollback occurs. r?mjf,jib → Bug 1401592: Restore old ridless unicast encoding when JSEP rolls back an initial simulcast offer. r?mjf,jib
Attachment #9293707 - Attachment description: Bug 1401592: Test cases that expose a race between sRD(unicast offer) and setParameters(simulcast). r?jib → Bug 1401592: Test cases that expose races between sRD/sLD and setParameters. r?jib
Attachment #9293708 - Attachment description: Bug 1401592: Race fix for sRD(unicast offer) vs setParameters(simulcast) r?jib,mjf → Bug 1401592: Race fixes for sRD/sLD vs setParameters r?jib,mjf
Attached file data-review-1401592.md (obsolete) —
Attachment #9302399 - Flags: data-review?(chutten)
Attached file data-review-1401592.md (obsolete) —
Attachment #9302399 - Attachment is obsolete: true
Attachment #9302399 - Flags: data-review?(chutten)
Attachment #9302409 - Flags: data-review?(chutten)
Attached file data-review-1401592.md —
Attachment #9302409 - Attachment is obsolete: true
Attachment #9302409 - Flags: data-review?(chutten)
Attachment #9302711 - Flags: data-review?(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+

Attachment #9302711 - Flags: data-review?(chutten) → data-review+
Blocks: 1799977
Blocks: 1803388
Pushed by bcampen@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/37477 for changes under testing/web-platform/tests
Whiteboard: [jitsi-meet] → [jitsi-meet], [wptsync upstream]
Regressions: 1805510

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

Flags: needinfo?(docfaraday) → needinfo?(abutkovits)

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

Flags: needinfo?(abutkovits)

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

Pushed by bcampen@mozilla.com:
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
Upstream PR merged by moz-wptsync-bot
Regressions: 1805901
No longer regressions: 1726247
Duplicate of this bug: 1683934

Copying crash signatures from duplicate bugs.

Crash Signature: [@ mozalloc_abort | abort | webrtc::VCMEncodedFrameCallback::OnEncodedImage] [@ mozilla::WebrtcVideoConduit::CreateSendStream]

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

Crash Signature: [@ mozalloc_abort | abort | webrtc::VCMEncodedFrameCallback::OnEncodedImage] [@ mozilla::WebrtcVideoConduit::CreateSendStream] → [@ mozalloc_abort | abort | webrtc::VCMEncodedFrameCallback::OnEncodedImage] [@ mozilla::WebrtcVideoConduit::CreateSendStream]
Regressions: 1812293
Duplicate of this bug: 1547036

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?
Flags: needinfo?(docfaraday)

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?

See Also: → 1832459

(In reply to Hamish Willee from comment #47)

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?

The main change is implementation of the transactionId mechanic. Previously, Firefox was expecting JS to build the RTCRtpSendParameters from scratch, and call setParameters with it. Now, the spec requires JS to obtain a pre-built RTCRtpSendParameters using getParameters(), make (allowed) alterations, and then call setParameters() with the altered version, without relinquishing the event loop. Making this usable required implementing addTransceiver({sendEncodings: [...]}, since that is now the only way for JS to specify how many encodings to use (since JS can no longer build the parameters object from scratch).

Regarding .active, the idea is that you'll typically be sending on more than one encoding at a time (at different quality levels, so a conferencing server can transmit low quality to some participants, and high quality to others). JS can use .active to pause transmission of a particular encoding (just as a contrived example, maybe you're in a long conference but have never spoken, so transmitting the high quality video is not necessary, but we want to be able to enable it if it becomes useful later).

The codecs stuff is read only, and we don't really support it right now.

Flags: needinfo?(docfaraday)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: