Change FrameBuffer::Stop to not require a critical section.

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 {
 namespace video_coding {
 
 namespace {
 // Max number of frames the buffer will hold.
 constexpr int kMaxFramesBuffered = 600;
 
 // Max number of decoded frame info that will be saved.
 constexpr int kMaxFramesHistory = 50;
 }  // namespace
 
 FrameBuffer::FrameBuffer(Clock* clock,
                          VCMJitterEstimator* jitter_estimator,
                          VCMTiming* timing,
                          VCMReceiveStatisticsCallback* stats_callback)
     : clock_(clock),
-      new_countinuous_frame_event_(false, false),
+      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_(false),
+      stopped_(0),
       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;
 
   do {
+    if (rtc::AtomicOps::AcquireLoad(&stopped_))
+      return kStopped;
+
     int64_t now_ms = clock_->TimeInMilliseconds();
+    wait_ms = max_wait_time_ms;
     {
       rtc::CritScope lock(&crit_);
-      new_countinuous_frame_event_.Reset();
-      if (stopped_)
-        return kStopped;
-
-      wait_ms = max_wait_time_ms;
-
+      new_continuous_frame_event_.Reset();
       // 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 25 matching lines @@
         // enough and the stream has multiple spatial and temporal layers.
         if (wait_ms == 0)
           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_countinuous_frame_event_.Wait(wait_ms));
+  } 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);
 
     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());
       }
 
       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;
-  } else if (latest_return_time_ms - now_ms > 0) {
+  }
+
+  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);
-  } else {
-    return kTimeout;
   }
+
+  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::CritScope lock(&crit_);
-  stopped_ = false;
+  rtc::AtomicOps::ReleaseStore(&stopped_, 0);
 }
 
 void FrameBuffer::Stop() {
   TRACE_EVENT0("webrtc", "FrameBuffer::Stop");
-  rtc::CritScope lock(&crit_);
-  stopped_ = true;
-  new_countinuous_frame_event_.Set();
+  // 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_);
+  new_continuous_frame_event_.Set();
 }
 
 int FrameBuffer::InsertFrame(std::unique_ptr<FrameObject> frame) {
   TRACE_EVENT0("webrtc", "FrameBuffer::InsertFrame");
-  rtc::CritScope lock(&crit_);
   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 48 matching lines @@
   ++num_frames_buffered_;
 
   if (info->second.num_missing_continuous == 0) {
     info->second.continuous = true;
     PropagateContinuity(info);
     last_continuous_picture_id = last_continuous_frame_it_->first.picture_id;
 
     // Since we now have new continuous frames there might be a better frame
     // to return from NextFrame. Signal that thread so that it again can choose
     // which frame to return.
-    new_countinuous_frame_event_.Set();
+    new_continuous_frame_event_.Set();
   }
 
   return last_continuous_picture_id;
 }
 
 void FrameBuffer::PropagateContinuity(FrameMap::iterator start) {
   TRACE_EVENT0("webrtc", "FrameBuffer::PropagateContinuity");
   RTC_DCHECK(start->second.continuous);
   if (last_continuous_frame_it_ == frames_.end())
     last_continuous_frame_it_ = start;
@@ skipping 158 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