Move ownership of PacketRouter from CongestionController to Call.

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/api/call/audio_sink.h"
 #include "webrtc/audio/audio_send_stream.h"
 #include "webrtc/audio/audio_state.h"
 #include "webrtc/audio/conversion.h"
 #include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/timeutils.h"
-#include "webrtc/modules/congestion_controller/include/congestion_controller.h"
 #include "webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.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"
 
@@ skipping 39 matching lines @@
   ss << ", voe_channel_id: " << voe_channel_id;
   if (!sync_group.empty()) {
     ss << ", sync_group: " << sync_group;
   }
   ss << '}';
   return ss.str();
 }
 
 namespace internal {
 AudioReceiveStream::AudioReceiveStream(
-    CongestionController* congestion_controller,
+    PacketRouter* packet_router,
+    RemoteBitrateEstimator* remote_bitrate_estimator,
     const webrtc::AudioReceiveStream::Config& config,
     const rtc::scoped_refptr<webrtc::AudioState>& audio_state,
     webrtc::RtcEventLog* event_log)
     : 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(audio_state_.get());
-  RTC_DCHECK(congestion_controller);
+  RTC_DCHECK(packet_router);

[the sun, 2016/11/21 13:55:31]

Can we DCHECK(remote_bitrate_estimator) too?

[nisse-webrtc, 2016/11/21 14:07:36]

Not sure if that's appropriate, it's used only if UseSendSideBwe(config) is true
(whatever that means), and otherwise the corresponding member variable is NULL
(by an initializer where it is declared in the header).

Since the class handles NULL, it makes some sense to allow NULL also for the
constructor argument.

[the sun, 2016/11/21 15:36:11]

If we can avoid having to handle the null case, that makes the code simpler so
should be preferred.

[nisse-webrtc, 2016/11/23 08:11:44]

In this cl, the call site looks like this:

  AudioReceiveStream* receive_stream = new AudioReceiveStream(
      &packet_router_,
      // TODO(nisse): Used only when UseSendSideBwe(config) is true.
      congestion_controller_->GetRemoteBitrateEstimator(true),
      config, config_.audio_state, event_log_);

congestion_controller->GetRemoteBitrateEstimator(true) returns
&congestion_controller->remote_bitrate_estimator_proxy_, so this is non-NULL and
pretty cheap. 

Nevertheless, I'm not sure I like to DCHECK that an input parameter is non-NULL
when there are cases where the parameter is otherwise unused. 

   RTC_DCHECK(rtp_header_parser_);
 
   VoiceEngineImpl* voe_impl = static_cast<VoiceEngineImpl*>(voice_engine());
   channel_proxy_ = voe_impl->GetChannelProxy(config_.voe_channel_id);
   channel_proxy_->SetRtcEventLog(event_log);
   channel_proxy_->SetLocalSSRC(config.rtp.local_ssrc);
   // TODO(solenberg): Config NACK history window (which is a packet count),
   // using the actual packet size for the configured codec.
   channel_proxy_->SetNACKStatus(config_.rtp.nack.rtp_history_ms != 0,
                                 config_.rtp.nack.rtp_history_ms / 20);
@@ skipping 18 matching lines @@
     } else if (extension.uri == RtpExtension::kTransportSequenceNumberUri) {
       channel_proxy_->EnableReceiveTransportSequenceNumber(extension.id);
       bool registered = rtp_header_parser_->RegisterRtpHeaderExtension(
           kRtpExtensionTransportSequenceNumber, extension.id);
       RTC_DCHECK(registered);
     } else {
       RTC_NOTREACHED() << "Unsupported RTP extension.";
     }
   }
   // Configure bandwidth estimation.
-  channel_proxy_->RegisterReceiverCongestionControlObjects(
-      congestion_controller->packet_router());
+  channel_proxy_->RegisterReceiverCongestionControlObjects(packet_router);
   if (UseSendSideBwe(config)) {
-    remote_bitrate_estimator_ =
-        congestion_controller->GetRemoteBitrateEstimator(true);
+    remote_bitrate_estimator_ = remote_bitrate_estimator;

[nisse-webrtc, 2016/11/21 14:07:36]

A DCHECK(remote_bitrate_estimator) might make more sense at this location?

[the sun, 2016/11/21 15:36:11]

If Call might be giving us a null in this situation, then yes. If the idea is to
avoid iterating config_.rtp.extensions in DeliverRtp(), it is probably better to
cache the result in a bool.

[nisse-webrtc, 2016/11/23 08:11:44]

Currently we never get null. Motivation for a DCHECK here is that we might want
to enforce that UseSendSideBwe implies that we have a valid bitrate estimator.
I'm not entirely sure I follow. You're suggesting always initializing
remote_bitrate_estimator_, add a bool use_remote_bitrate_estimator_, and replace
all "if (remote_bitrate_estimator_)" by "if (use_remote_bitrate_estimator)"? In
other places, I think we prefer using rtc::Optional over extra flags like
use_foo, foo_is_valid etc, and in this case, a raw possibly null pointer seems
to work just as fine as an rtc::Optional.

I guess it might clarify intention for hardcore C++-lovers to write

  rtc::Optional<Foo&> 

instead of 

  Foo*

but I'm not convinced that's a good idea (if it even works)... 

Even if it would be somewhat neat if the implementation of Optional would
recognize it and implement the former in terms of the latter, without the
overhead of an extra bool. (Too go off on a tangent, that wouldn't be 100%
equivalent, because the "a reference can never be null"-thing isn't always true
at runtime).

   }
 }
 
 AudioReceiveStream::~AudioReceiveStream() {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
   LOG(LS_INFO) << "~AudioReceiveStream: " << config_.ToString();
   Stop();
   channel_proxy_->DisassociateSendChannel();
   channel_proxy_->DeRegisterExternalTransport();
   channel_proxy_->ResetCongestionControlObjects();
@@ skipping 151 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