Move ownership of send ViEChannels and ViEEncoder to VideoSendStream.

webrtc/video/video_send_stream.cc

 /*
  *  Copyright (c) 2013 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/video/video_send_stream.h"
 
 #include <algorithm>
 #include <sstream>
 #include <string>
 #include <vector>
 
 #include "webrtc/base/checks.h"
 #include "webrtc/common_video/libyuv/include/webrtc_libyuv.h"
 #include "webrtc/modules/pacing/include/packet_router.h"
 #include "webrtc/system_wrappers/interface/logging.h"
 #include "webrtc/system_wrappers/interface/trace_event.h"
 #include "webrtc/video/video_capture_input.h"
+#include "webrtc/video_engine/call_stats.h"
+#include "webrtc/video_engine/payload_router.h"
 #include "webrtc/video_engine/vie_channel.h"
 #include "webrtc/video_engine/vie_channel_group.h"
 #include "webrtc/video_engine/vie_defines.h"
 #include "webrtc/video_engine/vie_encoder.h"
 #include "webrtc/video_send_stream.h"
 
 namespace webrtc {
+
+class BitrateAllocator;
+class PacedSender;
+class RtcpIntraFrameObserver;
+class TransportFeedbackObserver;
+
 std::string
 VideoSendStream::Config::EncoderSettings::ToString() const {
   std::stringstream ss;
   ss << "{payload_name: " << payload_name;
   ss << ", payload_type: " << payload_type;
   ss << ", encoder: " << (encoder != nullptr ? "(VideoEncoder)" : "nullptr");
   ss << '}';
   return ss.str();
 }
 
@@ skipping 66 matching lines @@
     int channel_id,
     const VideoSendStream::Config& config,
     const VideoEncoderConfig& encoder_config,
     const std::map<uint32_t, RtpState>& suspended_ssrcs)
     : transport_adapter_(config.send_transport),
       encoded_frame_proxy_(config.post_encode_callback),
       config_(config),
       suspended_ssrcs_(suspended_ssrcs),
       module_process_thread_(module_process_thread),
       channel_group_(channel_group),
-      channel_id_(channel_id),
       use_config_bitrate_(true),
       stats_proxy_(Clock::GetRealTimeClock(), config) {
   RTC_DCHECK(!config_.rtp.ssrcs.empty());
-  RTC_CHECK(channel_group->CreateSendChannel(
-      channel_id_, &transport_adapter_, &stats_proxy_,
-      config.pre_encode_callback, num_cpu_cores, config_));
-  vie_channel_ = channel_group_->GetChannel(channel_id_);
-  vie_encoder_ = channel_group_->GetEncoder(channel_id_);
+
+  // Set up Call-wide sequence numbers, if configured for this send stream.
+  TransportFeedbackObserver* transport_feedback_observer = nullptr;
+  for (const RtpExtension& extension : config.rtp.extensions) {
+    if (extension.name == RtpExtension::kTransportSequenceNumber) {
+      transport_feedback_observer =
+          channel_group_->GetTransportFeedbackObserver();
+      break;
+    }
+  }
+
+  const std::vector<uint32_t>& ssrcs = config.rtp.ssrcs;
+  RTC_DCHECK(!ssrcs.empty());

[stefan-webrtc, 2015/10/15 11:33:45]

No need to DCHECK this here and on line 127.

[mflodman, 2015/10/15 14:39:22]

Thanks, copy paste generated from channel group.

+
+  vie_encoder_.reset(new ViEEncoder(
+      channel_id, num_cpu_cores, module_process_thread_, &stats_proxy_,
+      config.pre_encode_callback, channel_group_->pacer(),

[pbos-webrtc, 2015/10/15 12:07:55]

Do we want pacer/allocator through constructor instead of hopping between
channel_group_?

I think call and channel_group_ should be unified so Call should own these when
calling down (but that can easily be punted of course).

[mflodman, 2015/10/15 14:39:21]

Kept it as is for now, since I need to leave for today :)
Let me know if you think we should do this alternatively keep it until we decide
how to treat the ChannelManager members.

The rest is something we should discuss, maybe outside of this CL. I don't think
Call should own all of the ChannelManager content and I think Stefan was on the
same track. Let's discuss that outside of this CL.

+      channel_group_->bitrate_allocator()));
+  RTC_CHECK(vie_encoder_->Init());
+
+  vie_channel_.reset(new ViEChannel(
+      num_cpu_cores, config.send_transport, module_process_thread_,
+      channel_group_->GetRtcpIntraFrameObserver(),
+      channel_group_->GetBitrateController()->CreateRtcpBandwidthObserver(),
+      transport_feedback_observer,
+      channel_group_->GetRemoteBitrateEstimator(),
+      channel_group_->GetCallStats()->rtcp_rtt_stats(), channel_group_->pacer(),
+      channel_group_->packet_router(), ssrcs.size(), true));
+  RTC_CHECK(vie_channel_->Init() == 0);

[pbos-webrtc, 2015/10/15 12:07:55]

Can we make this one a bool while we're at it?

[mflodman, 2015/10/15 14:39:22]

I did that change, but reverted. I'm not a big fan of mixing bool and int
returns in the same class for error values.

+
+  vie_encoder_->StartThreadsAndSetSharedMembers(
+      vie_channel_->send_payload_router(),
+      vie_channel_->vcm_protection_callback());
+
+  std::vector<uint32_t> first_ssrc(1, ssrcs[0]);
+  vie_encoder_->SetSsrcs(first_ssrc);

[stefan-webrtc, 2015/10/15 11:33:45]

It's not entirely clear to me why we only set the first ssrc here. Maybe we
could add a comment?

[pbos-webrtc, 2015/10/15 12:07:55]

Pref setting all the ssrcs, and then vie_encoder_ can do what it pleases with
it?

[mflodman, 2015/10/15 14:39:21]

This is just copy paste, I can try to figure out and comment but would prefer to
not change too much logic at the same time.

 
   for (size_t i = 0; i < config_.rtp.extensions.size(); ++i) {
     const std::string& extension = config_.rtp.extensions[i].name;
     int id = config_.rtp.extensions[i].id;
     // One-byte-extension local identifiers are in the range 1-14 inclusive.
     RTC_DCHECK_GE(id, 1);
     RTC_DCHECK_LE(id, 14);
     if (extension == RtpExtension::kTOffset) {
       RTC_CHECK_EQ(0, vie_channel_->SetSendTimestampOffsetStatus(true, id));
     } else if (extension == RtpExtension::kAbsSendTime) {
       RTC_CHECK_EQ(0, vie_channel_->SetSendAbsoluteSendTimeStatus(true, id));
     } else if (extension == RtpExtension::kVideoRotation) {
       RTC_CHECK_EQ(0, vie_channel_->SetSendVideoRotationStatus(true, id));
     } else if (extension == RtpExtension::kTransportSequenceNumber) {
       RTC_CHECK_EQ(0, vie_channel_->SetSendTransportSequenceNumber(true, id));
     } else {
       RTC_NOTREACHED() << "Registering unsupported RTP extension.";
     }
   }
 
   // TODO(pbos): Consider configuring REMB in Call.
-  channel_group_->SetChannelRembStatus(true, false, vie_channel_);
+  channel_group_->SetChannelRembStatus(true, false, vie_channel_.get());
 
   // Enable NACK, FEC or both.
   const bool enable_protection_nack = config_.rtp.nack.rtp_history_ms > 0;
   const bool enable_protection_fec = config_.rtp.fec.red_payload_type != -1;
   // TODO(changbin): Should set RTX for RED mapping in RTP sender in future.
   vie_channel_->SetProtectionMode(enable_protection_nack, enable_protection_fec,
                                   config_.rtp.fec.red_payload_type,
                                   config_.rtp.fec.ulpfec_payload_type);
   vie_encoder_->UpdateProtectionMethod(enable_protection_nack,
                                        enable_protection_fec);
 
   ConfigureSsrcs();
 
   vie_channel_->SetRTCPCName(config_.rtp.c_name.c_str());
 
   input_.reset(new internal::VideoCaptureInput(
-      module_process_thread_, vie_encoder_, config_.local_renderer,
+      module_process_thread_, vie_encoder_.get(), config_.local_renderer,
       &stats_proxy_, this, config_.encoding_time_observer));
 
   // 28 to match packet overhead in ModuleRtpRtcpImpl.
   RTC_DCHECK_LE(config_.rtp.max_packet_size, static_cast<size_t>(0xFFFF - 28));
   vie_channel_->SetMTU(static_cast<uint16_t>(config_.rtp.max_packet_size + 28));
 
   RTC_DCHECK(config.encoder_settings.encoder != nullptr);
   RTC_DCHECK_GE(config.encoder_settings.payload_type, 0);
   RTC_DCHECK_LE(config.encoder_settings.payload_type, 127);
   RTC_CHECK_EQ(0, vie_encoder_->RegisterExternalEncoder(
                       config.encoder_settings.encoder,
                       config.encoder_settings.payload_type,
                       config.encoder_settings.internal_source));
 
   RTC_CHECK(ReconfigureVideoEncoder(encoder_config));
 
   vie_channel_->RegisterSendSideDelayObserver(&stats_proxy_);
 
   if (config_.post_encode_callback)
     vie_encoder_->RegisterPostEncodeImageCallback(&encoded_frame_proxy_);
 
   if (config_.suspend_below_min_bitrate)
     vie_encoder_->SuspendBelowMinBitrate();
 
+  channel_group_->AddEncoder(ssrcs, vie_encoder_.get());
+
   vie_channel_->RegisterSendChannelRtcpStatisticsCallback(&stats_proxy_);
   vie_channel_->RegisterSendChannelRtpStatisticsCallback(&stats_proxy_);
   vie_channel_->RegisterRtcpPacketTypeCounterObserver(&stats_proxy_);
   vie_channel_->RegisterSendBitrateObserver(&stats_proxy_);
   vie_channel_->RegisterSendFrameCountObserver(&stats_proxy_);
 }
 
 VideoSendStream::~VideoSendStream() {
   vie_channel_->RegisterSendFrameCountObserver(nullptr);
   vie_channel_->RegisterSendBitrateObserver(nullptr);
   vie_channel_->RegisterRtcpPacketTypeCounterObserver(nullptr);
   vie_channel_->RegisterSendChannelRtpStatisticsCallback(nullptr);
   vie_channel_->RegisterSendChannelRtcpStatisticsCallback(nullptr);
 
   // Remove capture input (thread) so that it's not running after the current
   // channel is deleted.
   input_.reset();
 
   vie_encoder_->DeRegisterExternalEncoder(
       config_.encoder_settings.payload_type);
 
-  channel_group_->DeleteChannel(channel_id_);
+  channel_group_->GetCallStats()->DeregisterStatsObserver(
+      vie_channel_->GetStatsObserver());
+  channel_group_->SetChannelRembStatus(false, false, vie_channel_.get());
+
+  // Remove the feedback, stop all encoding threads and processing. This must be
+  // done before deleting the channel.
+  channel_group_->RemoveEncoder(vie_encoder_.get());
+  vie_encoder_->StopThreadsAndRemoveSharedMembers();
+
+  unsigned int remote_ssrc = 0;

[pbos-webrtc, 2015/10/15 12:07:55]

Is this not an uint32_t?

[mflodman, 2015/10/15 14:39:21]

Yes, should be. Copy paste from ChannelManager.

+  vie_channel_->GetRemoteSSRC(&remote_ssrc);

[pbos-webrtc, 2015/10/15 12:07:55]

Can this fail? If so should we remove the stream zero?

[mflodman, 2015/10/15 14:39:21]

No, it can't fail.

+
+  channel_group_->GetRemoteBitrateEstimator()->RemoveStream(remote_ssrc);
 }
 
 VideoCaptureInput* VideoSendStream::Input() {
   return input_.get();
 }
 
 void VideoSendStream::Start() {
   transport_adapter_.Enable();
   vie_encoder_->Pause();
   if (vie_channel_->StartSend() == 0) {
@@ skipping 275 matching lines @@
                                        vie_channel_->IsSendingFecEnabled());
 
   // Restart the media flow
   vie_encoder_->Restart();
 
   return true;
 }
 
 }  // namespace internal
 }  // namespace webrtc