Reimplement PooledI420Buffer, without an extra indirection.

webrtc/common_video/i420_buffer_pool.cc

Index: webrtc/common_video/i420_buffer_pool.cc
diff --git a/webrtc/common_video/i420_buffer_pool.cc b/webrtc/common_video/i420_buffer_pool.cc
index 8896260fc0ecf4500e195757ac6b0390dee948ba..a18819a29cd059669c6cbe6863540b90c02f4291 100644
--- a/webrtc/common_video/i420_buffer_pool.cc
+++ b/webrtc/common_video/i420_buffer_pool.cc
@@ -11,95 +11,133 @@
 #include "webrtc/common_video/include/i420_buffer_pool.h"
 
 #include "webrtc/base/checks.h"
+#include "webrtc/base/atomicops.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) {}
+namespace webrtc {
 
- private:
-  ~PooledI420Buffer() override {}
+I420BufferPool::PooledI420Buffer::PooledI420Buffer(int width, int height)
+    : I420Buffer(width, height), managed_(1) {}
 
-  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 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.
 
-  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; }
+  // 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.
 
-  rtc::scoped_refptr<VideoFrameBuffer> NativeToI420Buffer() override {
-    RTC_NOTREACHED();
-    return nullptr;
-  }
+  // 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);
+}
 
-  friend class rtc::RefCountedObject<PooledI420Buffer>;
-  rtc::scoped_refptr<webrtc::I420Buffer> buffer_;
-};
+bool I420BufferPool::PooledI420Buffer::IsManaged() const {
+  return rtc::AtomicOps::AcquireLoad(&managed_) == 1;
+}
 
-}  // namespace
+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;
+}
 
-namespace webrtc {
+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() {
-  thread_checker_.DetachFromThread();
+  for (auto& it : buffers_) {
+    it->UnManage();
+  }
   buffers_.clear();
+  thread_checker_.DetachFromThread();
 }
 
-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