Closed Bug 1632489 Opened 4 years ago Closed 4 years ago

Add support for repaired-stream-id (rrid)

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla78
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.

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

Depends on D74841

Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/45ad6b848a8f
Enable repaired-rtp-stream-id extension in jsep; r=bwc
https://hg.mozilla.org/integration/autoland/rev/cc52615f7682
Use repaired-rtp-stream-id in rtx packets; r=ng
https://hg.mozilla.org/integration/autoland/rev/8e0260c41854
Negotiate repaired-rtp-stream-id in simulcast mochitests; r=ng
https://hg.mozilla.org/integration/autoland/rev/22ef19e55421
Enable rtx for early beta or earlier; r=ng
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/130e6eba3565
acked out 4 changesets for android crashes at test_peerConnection_maxFsConstraint.html. CLOSED TREE

Android hardware jobs are not run by default on try, so I missed this.

Flags: needinfo?(dminor)

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

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.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla78 → ---

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.

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

Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/58b080b54b6f
Enable repaired-rtp-stream-id extension in jsep; r=bwc
https://hg.mozilla.org/integration/autoland/rev/078662142da6
Use repaired-rtp-stream-id in rtx packets; r=ng
https://hg.mozilla.org/integration/autoland/rev/ad1a558b01bf
Negotiate repaired-rtp-stream-id in simulcast mochitests; r=ng
https://hg.mozilla.org/integration/autoland/rev/aae972036546
Enable rtx for early beta or earlier; r=ng
https://hg.mozilla.org/integration/autoland/rev/09a18345c269
Only adjust valid capture times when generating padding; r=bwc
Regressions: 1638942
Blocks: 1639377
No longer regressions: CVE-2020-12416

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.

No longer depends on: 1646904

The upstream changes have been reviewed and landed.

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 ?

Flags: needinfo?(drno)

Declaring NI bankruptcy.

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