search

algesten / str0m

A Sans I/O WebRTC implementation in Rust.
MIT License
335 stars 50 forks source link

Add support for loss in BWE #579

Closed k0nserv closed 3 weeks ago

k0nserv commented 1 month ago

This PR, which resurrects #147, adds support for BWE based on loss. Prior to this the BWE system relied solely on congestion manifesting as increasing delay, but it can also manifest as loss, with this PR we'll also react to loss.

Like the existing delay based BWE this is based on goog_cc, in particular it's based on the commit 14e2779a6c from last year.

How it works

The fundamental problem with reacting to loss arising from congestion, is separating that loss from any inherent loss in the environment. For example, a peer might be using a poor WiFi with lots of packet loss on a downlink that can sustain, say 200Mbit/s. In such a situation we'd want to estimate their bandwidth at 200Mbit/s in the extreme, even if the inherent loss is 10%.

Simply reducing estimates based on observed packet loss does not work, in the above scenario we'd cap the peer at a very low estimate incorrectly.

This implementation uses maximum likelihood estimation to estimate the inherent loss $p$. This can then be used to distinguish inherent loss from loss arising from congestion.

k0nserv commented 1 month ago

Thanks for the feedback @xnorpx. I really should've created this as draft since I'm still working on it. I don't expect big changes, but the crate level configuration is particularly unfleshed out

pthatcher commented 1 month ago

Is this based on libwebrtc's loss_based_bwe_v2 (https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc)?

pthatcher commented 1 month ago

Is this based on libwebrtc's loss_based_bwe_v2 (https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc)?

In particular, is it based on "v2"? The description says it's based on a commit from a year ago. Is that too old?

k0nserv commented 1 month ago

Is this based on libwebrtc's loss_based_bwe_v2 (https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc)?

Yeah, it is based on that one.

In particular, is it based on "v2"? The description says it's based on a commit from a year ago. Is that too old?

It's not the latest version of "v2", but since it's tightly coupled with the delay estimator I thought it was good to base them both on the same era of code from libWebRTC. They can then be upgraded to the latest libWebRTC in tandem

xnorpx commented 3 weeks ago

Integration tests?

xnorpx commented 3 weeks ago

Maybe you already have something that can replay pcap and plot them is quite useful. But it's probably enough with debug logging and an parser for that.

k0nserv commented 3 weeks ago

Integration tests?

Hmm, yes. We don't have them for the delay based version either. I think to keep the size of this down I'd like to do that as a separate PR

Maybe you already have something that can replay pcap and plot them is quite useful. But it's probably enough with debug logging and an parser for that.

Yeah, I use the stuff behind the feature _internal_dont_use_log_stats a lot when working on this. I pipe stdout to a file and plot the resulting metrics.