Always call RemoteBitrateEstimator::IncomingPacket from 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 92 matching lines @@
 
   // Implements PacketReceiver.
   DeliveryStatus DeliverPacket(MediaType media_type,
                                const uint8_t* packet,
                                size_t length,
                                const PacketTime& packet_time) override;
 
   // Implements RecoveredPacketReceiver.
   bool OnRecoveredPacket(const uint8_t* packet, size_t length) override;
 
-  void NotifyBweOfReceivedPacket(const RtpPacketReceived& packet);
-
   void SetBitrateConfig(
       const webrtc::Call::Config::BitrateConfig& bitrate_config) override;
 
   void SignalChannelNetworkState(MediaType media, NetworkState state) override;
 
   void OnTransportOverheadChanged(MediaType media,
                                   int transport_overhead_per_packet) override;
 
   void OnNetworkRouteChanged(const std::string& transport_name,
                              const rtc::NetworkRoute& network_route) override;
@@ skipping 23 matching lines @@
 
   VoiceEngine* voice_engine() {
     internal::AudioState* audio_state =
         static_cast<internal::AudioState*>(config_.audio_state.get());
     if (audio_state)
       return audio_state->voice_engine();
     else
       return nullptr;
   }
 
+  void NotifyBweOfReceivedPacket(const RtpPacketReceived& packet)
+      SHARED_LOCKS_REQUIRED(receive_crit_);
+
   rtc::Optional<RtpPacketReceived> ParseRtpPacket(const uint8_t* packet,
                                                   size_t length,
                                                   const PacketTime& packet_time)
       SHARED_LOCKS_REQUIRED(receive_crit_);
 
   void UpdateSendHistograms() EXCLUSIVE_LOCKS_REQUIRED(&bitrate_crit_);
   void UpdateReceiveHistograms();
   void UpdateHistograms();
   void UpdateAggregateNetworkState();
 
@@ skipping 23 matching lines @@
   // streams.
   std::multimap<uint32_t, FlexfecReceiveStreamImpl*>
       flexfec_receive_ssrcs_media_ GUARDED_BY(receive_crit_);
   std::map<uint32_t, FlexfecReceiveStreamImpl*>
       flexfec_receive_ssrcs_protection_ GUARDED_BY(receive_crit_);
   std::set<FlexfecReceiveStreamImpl*> flexfec_receive_streams_
       GUARDED_BY(receive_crit_);
   std::map<std::string, AudioReceiveStream*> sync_stream_mapping_
       GUARDED_BY(receive_crit_);
 
-  // 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.
-  std::map<uint32_t, RtpHeaderExtensionMap> received_rtp_header_extensions_
+  // 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) {}

[brandtr, 2017/01/30 12:03:53]

Should we store a bool for REMB here too? The mechanism for the feedback is
different, but |transport_cc| and |remb| in VideoReceiveStream::Config::Rtp are
conceptually similar.

[nisse-webrtc, 2017/01/30 14:56:57]

Maybe. Are you suggesting I do something about it in this cl? Then I'd need a
little more guidance. 

The intention is that this struct and the below mapping should hold all
information needed for the media-type-independent processing of received
packets. 

[brandtr, 2017/02/01 10:05:11]

Actually, there is probably no direct value in keeping the REMB status here.
Currently, it is set on the RTP module here: 
https://cs.chromium.org/chromium/src/third_party/webrtc/video/rtp_stream_rece...,
and that won't change with this CL.

+
+    // 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;
+    // True if send side bandwith estimation was negotiated.

[brandtr, 2017/01/30 12:03:53]

Mention that |transport_cc| only has to do with the RTCP feedback message. I.e.,
only one part of negotiating send-side BWE.

[nisse-webrtc, 2017/01/30 14:56:57]

Done.

+    bool transport_cc;
+  };
+  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_);
 
   VideoSendStream::RtpStateMap suspended_video_send_ssrcs_;
   webrtc::RtcEventLog* event_log_;
@@ skipping 143 matching lines @@
 }
 
 rtc::Optional<RtpPacketReceived> Call::ParseRtpPacket(
     const uint8_t* packet,
     size_t length,
     const PacketTime& packet_time) {
   RtpPacketReceived parsed_packet;
   if (!parsed_packet.Parse(packet, length))
     return rtc::Optional<RtpPacketReceived>();
 
-  auto it = received_rtp_header_extensions_.find(parsed_packet.Ssrc());
-  if (it != received_rtp_header_extensions_.end())
-    parsed_packet.IdentifyExtensions(it->second);
+  auto it = receive_rtp_config_.find(parsed_packet.Ssrc());
+  if (it != receive_rtp_config_.end())
+    parsed_packet.IdentifyExtensions(it->second.extensions);
 
   int64_t arrival_time_ms;
   if (packet_time.timestamp != -1) {
     arrival_time_ms = (packet_time.timestamp + 500) / 1000;
   } else {
     arrival_time_ms = clock_->TimeInMilliseconds();
   }
   parsed_packet.set_arrival_time_ms(arrival_time_ms);
 
   return rtc::Optional<RtpPacketReceived>(std::move(parsed_packet));
@@ skipping 129 matching lines @@
   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_,
       // TODO(nisse): Used only when UseSendSideBwe(config) is true.

[brandtr, 2017/01/30 12:03:53]

Could this TODO be removed now?

[nisse-webrtc, 2017/01/30 14:56:57]

Done.

       congestion_controller_->GetRemoteBitrateEstimator(true), 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);
+
     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_);
-    size_t num_deleted = audio_receive_ssrcs_.erase(
-        audio_receive_stream->config().rtp.remote_ssrc);
+    uint32_t ssrc = audio_receive_stream->config().rtp.remote_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);
   }
   UpdateAggregateNetworkState();
   delete audio_receive_stream;
 }
 
 webrtc::VideoSendStream* Call::CreateVideoSendStream(
     webrtc::VideoSendStream::Config config,
     VideoEncoderConfig encoder_config) {
   TRACE_EVENT0("webrtc", "Call::CreateVideoSendStream");
   RTC_DCHECK(configuration_thread_checker_.CalledOnValidThread());
@@ skipping 72 matching lines @@
     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(),
       &packet_router_, std::move(configuration), voice_engine(),
       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);
   {
     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)
+    if (config.rtp.rtx_ssrc) {
       video_receive_ssrcs_[config.rtp.rtx_ssrc] = receive_stream;
+      receive_rtp_config_[config.rtp.rtx_ssrc] = receive_config;

[brandtr, 2017/01/30 12:03:53]

config.rtp.transport_cc describes if the RTCP feedback message has been
negotiated for the video payload type. When storing the same ReceiveRtpConfig
for the RTX payload type here, that information may therefore be incorrect.

Not sure that it matters in practice, but could warrant a comment here.

[nisse-webrtc, 2017/01/30 14:56:57]

If it's more correct to set transport_cc always false in the entry for the rtx
ssrc, that should be a fairly easy change.

[nisse-webrtc, 2017/02/01 09:59:53]

Added a comment now.

[brandtr, 2017/02/01 10:05:11]

I think neither is more correct, so it's probably better to keep the current
behaviour: that transport_cc for the RTX payloads are set based on transport_cc
for the video payload. Then received RTX packets will not be thrown away, from a
BWE perspective.

+    }
+    receive_rtp_config_[config.rtp.remote_ssrc] = receive_config;
     video_receive_streams_.insert(receive_stream);
     ConfigureSync(config.sync_group);
   }
   receive_stream->SignalNetworkState(video_network_state_);
   UpdateAggregateNetworkState();
   event_log_->LogVideoReceiveStreamConfig(config);
   return receive_stream;
 }
 
 void Call::DestroyVideoReceiveStream(
     webrtc::VideoReceiveStream* receive_stream) {
   TRACE_EVENT0("webrtc", "Call::DestroyVideoReceiveStream");
   RTC_DCHECK(configuration_thread_checker_.CalledOnValidThread());
   RTC_DCHECK(receive_stream != nullptr);
   VideoReceiveStream* receive_stream_impl = nullptr;
   {
     WriteLockScoped write_lock(*receive_crit_);
     // Remove all ssrcs pointing to a receive stream. As RTX retransmits on a
     // separate SSRC there can be either one or two.
     auto it = video_receive_ssrcs_.begin();
     while (it != video_receive_ssrcs_.end()) {
       if (it->second == static_cast<VideoReceiveStream*>(receive_stream)) {
         if (receive_stream_impl != nullptr)
           RTC_DCHECK(receive_stream_impl == it->second);
         receive_stream_impl = it->second;
-        video_receive_ssrcs_.erase(it++);
+        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);
   }
   UpdateAggregateNetworkState();
   delete receive_stream_impl;
@@ skipping 16 matching lines @@
                flexfec_receive_streams_.end());
     flexfec_receive_streams_.insert(receive_stream);
 
     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(received_rtp_header_extensions_.find(config.remote_ssrc) ==
-               received_rtp_header_extensions_.end());
-    RtpHeaderExtensionMap rtp_header_extensions(config.rtp_header_extensions);
-    received_rtp_header_extensions_[config.remote_ssrc] = rtp_header_extensions;
+    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);
   }
 
   // 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());
 
   RTC_DCHECK(receive_stream != nullptr);
   // There exist no other derived classes of FlexfecReceiveStream,
   // so this downcast is safe.
   FlexfecReceiveStreamImpl* receive_stream_impl =
       static_cast<FlexfecReceiveStreamImpl*>(receive_stream);
   {
     WriteLockScoped write_lock(*receive_crit_);
 
     uint32_t ssrc = receive_stream_impl->GetConfig().remote_ssrc;
-    received_rtp_header_extensions_.erase(ssrc);
+    receive_rtp_config_.erase(ssrc);
 
     // Remove all SSRCs pointing to the FlexfecReceiveStreamImpl to be
     // destroyed.
     auto prot_it = flexfec_receive_ssrcs_protection_.begin();
     while (prot_it != flexfec_receive_ssrcs_protection_.end()) {
       if (prot_it->second == receive_stream_impl)
         prot_it = flexfec_receive_ssrcs_protection_.erase(prot_it);
       else
         ++prot_it;
     }
@@ skipping 352 matching lines @@
     event_log_->LogRtcpPacket(kIncomingPacket, media_type, packet, length);
 
   return rtcp_delivered ? DELIVERY_OK : DELIVERY_PACKET_ERROR;
 }
 
 PacketReceiver::DeliveryStatus Call::DeliverRtp(MediaType media_type,
                                                 const uint8_t* packet,
                                                 size_t length,
                                                 const PacketTime& packet_time) {
   TRACE_EVENT0("webrtc", "Call::DeliverRtp");
-  // Minimum RTP header size.
-  if (length < 12)
+
+  ReadLockScoped read_lock(*receive_crit_);
+  // TODO(nisse): We should parse the RTP header only here, and pass
+  // on parsed_packet to the receive streams.
+  rtc::Optional<RtpPacketReceived> parsed_packet =
+      ParseRtpPacket(packet, length, packet_time);
+
+  if (!parsed_packet)
     return DELIVERY_PACKET_ERROR;
 
-  uint32_t ssrc = ByteReader<uint32_t>::ReadBigEndian(&packet[8]);
-  ReadLockScoped read_lock(*receive_crit_);
+  NotifyBweOfReceivedPacket(*parsed_packet);
+
+  uint32_t ssrc = parsed_packet->Ssrc();
+
   if (media_type == MediaType::ANY || media_type == MediaType::AUDIO) {
     auto it = audio_receive_ssrcs_.find(ssrc);
     if (it != audio_receive_ssrcs_.end()) {
       received_bytes_per_second_counter_.Add(static_cast<int>(length));
       received_audio_bytes_per_second_counter_.Add(static_cast<int>(length));
       auto status = it->second->DeliverRtp(packet, length, packet_time)
                         ? DELIVERY_OK
                         : DELIVERY_PACKET_ERROR;
       if (status == DELIVERY_OK)
         event_log_->LogRtpHeader(kIncomingPacket, media_type, packet, length);
       return status;
     }
   }
   if (media_type == MediaType::ANY || media_type == MediaType::VIDEO) {
     auto it = video_receive_ssrcs_.find(ssrc);
     if (it != video_receive_ssrcs_.end()) {
       received_bytes_per_second_counter_.Add(static_cast<int>(length));
       received_video_bytes_per_second_counter_.Add(static_cast<int>(length));
       // TODO(brandtr): Notify the BWE of received media packets here.
       auto status = it->second->DeliverRtp(packet, length, packet_time)
                         ? DELIVERY_OK
                         : DELIVERY_PACKET_ERROR;
       // Deliver media packets to FlexFEC subsystem. RTP header extensions need
       // not be parsed, as FlexFEC is oblivious to the semantic meaning of the
       // packet contents beyond the 12 byte RTP base header. The BWE is fed
       // information about these media packets from the regular media pipeline.
-      rtc::Optional<RtpPacketReceived> parsed_packet =
-          ParseRtpPacket(packet, length, packet_time);
       if (parsed_packet) {
         auto it_bounds = flexfec_receive_ssrcs_media_.equal_range(ssrc);
         for (auto it = it_bounds.first; it != it_bounds.second; ++it)
           it->second->AddAndProcessReceivedPacket(*parsed_packet);
       }
       if (status == DELIVERY_OK)
         event_log_->LogRtpHeader(kIncomingPacket, media_type, packet, length);
       return status;
     }
   }
   if (media_type == MediaType::ANY || media_type == MediaType::VIDEO) {
     auto it = flexfec_receive_ssrcs_protection_.find(ssrc);
     if (it != flexfec_receive_ssrcs_protection_.end()) {
-      rtc::Optional<RtpPacketReceived> parsed_packet =
-          ParseRtpPacket(packet, length, packet_time);
       if (parsed_packet) {
-        NotifyBweOfReceivedPacket(*parsed_packet);
         auto status = it->second->AddAndProcessReceivedPacket(*parsed_packet)
                           ? DELIVERY_OK
                           : DELIVERY_PACKET_ERROR;
         if (status == DELIVERY_OK)
           event_log_->LogRtpHeader(kIncomingPacket, media_type, packet, length);
         return status;
       }
     }
   }
   return DELIVERY_UNKNOWN_SSRC;
@@ skipping 19 matching lines @@
 bool Call::OnRecoveredPacket(const uint8_t* packet, size_t length) {
   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;
+
   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) {
+    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