Reland 8631 "Speculative revert of 8631 "Remove lock from Bitrat..."

> Speculative revert of 8631 "Remove lock from Bitrate() and FrameRate() in Video..."
> 
> We ran into the alignment problem on Mac 10.9 debug again.  This is the only CL I see in the range that adds an rtc::CriticalSection, so I'm trying out reverting it before attempting another roll.
> 
> > Remove lock from Bitrate() and FrameRate() in VideoSender.
> > These methods are called on the VideoSender's construction thread, which is the same thread as modifies the value of _encoder.  It's therefore safe to not require a lock to access _encoder on this thread.
> > 
> > I'm making access to the rate variables from VCMGenericEncoder, thread safe, by using a lock that's not associated with the encoder.  There should be little to no contention there.  While modifying VCMGenericEncoder, I noticed that a couple of member variables weren't needed, so I removed them.
> > 
> > The reason for this change is that getStats is currently contending with the encoder when Bitrate() is called. On my machine, this means that getStats can take about 25-30ms instead of ~1ms.
> > 
> > Also adding some documentation for other methods and a suggestion for how we could avoid contention between the encoder and the network thread.
> > 
> > BUG=2822
> > R=mflodman@webrtc.org
> > 
> > Review URL: https://webrtc-codereview.appspot.com/43479004
> 
> TBR=tommi@webrtc.org
> 
> Review URL: https://webrtc-codereview.appspot.com/45529004

TBR=tommi@webrtc.org

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

Cr-Commit-Position: refs/heads/master@{#8645}
git-svn-id: http://webrtc.googlecode.com/svn/trunk@8645 4adac7df-926f-26a2-2b94-8c16560cd09d

webrtc/modules/video_coding/main/source/generic_encoder.cc

@@ -60,11 +60,9 @@ VCMGenericEncoder::VCMGenericEncoder(VideoEncoder* encoder,
                                      bool internalSource)
     : encoder_(encoder),
       rate_observer_(rate_observer),
-      _codecType(kVideoCodecUnknown),
+      bit_rate_(0),
-      _VCMencodedFrameCallback(NULL),
+      frame_rate_(0),
-      _bitRate(0),
+      internal_source_(internalSource) {
-      _frameRate(0),
-      _internalSource(internalSource) {
 }
 
 VCMGenericEncoder::~VCMGenericEncoder()
@@ -73,9 +71,12 @@ VCMGenericEncoder::~VCMGenericEncoder()
 
 int32_t VCMGenericEncoder::Release()
 {
-    _bitRate = 0;
+    {
-    _frameRate = 0;
+      rtc::CritScope lock(&rates_lock_);
-    _VCMencodedFrameCallback = NULL;
+      bit_rate_ = 0;
+      frame_rate_ = 0;
+    }
+
     return encoder_->Release();
 }
 
@@ -84,9 +85,12 @@ VCMGenericEncoder::InitEncode(const VideoCodec* settings,
                               int32_t numberOfCores,
                               size_t maxPayloadSize)
 {
-    _bitRate = settings->startBitrate * 1000;
+    {
-    _frameRate = settings->maxFramerate;
+      rtc::CritScope lock(&rates_lock_);
-    _codecType = settings->codecType;
+      bit_rate_ = settings->startBitrate * 1000;
+      frame_rate_ = settings->maxFramerate;
+    }
+
     if (encoder_->InitEncode(settings, numberOfCores, maxPayloadSize) != 0) {
       LOG(LS_ERROR) << "Failed to initialize the encoder associated with "
                        "payload name: " << settings->plName;
@@ -120,8 +124,13 @@ VCMGenericEncoder::SetRates(uint32_t newBitRate, uint32_t frameRate)
     {
         return ret;
     }
-    _bitRate = newBitRate;
+
-    _frameRate = frameRate;
+    {
+      rtc::CritScope lock(&rates_lock_);
+      bit_rate_ = newBitRate;
+      frame_rate_ = frameRate;
+    }
+
     if (rate_observer_ != nullptr)
       rate_observer_->OnSetRates(newBitRate, frameRate);
     return VCM_OK;
@@ -140,12 +149,14 @@ VCMGenericEncoder::CodecConfigParameters(uint8_t* buffer, int32_t size)
 
 uint32_t VCMGenericEncoder::BitRate() const
 {
-    return _bitRate;
+    rtc::CritScope lock(&rates_lock_);
+    return bit_rate_;
 }
 
 uint32_t VCMGenericEncoder::FrameRate() const
 {
-    return _frameRate;
+    rtc::CritScope lock(&rates_lock_);
+    return frame_rate_;
 }
 
 int32_t
@@ -166,15 +177,14 @@ int32_t VCMGenericEncoder::RequestFrame(
 int32_t
 VCMGenericEncoder::RegisterEncodeCallback(VCMEncodedFrameCallback* VCMencodedFrameCallback)
 {
-   _VCMencodedFrameCallback = VCMencodedFrameCallback;
+    VCMencodedFrameCallback->SetInternalSource(internal_source_);
-   _VCMencodedFrameCallback->SetInternalSource(_internalSource);
+    return encoder_->RegisterEncodeCompleteCallback(VCMencodedFrameCallback);
-   return encoder_->RegisterEncodeCompleteCallback(_VCMencodedFrameCallback);
 }
 
 bool
 VCMGenericEncoder::InternalSource() const
 {
-    return _internalSource;
+    return internal_source_;
 }
 
  /***************************

webrtc/modules/video_coding/main/source/generic_encoder.h

@@ -16,6 +16,7 @@
 
 #include <stdio.h>
 
+#include "webrtc/base/criticalsection.h"
 #include "webrtc/base/scoped_ptr.h"
 
 namespace webrtc {
@@ -102,6 +103,9 @@ public:
     * Set new target bitrate (bits/s) and framerate.
     * Return Value: new bit rate if OK, otherwise <0s.
     */
+    // TODO(tommi): We could replace BitRate and FrameRate below with a GetRates
+    // method that matches SetRates. For fetching current rates, we'd then only
+    // grab the lock once instead of twice.
     int32_t SetRates(uint32_t target_bitrate, uint32_t frameRate);
     /**
     * Set a new packet loss rate and a new round-trip time in milliseconds.
@@ -132,11 +136,10 @@ public:
 private:
     VideoEncoder* const encoder_;
     VideoEncoderRateObserver* const rate_observer_;
-    VideoCodecType              _codecType;
+    uint32_t bit_rate_;
-    VCMEncodedFrameCallback*    _VCMencodedFrameCallback;
+    uint32_t frame_rate_;
-    uint32_t                    _bitRate;
+    const bool internal_source_;
-    uint32_t                    _frameRate;
+    mutable rtc::CriticalSection rates_lock_;
-    bool                        _internalSource;
 }; // end of VCMGenericEncoder class
 
 }  // namespace webrtc

webrtc/modules/video_coding/main/source/video_sender.cc

@@ -193,6 +193,8 @@ VideoCodecType VideoSender::SendCodecBlocking() const {
 int32_t VideoSender::RegisterExternalEncoder(VideoEncoder* externalEncoder,
                                              uint8_t payloadType,
                                              bool internalSource /*= false*/) {
+  DCHECK(main_thread_.CalledOnValidThread());
+
   CriticalSectionScoped cs(_sendCritSect);
 
   if (externalEncoder == NULL) {
@@ -229,7 +231,10 @@ int32_t VideoSender::SentFrameCount(VCMFrameCount* frameCount) {
 
 // Get encode bitrate
 int VideoSender::Bitrate(unsigned int* bitrate) const {
-  CriticalSectionScoped cs(_sendCritSect);
+  DCHECK(main_thread_.CalledOnValidThread());
+  // Since we're running on the thread that's the only thread known to modify
+  // the value of _encoder, we don't need to grab the lock here.
+
   // return the bit rate which the encoder is set to
   if (!_encoder) {
     return VCM_UNINITIALIZED;
@@ -240,7 +245,10 @@ int VideoSender::Bitrate(unsigned int* bitrate) const {
 
 // Get encode frame rate
 int VideoSender::FrameRate(unsigned int* framerate) const {
-  CriticalSectionScoped cs(_sendCritSect);
+  DCHECK(main_thread_.CalledOnValidThread());
+  // Since we're running on the thread that's the only thread known to modify
+  // the value of _encoder, we don't need to grab the lock here.
+
   // input frame rate, not compensated
   if (!_encoder) {
     return VCM_UNINITIALIZED;
@@ -249,32 +257,33 @@ int VideoSender::FrameRate(unsigned int* framerate) const {
   return 0;
 }
 
-// Set channel parameters
 int32_t VideoSender::SetChannelParameters(uint32_t target_bitrate,
                                           uint8_t lossRate,
                                           int64_t rtt) {
-  int32_t ret = 0;
+  // TODO(tommi,mflodman): This method is called on the network thread via the
-  {
+  // OnNetworkChanged event (ViEEncoder::OnNetworkChanged). Could we instead
+  // post the updated information to the encoding thread and not grab a lock
+  // here?  This effectively means that the network thread will be blocked for
+  // as much as frame encoding period.
+
   CriticalSectionScoped sendCs(_sendCritSect);
-    uint32_t targetRate = _mediaOpt.SetTargetRates(target_bitrate,
+  uint32_t target_rate = _mediaOpt.SetTargetRates(target_bitrate,
                                                   lossRate,
                                                   rtt,
                                                   protection_callback_,
                                                   qm_settings_callback_);
+  uint32_t input_frame_rate = _mediaOpt.InputFrameRate();
+
+  int32_t ret = VCM_UNINITIALIZED;
+  static_assert(VCM_UNINITIALIZED < 0, "VCM_UNINITIALIZED must be negative.");
+
   if (_encoder != NULL) {
     ret = _encoder->SetChannelParameters(lossRate, rtt);
-      if (ret < 0) {
+    if (ret >= 0) {
-        return ret;
+      ret = _encoder->SetRates(target_rate, input_frame_rate);
     }
-      ret = (int32_t)_encoder->SetRates(targetRate, _mediaOpt.InputFrameRate());
-      if (ret < 0) {
-        return ret;
   }
-    } else {
+  return ret;
-      return VCM_UNINITIALIZED;
-    }  // encoder
-  }    // send side
-  return VCM_OK;
 }
 
 int32_t VideoSender::RegisterTransportCallback(
@@ -300,6 +309,9 @@ int32_t VideoSender::RegisterSendStatisticsCallback(
 int32_t VideoSender::RegisterVideoQMCallback(
     VCMQMSettingsCallback* qm_settings_callback) {
   CriticalSectionScoped cs(_sendCritSect);
+  DCHECK(qm_settings_callback_ == qm_settings_callback ||
+         !qm_settings_callback_ ||
+         !qm_settings_callback) << "Overwriting the previous callback?";
   qm_settings_callback_ = qm_settings_callback;
   _mediaOpt.EnableQM(qm_settings_callback_ != NULL);
   return VCM_OK;
@@ -310,6 +322,9 @@ int32_t VideoSender::RegisterVideoQMCallback(
 int32_t VideoSender::RegisterProtectionCallback(
     VCMProtectionCallback* protection_callback) {
   CriticalSectionScoped cs(_sendCritSect);
+  DCHECK(protection_callback_ == protection_callback ||
+         !protection_callback_ ||
+         !protection_callback) << "Overwriting the previous callback?";
   protection_callback_ = protection_callback;
   return VCM_OK;
 }