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.

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

min_pending_time should not be negative #849

Closed
HustCoderHu opened this issue Jun 30, 2022 · 7 comments · Fixed by #944 · May be fixed by #922
Closed

min_pending_time should not be negative #849

HustCoderHu opened this issue Jun 30, 2022 · 7 comments · Fixed by #944 · May be fixed by #922
Labels

Comments

@HustCoderHu
Copy link

TimeDelta min_pending_time = feedback.receive_time - max_recv_time;

Should it be like following?

TimeDelta min_pending_time = max_recv_time - feedback.receive_time;
@ibc
Copy link
Member

ibc commented Jul 1, 2022

This is code from libwebrtc. Can you verify it in latest libwebrtc source code?

@ibc
Copy link
Member

ibc commented Jul 4, 2022

Why is it obviously wrong?

@HustCoderHu
Copy link
Author

  Timestamp max_recv_time = Timestamp::MinusInfinity();

  std::vector<PacketResult> feedbacks = report.ReceivedWithSendInfo();
  for (const auto& feedback : feedbacks)
    max_recv_time = std::max(max_recv_time, feedback.receive_time);

  for (const auto& feedback : feedbacks) {
    TimeDelta feedback_rtt =
        report.feedback_time - feedback.sent_packet.send_time;
    TimeDelta min_pending_time = feedback.receive_time - max_recv_time;
    TimeDelta propagation_rtt = feedback_rtt - min_pending_time;
    // propagation_rtt ought to be smaller than feedback_rtt, coz feedback_rtt includes packet pending time at remote, 
    // thus pending_time is a positive value
    max_feedback_rtt = std::max(max_feedback_rtt, feedback_rtt);
    min_propagation_rtt = std::min(min_propagation_rtt, propagation_rtt);
  }

@ibc
Copy link
Member

ibc commented Jul 5, 2022

Here you are asking for a change in libwebrtc. I'm not saying that your proposed change is bad but we need much more info and rationale about it.

@fippo
Copy link
Contributor

fippo commented Jul 20, 2022

libwebrtc issue here: https://bugs.chromium.org/p/webrtc/issues/detail?id=13106, no movement for a while

ibc added a commit that referenced this issue Nov 4, 2022
- 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
Copy link
Member

ibc commented Nov 4, 2022

Changes are applied into current v3 in this PR: #944

@ibc ibc linked a pull request Nov 4, 2022 that will close this issue
@ibc ibc closed this as completed in #944 Nov 4, 2022
ibc added a commit that referenced this issue Nov 4, 2022
- 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
- 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
Labels
3 participants