Move RemoteBitrateEstimator::RemoveStream calls from receive streams to Call.

webrtc/call/call.cc

Index: webrtc/call/call.cc
diff --git a/webrtc/call/call.cc b/webrtc/call/call.cc
index 37b5c6a4fbf5fef7683d9b6eea3f6e08c1b7fff6..4add560e2a66a5ff00e6fe3dee8858b29bf3b1c7 100644
--- a/webrtc/call/call.cc
+++ b/webrtc/call/call.cc
@@ -60,6 +60,34 @@ namespace webrtc {
 
 const int Call::Config::kDefaultStartBitrateBps = 300000;
 
+namespace {
+
+// TODO(nisse): This really begs for a shared context struct.
+bool UseSendSideBwe(const std::vector<RtpExtension>& extensions,
+                    bool transport_cc) {
+  if (!transport_cc)
+    return false;
+  for (const auto& extension : extensions) {
+    if (extension.uri == RtpExtension::kTransportSequenceNumberUri)
+      return true;
+  }
+  return false;
+}
+
+bool UseSendSideBwe(const VideoReceiveStream::Config& config) {

[stefan-webrtc, 2017/02/02 10:42:52]

Maybe use a template instead?

[brandtr_google, 2017/02/02 10:45:14]

Doesn't work with the FlexfecReceiveStream::Config, see below :( Since
everything in that config has to do with RTP/RTCP, I never added an rtp struct
to it.

[stefan-webrtc, 2017/02/02 10:47:21]

ah, i see

+  return UseSendSideBwe(config.rtp.extensions, config.rtp.transport_cc);
+}
+
+bool UseSendSideBwe(const AudioReceiveStream::Config& config) {
+  return UseSendSideBwe(config.rtp.extensions, config.rtp.transport_cc);
+}
+
+bool UseSendSideBwe(const FlexfecReceiveStream::Config& config) {
+  return UseSendSideBwe(config.rtp_header_extensions, config.transport_cc);
+}
+
+}  // namespace
+
 namespace internal {
 
 class Call : public webrtc::Call,
@@ -198,16 +226,17 @@ class Call : public webrtc::Call,
   struct ReceiveRtpConfig {
     ReceiveRtpConfig() = default;  // Needed by std::map
     ReceiveRtpConfig(const std::vector<RtpExtension>& extensions,
-                     bool transport_cc)
-        : extensions(extensions), transport_cc(transport_cc) {}
+                     bool use_send_side_bwe)
+        : extensions(extensions), use_send_side_bwe(use_send_side_bwe) {}
 
     // Registered RTP header extensions for each stream. Note that RTP header
     // extensions are negotiated per track ("m= line") in the SDP, but we have
     // no notion of tracks at the Call level. We therefore store the RTP header
     // extensions per SSRC instead, which leads to some storage overhead.
     RtpHeaderExtensionMap extensions;
-    // Set if the RTCP feedback message needed for send side BWE was negotiated.
-    bool transport_cc;
+    // Set if both RTP extension the RTCP feedback message needed for
+    // send side BWE are negotiated.
+    bool use_send_side_bwe;
   };
   std::map<uint32_t, ReceiveRtpConfig> receive_rtp_config_
       GUARDED_BY(receive_crit_);
@@ -524,8 +553,7 @@ webrtc::AudioReceiveStream* Call::CreateAudioReceiveStream(
   RTC_DCHECK(configuration_thread_checker_.CalledOnValidThread());
   event_log_->LogAudioReceiveStreamConfig(config);
   AudioReceiveStream* receive_stream = new AudioReceiveStream(
-      &packet_router_,
-      congestion_controller_->GetRemoteBitrateEstimator(true), config,
+      &packet_router_, config,
       config_.audio_state, event_log_);
   {
     WriteLockScoped write_lock(*receive_crit_);
@@ -533,7 +561,7 @@ webrtc::AudioReceiveStream* Call::CreateAudioReceiveStream(
                audio_receive_ssrcs_.end());
     audio_receive_ssrcs_[config.rtp.remote_ssrc] = receive_stream;
     receive_rtp_config_[config.rtp.remote_ssrc] =
-        ReceiveRtpConfig(config.rtp.extensions, config.rtp.transport_cc);
+        ReceiveRtpConfig(config.rtp.extensions, UseSendSideBwe(config));
 
     ConfigureSync(config.sync_group);
   }
@@ -558,8 +586,10 @@ void Call::DestroyAudioReceiveStream(
       static_cast<webrtc::internal::AudioReceiveStream*>(receive_stream);
   {
     WriteLockScoped write_lock(*receive_crit_);
-    uint32_t ssrc = audio_receive_stream->config().rtp.remote_ssrc;
-
+    const AudioReceiveStream::Config& config = audio_receive_stream->config();
+    uint32_t ssrc = config.rtp.remote_ssrc;
+    congestion_controller_->GetRemoteBitrateEstimator(UseSendSideBwe(config))
+        ->RemoveStream(ssrc);
     size_t num_deleted = audio_receive_ssrcs_.erase(ssrc);
     RTC_DCHECK(num_deleted == 1);
     const std::string& sync_group = audio_receive_stream->config().sync_group;
@@ -657,13 +687,13 @@ webrtc::VideoReceiveStream* Call::CreateVideoReceiveStream(
         flexfec_receive_ssrcs_media_.end();
   }
   VideoReceiveStream* receive_stream = new VideoReceiveStream(
-      num_cpu_cores_, protected_by_flexfec, congestion_controller_.get(),
+      num_cpu_cores_, protected_by_flexfec,
       &packet_router_, std::move(configuration), module_process_thread_.get(),
       call_stats_.get(), &remb_);
 
   const webrtc::VideoReceiveStream::Config& config = receive_stream->config();
   ReceiveRtpConfig receive_config(config.rtp.extensions,
-                                  config.rtp.transport_cc);
+                                  UseSendSideBwe(config));
   {
     WriteLockScoped write_lock(*receive_crit_);
     RTC_DCHECK(video_receive_ssrcs_.find(config.rtp.remote_ssrc) ==
@@ -713,6 +743,11 @@ void Call::DestroyVideoReceiveStream(
     RTC_CHECK(receive_stream_impl != nullptr);
     ConfigureSync(receive_stream_impl->config().sync_group);
   }
+  const VideoReceiveStream::Config& config = receive_stream_impl->config();
+
+  congestion_controller_->GetRemoteBitrateEstimator(UseSendSideBwe(config))
+      ->RemoveStream(config.rtp.remote_ssrc);
+
   UpdateAggregateNetworkState();
   delete receive_stream_impl;
 }
@@ -744,7 +779,7 @@ FlexfecReceiveStream* Call::CreateFlexfecReceiveStream(
     RTC_DCHECK(receive_rtp_config_.find(config.remote_ssrc) ==
                receive_rtp_config_.end());
     receive_rtp_config_[config.remote_ssrc] =
-        ReceiveRtpConfig(config.rtp_header_extensions, config.transport_cc);
+        ReceiveRtpConfig(config.rtp_header_extensions, UseSendSideBwe(config));
   }
 
   // TODO(brandtr): Store config in RtcEventLog here.
@@ -1232,17 +1267,20 @@ bool Call::OnRecoveredPacket(const uint8_t* packet, size_t length) {
 
 void Call::NotifyBweOfReceivedPacket(const RtpPacketReceived& packet) {
   auto it = receive_rtp_config_.find(packet.Ssrc());
-  bool transport_cc =
-      (it != receive_rtp_config_.end()) && it->second.transport_cc;
+  bool use_send_side_bwe =
+      (it != receive_rtp_config_.end()) && it->second.use_send_side_bwe;
 
   RTPHeader header;
   packet.GetHeader(&header);
 
-  // transport_cc represents the negotiation of the RTCP feedback
-  // message used for send side BWE. If it was negotiated but the
-  // corresponding RTP header extension is not present, or vice versa,
-  // bandwidth estimation is not correctly configured.
-  if (transport_cc != header.extension.hasTransportSequenceNumber) {
+  // This can fail in two ways: Either send side BWE was negotiated (both RTP
+  // extension and RTCP feedback), but the received RTP packets don't carry any
+  // transport sequence number. Or the transport sequence number extension was
+  // negotiated and is in use, but RTCP feed wasn't negotiated, so we can't send
+  // any feedback. In the latter case, it might be desirable to fall back to
+  // receive side bandwidth estimation, but to do that, we would need to tell
+  // the congestion controller to ignore the transport sequence number.
+  if (use_send_side_bwe != header.extension.hasTransportSequenceNumber) {

[stefan-webrtc, 2017/02/02 10:42:52]

I'm not entirely sure it's worth logging when this happens. What if we have BWE
negotiated on video but not on audio (current default)? That means audio packets
will end up here without the header extension, which simply means we shouldn't
include them in the estimate. It's a valid case.

In theory you can also configure your receiver to be able to receive both
abs-send-time and transport sequence numbers, and to send both remb and
transport_cc, but it's up to the sender to decide which extension to use.

[nisse-webrtc, 2017/02/02 11:56:00]

In this case I think use_send_side_bwe will be false, and this if-block doesn't
apply. That's because use_send_side_bwe is set if both RTCP feedback and the RTP
extension are negotiated. And that differs from the preceding cl which instead
used transport_cc here.

[stefan-webrtc, 2017/02/02 12:27:37]

But as I see it use_send_side_bwe can be true if extension and transport_cc has
been negotiated. But a packet still may not have the extension for some of the
streams we receive? Won't that hit this?

     LOG(LS_ERROR) << "Inconsistent configuration of send side BWE.";
     return;
   }