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..f0610f320b2c1d02568af939e8c6fc7aa535ddf8 100644
--- a/webrtc/common_video/i420_buffer_pool.cc
+++ b/webrtc/common_video/i420_buffer_pool.cc
@@ -11,95 +11,97 @@
 #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 is_managed == 0, not
+  // the previous value of 1.
+  int refs = GetRefCount();
+  int is_managed = IsManaged();
+  return refs <= 1 + is_managed;
+}
 
-  friend class rtc::RefCountedObject<PooledI420Buffer>;
-  rtc::scoped_refptr<webrtc::I420Buffer> buffer_;
-};
+int I420BufferPool::PooledI420Buffer::IsManaged() const {

[pbos-webrtc, 2016/05/19 15:03:22]

bool, even if that means you have to do IsManaged() ? 1 : 0

[nisse-webrtc, 2016/05/20 07:30:26]

Done.

+  return rtc::AtomicOps::AcquireLoad(&managed_);

[pbos-webrtc, 2016/05/19 15:03:21]

== 1

[nisse-webrtc, 2016/05/20 07:30:25]

Done.

+}
 
-}  // namespace
+bool I420BufferPool::PooledI420Buffer::IsFree() {
+  bool is_free = (GetRefCount() == 1);

[pbos-webrtc, 2016/05/19 15:03:22]

Add a thread_checker_ to IsFree and UnManage.

[nisse-webrtc, 2016/05/20 07:30:26]

At first I thought that it's kind-of redundant. The methods are called from the
buffer bool, which has a thread checker of its own. And even if the methods
formally are "public", they belong to a private class so no one else should be
able to access them.

But in fact, UnManaged is called by Release, which is called by the pool
destructor. Which may be called on some other thread. Example backtrace after
adding a thread checker:

#2  0x000000000075e792 in rtc::FatalMessage::~FatalMessage (this=0x7fffffffc190)
    at ../../webrtc/base/checks.cc:109
#3  0x0000000000f5abdd in webrtc::I420BufferPool::PooledI420Buffer::UnManage (
    this=0x7fffd8000d60) at ../../webrtc/common_video/i420_buffer_pool.cc:59
#4  0x0000000000f5ad39 in webrtc::I420BufferPool::Release (this=0x14e4520)
    at ../../webrtc/common_video/i420_buffer_pool.cc:74
#5  0x0000000000f5ac89 in webrtc::I420BufferPool::~I420BufferPool
(this=0x14e4520)
    at ../../webrtc/common_video/i420_buffer_pool.cc:69
#6  0x0000000000f5c9e9 in webrtc::Scaler::~Scaler (this=0x14e4508)
    at ../../webrtc/common_video/libyuv/scaler.cc:28
#7  0x0000000000bea251 in
webrtc::VPMSimpleSpatialResampler::~VPMSimpleSpatialResampler (this=0x14e44f0)
    at ../../webrtc/modules/video_processing/spatial_resampler.cc:21
#8  0x0000000000bea289 in
webrtc::VPMSimpleSpatialResampler::~VPMSimpleSpatialResampler (this=0x14e44f0)
    at ../../webrtc/modules/video_processing/spatial_resampler.cc:21
#9  0x0000000000be9527 in webrtc::VPMFramePreprocessor::~VPMFramePreprocessor (
    this=0x14e4450)
    at ../../webrtc/modules/video_processing/frame_preprocessor.cc:28
#10 0x0000000000be8fe1 in webrtc::VideoProcessingImpl::~VideoProcessingImpl (
    this=0x14e4410)
    at ../../webrtc/modules/video_processing/video_processing_impl.cc:26
#11 0x0000000000be9029 in webrtc::VideoProcessingImpl::~VideoProcessingImpl (
    this=0x14e4410)
    at ../../webrtc/modules/video_processing/video_processing_impl.cc:26
...
#15 0x00000000008f6eec in webrtc::ViEEncoder::~ViEEncoder (this=0x14e3330)
    at ../../webrtc/video/vie_encoder.cc:64
#16 0x00000000008f0b6c in webrtc::internal::VideoSendStream::~VideoSendStream (
    this=0x14e2ac0) at ../../webrtc/video/video_send_stream.cc:492
#17 0x00000000008f0d89 in webrtc::internal::VideoSendStream::~VideoSendStream (
    this=0x14e2ac0) at ../../webrtc/video/video_send_stream.cc:468
...
#21 0x0000000000691eed in cricket::WebRtcVideoChannel2::RemoveSendStream (
    this=0x14e0830, ssrc=1234)
    at ../../webrtc/media/engine/webrtcvideoengine2.cc:1160
#22 0x00000000004f1d62 in
cricket::WebRtcVideoEngine2Test_UseExternalFactoryForVp8WhenSupported_Test::TestBody
(this=0x13dec70)
    at ../../webrtc/media/engine/webrtcvideoengine2_unittest.cc:391

So it seems we have to tolerate UnManaged being called from any arbitrary
thread. I could still add a thread checker for IsFree only if you like, be to me
it seems a bit pointless.

+  RTC_DCHECK(IsManaged());
+  return is_free;
+}
 
-namespace webrtc {
+void I420BufferPool::PooledI420Buffer::UnManage() {
+  rtc::AtomicOps::ReleaseStore(&managed_, 0);
+}
 
 I420BufferPool::I420BufferPool(bool zero_initialize)
     : zero_initialize_(zero_initialize) {
   Release();
 }
 
+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_) {
+  for (const rtc::scoped_refptr<PooledI420Buffer>& buffer : buffers_) {
     // If the buffer is in use, the ref count will be 2, one from the list we

[pbos-webrtc, 2016/05/19 15:03:21]

This comment barely applies to the current implementation since we don't check
refcount right here either way, I think you can remove the comment completely.

[nisse-webrtc, 2016/05/20 07:30:25]

Removed, and added a shorter comment to IsFree.

-    // 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);
+    // are looping over and one from the application. If the ref count is 1 then
+    // the list we are looping over holds the only reference and it's safe to
+    // reuse.
+    if (buffer->IsFree())
+      return buffer;
   }
   // Allocate new buffer.
-  rtc::scoped_refptr<I420Buffer> buffer = new rtc::RefCountedObject<I420Buffer>(
-      width, height);
+  rtc::scoped_refptr<PooledI420Buffer> buffer =

[pbos-webrtc, 2016/05/19 15:03:21]

do buffer(new ...) for initialization here.

[nisse-webrtc, 2016/05/20 07:30:26]

Done.

+      new rtc::RefCountedObject<PooledI420Buffer>(width, height);
   if (zero_initialize_)

[pbos-webrtc, 2016/05/19 15:03:22]

Do we need the zero_initialize_ argument ever?

[nisse-webrtc, 2016/05/20 07:30:26]

Comment says it's ffmpeg-related.
It was added by hbos in cl https://codereview.webrtc.org/1645543003, used by
H264DecoderImpl.

So I'm afraid we have to keep it for now.

     buffer->InitializeData();
   buffers_.push_back(buffer);
-  return new rtc::RefCountedObject<PooledI420Buffer>(buffers_.back());
+  return buffer;
 }
 
 }  // namespace webrtc