Initialize VideoEncoder objects asynchronously.

webrtc/video/video_capture_input.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.
  */
 
 #include "webrtc/video/video_capture_input.h"
 
 #include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/trace_event.h"
 #include "webrtc/modules/include/module_common_types.h"
 #include "webrtc/modules/utility/include/process_thread.h"
 #include "webrtc/modules/video_capture/video_capture_factory.h"
 #include "webrtc/modules/video_processing/include/video_processing.h"
 #include "webrtc/modules/video_render/video_render_defines.h"
 #include "webrtc/system_wrappers/include/clock.h"
 #include "webrtc/system_wrappers/include/critical_section_wrapper.h"
 #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 {
 
 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),
       last_captured_timestamp_(0),
       delta_ntp_internal_ms_(
           Clock::GetRealTimeClock()->CurrentNtpInMilliseconds() -
           TickTime::MillisecondTimestamp()),
       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());
 }
 
 VideoCaptureInput::~VideoCaptureInput() {
   module_process_thread_->DeRegisterModule(overuse_detector_.get());
 
   // Stop the thread.
   rtc::AtomicOps::ReleaseStore(&stop_, 1);
   capture_event_.Set();
   encoder_thread_.Stop();
 }
 
 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());
 
   VideoFrame incoming_frame = video_frame;
 
   if (incoming_frame.ntp_time_ms() != 0) {
     // If a NTP time stamp is set, this is the time stamp we will use.
     incoming_frame.set_render_time_ms(incoming_frame.ntp_time_ms() -
                                       delta_ntp_internal_ms_);
   } else {  // NTP time stamp not set.
@@ skipping 26 matching lines @@
   overuse_detector_->FrameCaptured(captured_frame_.width(),
                                    captured_frame_.height(),
                                    captured_frame_.render_time_ms());
 
   TRACE_EVENT_ASYNC_BEGIN1("webrtc", "Video", video_frame.render_time_ms(),
                            "render_time", video_frame.render_time_ms());
 
   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();
 }
 
 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();
       frame_callback_->DeliverFrame(deliver_frame);
     }
     // Update the overuse detector with the duration.
     if (encode_start_time != -1) {
       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);
       }
     }
   }
   // We're done!
   if (capture_time != -1) {
     overuse_detector_->FrameSent(capture_time);
   }
   return true;
 }
 
 }  // namespace internal
 }  // namespace webrtc