Move ownership of send ViEChannels and ViEEncoder to VideoSendStream.

webrtc/video_engine/vie_channel_group.cc

 /*
  *  Copyright (c) 2012 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.
  */
 
@@ skipping 175 matching lines @@
   pacer_thread_->DeRegisterModule(pacer_.get());
   process_thread_->DeRegisterModule(bitrate_controller_.get());
   process_thread_->DeRegisterModule(call_stats_.get());
   process_thread_->DeRegisterModule(remote_bitrate_estimator_.get());
   process_thread_->DeRegisterModule(remote_estimator_proxy_.get());
   call_stats_->DeregisterStatsObserver(remote_bitrate_estimator_.get());
   if (transport_feedback_adapter_.get())
     call_stats_->DeregisterStatsObserver(transport_feedback_adapter_.get());
   RTC_DCHECK(channel_map_.empty());
   RTC_DCHECK(!remb_->InUse());
-  RTC_DCHECK(vie_encoder_map_.empty());
-}
-
-bool ChannelGroup::CreateSendChannel(int channel_id,
-                                     Transport* transport,
-                                     SendStatisticsProxy* stats_proxy,
-                                     I420FrameCallback* pre_encode_callback,
-                                     int number_of_cores,
-                                     const VideoSendStream::Config& config) {
-  TransportFeedbackObserver* transport_feedback_observer = nullptr;
-  bool transport_seq_enabled = false;
-  for (const RtpExtension& extension : config.rtp.extensions) {
-    if (extension.name == RtpExtension::kTransportSequenceNumber) {
-      transport_seq_enabled = true;
-      break;
-    }
-  }
-  if (transport_seq_enabled) {
-    if (transport_feedback_adapter_.get() == nullptr) {
-      transport_feedback_adapter_.reset(new TransportFeedbackAdapter(
-          bitrate_controller_->CreateRtcpBandwidthObserver(),
-          Clock::GetRealTimeClock(), process_thread_));
-      transport_feedback_adapter_->SetBitrateEstimator(
-          new RemoteBitrateEstimatorAbsSendTime(
-              transport_feedback_adapter_.get(), Clock::GetRealTimeClock()));
-      transport_feedback_adapter_->GetBitrateEstimator()->SetMinBitrate(
-          min_bitrate_bps_);
-      call_stats_->RegisterStatsObserver(transport_feedback_adapter_.get());
-    }
-    transport_feedback_observer = transport_feedback_adapter_.get();
-  }
-
-  const std::vector<uint32_t>& ssrcs = config.rtp.ssrcs;
-  RTC_DCHECK(!ssrcs.empty());
-  rtc::scoped_ptr<ViEEncoder> vie_encoder(new ViEEncoder(
-      channel_id, number_of_cores, process_thread_, stats_proxy,
-      pre_encode_callback, pacer_.get(), bitrate_allocator_.get()));
-  if (!vie_encoder->Init()) {
-    return false;
-  }
-  ViEEncoder* encoder = vie_encoder.get();
-  if (!CreateChannel(channel_id, transport, number_of_cores,
-                     vie_encoder.release(), ssrcs.size(), true,
-                     remote_bitrate_estimator_.get(),
-                     transport_feedback_observer)) {
-    return false;
-  }
-  ViEChannel* channel = channel_map_[channel_id];
-  // Connect the encoder with the send packet router, to enable sending.
-  encoder->StartThreadsAndSetSharedMembers(channel->send_payload_router(),
-                                           channel->vcm_protection_callback());
-
-  encoder_state_feedback_->AddEncoder(ssrcs, encoder);
-  std::vector<uint32_t> first_ssrc(1, ssrcs[0]);
-  encoder->SetSsrcs(first_ssrc);
-  return true;
+  RTC_DCHECK(encoders_.empty());
 }
 
 bool ChannelGroup::CreateReceiveChannel(
     int channel_id,
     Transport* transport,
     int number_of_cores,
     const VideoReceiveStream::Config& config) {
   bool send_side_bwe = false;
   for (const RtpExtension& extension : config.rtp.extensions) {
     if (extension.name == RtpExtension::kTransportSequenceNumber) {
       send_side_bwe = true;
       break;
     }
   }
 
   RemoteBitrateEstimator* bitrate_estimator;
   if (send_side_bwe) {
     bitrate_estimator = remote_estimator_proxy_.get();
   } else {
     bitrate_estimator = remote_bitrate_estimator_.get();
   }
-  return CreateChannel(channel_id, transport, number_of_cores, nullptr, 1,
-                       false, bitrate_estimator, nullptr);
+  return CreateChannel(channel_id, transport, number_of_cores, 1, false,
+                       bitrate_estimator, nullptr);
 }
 
 bool ChannelGroup::CreateChannel(int channel_id,
                                  Transport* transport,
                                  int number_of_cores,
-                                 ViEEncoder* vie_encoder,
                                  size_t max_rtp_streams,
                                  bool sender,
                                  RemoteBitrateEstimator* bitrate_estimator,
                                  TransportFeedbackObserver* feedback_observer) {
   rtc::scoped_ptr<ViEChannel> channel(new ViEChannel(
       number_of_cores, transport, process_thread_,
       encoder_state_feedback_->GetRtcpIntraFrameObserver(),
       bitrate_controller_->CreateRtcpBandwidthObserver(), feedback_observer,
       bitrate_estimator, call_stats_->rtcp_rtt_stats(), pacer_.get(),
       packet_router_.get(), max_rtp_streams, sender));
   if (channel->Init() != 0) {
     return false;
   }
 
   // Register the channel to receive stats updates.
   call_stats_->RegisterStatsObserver(channel->GetStatsObserver());
 
-  // Store the channel, add it to the channel group and save the vie_encoder.
+  // Store the channel and add it to the channel group.
   channel_map_[channel_id] = channel.release();
-  if (vie_encoder) {
-    rtc::CritScope lock(&encoder_map_crit_);
-    vie_encoder_map_[channel_id] = vie_encoder;
-  }
-
   return true;
 }
 
 void ChannelGroup::DeleteChannel(int channel_id) {
   ViEChannel* vie_channel = PopChannel(channel_id);
 
-  ViEEncoder* vie_encoder = GetEncoder(channel_id);
-
   call_stats_->DeregisterStatsObserver(vie_channel->GetStatsObserver());
   SetChannelRembStatus(false, false, vie_channel);
 
-  // If we're a sender, remove the feedback and stop all encoding threads and
-  // processing. This must be done before deleting the channel.
-  if (vie_encoder) {
-    encoder_state_feedback_->RemoveEncoder(vie_encoder);
-    vie_encoder->StopThreadsAndRemoveSharedMembers();
-  }
-
   unsigned int remote_ssrc = 0;
   vie_channel->GetRemoteSSRC(&remote_ssrc);
   channel_map_.erase(channel_id);
   remote_bitrate_estimator_->RemoveStream(remote_ssrc);
 
   delete vie_channel;
 
-  if (vie_encoder) {
-    {
-      rtc::CritScope lock(&encoder_map_crit_);
-      vie_encoder_map_.erase(vie_encoder_map_.find(channel_id));
-    }
-    delete vie_encoder;
-  }
-
   LOG(LS_VERBOSE) << "Channel deleted " << channel_id;
 }
 
 ViEChannel* ChannelGroup::GetChannel(int channel_id) const {
   ChannelMap::const_iterator it = channel_map_.find(channel_id);
   if (it == channel_map_.end()) {
     LOG(LS_ERROR) << "Channel doesn't exist " << channel_id;
     return NULL;
   }
   return it->second;
 }
 
-ViEEncoder* ChannelGroup::GetEncoder(int channel_id) const {
-  rtc::CritScope lock(&encoder_map_crit_);
-  EncoderMap::const_iterator it = vie_encoder_map_.find(channel_id);
-  if (it == vie_encoder_map_.end())
-    return nullptr;
-  return it->second;
+void ChannelGroup::AddEncoder(const std::vector<uint32_t>& ssrcs,
+                              ViEEncoder* encoder) {
+  encoder_state_feedback_->AddEncoder(ssrcs, encoder);
+  rtc::CritScope lock(&encoder_crit_);
+  encoders_.push_back(encoder);
+}
+
+void ChannelGroup::RemoveEncoder(ViEEncoder* encoder) {
+

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

Remove empty line

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

Done.

+  encoder_state_feedback_->RemoveEncoder(encoder);
+  rtc::CritScope lock(&encoder_crit_);
+  for (auto it = encoders_.begin(); it != encoders_.end(); ++it) {
+    if (*it == encoder) {
+      encoders_.erase(it);
+      return;
+    }
+  }

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

Maybe make encoders_ a list and do encoders_.remove(encoder) instead?

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

Don't think so, iterating a vector is faster and this is rarely ever added
to/deleted from.

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

Yes, I chose a vector since we do iterate over this quite often.

[stefan-webrtc, 2015/10/16 06:51:53]

I doubt the loop we do over this will make a difference regarding performance,
and personally prefer simpler code for that reason. But I can live with it since
it's going away.

 }
 
 ViEChannel* ChannelGroup::PopChannel(int channel_id) {
   ChannelMap::iterator c_it = channel_map_.find(channel_id);
   RTC_DCHECK(c_it != channel_map_.end());
   ViEChannel* channel = c_it->second;
   channel_map_.erase(c_it);
 
   return channel;
 }
@@ skipping 22 matching lines @@
 }
 
 RemoteBitrateEstimator* ChannelGroup::GetRemoteBitrateEstimator() const {
   return remote_bitrate_estimator_.get();
 }
 
 CallStats* ChannelGroup::GetCallStats() const {
   return call_stats_.get();
 }
 
-EncoderStateFeedback* ChannelGroup::GetEncoderStateFeedback() const {
-  return encoder_state_feedback_.get();
+TransportFeedbackObserver* ChannelGroup::GetTransportFeedbackObserver() {

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

It doesn't look like transport feedback adapter should be owned by the
ChannelGroup... I think TransportFeedbackAdapter soon will become
BandwidthEstimator, and also get to own BitrateController /
SendSideBandwidthEstimation, and if we run the send-side-only BWE that will be
the only BWE object.

I'm not sure this makes a difference for this CL, probably not. It may not be
worth the trouble to remove this method now.

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

I agree. I do prefer to keep this logic in ChannelGroup for this CL since it was
here earlier. But a good point and we should agree on where it should be.

+  if (transport_feedback_adapter_.get() == nullptr) {
+    transport_feedback_adapter_.reset(new TransportFeedbackAdapter(
+        bitrate_controller_->CreateRtcpBandwidthObserver(),
+        Clock::GetRealTimeClock(), process_thread_));
+    transport_feedback_adapter_->SetBitrateEstimator(
+        new RemoteBitrateEstimatorAbsSendTime(
+            transport_feedback_adapter_.get(), Clock::GetRealTimeClock()));
+    transport_feedback_adapter_->GetBitrateEstimator()->SetMinBitrate(
+        min_bitrate_bps_);
+    call_stats_->RegisterStatsObserver(transport_feedback_adapter_.get());
+  }
+  return transport_feedback_adapter_.get();
+}
+
+RtcpIntraFrameObserver* ChannelGroup::GetRtcpIntraFrameObserver() const {
+  return encoder_state_feedback_->GetRtcpIntraFrameObserver();
 }
 
 int64_t ChannelGroup::GetPacerQueuingDelayMs() const {
   return pacer_->QueueInMs();
 }
 
 void ChannelGroup::SetChannelRembStatus(bool sender,
                                         bool receiver,
                                         ViEChannel* channel) {
   // Update the channel state.
@@ skipping 11 matching lines @@
     remb_->RemoveReceiveChannel(rtp_module);
   }
 }
 
 void ChannelGroup::OnNetworkChanged(uint32_t target_bitrate_bps,
                                     uint8_t fraction_loss,
                                     int64_t rtt) {
   bitrate_allocator_->OnNetworkChanged(target_bitrate_bps, fraction_loss, rtt);
   int pad_up_to_bitrate_bps = 0;
   {
-    rtc::CritScope lock(&encoder_map_crit_);
-    for (const auto& encoder : vie_encoder_map_)
-      pad_up_to_bitrate_bps += encoder.second->GetPaddingNeededBps();
+    rtc::CritScope lock(&encoder_crit_);
+    for (const auto& encoder : encoders_)
+      pad_up_to_bitrate_bps += encoder->GetPaddingNeededBps();

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

It would really be nice to improve this... Maybe add a todo that we should
rethink how we decide how much padding we need, and that we should try to remove
encoders_ from ChannelGroup?

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

Yeah I think registering encoders on the pacer directly makes more sense. That's
definitely a TODO though.

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

TODO added.

   }
   pacer_->UpdateBitrate(
       target_bitrate_bps / 1000,
       PacedSender::kDefaultPaceMultiplier * target_bitrate_bps / 1000,
       pad_up_to_bitrate_bps / 1000);
 }
 }  // namespace webrtc