Initialize VideoEncoder objects asynchronously.

webrtc/video/vie_channel.cc

Index: webrtc/video/vie_channel.cc
diff --git a/webrtc/video/vie_channel.cc b/webrtc/video/vie_channel.cc
index bc23c9d46724e6dbd15e0a23f6cd9bc3f71c5657..7b48cd9276902113b530966aa84d41ad4a46cefe 100644
--- a/webrtc/video/vie_channel.cc
+++ b/webrtc/video/vie_channel.cc
@@ -146,8 +146,6 @@ int32_t ViEChannel::Init() {
   module_process_thread_->RegisterModule(vie_receiver_.GetReceiveStatistics());
 
   // RTP/RTCP initialization.
-  module_process_thread_->RegisterModule(rtp_rtcp_modules_[0]);
-
   rtp_rtcp_modules_[0]->SetKeyFrameRequestMethod(kKeyFrameReqPliRtcp);
   if (paced_sender_) {
     for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_)
@@ -155,8 +153,8 @@ int32_t ViEChannel::Init() {
   }
   packet_router_->AddRtpModule(rtp_rtcp_modules_[0]);
   if (sender_) {
-    std::list<RtpRtcp*> send_rtp_modules(1, rtp_rtcp_modules_[0]);
-    send_payload_router_->SetSendingRtpModules(send_rtp_modules);
+    send_payload_router_->SetSendingRtpModules(
+        std::vector<RtpRtcp*>(1, rtp_rtcp_modules_[0]));
     RTC_DCHECK(!send_payload_router_->active());
   }
   if (vcm_->RegisterReceiveCallback(this) != 0) {
@@ -169,18 +167,51 @@ int32_t ViEChannel::Init() {
 
   module_process_thread_->RegisterModule(vcm_);
   module_process_thread_->RegisterModule(&vie_sync_);
+  module_process_thread_->RegisterModule(this);
 
   return 0;
 }
 
+// TODO(pbos): Consider atomics, this might block.
+int64_t ViEChannel::TimeUntilNextProcess() {
+  size_t active_modules;
+  {
+    CriticalSectionScoped cs(crit_.get());
+    active_modules = num_active_rtp_rtcp_modules_;
+  }
+
+  int64_t min_time = rtp_rtcp_modules_[0]->TimeUntilNextProcess();
+  for (size_t i = 1; i < active_modules; ++i) {
+    int64_t time = rtp_rtcp_modules_[i]->TimeUntilNextProcess();
+    if (time < min_time)
+      min_time = time;
+  }
+  return min_time;
+}
+
+// TODO(pbos): Consider atomics, this might block.
+int32_t ViEChannel::Process() {
+  size_t active_modules;
+  {
+    CriticalSectionScoped cs(crit_.get());
+    active_modules = num_active_rtp_rtcp_modules_;
+  }
+
+  for (size_t i = 0; i < active_modules; ++i) {
+    rtp_rtcp_modules_[i]->Process();
+  }
+  return 0;
+}

[stefan-webrtc, 2016/01/19 12:02:10]

Could you enlighten why this was necessary to change in this CL?

[pbos-webrtc, 2016/01/19 15:27:58]

I can only add/remove modules from the construction thread, but I need to turn
on/off RTP modules on the fly from the encoder thread.

[stefan-webrtc, 2016/01/20 12:22:48]

I don't think I fully understand. How is the process thread involved with the
construction thread and the encoder thread?

Is it because you want to avoid having to register/deregister modules on the
process thread for some reason? Seems like we are basically duplicating code
that the ProcessThread aldready has?

[stefan-webrtc, 2016/01/27 09:06:43]

Ping

+
 ViEChannel::~ViEChannel() {
   UpdateHistograms();
   // Make sure we don't get more callbacks from the RTP module.
+  module_process_thread_->DeRegisterModule(this);
   module_process_thread_->DeRegisterModule(
       vie_receiver_.GetReceiveStatistics());
   module_process_thread_->DeRegisterModule(vcm_);
   module_process_thread_->DeRegisterModule(&vie_sync_);
-  send_payload_router_->SetSendingRtpModules(std::list<RtpRtcp*>());
+  send_payload_router_->SetSendingRtpModules(std::vector<RtpRtcp*>());
   for (size_t i = 0; i < num_active_rtp_rtcp_modules_; ++i)
     packet_router_->RemoveRtpModule(rtp_rtcp_modules_[i]);
   for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) {
@@ -337,89 +368,66 @@ void ViEChannel::UpdateHistograms() {
   }
 }
 
-int32_t ViEChannel::SetSendCodec(const VideoCodec& video_codec,
-                                 bool new_stream) {
+void ViEChannel::SetSendCodec(const VideoCodec& video_codec) {

[stefan-webrtc, 2016/01/19 12:02:10]

Should we check what thread this is being called on?

[pbos-webrtc, 2016/01/19 15:27:58]

It'd be interesting to inject thread checkers from VideoSendStream that'd be
shared across objects (encoder_thread_checker). I think that sounds like a
follow-up though?

[stefan-webrtc, 2016/01/20 12:22:48]

Can't you have a separate thread checker?

+  // Here crit_ is used to synchronize between this method, StartSend and
+  // StopSend and not end up in an inconsistent sending state.
+  CriticalSectionScoped cs(crit_.get());
   RTC_DCHECK(sender_);
-  if (video_codec.codecType == kVideoCodecRED ||
-      video_codec.codecType == kVideoCodecULPFEC) {
-    LOG_F(LS_ERROR) << "Not a valid send codec " << video_codec.codecType;
-    return -1;
-  }
-  if (kMaxSimulcastStreams < video_codec.numberOfSimulcastStreams) {
-    LOG_F(LS_ERROR) << "Incorrect config "
-                    << video_codec.numberOfSimulcastStreams;
-    return -1;
-  }
+  RTC_DCHECK(video_codec.codecType != kVideoCodecRED);
+  RTC_DCHECK(video_codec.codecType != kVideoCodecULPFEC);
+  RTC_DCHECK_LE(video_codec.numberOfSimulcastStreams, kMaxSimulcastStreams);
   // Update the RTP module with the settings.
   // Stop and Start the RTP module -> trigger new SSRC, if an SSRC hasn't been
   // set explicitly.
   // The first layer is always active, so the first module can be checked for
   // sending status.
-  bool is_sending = rtp_rtcp_modules_[0]->Sending();
   bool router_was_active = send_payload_router_->active();
   send_payload_router_->set_active(false);
-  send_payload_router_->SetSendingRtpModules(std::list<RtpRtcp*>());
+  send_payload_router_->SetSendingRtpModules(std::vector<RtpRtcp*>());
 
+  size_t num_prev_active_modules = num_active_rtp_rtcp_modules_;
+  num_active_rtp_rtcp_modules_ = video_codec.numberOfSimulcastStreams > 0
+                                     ? video_codec.numberOfSimulcastStreams
+                                     : 1;
   std::vector<RtpRtcp*> registered_modules;
-  std::vector<RtpRtcp*> deregistered_modules;
-  size_t num_active_modules = video_codec.numberOfSimulcastStreams > 0
-                                   ? video_codec.numberOfSimulcastStreams
-                                   : 1;
-  size_t num_prev_active_modules;
-  {
-    // Cache which modules are active so StartSend can know which ones to start.
-    CriticalSectionScoped cs(crit_.get());
-    num_prev_active_modules = num_active_rtp_rtcp_modules_;
-    num_active_rtp_rtcp_modules_ = num_active_modules;
-  }
-  for (size_t i = 0; i < num_active_modules; ++i)
+  for (size_t i = 0; i < num_active_rtp_rtcp_modules_; ++i)
     registered_modules.push_back(rtp_rtcp_modules_[i]);
 
-  for (size_t i = num_active_modules; i < rtp_rtcp_modules_.size(); ++i)
-    deregistered_modules.push_back(rtp_rtcp_modules_[i]);
-
   // Disable inactive modules.
-  for (RtpRtcp* rtp_rtcp : deregistered_modules) {
+  for (size_t i = num_active_rtp_rtcp_modules_; i < rtp_rtcp_modules_.size();
+       ++i) {
+    RtpRtcp* const rtp_rtcp = rtp_rtcp_modules_[i];
     rtp_rtcp->SetSendingStatus(false);
     rtp_rtcp->SetSendingMediaStatus(false);
   }
 
   // Configure active modules.
+  bool is_sending = rtp_rtcp_modules_[0]->Sending();
   for (RtpRtcp* rtp_rtcp : registered_modules) {
     rtp_rtcp->DeRegisterSendPayload(video_codec.plType);
-    if (rtp_rtcp->RegisterSendPayload(video_codec) != 0) {
-      return -1;
-    }
+    RTC_CHECK_EQ(0, rtp_rtcp->RegisterSendPayload(video_codec));
     rtp_rtcp->SetSendingStatus(is_sending);
     rtp_rtcp->SetSendingMediaStatus(is_sending);
   }
 
-  // |RegisterSimulcastRtpRtcpModules| resets all old weak pointers and old
-  // modules can be deleted after this step.
   vie_receiver_.RegisterRtpRtcpModules(registered_modules);
 
-  // Update the packet and payload routers with the sending RtpRtcp modules.
-  if (sender_) {
-    std::list<RtpRtcp*> active_send_modules;
-    for (RtpRtcp* rtp_rtcp : registered_modules)
-      active_send_modules.push_back(rtp_rtcp);
-    send_payload_router_->SetSendingRtpModules(active_send_modules);
-  }
-
-  if (router_was_active)
-    send_payload_router_->set_active(true);
 
   // Deregister previously registered modules.
-  for (size_t i = num_active_modules; i < num_prev_active_modules; ++i) {
-    module_process_thread_->DeRegisterModule(rtp_rtcp_modules_[i]);
+  for (size_t i = num_active_rtp_rtcp_modules_; i < num_prev_active_modules;
+       ++i) {
     packet_router_->RemoveRtpModule(rtp_rtcp_modules_[i]);
   }
   // Register new active modules.
-  for (size_t i = num_prev_active_modules; i < num_active_modules; ++i) {
-    module_process_thread_->RegisterModule(rtp_rtcp_modules_[i]);
+  for (size_t i = num_prev_active_modules; i < num_active_rtp_rtcp_modules_;
+       ++i) {
     packet_router_->AddRtpModule(rtp_rtcp_modules_[i]);
   }
-  return 0;
+
+  // Update the packet and payload routers with the sending RtpRtcp modules.
+  send_payload_router_->SetSendingRtpModules(registered_modules);
+  if (router_was_active)
+    send_payload_router_->set_active(true);
 }

[stefan-webrtc, 2016/01/19 12:02:10]

When reading the changes for this method I can't see much that actually did
change. Can you please summarize what you have done here? Did you only remove
registration/deregistration of rtp modules?

[pbos-webrtc, 2016/01/19 15:27:58]

Yes, and move crit_ to cover the entire function (so that it remains
synchronized with StartSend and StopSend).

[stefan-webrtc, 2016/01/20 12:22:48]

Ok!

 
 int32_t ViEChannel::SetReceiveCodec(const VideoCodec& video_codec) {
@@ -900,6 +908,9 @@ int32_t ViEChannel::StartSend() {
 }
 
 int32_t ViEChannel::StopSend() {
+  // Acquire crit_ to synchronize sending status with StartSend and
+  // SetSendCodec.
+  CriticalSectionScoped cs(crit_.get());
   send_payload_router_->set_active(false);
   for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_)
     rtp_rtcp->SetSendingMediaStatus(false);