Add support for repaired-stream-id (rrid)
Categories
(Core :: WebRTC: Signaling, enhancement, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox78 | --- | fixed |
People
(Reporter: dminor, Assigned: dminor)
References
Details
Attachments
(5 files)
Once Bug 1164187 lands, we'll have support for rtx. We don't support signaling support for repaired-stream-id (rrid), so the rtx packets will have the rid from the original packet, but the not the rrid, which is what we should be doing as far as the standards go. The version of libwebrtc we're using appears to support rrid already, so this should just be a matter of adding support to our sdp parsers and the jsep code.
Assignee | ||
Comment 1•5 years ago
|
||
Assignee | ||
Comment 2•5 years ago
|
||
Assignee | ||
Comment 3•5 years ago
|
||
This imports a few fixes from tip of libwebrtc, which now supports mid, rid
and rrid. The rtx packet is now allocated as max_packet_size_, which is
necessary to have enough capacity for the rrid. It takes the
CopyHeaderAndExtensionsToRtxPacket function from upstream, which omits
copying extensions that should not be copied over, such as rid. It is necessary
to make AllocateExtension and FindExtension public in order for this
function to work.
It then copies the rid from the source packet over to rrid in the rtx packet.
Upstream has code for this as well, but taking it would require more
extensive changes to our copy of libwebrtc. We can drop these local
modifications with the next update.
Depends on D74839
Assignee | ||
Comment 4•5 years ago
|
||
Depends on D74840
Assignee | ||
Comment 5•5 years ago
|
||
Depends on D74841
Comment 8•5 years ago
|
||
Backed out 4 changesets (Bug 1632489) for android crashes at test_peerConnection_maxFsConstraint.html.
https://hg.mozilla.org/integration/autoland/rev/130e6eba35651147ed083bb4c72f2e1551ced912
Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=302160140&repo=autoland&lineNumber=2350
Assignee | ||
Comment 9•5 years ago
|
||
Android hardware jobs are not run by default on try, so I missed this.
Comment 10•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/45ad6b848a8f
https://hg.mozilla.org/mozilla-central/rev/cc52615f7682
https://hg.mozilla.org/mozilla-central/rev/8e0260c41854
https://hg.mozilla.org/mozilla-central/rev/22ef19e55421
https://hg.mozilla.org/mozilla-central/rev/130e6eba3565
Assignee | ||
Comment 11•5 years ago
|
||
This seems to be the same intermittent that Sergio hit when he was working on rtx. With rtx enabled, we rewrite timestamps when sending padding here [1], which occasionally causes us to fail the assertion here [2].
I suspect the problem is with the calculation of now_ms. In the current code, we calculate now_ms, then in a while loop, acquire a lock, do some calculations, release the lock and then send a packet, which seems dubious. Upstream has changed quite a bit here, the equivalent function takes the lock, builds an array of padding packets, and then sends them at a later point. I'm hoping I can find a smaller change than taking the current upstream version.
[1] https://searchfox.org/mozilla-central/rev/9f074fab9bf905fad62e7cc32faf121195f4ba46/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_sender.cc#565-572
[2] https://searchfox.org/mozilla-central/rev/4166c15e2a99a23a9b38ad62c9fdfe8e5448b354/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc#115
Assignee | ||
Comment 12•5 years ago
|
||
Reopening, I'm assuming this is still backed out. If not, we'll want to file a follow up bug for the intermittent assertion failure.
Updated•5 years ago
|
Assignee | ||
Comment 13•5 years ago
|
||
Fix is looking good on try: https://treeherder.mozilla.org/#/jobs?repo=try&selectedTaskRun=A6YFs24oSMe8ZOa3F6fwHw-0&revision=c0d497652d1dda7cac530e1a039ef509b67d94ec. Without this patch, I was seeing a crash rate of more than 80%. I don't want to retrigger too many times on the Android hardware.
Assignee | ||
Comment 14•5 years ago
|
||
The current code will only set the TranmissionOffset extension if
capture_time_ms is > 0, but when adjusting timestamps for rtx packets, it is
adjusted without first checking to see if it is valid, which will cause invalid
values of capture_time_ms to be written to TranmissionOffset, leading to assertion
failures.
This bug is still present on tip of libwebrtc, so we'll also need to prepare a
patch for upstream.
Depends on D74842
Comment 15•5 years ago
|
||
Comment 16•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/58b080b54b6f
https://hg.mozilla.org/mozilla-central/rev/078662142da6
https://hg.mozilla.org/mozilla-central/rev/ad1a558b01bf
https://hg.mozilla.org/mozilla-central/rev/aae972036546
https://hg.mozilla.org/mozilla-central/rev/09a18345c269
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 17•5 years ago
|
||
We need to upstream the changes to RTPSender::SendPadData. Upstream bug here: https://bugs.chromium.org/p/webrtc/issues/detail?id=11615 and upstream review here: https://webrtc-review.googlesource.com/c/src/+/176281.
Assignee | ||
Comment 18•5 years ago
|
||
The upstream changes have been reviewed and landed.
Comment 19•4 years ago
|
||
Any idea what release version will have support for rtp-stream-id and repaired-rtp-stream-id, I am on version 80 and i dont have it on the header extension ?
Description
•