Initialize VideoEncoder objects asynchronously.

webrtc/video/video_capture_input.cc

Index: webrtc/video/video_capture_input.cc
diff --git a/webrtc/video/video_capture_input.cc b/webrtc/video/video_capture_input.cc
index 1c5f2992910737e14b9cf72e036c3af7e358464d..c6cff997dc5e93d597b2a70384e80221c85d6773 100644
--- a/webrtc/video/video_capture_input.cc
+++ b/webrtc/video/video_capture_input.cc
@@ -23,7 +23,7 @@
 #include "webrtc/system_wrappers/include/tick_util.h"
 #include "webrtc/video/overuse_frame_detector.h"
 #include "webrtc/video/send_statistics_proxy.h"
-#include "webrtc/video/vie_encoder.h"
+#include "webrtc/video/video_send_stream.h"
 
 namespace webrtc {
 
@@ -31,16 +31,19 @@ namespace internal {
 VideoCaptureInput::VideoCaptureInput(
     ProcessThread* module_process_thread,
     VideoCaptureCallback* frame_callback,
-    VideoRenderer* local_renderer,
+    VideoCodingModule* vcm,
+    VideoSendStream* video_send_stream,
+    const webrtc::VideoSendStream::Config& config,
     SendStatisticsProxy* stats_proxy,
-    CpuOveruseObserver* overuse_observer,
-    EncodingTimeObserver* encoding_time_observer)
+    CpuOveruseObserver* overuse_observer)
     : capture_cs_(CriticalSectionWrapper::CreateCriticalSection()),
       module_process_thread_(module_process_thread),
       frame_callback_(frame_callback),
-      local_renderer_(local_renderer),
+      vcm_(vcm),
+      video_send_stream_(video_send_stream),
+      config_(config),
+      encoder_registered_(false),
       stats_proxy_(stats_proxy),
-      incoming_frame_cs_(CriticalSectionWrapper::CreateCriticalSection()),
       encoder_thread_(EncoderThreadFunction, this, "EncoderThread"),
       capture_event_(false, false),
       stop_(0),
@@ -51,8 +54,10 @@ VideoCaptureInput::VideoCaptureInput(
       overuse_detector_(new OveruseFrameDetector(Clock::GetRealTimeClock(),
                                                  CpuOveruseOptions(),
                                                  overuse_observer,
-                                                 stats_proxy)),
-      encoding_time_observer_(encoding_time_observer) {
+                                                 stats_proxy)) {
+  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);
   encoder_thread_.Start();
   encoder_thread_.SetPriority(rtc::kHighPriority);
   module_process_thread_->RegisterModule(overuse_detector_.get());
@@ -70,8 +75,8 @@ VideoCaptureInput::~VideoCaptureInput() {
 void VideoCaptureInput::IncomingCapturedFrame(const VideoFrame& video_frame) {
   // TODO(pbos): Remove local rendering, it should be handled by the client code
   // if required.
-  if (local_renderer_)
-    local_renderer_->RenderFrame(video_frame, 0);
+  if (config_.local_renderer)
+    config_.local_renderer->RenderFrame(video_frame, 0);
 
   stats_proxy_->OnIncomingFrame(video_frame.width(), video_frame.height());
 
@@ -118,6 +123,13 @@ void VideoCaptureInput::IncomingCapturedFrame(const VideoFrame& video_frame) {
   capture_event_.Set();
 }
 
+void VideoCaptureInput::TriggerSetSendCodec(const VideoCodec& video_codec) {
+  CriticalSectionScoped cs(capture_cs_.get());
+  codec_settings_ = video_codec;
+  replace_codec_settings_ = true;
+  capture_event_.Set();
+}
+
 bool VideoCaptureInput::EncoderThreadFunction(void* obj) {
   return static_cast<VideoCaptureInput*>(obj)->EncoderProcess();
 }
@@ -126,18 +138,54 @@ bool VideoCaptureInput::EncoderProcess() {
   static const int kThreadWaitTimeMs = 100;
   int64_t capture_time = -1;
   if (capture_event_.Wait(kThreadWaitTimeMs)) {
-    if (rtc::AtomicOps::AcquireLoad(&stop_))
+    if (rtc::AtomicOps::AcquireLoad(&stop_)) {
+      if (encoder_registered_) {

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

Maybe set this to false after the external encoder has been de-registered?

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

We're only doing this on a shutdown, so we should never run EncoderProcess again
after this point.

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

Right, I was mostly thinking if some other method start using
encoder_registered_. It is probably good to keep it in sync.

+        vcm_->RegisterExternalEncoder(nullptr,
+                                      config_.encoder_settings.payload_type);
+      }
       return false;
+    }
 
     int64_t encode_start_time = -1;
     VideoFrame deliver_frame;
+    bool replace_codec_settings;
+    VideoCodec codec_settings;
     {
       CriticalSectionScoped cs(capture_cs_.get());
-      if (!captured_frame_.IsZeroSize()) {
+      replace_codec_settings = replace_codec_settings_;
+      if (replace_codec_settings) {
+        codec_settings = codec_settings_;
+        replace_codec_settings_ = false;
+      } else if (encoder_registered_ && !captured_frame_.IsZeroSize()) {
+        // Don't pick up frames before the encoder has been configured or during
+        // reconfigures. This prevents picking up a frame, initializing the
+        // encoder (during which several frames may be dropped), encoding the
+        // quite old frame and then encoding new frames, leaving a very visible
+        // gap between the first and second frame encoded with the new settings.

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

I don't understand this last long sentence.

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

Hope this is better. :)

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

A lot better!

         deliver_frame = captured_frame_;
         captured_frame_.Reset();
       }
     }
+    if (replace_codec_settings) {

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

I don't see how this can ever be true?

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

Line 155 makes it true if TriggerSetSendCodec have been called.

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

Ah, I misread line 158 to be "replace_codec_settings = false". Maybe call it
old_replace_codec_settings or something to make it a bit easier to read?

[pbos-webrtc, 2016/01/20 19:04:44]

Calling it new_codec_settings, they're still the new ones that need to be
applied.

+      if (!encoder_registered_) {
+        RTC_CHECK_EQ(VCM_OK, vcm_->RegisterExternalEncoder(
+                                 config_.encoder_settings.encoder,
+                                 config_.encoder_settings.payload_type,
+                                 config_.encoder_settings.internal_source));
+      }
+      // Optional only for tests that don't instantiate a full VideoSendStream.

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

Are you saying the following code is only executed by tests? Or that it doesn't
have to be executed by tests? How?

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

Not executed by tests. Comment updated.

+      // TODO(pbos): Mock VideoSendStream.

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

I think this TODO should be more detailed. You shouldn't use mocks in production
code?

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

Updated TODO

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

Maybe it actually makes sense to do this now so that we can properly test this
class. I assume that now all tests are passing a nullptr as video_send_stream,
which means we actually don't test this path?

[pbos-webrtc, 2016/01/20 19:04:45]

All quality tests and otherwise will start failing if this doesn't hold, since
we won't reconfigure the encoder.

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

Ok, good, but if we think it's worth adding a todo here, isn't it worth taking
the time to actually add the test? Seems worthwhile to me to add tests when we
anyway are making a rather big change.

+      if (video_send_stream_)
+        video_send_stream_->SetSendCodec(codec_settings);

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

Shouldn't this call directly into the vcm? Seems a bit odd that it calls the vcm
directly for some things and then goes via video_send_stream_ for other. I get
the feeling like we're mixing layers here somehow. Should the VideoCaptureInput
be allowed to talk to the vcm at all?

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

It needs to configure vie_channel, vie_channel and stats_proxy which
VideoSendStream already has. Is this too ugly, should I move vie_encoder and
vie_channel down here? Then I'll have to have both frame_callback (for testing)
and vie_encoder for the actual object (when not testing).

I'm not sure I like the alternative right now.

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

I guess one of the vie_channel you mentioned should be vie_encoder?

Not sure what is best. Maybe we should accept a dependency from
VideoCaptureInput on VideoSendStream? I dislike it because of the loop we get,
where both depend on each other.

What you really want is to post the call to SetSendCodec onto the encoder
thread. If that would have been possible that would have been your preferred
solution, right? Or would you still have involved VideoCaptureInput?

I'm mostly trying to understand how this needs to work.

[pbos-webrtc, 2016/01/20 19:04:45]

I still think that VideoCaptureInput should be aware of whether the codec is
initialized or not. Maybe the encoder thread should be broken out of
VideoCaptureInput and put in VideoSendStream?

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

Yes, that might be an option? I think I would like that better. Let's here what
Magnus thinks.

+      if (config_.suspend_below_min_bitrate)

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

Doesn't this mean that suspend below min bitrate is enabled as a feature? Seems
like the line below actually suspends the video? Maybe just a bad API name :/

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

Bad API name, it turns on the option.

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

Acknowledged.

+        vcm_->SuspendBelowMinBitrate();
+      encoder_registered_ = true;
+      // We could still have a frame to pick up here since none was picked up
+      // while the encoder was being registered/configured.
+      // Flag event again and wake up as soon as possible.
+      capture_event_.Set();
+      return true;
+    }
     if (!deliver_frame.IsZeroSize()) {
       capture_time = deliver_frame.render_time_ms();
       encode_start_time = Clock::GetRealTimeClock()->TimeInMilliseconds();
@@ -148,8 +196,8 @@ bool VideoCaptureInput::EncoderProcess() {
       int encode_time_ms = static_cast<int>(
           Clock::GetRealTimeClock()->TimeInMilliseconds() - encode_start_time);
       stats_proxy_->OnEncodedFrame(encode_time_ms);
-      if (encoding_time_observer_) {
-        encoding_time_observer_->OnReportEncodedTime(
+      if (config_.encoding_time_observer) {
+        config_.encoding_time_observer->OnReportEncodedTime(
             deliver_frame.ntp_time_ms(), encode_time_ms);
       }
     }