Reimplement PooledI420Buffer, without an extra indirection.

webrtc/common_video/i420_buffer_pool.cc

 /*
  *  Copyright (c) 2015 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/common_video/include/i420_buffer_pool.h"
 
 #include "webrtc/base/checks.h"
-
-namespace {
-
-// One extra indirection is needed to make |HasOneRef| work.
-class PooledI420Buffer : public webrtc::VideoFrameBuffer {
- public:
-  explicit PooledI420Buffer(
-      const rtc::scoped_refptr<webrtc::I420Buffer>& buffer)
-      : buffer_(buffer) {}
-
- private:
-  ~PooledI420Buffer() override {}
-
-  int width() const override { return buffer_->width(); }
-  int height() const override { return buffer_->height(); }
-  const uint8_t* DataY() const override { return buffer_->DataY(); }
-  const uint8_t* DataU() const override { return buffer_->DataU(); }
-  const uint8_t* DataV() const override { return buffer_->DataV(); }
-
-  bool IsMutable() override { return HasOneRef(); }
-  // Make the IsMutable() check here instead of in |buffer_|, because the pool
-  // also has a reference to |buffer_|.
-  uint8_t* MutableDataY() override {
-    RTC_DCHECK(IsMutable());
-    return const_cast<uint8_t*>(buffer_->DataY());
-  }
-  uint8_t* MutableDataU() override {
-    RTC_DCHECK(IsMutable());
-    return const_cast<uint8_t*>(buffer_->DataU());
-  }
-  uint8_t* MutableDataV() override {
-    RTC_DCHECK(IsMutable());
-    return const_cast<uint8_t*>(buffer_->DataV());
-  }
-  int StrideY() const override { return buffer_->StrideY(); }
-  int StrideU() const override { return buffer_->StrideU(); }
-  int StrideV() const override { return buffer_->StrideV(); }
-  void* native_handle() const override { return nullptr; }
-
-  rtc::scoped_refptr<VideoFrameBuffer> NativeToI420Buffer() override {
-    RTC_NOTREACHED();
-    return nullptr;
-  }
-
-  friend class rtc::RefCountedObject<PooledI420Buffer>;
-  rtc::scoped_refptr<webrtc::I420Buffer> buffer_;
-};
-
-}  // namespace
+#include "webrtc/base/atomicops.h"
 
 namespace webrtc {
 
+I420BufferPool::PooledI420Buffer::PooledI420Buffer(int width, int height)
+    : I420Buffer(width, height), managed_(1) {}
+
+bool I420BufferPool::PooledI420Buffer::IsMutable() {
+  // Order is important. When the buffer becomes unmanaged, naturally
+  // managed_ is cleared before the pool drops its reference.
+  // Potentially dangerous race is as follows:
+  //
+  // 1. There are two users of the buffer, plus the pool, so refs_ == 3,
+  //    managed_ == 1.
+  //
+  // 2. One thread calls IsMutable, and at about the same time, some
+  //    other thread calls UnManage (application called Release, or
+  //    CreateBuffer, or the pool destructor). refs is changed to 2,
+  //    and managed_ to 0.
+  //
+  // 3. Now, the possible states IsMutable could see are
+  //
+  //      (refs_ == 3, managed_ == 1)    Before UnManaged
+  //      (refs_ == 3, managed_ == 0)    Transient state
+  //      (refs_ == 2, managed_ == 0)    After UnManaged
+  //
+  //    Since refs_ is read first, and AtomicOps implies that
+  //    appropriate memory barriers are used, it can't see
+  //
+  //      (refs_ == 2, managed_ == 1)    Incorrectly appears mutable.
+  //
+  //    So IsMutable correctly returns false, regardless of the race outcome.
+
+  // There remains a race with the possibility of a false negative
+  // answer. Assume we have a single user of the buffer, and have
+  // IsMutable and UnManaged called at the same time. In this case we
+  // have a transition
+  //
+  //   (refs_ == 2, managed_ == 1)       Before
+  //   (refs_ == 2, managed_ == 0)       Transient state
+  //   (refs_ == 1, managed_ == 0)       After
+  //
+  // IsMutable is expected to return true, but it may pick up the transient
+  // state, and return false. However, if multiple threads use the buffer and
+  // pool in ways making this race possible, the IsMutable caller ought to be
+  // prepared to handle failure and make its own copy.
+  //
+  // I'm afraid this can affect debug builds, correct code using
+  //
+  // if (buffer->IsMutable()) {
+  //   ... buffer->MutableDataY() ...
+  // }
+  //
+  // may fail the RTC_DCHECK(IsMutable()); inside MutableDataY, if
+  // it's unlucky enough that this second call to IsMutable triggers
+  // the above false-negative race.
+
+  // TODO(nisse): All code modifying pixel data should be fixed to
+  // either keep an explicitly owned I420Buffer, or write to newly
+  // allocated buffer, or to a buffer produced by
+  // I420BufferPool::CreateBuffer. At this point, we can delete the
+  // MutableDataY(,U,V) methods from the VideoFrameBuffer base class,
+  // together with IsMutable. The GetRefCount/HasOneRef hack will be
+  // limited to the I420BufferPool, which can then use I420Buffer
+  // instances directly.
+  int refs = GetRefCount();
+  return refs <= (IsManaged() ? 2 : 1);

[pbos-webrtc, 2016/05/20 13:32:21]

I think we can avoid the transient state:

int refs = GetRefCount();
if (!IsManaged()) {
  // Not managed, since management could've disappeared between GetRefCount and
IsManaged call we need to check the refcount again to make sure it matches the
unmanaged state.
  return GetRefCount() == 1;
}
return refs <= 2;

But write a better comment.

[nisse-webrtc, 2016/05/23 07:35:06]

That will only reduce the probability of false negative. We will usually not
stay in the transient state (refs_ == 2, managed_ == 0) for a long time, but the
thread calling UnManage could well be preempted so that it's a more or less
arbitrary delay between the assignment to managed_ and refs_ dropping to 1. And
until that thread makes progress, rereading the reference count makes no
difference.

If we want to eliminate the race, I'm afraid we'd need a way to wait until the
transient state is exited, e.g., using a critical section to protect access to
the managed_ flag, and arrange things so that we clear the flag and drop the
reference before leaving the critical section. 

Another critical section here might be ok, IsMutable and UnManaged shouldn't be
very performance critical, except that IsMutable is used in a bunch of
RTC_DCHECKS. I can try that if you really want, but I think we can live with the
false-negative race for the time being. I just filed a bug to track removal of
IsMutable.

[pbos-webrtc, 2016/05/23 07:39:29]

Right, I thought the reference was dropped before managed_. Never mind then. :\

+}
+
+bool I420BufferPool::PooledI420Buffer::IsManaged() const {
+  return rtc::AtomicOps::AcquireLoad(&managed_) == 1;
+}
+
+bool I420BufferPool::PooledI420Buffer::IsFree() {
+  // We always have a reference from the pool. If that's the only
+  // reference, the buffer is free for reuse.
+  bool is_free = (GetRefCount() == 1);
+  RTC_DCHECK(IsManaged());
+  return is_free;
+}
+
+void I420BufferPool::PooledI420Buffer::UnManage() {
+  // May run on any thread, when called by the pool destructor.
+  rtc::AtomicOps::ReleaseStore(&managed_, 0);
+}
+
 I420BufferPool::I420BufferPool(bool zero_initialize)
     : zero_initialize_(zero_initialize) {
+  thread_checker_.DetachFromThread();
+}
+
+I420BufferPool::~I420BufferPool() {
   Release();
 }
 
 void I420BufferPool::Release() {
+  for (auto& it : buffers_) {
+    it->UnManage();
+  }
+  buffers_.clear();
   thread_checker_.DetachFromThread();
-  buffers_.clear();
 }
 
-rtc::scoped_refptr<VideoFrameBuffer> I420BufferPool::CreateBuffer(int width,
-                                                                  int height) {
+rtc::scoped_refptr<I420Buffer> I420BufferPool::CreateBuffer(int width,
+                                                            int height) {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
   // Release buffers with wrong resolution.
   for (auto it = buffers_.begin(); it != buffers_.end();) {
-    if ((*it)->width() != width || (*it)->height() != height)
+    if ((*it)->width() != width || (*it)->height() != height) {
+      (*it)->UnManage();
       it = buffers_.erase(it);
-    else
+    } else {
       ++it;
+    }
   }
   // Look for a free buffer.
-  for (const rtc::scoped_refptr<I420Buffer>& buffer : buffers_) {
-    // If the buffer is in use, the ref count will be 2, one from the list we
-    // are looping over and one from a PooledI420Buffer returned from
-    // CreateBuffer that has not been released yet. If the ref count is 1
-    // (HasOneRef), then the list we are looping over holds the only reference
-    // and it's safe to reuse.
-    if (buffer->IsMutable())
-      return new rtc::RefCountedObject<PooledI420Buffer>(buffer);
+  for (const rtc::scoped_refptr<PooledI420Buffer>& buffer : buffers_) {
+    if (buffer->IsFree())
+      return buffer;
   }
   // Allocate new buffer.
-  rtc::scoped_refptr<I420Buffer> buffer = new rtc::RefCountedObject<I420Buffer>(
-      width, height);
+  rtc::scoped_refptr<PooledI420Buffer> buffer(
+      new rtc::RefCountedObject<PooledI420Buffer>(width, height));
   if (zero_initialize_)
     buffer->InitializeData();
   buffers_.push_back(buffer);
-  return new rtc::RefCountedObject<PooledI420Buffer>(buffers_.back());
+  return buffer;
 }
 
 }  // namespace webrtc