Release the critial section between recursive calls in FrameBuffer::NextFrame.

webrtc/modules/video_coding/frame_buffer2.cc

Index: webrtc/modules/video_coding/frame_buffer2.cc
diff --git a/webrtc/modules/video_coding/frame_buffer2.cc b/webrtc/modules/video_coding/frame_buffer2.cc
index fa7e3424bc7d5f68bbcb5e215a5fee89118a1098..3d43936314f5a08ff7db623336bf337cb0df9735 100644
--- a/webrtc/modules/video_coding/frame_buffer2.cc
+++ b/webrtc/modules/video_coding/frame_buffer2.cc
@@ -14,7 +14,6 @@
 #include <cstring>
 #include <queue>
 
-#include "webrtc/base/atomicops.h"
 #include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/trace_event.h"
@@ -48,7 +47,7 @@ FrameBuffer::FrameBuffer(Clock* clock,
       last_continuous_frame_it_(frames_.end()),
       num_frames_history_(0),
       num_frames_buffered_(0),
-      stopped_(0),
+      stopped_(false),
       protection_mode_(kProtectionNack),
       stats_callback_(stats_callback) {}
 
@@ -61,16 +60,18 @@ FrameBuffer::ReturnReason FrameBuffer::NextFrame(
   int64_t latest_return_time_ms =
       clock_->TimeInMilliseconds() + max_wait_time_ms;
   int64_t wait_ms = max_wait_time_ms;
+  int64_t now_ms = 0;
 
   do {
-    if (rtc::AtomicOps::AcquireLoad(&stopped_))
-      return kStopped;
-
-    int64_t now_ms = clock_->TimeInMilliseconds();
-    wait_ms = max_wait_time_ms;
+    now_ms = clock_->TimeInMilliseconds();
     {
       rtc::CritScope lock(&crit_);
       new_continuous_frame_event_.Reset();
+      if (stopped_)
+        return kStopped;
+
+      wait_ms = max_wait_time_ms;
+
       // Need to hold |crit_| in order to use |frames_|, therefore we
       // set it here in the loop instead of outside the loop in order to not
       // acquire the lock unnecesserily.
@@ -118,32 +119,34 @@ FrameBuffer::ReturnReason FrameBuffer::NextFrame(
     wait_ms = std::max<int64_t>(wait_ms, 0);
   } while (new_continuous_frame_event_.Wait(wait_ms));
 
-  rtc::CritScope lock(&crit_);
-  int64_t now_ms = clock_->TimeInMilliseconds();
-  if (next_frame_it_ != frames_.end()) {
-    std::unique_ptr<FrameObject> frame =
-        std::move(next_frame_it_->second.frame);
+  {
+    rtc::CritScope lock(&crit_);
+    now_ms = clock_->TimeInMilliseconds();
+    if (next_frame_it_ != frames_.end()) {
+      std::unique_ptr<FrameObject> frame =
+          std::move(next_frame_it_->second.frame);
 
-    if (!frame->delayed_by_retransmission()) {
-      int64_t frame_delay;
+      if (!frame->delayed_by_retransmission()) {
+        int64_t frame_delay;
 
-      if (inter_frame_delay_.CalculateDelay(frame->timestamp, &frame_delay,
-                                            frame->ReceivedTime())) {
-        jitter_estimator_->UpdateEstimate(frame_delay, frame->size());
-      }
+        if (inter_frame_delay_.CalculateDelay(frame->timestamp, &frame_delay,
+                                              frame->ReceivedTime())) {
+          jitter_estimator_->UpdateEstimate(frame_delay, frame->size());
+        }
 
-      float rtt_mult = protection_mode_ == kProtectionNackFEC ? 0.0 : 1.0;
-      timing_->SetJitterDelay(jitter_estimator_->GetJitterEstimate(rtt_mult));
-      timing_->UpdateCurrentDelay(frame->RenderTime(), now_ms);
-    }
+        float rtt_mult = protection_mode_ == kProtectionNackFEC ? 0.0 : 1.0;
+        timing_->SetJitterDelay(jitter_estimator_->GetJitterEstimate(rtt_mult));
+        timing_->UpdateCurrentDelay(frame->RenderTime(), now_ms);
+      }
 
-    UpdateJitterDelay();
+      UpdateJitterDelay();
 
-    PropagateDecodability(next_frame_it_->second);
-    AdvanceLastDecodedFrame(next_frame_it_);
-    last_decoded_frame_timestamp_ = frame->timestamp;
-    *frame_out = std::move(frame);
-    return kFrameFound;
+      PropagateDecodability(next_frame_it_->second);
+      AdvanceLastDecodedFrame(next_frame_it_);
+      last_decoded_frame_timestamp_ = frame->timestamp;
+      *frame_out = std::move(frame);
+      return kFrameFound;
+    }
   }
 
   if (latest_return_time_ms - now_ms > 0) {
@@ -163,40 +166,28 @@ void FrameBuffer::SetProtectionMode(VCMVideoProtection mode) {
   protection_mode_ = mode;
 }
 
-// Start() and Stop() must be called on the same thread.
-// The value of stopped_ can only be changed on this thread.
 void FrameBuffer::Start() {
   TRACE_EVENT0("webrtc", "FrameBuffer::Start");
-  rtc::AtomicOps::ReleaseStore(&stopped_, 0);
+  rtc::CritScope lock(&crit_);
+  stopped_ = false;
 }
 
 void FrameBuffer::Stop() {
   TRACE_EVENT0("webrtc", "FrameBuffer::Stop");
-  // TODO(tommi,philipel): Previously, we grabbed the |crit_| lock when decoding
-  // was being stopped. On Android, with captured frames being delivered on the
-  // decoder thread, this consistently caused the 'busy loop' checks in the
-  // PlatformThread to trigger on "VoiceProcessThread", "ModuleProcessThread"
-  // and occasionally on "PacerThread".
-  // Look into what was going on and why |Stop()| wasn't able to grab the lock
-  // and stop the FrameBuffer while that happened.
-  // See bug: https://bugs.chromium.org/p/webrtc/issues/detail?id=7331
-  rtc::AtomicOps::Increment(&stopped_);
+  rtc::CritScope lock(&crit_);
+  stopped_ = true;
   new_continuous_frame_event_.Set();
 }
 
 int FrameBuffer::InsertFrame(std::unique_ptr<FrameObject> frame) {
   TRACE_EVENT0("webrtc", "FrameBuffer::InsertFrame");
+  rtc::CritScope lock(&crit_);

[tommi, 2017/03/15 13:33:48]

nit: the things that were outside the lock, can still be outside the lock.

For locks, we should generally ask ourselves:
* Do we really really really need a lock?
  - Sometimes we don't need a lock even if a variable is used on more than one
thread. In our code, I've seen several cases of variables being set, a thread is
started and while that thread runs, the variable does not change value.  Then
the thread is stopped and then the variable is modified again.  In cases like
that, we today incorporate locks, but a lock is actually not needed.  Ensuring
the code is correct however, can be achieved by thoroughly [D]CHECKing thread
correctness, which also has the benefit of documenting the execution context of
the methods for anyone that reads the code.

* Is the scope as small as it can be?
 - I think that in this case it can be smaller.  |stats_callback_| is const, so
its value will never change.  It also represents an external implementation.

* Are we calling out to other functions while calling the lock?
 - Calling out to OnCompleteFrame, while holding the lock, could leave us
vulnerable to things like lock inversion or potentially slowing things down for
reasons that don't matter for InsertFrame.  It could also mask an issue with the
external implementation by introducing synchronization for code that doesn't
have it but should.

[philipel, 2017/03/15 14:10:27]

Fixed!

Comments like these are great, it is one of the main sources of learning I have
ever since I started working, so they are much appreciated. :)

   RTC_DCHECK(frame);
 
-  if (rtc::AtomicOps::AcquireLoad(&stopped_))
-    return -1;
-
   if (stats_callback_)
     stats_callback_->OnCompleteFrame(frame->num_references == 0, frame->size());
 
   FrameKey key(frame->picture_id, frame->spatial_layer);
-
-  rtc::CritScope lock(&crit_);
   int last_continuous_picture_id =
       last_continuous_frame_it_ == frames_.end()
           ? -1