Introduce RtpStreamReceiver and RtpStreamReceiverControllerInterface.

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/call/rtp_stream_receiver_controller_interface.h"
 #include "webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h"
 #include "webrtc/modules/rtp_rtcp/include/rtp_receiver.h"
 #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp.h"
 #include "webrtc/voice_engine/channel_proxy.h"
 #include "webrtc/voice_engine/include/voe_base.h"
 #include "webrtc/voice_engine/voice_engine_impl.h"
 
 namespace webrtc {
 
 std::string AudioReceiveStream::Config::Rtp::ToString() const {
@@ skipping 22 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(
+    RtpStreamReceiverControllerInterface* receiver_controller,
     PacketRouter* packet_router,
     const webrtc::AudioReceiveStream::Config& config,
     const rtc::scoped_refptr<webrtc::AudioState>& audio_state,
     webrtc::RtcEventLog* event_log)
-    : config_(config),
-      audio_state_(audio_state) {
+    : config_(config), audio_state_(audio_state) {
   LOG(LS_INFO) << "AudioReceiveStream: " << config_.ToString();
   RTC_DCHECK_NE(config_.voe_channel_id, -1);
   RTC_DCHECK(audio_state_.get());
   RTC_DCHECK(packet_router);
 
   module_process_thread_checker_.DetachFromThread();
 
   VoiceEngineImpl* voe_impl = static_cast<VoiceEngineImpl*>(voice_engine());
   channel_proxy_ = voe_impl->GetChannelProxy(config_.voe_channel_id);
   channel_proxy_->SetRtcEventLog(event_log);
@@ skipping 19 matching lines @@
     if (extension.uri == RtpExtension::kAudioLevelUri) {
       channel_proxy_->SetReceiveAudioLevelIndicationStatus(true, extension.id);
     } else if (extension.uri == RtpExtension::kTransportSequenceNumberUri) {
       channel_proxy_->EnableReceiveTransportSequenceNumber(extension.id);
     } else {
       RTC_NOTREACHED() << "Unsupported RTP extension.";
     }
   }
   // Configure bandwidth estimation.
   channel_proxy_->RegisterReceiverCongestionControlObjects(packet_router);
+
+  // Register with transport.
+  rtp_stream_receiver_ =

[the sun, 2017/06/16 13:37:39]

Exposing the controller to the stream so that it can register itself with it
feels somewhat questionable architecturally. Why does the stream need to own the
receiver?

Also, giving the receiver a pointer to "this" before the object is fully
constructed, is brittle even if it will work in this situation.

[nisse-webrtc, 2017/06/16 14:33:25]

The point is that the controller should own media-independent state, while the
receive stream should hook that up to any processing which depends on *both* rtp
and the media type.

The corresponding method on the video stream creates two receivers, one for
media and one for rtx, and the idea is that neither the controller or call
should need to know that's how we do video.
Who else should own the receiver(s)? In any case, we need to deregister from the
demuxer, and letting the receiver's destructor be responsible for that seems
reasonable to me (it was originally danil's suggestion, iirc).
It looks like we could pass channel_proxy_ instead, and delete the OnRtpPacket
method from this class. (But I think this way to use this is reasonable, even if
the subtleties could perhaps be better documented).

[the sun, 2017/06/16 15:00:13]

That sgtm.
I guess the Receiver could have shared ownership of the stream and be created
outside of the stream class. That would achieve the objective of decoupling the
transport layer from stream objects too. But very well...
No, please don't pass ChannelProxy. That's an interim solution.

[nisse-webrtc, 2017/06/19 07:21:01]

I did that change in ps#18. Please have a look, and tell me if you still want me
to undo that part. Note that this change is only about the internal division of
work between AudioReceiveStrean and ChannelProxy, and not in any other
interfaces. Whenever ChannelProxy is replaced by something else, the
CreateReceiver call needs to be updated with whatever is the replacement. 

ChannelProxy already has an OnRtpPacket method, to pass it to CreateReceiver all
that's needed is to tell the compiler it actually implements the
RtpPacketSinkInterface.

[the sun, 2017/06/20 07:11:36]

The problem is that it muddies the image of what ChannelProxy is - it is not
longer just a proxy for voe::Channel, mirroring what Channel provides. Now it is
an actor all of its own.

Please revert to the previous state.

+      receiver_controller->CreateReceiver(config_.rtp.remote_ssrc, this);
 }
 
 AudioReceiveStream::~AudioReceiveStream() {
   RTC_DCHECK_RUN_ON(&worker_thread_checker_);
   LOG(LS_INFO) << "~AudioReceiveStream: " << config_.ToString();
   if (playing_) {
     Stop();
   }
   channel_proxy_->DisassociateSendChannel();
   channel_proxy_->DeRegisterExternalTransport();
@@ skipping 213 matching lines @@
 int AudioReceiveStream::SetVoiceEnginePlayout(bool playout) {
   ScopedVoEInterface<VoEBase> base(voice_engine());
   if (playout) {
     return base->StartPlayout(config_.voe_channel_id);
   } else {
     return base->StopPlayout(config_.voe_channel_id);
   }
 }
 }  // namespace internal
 }  // namespace webrtc