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

webrtc/modules/video_coding/frame_buffer2.cc

 /*
  *  Copyright (c) 2016 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/modules/video_coding/frame_buffer2.h"
 
 #include <algorithm>
 #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"
 #include "webrtc/modules/video_coding/include/video_coding_defines.h"
 #include "webrtc/modules/video_coding/jitter_estimator.h"
 #include "webrtc/modules/video_coding/timing.h"
 #include "webrtc/system_wrappers/include/clock.h"
 #include "webrtc/system_wrappers/include/metrics.h"
 
 namespace webrtc {
@@ skipping 13 matching lines @@
                          VCMReceiveStatisticsCallback* stats_callback)
     : clock_(clock),
       new_continuous_frame_event_(false, false),
       jitter_estimator_(jitter_estimator),
       timing_(timing),
       inter_frame_delay_(clock_->TimeInMilliseconds()),
       last_decoded_frame_it_(frames_.end()),
       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) {}
 
 FrameBuffer::~FrameBuffer() {}
 
 FrameBuffer::ReturnReason FrameBuffer::NextFrame(
     int64_t max_wait_time_ms,
     std::unique_ptr<FrameObject>* frame_out) {
   TRACE_EVENT0("webrtc", "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.
       next_frame_it_ = frames_.end();
 
       // |frame_it| points to the first frame after the
       // |last_decoded_frame_it_|.
       auto frame_it = frames_.end();
       if (last_decoded_frame_it_ == frames_.end()) {
         frame_it = frames_.begin();
@@ skipping 27 matching lines @@
           continue;
 
         break;
       }
     }  // rtc::Critscope lock(&crit_);
 
     wait_ms = std::min<int64_t>(wait_ms, latest_return_time_ms - now_ms);
     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();
+
+      PropagateDecodability(next_frame_it_->second);
+      AdvanceLastDecodedFrame(next_frame_it_);
+      last_decoded_frame_timestamp_ = frame->timestamp;
+      *frame_out = std::move(frame);
+      return kFrameFound;
     }
-
-    UpdateJitterDelay();
-
-    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) {
     // If |next_frame_it_ == frames_.end()| and there is still time left, it
     // means that the frame buffer was cleared as the thread in this function
     // was waiting to acquire |crit_| in order to return. Wait for the
     // remaining time and then return.
     return NextFrame(latest_return_time_ms - now_ms, frame_out);
   }
 
   return kTimeout;
 }
 
 void FrameBuffer::SetProtectionMode(VCMVideoProtection mode) {
   TRACE_EVENT0("webrtc", "FrameBuffer::SetProtectionMode");
   rtc::CritScope lock(&crit_);
   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
           : last_continuous_frame_it_->first.picture_id;
 
   if (num_frames_buffered_ >= kMaxFramesBuffered) {
     LOG(LS_WARNING) << "Frame with (picture_id:spatial_id) (" << key.picture_id
                     << ":" << static_cast<int>(key.spatial_layer)
                     << ") could not be inserted due to the frame "
                     << "buffer being full, dropping frame.";
@@ skipping 227 matching lines @@
   frames_.clear();
   last_decoded_frame_it_ = frames_.end();
   last_continuous_frame_it_ = frames_.end();
   next_frame_it_ = frames_.end();
   num_frames_history_ = 0;
   num_frames_buffered_ = 0;
 }
 
 }  // namespace video_coding
 }  // namespace webrtc