mediasoup icon indicating copy to clipboard operation
mediasoup copied to clipboard

Published 20 hours ago • verified publisher icon versatica

signed-to-unsigned overflow in send_side_bandwidth_estimation.cc

Open fippo opened this issue 1 year ago • 4 comments

Issue description

From libwebrtc upstream: https://bugs.chromium.org/p/webrtc/issues/detail?id=14272

mediasoup has the similar (but not exactly the same, probably an old version) code here, the fix should still apply

fippo avatar Jul 21 '22 17:07 fippo

Thanks @fippo, we'll do some tests next week and see if there is any difference on the resulting BWE.

jmillan avatar Jul 22 '22 21:07 jmillan

The change does not make any better in the local tests. I can even see worst results with it applied :-/

The tests is as simple as using mediasoup-demo with several consumers and periodically checking the mediasoup transport stats in order to see the available outgoing bitrate.

jmillan avatar Jul 28 '22 16:07 jmillan

@jmillan This issue will occur when there is a small packet loss (maybe 2%), and will make the available outgoing bitrate decline to a very low value. Such as: First RR report says there is 5 packets loss total, maybe 2 packets is repaired by rtx or received out of order then, and there is no more packets loss tile untile next RR report, so in the second RR report, the packets loss total will be 3. Then we will get a negative packets_lost_delta.

We found this issue several months ago, and apply a similar fix as what libwebrtc do.

penguinol avatar Sep 13 '22 05:09 penguinol

By the way, this issue is in LossBasedBandwidthEstimation, and there is a defect in it which is not easy to be modified: Upstream packet loss will effect the downstream bandwidth estimation even there is no packet loss on downstream. LossBasedBandwidthEstimation is based on the total packet loss num in RR report, and total packet loss num is related to rtp seq num. It's hard to discriminate whether a packet is lost by upstream or downstream. Transport-CC maybe is a better way to estimation bandwidth for sfu, because it's related on transport-cc seq num.

penguinol avatar Sep 13 '22 07:09 penguinol

@sarumjanuch can you consider these changes into your ongoing PR https://github.com/versatica/mediasoup/pull/922?

UPDATE: Ignore, since it's already done in your PR: https://github.com/versatica/mediasoup/pull/922/files#diff-531cc543a44ef9ae28d7eaea4e06c92879598eeac34186fcf2c5c225c3ad6833R395

ibc avatar Nov 04 '22 11:11 ibc

Anyway changes are applied into current v3 in this PR: https://github.com/versatica/mediasoup/pull/944

ibc avatar Nov 04 '22 12:11 ibc