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

webrtc/call/call.cc

 /*
  *  Copyright (c) 2013 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.
  */
 
@@ skipping 42 matching lines @@
 #include "webrtc/video/send_delay_stats.h"
 #include "webrtc/video/stats_counter.h"
 #include "webrtc/video/video_receive_stream.h"
 #include "webrtc/video/video_send_stream.h"
 #include "webrtc/video/vie_remb.h"
 
 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,
              public PacketReceiver,
              public RecoveredPacketReceiver,
              public CongestionController::Observer,
              public BitrateAllocator::LimitObserver {
  public:
   explicit Call(const Call::Config& config);
   virtual ~Call();
@@ skipping 118 matching lines @@
 
   // This extra map is used for receive processing which is
   // independent of media type.
 
   // TODO(nisse): In the RTP transport refactoring, we should have a
   // single mapping from ssrc to a more abstract receive stream, with
   // accessor methods for all configuration we need at this level.
   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_);
 
   std::unique_ptr<RWLockWrapper> send_crit_;
   // Audio and Video send streams are owned by the client that creates them.
   std::map<uint32_t, AudioSendStream*> audio_send_ssrcs_ GUARDED_BY(send_crit_);
   std::map<uint32_t, VideoSendStream*> video_send_ssrcs_ GUARDED_BY(send_crit_);
   std::set<VideoSendStream*> video_send_streams_ GUARDED_BY(send_crit_);
 
@@ skipping 296 matching lines @@
   UpdateAggregateNetworkState();
   delete audio_send_stream;
 }
 
 webrtc::AudioReceiveStream* Call::CreateAudioReceiveStream(
     const webrtc::AudioReceiveStream::Config& config) {
   TRACE_EVENT0("webrtc", "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_);
     RTC_DCHECK(audio_receive_ssrcs_.find(config.rtp.remote_ssrc) ==
                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);
   }
   {
     ReadLockScoped read_lock(*send_crit_);
     auto it = audio_send_ssrcs_.find(config.rtp.local_ssrc);
     if (it != audio_send_ssrcs_.end()) {
       receive_stream->AssociateSendStream(it->second);
     }
   }
   receive_stream->SignalNetworkState(audio_network_state_);
   UpdateAggregateNetworkState();
   return receive_stream;
 }
 
 void Call::DestroyAudioReceiveStream(
     webrtc::AudioReceiveStream* receive_stream) {
   TRACE_EVENT0("webrtc", "Call::DestroyAudioReceiveStream");
   RTC_DCHECK(configuration_thread_checker_.CalledOnValidThread());
   RTC_DCHECK(receive_stream != nullptr);
   webrtc::internal::AudioReceiveStream* audio_receive_stream =
       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;
     const auto it = sync_stream_mapping_.find(sync_group);
     if (it != sync_stream_mapping_.end() &&
         it->second == audio_receive_stream) {
       sync_stream_mapping_.erase(it);
       ConfigureSync(sync_group);
     }
     receive_rtp_config_.erase(ssrc);
@@ skipping 77 matching lines @@
   RTC_DCHECK(configuration_thread_checker_.CalledOnValidThread());
 
   bool protected_by_flexfec = false;
   {
     ReadLockScoped read_lock(*receive_crit_);
     protected_by_flexfec =
         flexfec_receive_ssrcs_media_.find(configuration.rtp.remote_ssrc) !=
         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) ==
                video_receive_ssrcs_.end());
     video_receive_ssrcs_[config.rtp.remote_ssrc] = receive_stream;
     if (config.rtp.rtx_ssrc) {
       video_receive_ssrcs_[config.rtp.rtx_ssrc] = receive_stream;
       // We record identical config for the rtx stream as for the main
       // stream. Since the transport_cc negotiation is per payload
       // type, we may get an incorrect value for the rtx stream, but
@@ skipping 29 matching lines @@
         receive_rtp_config_.erase(it->first);
         it = video_receive_ssrcs_.erase(it);
       } else {
         ++it;
       }
     }
     video_receive_streams_.erase(receive_stream_impl);
     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;
 }
 
 FlexfecReceiveStream* Call::CreateFlexfecReceiveStream(
     const FlexfecReceiveStream::Config& config) {
   TRACE_EVENT0("webrtc", "Call::CreateFlexfecReceiveStream");
   RTC_DCHECK(configuration_thread_checker_.CalledOnValidThread());
 
   RecoveredPacketReceiver* recovered_packet_receiver = this;
@@ skipping 11 matching lines @@
     for (auto ssrc : config.protected_media_ssrcs)
       flexfec_receive_ssrcs_media_.insert(std::make_pair(ssrc, receive_stream));
 
     RTC_DCHECK(flexfec_receive_ssrcs_protection_.find(config.remote_ssrc) ==
                flexfec_receive_ssrcs_protection_.end());
     flexfec_receive_ssrcs_protection_[config.remote_ssrc] = receive_stream;
 
     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.
 
   return receive_stream;
 }
 
 void Call::DestroyFlexfecReceiveStream(FlexfecReceiveStream* receive_stream) {
   TRACE_EVENT0("webrtc", "Call::DestroyFlexfecReceiveStream");
   RTC_DCHECK(configuration_thread_checker_.CalledOnValidThread());
@@ skipping 467 matching lines @@
   uint32_t ssrc = ByteReader<uint32_t>::ReadBigEndian(&packet[8]);
   ReadLockScoped read_lock(*receive_crit_);
   auto it = video_receive_ssrcs_.find(ssrc);
   if (it == video_receive_ssrcs_.end())
     return false;
   return it->second->OnRecoveredPacket(packet, 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;
   }
   congestion_controller_->OnReceivedPacket(packet.arrival_time_ms(),
                                            packet.payload_size(), header);
 }
 
 }  // namespace internal
 }  // namespace webrtc