Remove usage of VoEBase::AssociateSendChannel() from WVoMC.

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"
@@ skipping 109 matching lines @@
   if (UseSendSideBwe(config)) {
     remote_bitrate_estimator_ =
         congestion_controller->GetRemoteBitrateEstimator(true);
   }
 }
 
 AudioReceiveStream::~AudioReceiveStream() {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
   LOG(LS_INFO) << "~AudioReceiveStream: " << config_.ToString();
   Stop();
+  AssociateSendStream(nullptr);

[kwiberg-webrtc, 2016/11/11 13:47:39]

Or just channel_proxy_->DisassociateSendChannel();, so that it's easier to see
what it does.

[the sun, 2016/11/11 20:03:39]

Yes, thanks.

   channel_proxy_->DeRegisterExternalTransport();
   channel_proxy_->ResetCongestionControlObjects();
   channel_proxy_->SetRtcEventLog(nullptr);
   if (remote_bitrate_estimator_) {
     remote_bitrate_estimator_->RemoveStream(config_.rtp.remote_ssrc);
   }
 }
 
 void AudioReceiveStream::Start() {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
@@ skipping 67 matching lines @@
 void AudioReceiveStream::SetGain(float gain) {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
   channel_proxy_->SetChannelOutputVolumeScaling(gain);
 }
 
 const webrtc::AudioReceiveStream::Config& AudioReceiveStream::config() const {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
   return config_;
 }
 
+void AudioReceiveStream::AssociateSendStream(AudioSendStream* send_stream) {
+  RTC_DCHECK(thread_checker_.CalledOnValidThread());
+  if (send_stream) {
+    VoiceEngineImpl* voe_impl = static_cast<VoiceEngineImpl*>(voice_engine());

[kwiberg-webrtc, 2016/11/11 13:47:39]

Is this static_cast guaranteed to be safe? And so, would it be possible to
convince the type system of that fact so that we could avoid the cast?

[the sun, 2016/11/11 20:03:39]

I don't see how, without exposing VoiceEngineImpl in AudioState. We need to cast
because of the GetChannelProxy() method, which is on VoiceEngineImpl but not on
VoiceEngine. What we do know is that a VoiceEngine* will always point to an
instance of VoiceEngineImpl so the cast is safe (the same pattern is used in the
ctor for both AudioSendStream and AudioReceiveStream.

Do you have any persuasive arguments I could throw on the type system?

[kwiberg-webrtc, 2016/11/13 20:17:02]

The only way to make this statically safe (since you can't verify that a class
has only one subclass) is to not do down casts, which means you can't do up
casts either---in other words, you'd need to pass it around as the most specific
type you need. Meaning either you'd need to expose the Impl class, or move
GetChannelProxy to VoiceEngine. (The latter sounds more appealing.)

This makes me a bit suspicious, though---if you depend on VoiceEngineImpl being
the only subclass of the abstract class VoiceEngine, why not just turn
VoiceEngine into a concrete, final class?

I realize that whatever the answers are to my questions, getting rid of the down
cast is going to be outside the scope of this CL...

[the sun, 2016/11/14 18:58:06]

Well, no, we don't want to expose that method in the public API.
Not only that, but we also depend on VoiceEngineImpl to inherit from all the
VoENnn interface classes. See e.g. VoEBase::GetInterface(). I consider this part
of the legacy API and will not change it now.
Indeed. :)

+    std::unique_ptr<voe::ChannelProxy> send_channel_proxy =
+        voe_impl->GetChannelProxy(send_stream->config().voe_channel_id);
+    channel_proxy_->AssociateSendChannel(*send_channel_proxy.get());
+  } else {
+    channel_proxy_->DisassociateSendChannel();
+  }
+}
+
 void AudioReceiveStream::SignalNetworkState(NetworkState state) {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
 }
 
 bool AudioReceiveStream::DeliverRtcp(const uint8_t* packet, size_t length) {
   // 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());
   return channel_proxy_->ReceivedRTCPPacket(packet, length);
@@ skipping 43 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