Make SendCodec() lock-free.

Fetching the current codec for sake of gathering stats, is frequently blocked since it's done by acquiring the same lock as is held while encoding frames.  This can mean tens of milliseconds.

To improve this, I'm taking advantage of the fact that the codec information is set on the same thread as is used to query the information.  This means that locking isn't needed for querying this information.  I'm adding checks to make sure debug builds will crash if this isn't followed.

An alternative to this approach could be to add one more lock that is specifically used for the codec information variable.  This would also decouple querying codec information from the encoder itself, but still requires a lock.

This patch depends on making ThreadChecker part of rtc_base_approved:
https://webrtc-codereview.appspot.com/40539004/

BUG=2822
R=mflodman@webrtc.org, pthatcher@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/37779004

Cr-Commit-Position: refs/heads/master@{#8435}
git-svn-id: http://webrtc.googlecode.com/svn/trunk@8435 4adac7df-926f-26a2-2b94-8c16560cd09d
diff --git a/talk/media/webrtc/webrtcvideocapturer.cc b/talk/media/webrtc/webrtcvideocapturer.cc
index 259f53a..aaa6f1e 100644
--- a/talk/media/webrtc/webrtcvideocapturer.cc
+++ b/talk/media/webrtc/webrtcvideocapturer.cc
@@ -34,6 +34,8 @@
 #ifdef HAVE_WEBRTC_VIDEO
 #include "talk/media/webrtc/webrtcvideoframe.h"
 #include "talk/media/webrtc/webrtcvideoframefactory.h"
+#include "webrtc/base/bind.h"
+#include "webrtc/base/checks.h"
 #include "webrtc/base/criticalsection.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/safe_conversions.h"
@@ -127,14 +129,16 @@
 WebRtcVideoCapturer::WebRtcVideoCapturer()
     : factory_(new WebRtcVcmFactory),
       module_(NULL),
-      captured_frames_(0) {
+      captured_frames_(0),
+      start_thread_(nullptr) {
   set_frame_factory(new WebRtcVideoFrameFactory());
 }
 
 WebRtcVideoCapturer::WebRtcVideoCapturer(WebRtcVcmFactoryInterface* factory)
     : factory_(factory),
       module_(NULL),
-      captured_frames_(0) {
+      captured_frames_(0),
+      start_thread_(nullptr) {
   set_frame_factory(new WebRtcVideoFrameFactory());
 }
 
@@ -145,6 +149,7 @@
 }
 
 bool WebRtcVideoCapturer::Init(const Device& device) {
+  DCHECK(!start_thread_);
   if (module_) {
     LOG(LS_ERROR) << "The capturer is already initialized";
     return false;
@@ -221,6 +226,7 @@
 }
 
 bool WebRtcVideoCapturer::Init(webrtc::VideoCaptureModule* module) {
+  DCHECK(!start_thread_);
   if (module_) {
     LOG(LS_ERROR) << "The capturer is already initialized";
     return false;
@@ -271,12 +277,15 @@
   }
 
   rtc::CritScope cs(&critical_section_stopping_);
-  // TODO(hellner): weird to return failure when it is in fact actually running.
   if (IsRunning()) {
     LOG(LS_ERROR) << "The capturer is already running";
     return CS_FAILED;
   }
 
+  DCHECK(!start_thread_);
+
+  start_thread_ = rtc::Thread::Current();
+
   SetCaptureFormat(&capture_format);
 
   webrtc::VideoCaptureCapability cap;
@@ -290,6 +299,7 @@
   module_->RegisterCaptureDataCallback(*this);
   if (module_->StartCapture(cap) != 0) {
     LOG(LS_ERROR) << "Camera '" << camera_id << "' failed to start";
+    start_thread_ = nullptr;
     return CS_FAILED;
   }
 
@@ -310,6 +320,7 @@
 void WebRtcVideoCapturer::Stop() {
   rtc::CritScope cs(&critical_section_stopping_);
   if (IsRunning()) {
+    DCHECK(start_thread_);
     rtc::Thread::Current()->Clear(this);
     module_->StopCapture();
     module_->DeRegisterCaptureDataCallback();
@@ -322,6 +333,7 @@
                  << drop_ratio << "%";
   }
   SetCaptureFormat(NULL);
+  start_thread_ = nullptr;
 }
 
 bool WebRtcVideoCapturer::IsRunning() {
@@ -363,16 +375,18 @@
                  << ". Expected format " << GetCaptureFormat()->ToString();
   }
 
-  // Signal down stream components on captured frame.
-  // The CapturedFrame class doesn't support planes. We have to ExtractBuffer
-  // to one block for it.
-  size_t length =
-      webrtc::CalcBufferSize(webrtc::kI420, sample.width(), sample.height());
-  capture_buffer_.resize(length);
-  // TODO(ronghuawu): Refactor the WebRtcCapturedFrame to avoid memory copy.
-  webrtc::ExtractBuffer(sample, length, &capture_buffer_[0]);
-  WebRtcCapturedFrame frame(sample, &capture_buffer_[0], length);
-  SignalFrameCaptured(this, &frame);
+  if (start_thread_->IsCurrent()) {
+    SignalFrameCapturedOnStartThread(&sample);
+  } else {
+    // This currently happens on with at least VideoCaptureModuleV4L2 and
+    // possibly other implementations of WebRTC's VideoCaptureModule.
+    // In order to maintain the threading contract with the upper layers and
+    // consistency with other capturers such as in Chrome, we need to do a
+    // thread hop.
+    start_thread_->Invoke<void>(
+        rtc::Bind(&WebRtcVideoCapturer::SignalFrameCapturedOnStartThread,
+                  this, &sample));
+  }
 }
 
 void WebRtcVideoCapturer::OnCaptureDelayChanged(const int32_t id,
@@ -380,6 +394,22 @@
   LOG(LS_INFO) << "Capture delay changed to " << delay << " ms";
 }
 
+void WebRtcVideoCapturer::SignalFrameCapturedOnStartThread(
+    webrtc::I420VideoFrame* frame) {
+  DCHECK(start_thread_->IsCurrent());
+  // Signal down stream components on captured frame.
+  // The CapturedFrame class doesn't support planes. We have to ExtractBuffer
+  // to one block for it.
+  size_t length =
+      webrtc::CalcBufferSize(webrtc::kI420, frame->width(), frame->height());
+  capture_buffer_.resize(length);
+  // TODO(magjed): Refactor the WebRtcCapturedFrame to avoid memory copy or
+  // take over ownership of the buffer held by |frame| if that's possible.
+  webrtc::ExtractBuffer(*frame, length, &capture_buffer_[0]);
+  WebRtcCapturedFrame webrtc_frame(*frame, &capture_buffer_[0], length);
+  SignalFrameCaptured(this, &webrtc_frame);
+}
+
 // WebRtcCapturedFrame
 WebRtcCapturedFrame::WebRtcCapturedFrame(const webrtc::I420VideoFrame& sample,
                                          void* buffer,
diff --git a/talk/media/webrtc/webrtcvideocapturer.h b/talk/media/webrtc/webrtcvideocapturer.h
index a529ddd..c0f7807 100644
--- a/talk/media/webrtc/webrtcvideocapturer.h
+++ b/talk/media/webrtc/webrtcvideocapturer.h
@@ -85,10 +85,19 @@
   virtual void OnCaptureDelayChanged(const int32_t id,
                                      const int32_t delay);
 
+  // Used to signal captured frames on the same thread as invoked Start().
+  // With WebRTC's current VideoCapturer implementations, this will mean a
+  // thread hop, but in other implementations (e.g. Chrome) it will be called
+  // directly from OnIncomingCapturedFrame.
+  // TODO(tommi): Remove this workaround when we've updated the WebRTC capturers
+  // to follow the same contract.
+  void SignalFrameCapturedOnStartThread(webrtc::I420VideoFrame* frame);
+
   rtc::scoped_ptr<WebRtcVcmFactoryInterface> factory_;
   webrtc::VideoCaptureModule* module_;
   int captured_frames_;
   std::vector<uint8_t> capture_buffer_;
+  rtc::Thread* start_thread_;  // Set in Start(), unset in Stop();
 
   // Critical section to avoid Stop during an OnIncomingCapturedFrame callback.
   rtc::CriticalSection critical_section_stopping_;
diff --git a/talk/media/webrtc/webrtcvideoengine.cc b/talk/media/webrtc/webrtcvideoengine.cc
index f9508e1..d4db7b7 100644
--- a/talk/media/webrtc/webrtcvideoengine.cc
+++ b/talk/media/webrtc/webrtcvideoengine.cc
@@ -3904,6 +3904,7 @@
 bool WebRtcVideoMediaChannel::SetSendParams(
     WebRtcVideoChannelSendInfo* send_channel,
     const VideoSendParams& send_params) {
+  ASSERT(engine()->worker_thread()->IsCurrent());
   const int channel_id = send_channel->channel_id();
 
   MaybeRegisterExternalEncoder(send_channel, send_params.codec);
diff --git a/webrtc/modules/video_coding/main/interface/video_coding.h b/webrtc/modules/video_coding/main/interface/video_coding.h
index 02b36b1..bba68c5 100644
--- a/webrtc/modules/video_coding/main/interface/video_coding.h
+++ b/webrtc/modules/video_coding/main/interface/video_coding.h
@@ -11,6 +11,16 @@
 #ifndef WEBRTC_MODULES_INTERFACE_VIDEO_CODING_H_
 #define WEBRTC_MODULES_INTERFACE_VIDEO_CODING_H_
 
+#if defined(WEBRTC_WIN)
+// This is a workaround on Windows due to the fact that some Windows
+// headers define CreateEvent as a macro to either CreateEventW or CreateEventA.
+// This can cause problems since we use that name as well and could
+// declare them as one thing here whereas in another place a windows header
+// may have been included and then implementing CreateEvent() causes compilation
+// errors.  So for consistency, we include the main windows header here.
+#include <windows.h>
+#endif
+
 #include "webrtc/common_video/interface/i420_video_frame.h"
 #include "webrtc/modules/interface/module.h"
 #include "webrtc/modules/interface/module_common_types.h"
@@ -112,6 +122,8 @@
     // encoding-related settings by calling this function.
     // For instance, a send codec has to be registered again.
     //
+    // NOTE: Must be called on the thread that constructed the VCM instance.
+    //
     // Return value      : VCM_OK, on success.
     //                     < 0,         on error.
     virtual int32_t InitializeSender() = 0;
@@ -119,6 +131,8 @@
     // Registers a codec to be used for encoding. Calling this
     // API multiple times overwrites any previously registered codecs.
     //
+    // NOTE: Must be called on the thread that constructed the VCM instance.
+    //
     // Input:
     //      - sendCodec      : Settings for the codec to be registered.
     //      - numberOfCores  : The number of cores the codec is allowed
@@ -132,6 +146,17 @@
                                             uint32_t numberOfCores,
                                             uint32_t maxPayloadSize) = 0;
 
+    // Get the current send codec in use.
+    //
+    // If a codec has not been set yet, the |id| property of the return value
+    // will be 0 and |name| empty.
+    //
+    // NOTE: This method intentionally does not hold locks and minimizes data
+    // copying.  It must be called on the thread where the VCM was constructed.
+    virtual const VideoCodec& GetSendCodec() const = 0;
+
+    // DEPRECATED: Use GetSendCodec() instead.
+    //
     // API to get the current send codec in use.
     //
     // Input:
@@ -139,12 +164,20 @@
     //
     // Return value      : VCM_OK, on success.
     //                     < 0,         on error.
+    //
+    // NOTE: The returned codec information is not guaranteed to be current when
+    // the call returns.  This method acquires a lock that is aligned with
+    // video encoding, so it should be assumed to be allowed to block for
+    // several milliseconds.
     virtual int32_t SendCodec(VideoCodec* currentSendCodec) const = 0;
 
+    // DEPRECATED: Use GetSendCodec() instead.
+    //
     // API to get the current send codec type
     //
     // Return value      : Codec type, on success.
     //                     kVideoCodecUnknown, on error or if no send codec is set
+    // NOTE: Same notes apply as for SendCodec() above.
     virtual VideoCodecType SendCodec() const = 0;
 
     // Register an external encoder object. This can not be used together with
diff --git a/webrtc/modules/video_coding/main/source/video_coding_impl.cc b/webrtc/modules/video_coding/main/source/video_coding_impl.cc
index a90ab07..89eb29b 100644
--- a/webrtc/modules/video_coding/main/source/video_coding_impl.cc
+++ b/webrtc/modules/video_coding/main/source/video_coding_impl.cc
@@ -111,12 +111,18 @@
     return sender_->RegisterSendCodec(sendCodec, numberOfCores, maxPayloadSize);
   }
 
-  virtual int32_t SendCodec(VideoCodec* currentSendCodec) const OVERRIDE {
-    return sender_->SendCodec(currentSendCodec);
+  virtual const VideoCodec& GetSendCodec() const OVERRIDE {
+    return sender_->GetSendCodec();
   }
 
+  // DEPRECATED.
+  virtual int32_t SendCodec(VideoCodec* currentSendCodec) const OVERRIDE {
+    return sender_->SendCodecBlocking(currentSendCodec);
+  }
+
+  // DEPRECATED.
   virtual VideoCodecType SendCodec() const OVERRIDE {
-    return sender_->SendCodec();
+    return sender_->SendCodecBlocking();
   }
 
   virtual int32_t RegisterExternalEncoder(VideoEncoder* externalEncoder,
@@ -351,6 +357,8 @@
 
  private:
   EncodedImageCallbackWrapper post_encode_callback_;
+  // TODO(tommi): Change sender_ and receiver_ to be non pointers
+  // (construction is 1 alloc instead of 3).
   scoped_ptr<vcm::VideoSender> sender_;
   scoped_ptr<vcm::VideoReceiver> receiver_;
   scoped_ptr<EventFactory> own_event_factory_;
diff --git a/webrtc/modules/video_coding/main/source/video_coding_impl.h b/webrtc/modules/video_coding/main/source/video_coding_impl.h
index 39e763f..ed38bd5 100644
--- a/webrtc/modules/video_coding/main/source/video_coding_impl.h
+++ b/webrtc/modules/video_coding/main/source/video_coding_impl.h
@@ -16,6 +16,7 @@
 #include <vector>
 
 #include "webrtc/base/thread_annotations.h"
+#include "webrtc/base/thread_checker.h"
 #include "webrtc/modules/video_coding/main/source/codec_database.h"
 #include "webrtc/modules/video_coding/main/source/frame_buffer.h"
 #include "webrtc/modules/video_coding/main/source/generic_decoder.h"
@@ -62,12 +63,25 @@
   int32_t InitializeSender();
 
   // Register the send codec to be used.
+  // This method must be called on the construction thread.
   int32_t RegisterSendCodec(const VideoCodec* sendCodec,
                             uint32_t numberOfCores,
                             uint32_t maxPayloadSize);
+  // Non-blocking access to the currently active send codec configuration.
+  // Must be called from the same thread as the VideoSender instance was
+  // created on.
+  const VideoCodec& GetSendCodec() const;
 
-  int32_t SendCodec(VideoCodec* currentSendCodec) const;
-  VideoCodecType SendCodec() const;
+  // Get a copy of the currently configured send codec.
+  // This method acquires a lock to copy the current configuration out,
+  // so it can block and the returned information is not guaranteed to be
+  // accurate upon return.  Consider using GetSendCodec() instead and make
+  // decisions on that thread with regards to the current codec.
+  int32_t SendCodecBlocking(VideoCodec* currentSendCodec) const;
+
+  // Same as SendCodecBlocking.  Try to use GetSendCodec() instead.
+  VideoCodecType SendCodecBlocking() const;
+
   int32_t RegisterExternalEncoder(VideoEncoder* externalEncoder,
                                   uint8_t payloadType,
                                   bool internalSource);
@@ -124,6 +138,10 @@
   bool frame_dropper_enabled_;
   VCMProcessTimer _sendStatsTimer;
 
+  // Must be accessed on the construction thread of VideoSender.
+  VideoCodec current_codec_;
+  rtc::ThreadChecker main_thread_;
+
   VCMQMSettingsCallback* qm_settings_callback_;
   VCMProtectionCallback* protection_callback_;
 };
diff --git a/webrtc/modules/video_coding/main/source/video_sender.cc b/webrtc/modules/video_coding/main/source/video_sender.cc
index b2dd23e..24240e2 100644
--- a/webrtc/modules/video_coding/main/source/video_sender.cc
+++ b/webrtc/modules/video_coding/main/source/video_sender.cc
@@ -12,6 +12,7 @@
 
 #include <algorithm>  // std::max
 
+#include "webrtc/base/checks.h"
 #include "webrtc/common_video/libyuv/include/webrtc_libyuv.h"
 #include "webrtc/modules/video_coding/codecs/interface/video_codec_interface.h"
 #include "webrtc/modules/video_coding/main/source/encoded_frame.h"
@@ -72,8 +73,10 @@
       _codecDataBase(),
       frame_dropper_enabled_(true),
       _sendStatsTimer(1000, clock_),
+      current_codec_(),
       qm_settings_callback_(NULL),
-      protection_callback_(NULL) {}
+      protection_callback_(NULL) {
+}
 
 VideoSender::~VideoSender() {
   delete _sendCritSect;
@@ -86,13 +89,8 @@
     _sendStatsTimer.Processed();
     CriticalSectionScoped cs(process_crit_sect_.get());
     if (_sendStatsCallback != NULL) {
-      uint32_t bitRate;
-      uint32_t frameRate;
-      {
-        CriticalSectionScoped cs(_sendCritSect);
-        bitRate = _mediaOpt.SentBitRate();
-        frameRate = _mediaOpt.SentFrameRate();
-      }
+      uint32_t bitRate = _mediaOpt.SentBitRate();
+      uint32_t frameRate = _mediaOpt.SentFrameRate();
       _sendStatsCallback->SendStatistics(bitRate, frameRate);
     }
   }
@@ -102,6 +100,7 @@
 
 // Reset send side to initial state - all components
 int32_t VideoSender::InitializeSender() {
+  DCHECK(main_thread_.CalledOnValidThread());
   CriticalSectionScoped cs(_sendCritSect);
   _codecDataBase.ResetSender();
   _encoder = NULL;
@@ -118,6 +117,7 @@
 int32_t VideoSender::RegisterSendCodec(const VideoCodec* sendCodec,
                                        uint32_t numberOfCores,
                                        uint32_t maxPayloadSize) {
+  DCHECK(main_thread_.CalledOnValidThread());
   CriticalSectionScoped cs(_sendCritSect);
   if (sendCodec == NULL) {
     return VCM_PARAMETER_ERROR;
@@ -129,6 +129,9 @@
   // Update encoder regardless of result to make sure that we're not holding on
   // to a deleted instance.
   _encoder = _codecDataBase.GetEncoder();
+  // Cache the current codec here so they can be fetched from this thread
+  // without requiring the _sendCritSect lock.
+  current_codec_ = *sendCodec;
 
   if (!ret) {
     LOG(LS_ERROR) << "Failed to initialize the encoder with payload name "
@@ -162,20 +165,21 @@
   return VCM_OK;
 }
 
-// Get current send codec
-int32_t VideoSender::SendCodec(VideoCodec* currentSendCodec) const {
-  CriticalSectionScoped cs(_sendCritSect);
+const VideoCodec& VideoSender::GetSendCodec() const {
+  DCHECK(main_thread_.CalledOnValidThread());
+  return current_codec_;
+}
 
+int32_t VideoSender::SendCodecBlocking(VideoCodec* currentSendCodec) const {
+  CriticalSectionScoped cs(_sendCritSect);
   if (currentSendCodec == NULL) {
     return VCM_PARAMETER_ERROR;
   }
   return _codecDataBase.SendCodec(currentSendCodec) ? 0 : -1;
 }
 
-// Get the current send codec type
-VideoCodecType VideoSender::SendCodec() const {
+VideoCodecType VideoSender::SendCodecBlocking() const {
   CriticalSectionScoped cs(_sendCritSect);
-
   return _codecDataBase.SendCodec();
 }
 
@@ -214,7 +218,6 @@
 // TODO(andresp): Make const once media_opt is thread-safe and this has a
 // pointer to it.
 int32_t VideoSender::SentFrameCount(VCMFrameCount* frameCount) {
-  CriticalSectionScoped cs(_sendCritSect);
   *frameCount = _mediaOpt.SentFrameCount();
   return VCM_OK;
 }
@@ -400,8 +403,6 @@
 }
 
 int VideoSender::SetSenderNackMode(SenderNackMode mode) {
-  CriticalSectionScoped cs(_sendCritSect);
-
   switch (mode) {
     case VideoCodingModule::kNackNone:
       _mediaOpt.EnableProtectionMethod(false, media_optimization::kNack);
@@ -411,7 +412,6 @@
       break;
     case VideoCodingModule::kNackSelective:
       return VCM_NOT_IMPLEMENTED;
-      break;
   }
   return VCM_OK;
 }
@@ -421,7 +421,6 @@
 }
 
 int VideoSender::SetSenderFEC(bool enable) {
-  CriticalSectionScoped cs(_sendCritSect);
   _mediaOpt.EnableProtectionMethod(enable, media_optimization::kFec);
   return VCM_OK;
 }
@@ -439,17 +438,12 @@
 }
 
 void VideoSender::SuspendBelowMinBitrate() {
-  CriticalSectionScoped cs(_sendCritSect);
-  VideoCodec current_send_codec;
-  if (SendCodec(&current_send_codec) != 0) {
-    assert(false);  // Must set a send codec before SuspendBelowMinBitrate.
-    return;
-  }
+  DCHECK(main_thread_.CalledOnValidThread());
   int threshold_bps;
-  if (current_send_codec.numberOfSimulcastStreams == 0) {
-    threshold_bps = current_send_codec.minBitrate * 1000;
+  if (current_codec_.numberOfSimulcastStreams == 0) {
+    threshold_bps = current_codec_.minBitrate * 1000;
   } else {
-    threshold_bps = current_send_codec.simulcastStream[0].minBitrate * 1000;
+    threshold_bps = current_codec_.simulcastStream[0].minBitrate * 1000;
   }
   // Set the hysteresis window to be at 10% of the threshold, but at least
   // 10 kbps.
diff --git a/webrtc/video_engine/vie_encoder.cc b/webrtc/video_engine/vie_encoder.cc
index dae03f0..9d4fdb2 100644
--- a/webrtc/video_engine/vie_encoder.cc
+++ b/webrtc/video_engine/vie_encoder.cc
@@ -414,9 +414,7 @@
 }
 
 int32_t ViEEncoder::GetEncoder(VideoCodec* video_codec) {
-  if (vcm_.SendCodec(video_codec) != 0) {
-    return -1;
-  }
+  *video_codec = vcm_.GetSendCodec();
   return 0;
 }