Always call RemoteBitrateEstimator::IncomingPacket from Call.

webrtc/call/call.cc

Index: webrtc/call/call.cc
diff --git a/webrtc/call/call.cc b/webrtc/call/call.cc
index d37d08ef8e7ac824669c787f6672ac633d1648d3..8ff4a50198407b69923cbfdfa2c59109e1c34603 100644
--- a/webrtc/call/call.cc
+++ b/webrtc/call/call.cc
@@ -110,8 +110,6 @@ class Call : public webrtc::Call,
   // 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;
 
@@ -155,6 +153,9 @@ class Call : public webrtc::Call,
       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)
@@ -198,12 +199,27 @@ class Call : public webrtc::Call,
   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) {}
+
+    // 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.
+    bool transport_cc;
+  };
+  std::map<uint32_t, ReceiveRtpConfig> receive_rtp_config_
       GUARDED_BY(receive_crit_);
 
   std::unique_ptr<RWLockWrapper> send_crit_;
@@ -367,9 +383,9 @@ rtc::Optional<RtpPacketReceived> Call::ParseRtpPacket(
   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) {
@@ -527,6 +543,9 @@ webrtc::AudioReceiveStream* Call::CreateAudioReceiveStream(
     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);
   }
   {
@@ -550,8 +569,9 @@ void Call::DestroyAudioReceiveStream(
       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);
@@ -560,6 +580,7 @@ void Call::DestroyAudioReceiveStream(
       sync_stream_mapping_.erase(it);
       ConfigureSync(sync_group);
     }
+    receive_rtp_config_.erase(ssrc);
   }
   UpdateAggregateNetworkState();
   delete audio_receive_stream;
@@ -644,17 +665,23 @@ webrtc::VideoReceiveStream* Call::CreateVideoReceiveStream(
       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;
+    receive_rtp_config_[config.rtp.remote_ssrc] = receive_config;
     // TODO(pbos): Configure different RTX payloads per receive payload.
     VideoReceiveStream::Config::Rtp::RtxMap::const_iterator it =
         config.rtp.rtx.begin();
-    if (it != config.rtp.rtx.end())
+    if (it != config.rtp.rtx.end()) {
       video_receive_ssrcs_[it->second.ssrc] = receive_stream;
+      receive_rtp_config_[it->second.ssrc] = receive_config;
+    }
     video_receive_streams_.insert(receive_stream);
+
     ConfigureSync(config.sync_group);
   }
   receive_stream->SignalNetworkState(video_network_state_);
@@ -679,7 +706,8 @@ void Call::DestroyVideoReceiveStream(
         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;
       }
@@ -716,10 +744,10 @@ FlexfecReceiveStream* Call::CreateFlexfecReceiveStream(
                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.
@@ -740,7 +768,7 @@ void Call::DestroyFlexfecReceiveStream(FlexfecReceiveStream* 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.
@@ -1111,12 +1139,20 @@ PacketReceiver::DeliveryStatus Call::DeliverRtp(MediaType media_type,
                                                 size_t length,
                                                 const PacketTime& packet_time) {
   TRACE_EVENT0("webrtc", "Call::DeliverRtp");
-  // Minimum RTP header size.
-  if (length < 12)
-    return DELIVERY_PACKET_ERROR;
 
-  uint32_t ssrc = ByteReader<uint32_t>::ReadBigEndian(&packet[8]);
   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;
+
+  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()) {
@@ -1143,8 +1179,6 @@ PacketReceiver::DeliveryStatus Call::DeliverRtp(MediaType media_type,
       // 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)
@@ -1158,10 +1192,7 @@ PacketReceiver::DeliveryStatus Call::DeliverRtp(MediaType media_type,
   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;
@@ -1201,8 +1232,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;
+
   RTPHeader header;
   packet.GetHeader(&header);
+
+  if (transport_cc != header.extension.hasTransportSequenceNumber) {

[nisse-webrtc, 2017/01/27 13:26:14]

I intended to use the check
 
  if (transport_cc && !header.extension.hasTransportSequenceNumber)

but then tests fail. Additional logging shows that we get

  transport_cc = 0, hasTransportSequenceNumber = 1

for the
TestWithNewVideoJitterBuffer/EndToEndTest.*TransportFeedbackNotConfigured/*
tests.

Can that reasonably happen? I though that transport_cc being false meant the RTP
extension wasn't negotiated, hence shouldn't be included in
RtpHeaderExtensionMap, and then the RTP parser should never set
hasTransportSequenceNumber true, no matter what bits arrive on the wire. Am I
missing something, or is there a bug somewhere?

+    // Send side BWE was negotiated, but required header extension not
+    // present on this packet. Then don't use it for bandwidth
+    // estimation at all.
+    LOG(LS_ERROR) << "Not calling OnReceivedPacket";
+    return;
+  }
   congestion_controller_->OnReceivedPacket(packet.arrival_time_ms(),
                                            packet.payload_size(), header);
 }