Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

signed-to-unsigned overflow in send_side_bandwidth_estimation.cc #872

Closed
fippo opened this issue Jul 21, 2022 · 6 comments · Fixed by #944 · May be fixed by #922
Closed

signed-to-unsigned overflow in send_side_bandwidth_estimation.cc #872

fippo opened this issue Jul 21, 2022 · 6 comments · Fixed by #944 · May be fixed by #922
Labels
bug

Comments

@fippo
Copy link
Contributor

fippo commented Jul 21, 2022

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 fippo added the bug label Jul 21, 2022
@jmillan
Copy link
Member

jmillan commented Jul 22, 2022

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

@jmillan
Copy link
Member

jmillan commented Jul 28, 2022

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.

@penguinol
Copy link
Contributor

penguinol commented Sep 13, 2022 •
edited
Loading

@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
Copy link
Contributor

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.

@ibc
Copy link
Member

ibc commented Nov 4, 2022 •
edited
Loading

@sarumjanuch can you consider these changes into your ongoing PR #922?

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

@ibc ibc linked a pull request Nov 4, 2022 that will close this issue
Backport changes to congestion control from libwebrtc #922
Draft
ibc added a commit that referenced this issue Nov 4, 2022
@ibc
libwebrtc fixes and updates
a064ffe 
- Fix calculation of feedback min_pending_time in goog_cc
  - Fixes #849
  - Commit in libwebrtc: https://webrtc.googlesource.com/src/+/d65dc979b17cdc7cd359aada59e5bce8a6f1b8ce%5E%21/
-
Fix signed-to-unsigned overflow in send_side_bandwidth_estimation.cc
  - Fixes #872
  - Issue in libwebrtc: https://bugs.chromium.org/p/webrtc/issues/detail?id=14272
  - Commit in libwebrtc: https://webrtc.googlesource.com/src/+/9804aa5f6ad26a45338d685da66497c3bbd88ca6%5E%21/

NOTE: Some changes are already present in ongoing PR #922 but it's not yet merged.
@ibc ibc mentioned this issue Nov 4, 2022
Merged
@ibc
Copy link
Member

ibc commented Nov 4, 2022

Anyway changes are applied into current v3 in this PR: #944

@ibc ibc closed this as completed in #944 Nov 4, 2022
ibc added a commit that referenced this issue Nov 4, 2022
@ibc
libwebrtc fixes and updates (#944)
60f8200 
- Fix calculation of feedback min_pending_time in goog_cc
  - Fixes #849
  - Commit in libwebrtc: https://webrtc.googlesource.com/src/+/d65dc979b17cdc7cd359aada59e5bce8a6f1b8ce%5E%21/
-
Fix signed-to-unsigned overflow in send_side_bandwidth_estimation.cc
  - Fixes #872
  - Issue in libwebrtc: https://bugs.chromium.org/p/webrtc/issues/detail?id=14272
  - Commit in libwebrtc: https://webrtc.googlesource.com/src/+/9804aa5f6ad26a45338d685da66497c3bbd88ca6%5E%21/

NOTE: Some changes are already present in ongoing PR #922 but it's not yet merged.
piranna pushed a commit to dyte-in/mediasoup that referenced this issue Feb 9, 2023
@ibc @piranna
libwebrtc fixes and updates (versatica#944)
921cd6d 
- Fix calculation of feedback min_pending_time in goog_cc
  - Fixes versatica#849
  - Commit in libwebrtc: https://webrtc.googlesource.com/src/+/d65dc979b17cdc7cd359aada59e5bce8a6f1b8ce%5E%21/
-
Fix signed-to-unsigned overflow in send_side_bandwidth_estimation.cc
  - Fixes versatica#872
  - Issue in libwebrtc: https://bugs.chromium.org/p/webrtc/issues/detail?id=14272
  - Commit in libwebrtc: https://webrtc.googlesource.com/src/+/9804aa5f6ad26a45338d685da66497c3bbd88ca6%5E%21/

NOTE: Some changes are already present in ongoing PR versatica#922 but it's not yet merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Milestone
No milestone
4 participants
@ibc @fippo @jmillan @penguinol