Wire-up BWE feedback for audio receive streams.

webrtc/audio/audio_receive_stream.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/audio/audio_receive_stream.h"
 
 #include <string>
 #include <utility>
 
 #include "webrtc/audio/audio_sink.h"
 #include "webrtc/audio/audio_state.h"
 #include "webrtc/audio/conversion.h"
 #include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
+#include "webrtc/call/congestion_controller.h"
 #include "webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h"
 #include "webrtc/system_wrappers/include/tick_util.h"
 #include "webrtc/voice_engine/channel_proxy.h"
 #include "webrtc/voice_engine/include/voe_base.h"
 #include "webrtc/voice_engine/include/voe_codec.h"
 #include "webrtc/voice_engine/include/voe_neteq_stats.h"
 #include "webrtc/voice_engine/include/voe_rtp_rtcp.h"
 #include "webrtc/voice_engine/include/voe_video_sync.h"
 #include "webrtc/voice_engine/include/voe_volume_control.h"
 #include "webrtc/voice_engine/voice_engine_impl.h"
 
 namespace webrtc {
+namespace {
+
+static bool UseSendSideBwe(const webrtc::AudioReceiveStream::Config& config) {

[the sun, 2015/12/22 00:14:14]

no need to declare static inside an anonymous namespace?

[stefan-webrtc, 2016/01/07 13:43:41]

Done.

+  if (!config.rtp.transport_cc)

[the sun, 2015/12/22 00:14:13]

nit: This file consistently uses {} for one-line control statements.

[stefan-webrtc, 2016/01/07 13:43:41]

Done.

+    return false;
+  for (const auto& extension : config.rtp.extensions) {
+    if (extension.name == RtpExtension::kTransportSequenceNumber)
+      return true;
+  }
+  return false;
+}
+}  // namespace
+
 std::string AudioReceiveStream::Config::Rtp::ToString() const {
   std::stringstream ss;
   ss << "{remote_ssrc: " << remote_ssrc;
   ss << ", local_ssrc: " << local_ssrc;
   ss << ", extensions: [";
   for (size_t i = 0; i < extensions.size(); ++i) {
     ss << extensions[i].ToString();
     if (i != extensions.size() - 1) {
       ss << ", ";
     }
@@ skipping 15 matching lines @@
     ss << ", sync_group: " << sync_group;
   }
   ss << ", combined_audio_video_bwe: "
      << (combined_audio_video_bwe ? "true" : "false");
   ss << '}';
   return ss.str();
 }
 
 namespace internal {
 AudioReceiveStream::AudioReceiveStream(
-    RemoteBitrateEstimator* remote_bitrate_estimator,
+    CongestionController* congestion_controller,
     const webrtc::AudioReceiveStream::Config& config,
     const rtc::scoped_refptr<webrtc::AudioState>& audio_state)
-    : remote_bitrate_estimator_(remote_bitrate_estimator),
+    : remote_bitrate_estimator_(
+          config.combined_audio_video_bwe
+              ? congestion_controller->GetRemoteBitrateEstimator(

[the sun, 2015/12/22 00:14:14]

Why not have two accessor methods on the CC? Sending in a flag to tell the
function which member to return is not a super nice interface.

I think you should also consider making the returned RBE ref counted so we can
keep it in a scoped_refptr here.

I'm ok with you addressing both in a separate CL though.

[stefan-webrtc, 2016/01/07 13:43:40]

Yes, I agree that it'd be nicer with two accessors. I'm not sure if it should be
refcounted though, I think it's simpler to have a single owner
(CongestionController).

Anyway, I'm making that change in a different CL.

[the sun, 2016/01/08 10:29:35]

In that case, making the CC ref counted would make the object life times
clearer+safer.
 

+                    UseSendSideBwe(config))
+              : nullptr),
       config_(config),
       audio_state_(audio_state),
       rtp_header_parser_(RtpHeaderParser::Create()) {
   LOG(LS_INFO) << "AudioReceiveStream: " << config_.ToString();
   RTC_DCHECK_NE(config_.voe_channel_id, -1);
-  RTC_DCHECK(remote_bitrate_estimator_);
   RTC_DCHECK(audio_state_.get());
   RTC_DCHECK(rtp_header_parser_);
 
   VoiceEngineImpl* voe_impl = static_cast<VoiceEngineImpl*>(voice_engine());
   channel_proxy_ = voe_impl->GetChannelProxy(config_.voe_channel_id);
   channel_proxy_->SetLocalSSRC(config.rtp.local_ssrc);
   for (const auto& extension : config.rtp.extensions) {
     if (extension.name == RtpExtension::kAudioLevel) {
       channel_proxy_->SetReceiveAudioLevelIndicationStatus(true, extension.id);
       bool registered = rtp_header_parser_->RegisterRtpHeaderExtension(
           kRtpExtensionAudioLevel, extension.id);
       RTC_DCHECK(registered);
     } else if (extension.name == RtpExtension::kAbsSendTime) {
       channel_proxy_->SetReceiveAbsoluteSenderTimeStatus(true, extension.id);
       bool registered = rtp_header_parser_->RegisterRtpHeaderExtension(
           kRtpExtensionAbsoluteSendTime, extension.id);
       RTC_DCHECK(registered);
     } else if (extension.name == RtpExtension::kTransportSequenceNumber) {
-      // TODO(holmer): Need to do something here or in  DeliverRtp() to actually
-      //               handle audio packets with this header extension.
       bool registered = rtp_header_parser_->RegisterRtpHeaderExtension(
           kRtpExtensionTransportSequenceNumber, extension.id);
       RTC_DCHECK(registered);
     } else {
       RTC_NOTREACHED() << "Unsupported RTP extension.";
     }
   }
+  if (config.combined_audio_video_bwe && UseSendSideBwe(config)) {
+    channel_proxy_->SetCongestionControlObjects(

[the sun, 2015/12/22 00:14:14]

We are relying on the underlying channel *only* being used for either send or
receive?

[stefan-webrtc, 2016/01/07 13:43:41]

Yes, but the packet router is needed both when sending and receiving. In the
receiving case it's needed for sending RTCP.

+        nullptr, nullptr, congestion_controller->packet_router());
+  }
 }
 
 AudioReceiveStream::~AudioReceiveStream() {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
   LOG(LS_INFO) << "~AudioReceiveStream: " << config_.ToString();
+  if (config_.combined_audio_video_bwe) {
+    if (UseSendSideBwe(config_))
+      channel_proxy_->SetCongestionControlObjects(nullptr, nullptr, nullptr);

[the sun, 2015/12/22 00:14:13]

Can we always set the CC obj nullptrs? Make the code a little simpler.

[stefan-webrtc, 2016/01/07 13:43:41]

Yes, good suggestion.

+    remote_bitrate_estimator_->RemoveStream(config_.rtp.remote_ssrc);
+  }
 }
 
 void AudioReceiveStream::Start() {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
 }
 
 void AudioReceiveStream::Stop() {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
 }
 
@@ skipping 14 matching lines @@
                                     const PacketTime& packet_time) {
   // TODO(solenberg): Tests call this function on a network thread, libjingle
   // calls on the worker thread. We should move towards always using a network
   // thread. Then this check can be enabled.
   // RTC_DCHECK(!thread_checker_.CalledOnValidThread());
   RTPHeader header;
   if (!rtp_header_parser_->Parse(packet, length, &header)) {
     return false;
   }
 
-  // Only forward if the parsed header has absolute sender time. RTP timestamps
-  // may have different rates for audio and video and shouldn't be mixed.
+  // Only forward if the parsed header has one of the headers necessary for
+  // bandwidth estimation. RTP timestamps has different rates for audio and
+  // video and shouldn't be mixed.
   if (config_.combined_audio_video_bwe &&
-      header.extension.hasAbsoluteSendTime) {
+      (header.extension.hasAbsoluteSendTime ||
+       header.extension.hasTransportSequenceNumber)) {

[the sun, 2015/12/22 00:14:13]

No need to logic-and with confg_.rtp.transport_cc for transport seq num? The RBE
will handle it?

[stefan-webrtc, 2016/01/07 13:43:41]

I think it's a matter of interpretation. Should audio packets be included in the
feedback message even though the audio receive stream itself doesn't send
feedback messages?

I think that would be ok.

     int64_t arrival_time_ms = TickTime::MillisecondTimestamp();
     if (packet_time.timestamp >= 0)
       arrival_time_ms = (packet_time.timestamp + 500) / 1000;
     size_t payload_size = length - header.headerLength;
     remote_bitrate_estimator_->IncomingPacket(arrival_time_ms, payload_size,
                                               header, false);
   }
   return true;
 }
 
@@ skipping 57 matching lines @@
 
 VoiceEngine* AudioReceiveStream::voice_engine() const {
   internal::AudioState* audio_state =
       static_cast<internal::AudioState*>(audio_state_.get());
   VoiceEngine* voice_engine = audio_state->voice_engine();
   RTC_DCHECK(voice_engine);
   return voice_engine;
 }
 }  // namespace internal
 }  // namespace webrtc