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..48862d0bad4384cc544517268690802c372b9e5f 100644
--- a/webrtc/common_video/i420_buffer_pool.cc
+++ b/webrtc/common_video/i420_buffer_pool.cc
@@ -11,95 +11,95 @@
 #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) {}
-
- 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(); }
+namespace webrtc {
 
-  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; }
+I420BufferPool::PooledI420Buffer::PooledI420Buffer(int width, int height)
+    : I420Buffer(width, height), managed_(1) {}
 
-  rtc::scoped_refptr<VideoFrameBuffer> NativeToI420Buffer() override {
-    RTC_NOTREACHED();
-    return nullptr;
-  }
+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.
+  //
+  // 3. Some other thread causes the buffer to become unmanaged, by
+  //    calling Release or CreateBuffer. refs is changed to 2, and
+  //    managed_ to 0.
+  //
+  // 4. If the IsMutable call picks up managed_ == 1 and refs == 2,
+  //    we have a problem, IsMutable gives a false positive answer.
+  //
+  // By reading the reference count first here, we know that if we get
+  // refs == 2 in this scenario, we will also get managed_ == 0, not
+  // the previous value of 1.
+  int refs = GetRefCount();
+  return refs <= IsManaged() ? 2 : 1;

[nisse-webrtc, 2016/05/20 08:20:48]

This is somewhat scary missing-parens-bug... 

+}
 
-  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