Delete VieRemb class, move functionality to PacketRouter.

webrtc/modules/pacing/packet_router.cc

 /*
  *  Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #include "webrtc/modules/pacing/packet_router.h"
 
 #include "webrtc/base/atomicops.h"
 #include "webrtc/base/checks.h"
+#include "webrtc/base/timeutils.h"
 #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp.h"
 #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h"
 
 namespace webrtc {
 
 PacketRouter::PacketRouter() : transport_seq_(0) {
   pacer_thread_checker_.DetachFromThread();
 }
 
@@ skipping 89 matching lines @@
     // time the CAS operation was executed. Thus, if prev_seq is returned, the
     // operation was successful - otherwise we need to retry. Saving the
     // return value saves us a load on retry.
     prev_seq = rtc::AtomicOps::CompareAndSwap(&transport_seq_, desired_prev_seq,
                                               new_seq);
   } while (prev_seq != desired_prev_seq);
 
   return new_seq;
 }
 
-bool PacketRouter::SendFeedback(rtcp::TransportFeedback* packet) {
+void PacketRouter::OnReceiveBitrateChanged(const std::vector<uint32_t>& ssrcs,
+                                           uint32_t bitrate_bps) {
+  const int kRembSendIntervalMs = 200;
+
+  // % threshold for if we should send a new REMB asap.
+  const uint32_t kSendThresholdPercent = 97;
+
+  int64_t now_ms = rtc::TimeMillis();
+  {
+    rtc::CritScope lock(&remb_crit_);
+
+    // If we already have an estimate, check if the new total estimate is below
+    // kSendThresholdPercent of the previous estimate.
+    if (last_send_bitrate_bps_ > 0) {
+      uint32_t new_remb_bitrate_bps =
+          last_send_bitrate_bps_ - bitrate_bps_ + bitrate_bps;
+
+      if (new_remb_bitrate_bps <
+          kSendThresholdPercent * last_send_bitrate_bps_ / 100) {
+        // The new bitrate estimate is less than kSendThresholdPercent % of the
+        // last report. Send a REMB asap.
+        last_remb_time_ms_ = now_ms - kRembSendIntervalMs;
+      }
+    }
+    bitrate_bps_ = bitrate_bps;
+
+    if (now_ms - last_remb_time_ms_ < kRembSendIntervalMs) {
+      return;
+    }
+    // NOTE: Updated if we intend to send the data; we might not have
+    // a module to actually send it.
+    last_remb_time_ms_ = now_ms;
+    last_send_bitrate_bps_ = bitrate_bps;
+  }
+  {

[danilchap, 2017/04/03 16:31:34]

Do you mind splitting this block into own public function?
bool PacketRouter::SendRemb(bitrate_bps, ssrcs) {
  rtc::CritScope cs(&modules_crit_);
  RtpRtcp* remb_module = nullptr;
  if (!rtp_send_modules_.empty())
    remb_module = rtp_send_modules_.front();
  else if (!rtp_receive_modules_.empty())
    remb_module = rtp_receive_modules_.front();
  else
    return false;
  remb_module->SetREMBData(bitrate_bps, ssrcs);
  // and may be to actually send remb...
  return remb_module->SendRtcp(kRtcpRemb) == 0;
}

[nisse-webrtc, 2017/04/04 09:46:44]

Not at all. (If possible, I'd actually prefer to not implement the
RtpReceiveBitrateUpdate call and move its logic elsewhere. But if you don't see
any easy way to do that, it has to wait for some other cl).
Is the last call needed? RTCPSender::SetREMBData ends with

  // Send a REMB immediately if we have a new REMB. The frequency of REMBs is
  // throttled by the caller.
  next_time_to_send_rtcp_ = clock_->TimeInMilliseconds();

[danilchap, 2017/04/04 10:18:27]

not strictly needed, but this is one of the way to resolve your TODO: Check REMB
status.

without explicit SendRtcp(Remb) it will be eventually send [on process thread]
only if remb_module was configured with remb (SetRembStatus(true)).
Calling SendRtcp(Remb) explicetly will ensure remb is actually sent.

On the other side there is risk here: SendRtcp is a large function that involve
taking bunch of locks, so need to be careful not add a deadlock.
Also, calling SendRtcp makes it harder to reason what threads rtcp messages are
send from.

not calling SendRtcp(Remb) probably a better decision in this CL.

+    rtc::CritScope lock(&modules_crit_);
+    // TODO(nisse): Check REMB status of the modules? Or add a loop,
+    // similar to SendTransportFeedback below?
+    if (!rtp_send_modules_.empty())
+      rtp_send_modules_.front()->SetREMBData(bitrate_bps, ssrcs);
+    else if (!rtp_receive_modules_.empty())
+      rtp_receive_modules_.front()->SetREMBData(bitrate_bps, ssrcs);
+  }
+}
+
+bool PacketRouter::SendTransportFeedback(rtcp::TransportFeedback* packet) {
   RTC_DCHECK(pacer_thread_checker_.CalledOnValidThread());
   rtc::CritScope cs(&modules_crit_);
   // Prefer send modules.
   for (auto* rtp_module : rtp_send_modules_) {
     packet->SetSenderSsrc(rtp_module->SSRC());
     if (rtp_module->SendFeedbackPacket(*packet))
       return true;
   }
   for (auto* rtp_module : rtp_receive_modules_) {
     packet->SetSenderSsrc(rtp_module->SSRC());
     if (rtp_module->SendFeedbackPacket(*packet))
       return true;
   }
   return false;
 }
 
 }  // namespace webrtc