Closed Bug 1606823 Opened 5 years ago Closed 4 years ago

Add support for WebRTC sender side bandwidth estimation based on transport-cc extensions

Categories

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

Product:
Core
Component:
WebRTC: Networking
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla76
Tracking Flags:
Tracking Status
firefox76 --- fixed

People

(Reporter: gustavogb, Assigned: jryans)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [jitsi-meet])

Attachments

(2 files)

Bug 1606823 - Add support for WebRTC sender side bandwidth estimation based on transport-cc extensions
47 bytes, text/x-phabricator-request
Details | Review
Bug 1606823 - Add support for WebRTC transport-cc extension. r=bwc,drno
47 bytes, text/x-phabricator-request
Details | Review

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/79.0.3945.88 Safari/537.36

Steps to reproduce:

While Chrome supports receiver-side bandwidth estimation based on REMB and sender-side bandwidth estimation based on TransportFeedback Firefox only supports the former.

The later is more flexible and modern and supporting it in Firefox would make it easier to interoperate with servers supporting only sender-side bandwidth estimation and also easier to experiment with new bandwidth estimation algorithms (f.e. NADA).

Add support for WebRTC sender side bandwidth estimation based on transport-cc extensions

This change includes support to negotiate the transport-wide-cc rtp extension needed to enable sender side bandwidth estimation in WebRTC. When this extension is supported in both sides during the Offer/Answer negotiation the transport_cc mode is enabled in the WebRTC engine so that this mode is used instead of the legacy receiver-side (REMB based) mechanism.

The change is inspired on this fork by medooze team: https://github.com/medooze/gecko-dev/pull/2/files

Assignee: nobody → gustavogb

So, to implement this, we need to tell webrtc.org about the transports, and which RTP streams are in those transports. How is that supposed to be done?

Flags: needinfo?(na-g)
Flags: needinfo?(dminor)
Status: UNCONFIRMED → NEW
Component: Untriaged → WebRTC: Networking
Ever confirmed: true
Product: Firefox → Core

(In reply to Byron Campen [:bwc] from comment #2)

So, to implement this, we need to tell webrtc.org about the transports, and which RTP streams are in those transports. How is that supposed to be done?

There is only one "transport" per PC, so there is no need for doing anything extra AFAIK.

(In reply to sergio.garcia.murillo from comment #3)

(In reply to Byron Campen [:bwc] from comment #2)

So, to implement this, we need to tell webrtc.org about the transports, and which RTP streams are in those transports. How is that supposed to be done?

There is only one "transport" per PC, so there is no need for doing anything extra AFAIK.

If everything is in a single bundle, yes, but that's not guaranteed.

The transport-cc tests in the sdp_unittests can be re-enabled in this landing. I can provide this patch, though further work may be done to get them to pass as expected.

I have been looking through the webrtc native library code. It looks like it is implemented there as Sergio has said (as far as I can see). Does anyone implement this differently?

Each stream has a "Transport" object and we could potentially teach lib webrtc to key on some type of transport ID.
Right now the AudioConduit and VideoConduit act as a webrtc::Transport and would need to be aware of their bundle or transport ID.

These changes would be large enough I think that optimally we would want to implement them upstream and cherry pick them back. This would have the upshot of fixing for every one the case where everything isn't in a single bundle.

:bwc signaling thoughts?
:dminor uplift / merge thoughts?

Flags: needinfo?(na-g) → needinfo?(docfaraday)

I agree that large changes should ideally land on upstream webrtc.org first and then be backported. That benefits everyone that uses webrtc.org and reduces our maintenance burden.

gustavogb, thank you for your patch! This was something we were thinking of for this quarter anyway, so having it come from an external contributor is very helpful.

:bwc, is it possible to only negotiate transport-cc if everything is in a single bundle? We could then break the work up into three chunks:

  • get this patch reviewed and landed
  • look at the upstream webrtc.org changes
  • cherrypick those changes back to Firefox and support the non-single bundle case
Flags: needinfo?(dminor)

(In reply to Dan Minor [:dminor] from comment #6)

I agree that large changes should ideally land on upstream webrtc.org first and then be backported. That benefits everyone that uses webrtc.org and reduces our maintenance burden.

gustavogb, thank you for your patch! This was something we were thinking of for this quarter anyway, so having it come from an external contributor is very helpful.

:bwc, is it possible to only negotiate transport-cc if everything is in a single bundle? We could then break the work up into three chunks:

  • get this patch reviewed and landed
  • look at the upstream webrtc.org changes
  • cherrypick those changes back to Firefox and support the non-single bundle case

If we're the answerer, that's easy, although if we're the offerer we don't know if the other end will accept our proposed bundle. If we're the offerer, I guess we could offer to use transport-cc if we're also offering everything in a single bundle, and just hope that the other end doesn't reject the bundle?

Flags: needinfo?(docfaraday)
Priority: -- → P2

(In reply to Byron Campen [:bwc] from comment #7)

(In reply to Dan Minor [:dminor] from comment #6)

I agree that large changes should ideally land on upstream webrtc.org first and then be backported. That benefits everyone that uses webrtc.org and reduces our maintenance burden.

gustavogb, thank you for your patch! This was something we were thinking of for this quarter anyway, so having it come from an external contributor is very helpful.

:bwc, is it possible to only negotiate transport-cc if everything is in a single bundle? We could then break the work up into three chunks:

  • get this patch reviewed and landed
  • look at the upstream webrtc.org changes
  • cherrypick those changes back to Firefox and support the non-single bundle case

If we're the answerer, that's easy, although if we're the offerer we don't know if the other end will accept our proposed bundle. If we're the offerer, I guess we could offer to use transport-cc if we're also offering everything in a single bundle, and just hope that the other end doesn't reject the bundle?

Without changes to the native webrtc library, we can offer it whenever we only have a single transport, correct? That would include a single simulcast group, which may be fairly common for audio only calls.

(In reply to Nico Grunbaum [:ng] from comment #8)

(In reply to Byron Campen [:bwc] from comment #7)

(In reply to Dan Minor [:dminor] from comment #6)

I agree that large changes should ideally land on upstream webrtc.org first and then be backported. That benefits everyone that uses webrtc.org and reduces our maintenance burden.

gustavogb, thank you for your patch! This was something we were thinking of for this quarter anyway, so having it come from an external contributor is very helpful.

:bwc, is it possible to only negotiate transport-cc if everything is in a single bundle? We could then break the work up into three chunks:

  • get this patch reviewed and landed
  • look at the upstream webrtc.org changes
  • cherrypick those changes back to Firefox and support the non-single bundle case

If we're the answerer, that's easy, although if we're the offerer we don't know if the other end will accept our proposed bundle. If we're the offerer, I guess we could offer to use transport-cc if we're also offering everything in a single bundle, and just hope that the other end doesn't reject the bundle?

Without changes to the native webrtc library, we can offer it whenever we only have a single transport, correct? That would include a single simulcast group, which may be fairly common for audio only calls.

Yes, or any case where there is only one m-section without a=bundle-only.

See Also: → 1600698

could it be a possibility to enable it for now when peerconnection is created with "max-bundle" RTCBundlePolicy as config?

ping? anything we can do to help on this?

ping? With the coronavirus crisis a video plattform like https://jitsi.org/ is needed more than ever! And jitsi needs firefox and to use it, this bug needs to be solved.

you could even say. solving this bug and helping jitsi.org is a way of firefox to help people at home, while the coronavirus/covid-19 is around! Should be noted in the release notes. maybe even mozilla.org will sponsor a videoconferencing server - the jitsi-bridge. All of jitsi is open source.

Are you still available to work on this? Or do you need one of us to take it the rest of the way?

Flags: needinfo?(gustavogb)

(In reply to Byron Campen [:bwc] from comment #14)

Are you still available to work on this? Or do you need one of us to take it the rest of the way?

Sorry. I guess, I'm not able to work on this - if C coding is required. I Just wanted to use jitsi with my favorite browser firefox.

(In reply to Jochen from comment #16)

Sorry. I guess, I'm not able to work on this - if C coding is required. I Just wanted to use jitsi with my favorite browser firefox.

The question was aimed at Gustavo, who provided the original patch. Nobody expected from you to take on this work only because you commented in this bug :-)

Although we are always open for contributors helping out in any way they can.

Blocks: 1600698
See Also: 1600698 → https://github.com/jitsi/jitsi-meet/issues/4758
Whiteboard: [jitsi-meet]

This change includes support to negotiate the transport-wide-cc RTP extension
needed to enable sender side bandwidth estimation in WebRTC. When this
extension is supported in both sides during the Offer/Answer negotiation the
transport_cc mode is enabled in the WebRTC engine so that this mode is used
instead of the legacy receiver-side (REMB based) mechanism.

The change is inspired on this fork by medooze team:
https://github.com/medooze/gecko-dev/pull/2/files

Co-authored-by: ggarber <gustavogb@gmail.com>

I'll attempt to nudge this forward. I have attempted to address the review comments from the previous patch.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ae09a640b91ac05d8f4038aa901a76e9d8ae632

Assignee: gustavogb → jryans
Status: NEW → ASSIGNED
Flags: needinfo?(gustavogb)

Ok. Given that REMB is broken in the non-bundle case anyway (bug 1576576), I guess we can live with transport-cc being broken in the non-bundle case too (see comment 2, comment 6, etc). We'll need to file a bug for this problem to follow up.

I have verified with Wireshark that transport-cc RTCP data is flowing in both directions with the attached patch.

(In reply to Byron Campen [:bwc] from comment #21)

Ok. Given that REMB is broken in the non-bundle case anyway (bug 1576576), I guess we can live with transport-cc being broken in the non-bundle case too (see comment 2, comment 6, etc). We'll need to file a bug for this problem to follow up.

Filed bug 1626383 to add support for the non-bundle case.

Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/67c48f5aea11
Add support for WebRTC transport-cc extension. r=bwc,drno

Thank you very much everyone involved!

I'll flip the pref to on my machine and start testing :-)

https://hg.mozilla.org/mozilla-central/rev/67c48f5aea11

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox76: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla76
Regressions: 1628678

Is this still behind a pref? Just tested with FF78 on https://webrtc.github.io/samples/src/content/peerconnection/pc1/ and not seeing transport-cc in the generated SDP.

(In reply to Justin Uberti from comment #27)

Is this still behind a pref? Just tested with FF78 on https://webrtc.github.io/samples/src/content/peerconnection/pc1/ and not seeing transport-cc in the generated SDP.

It is limited together with RTX to Nightly and early Beta's only right now. We are planing on letting this ride to release soon.

ah, excellent, will try this out in Nightly

Was able to get this working in nightly.

I noticed that transport-cc is not offered for the audio m=section, is this intentional? Or should I file a new bug for this?

Flags: needinfo?(dminor)

(In reply to Justin Uberti from comment #30)

Was able to get this working in nightly.

I noticed that transport-cc is not offered for the audio m=section, is this intentional? Or should I file a new bug for this?

As far as I know, this was not intentional. Please do file a new bug for this.

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

Attachment

General

Creator:
Created:
Updated:
Size: