mediasoup icon indicating copy to clipboard operation
mediasoup copied to clipboard

min_pending_time should not be negative

Open HustCoderHu opened this issue 2 years ago • 6 comments

https://github.com/versatica/mediasoup/blob/f23eceadc524c223b7bc4661cb8736e28f779820/worker/deps/libwebrtc/libwebrtc/modules/congestion_controller/goog_cc/goog_cc_network_control.cc#L434

Should it be like following?

TimeDelta min_pending_time = max_recv_time - feedback.receive_time;

HustCoderHu avatar Jun 30 '22 12:06 HustCoderHu

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

ibc avatar Jul 01 '22 09:07 ibc

https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/modules/congestion_controller/goog_cc/goog_cc_network_control.cc;l=1?q=goog_cc_network_control.cc&sq=&ss=chromium%2Fchromium%2Fsrc same in chromium, but it is obviously wrong.

HustCoderHu avatar Jul 02 '22 06:07 HustCoderHu

Why is it obviously wrong?

ibc avatar Jul 04 '22 11:07 ibc

  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);
  }

HustCoderHu avatar Jul 05 '22 03:07 HustCoderHu

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.

ibc avatar Jul 05 '22 09:07 ibc

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

fippo avatar Jul 20 '22 07:07 fippo

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

ibc avatar Nov 04 '22 12:11 ibc