Add I420 buffer pool to avoid unnecessary allocations

Now when we don't use SwapFrame consistently anymore, we need to recycle allocations with a buffer pool instead. This CL adds a buffer pool class, and updates the vp8 decoder to use it. If this CL lands successfully I will update the other video producers as well.

BUG=1128
R=stefan@webrtc.org, tommi@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/41189004

Cr-Commit-Position: refs/heads/master@{#8748}
git-svn-id: http://webrtc.googlecode.com/svn/trunk@8748 4adac7df-926f-26a2-2b94-8c16560cd09d

webrtc/common_video/BUILD.gn

@@ -17,8 +17,10 @@ config("common_video_config") {
 
 source_set("common_video") {
   sources = [
+    "i420_buffer_pool.cc",
     "i420_video_frame.cc",
     "interface/i420_video_frame.h",
+    "interface/i420_buffer_pool.h",
     "interface/native_handle.h",
     "interface/video_frame_buffer.h",
     "libyuv/include/scaler.h",

webrtc/common_video/common_video.gyp

@@ -39,9 +39,11 @@
         }],
       ],
       'sources': [
+        'interface/i420_buffer_pool.h',
         'interface/i420_video_frame.h',
         'interface/native_handle.h',
         'interface/video_frame_buffer.h',
+        'i420_buffer_pool.cc',
         'i420_video_frame.cc',
         'libyuv/include/webrtc_libyuv.h',
         'libyuv/include/scaler.h',

webrtc/common_video/common_video_unittests.gyp

@@ -19,6 +19,7 @@
          '<(webrtc_root)/test/test.gyp:test_support_main',
       ],
       'sources': [
+        'i420_buffer_pool_unittest.cc',
         'i420_video_frame_unittest.cc',
         'libyuv/libyuv_unittest.cc',
         'libyuv/scaler_unittest.cc',

webrtc/common_video/i420_buffer_pool.cc

@@ -0,0 +1,82 @@
+/*
+ *  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/interface/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* data(webrtc::PlaneType type) const override {
+    const webrtc::I420Buffer* cbuffer = buffer_.get();
+    return cbuffer->data(type);
+  }
+  uint8_t* data(webrtc::PlaneType type) {
+    DCHECK(HasOneRef());
+    const webrtc::I420Buffer* cbuffer = buffer_.get();
+    return const_cast<uint8_t*>(cbuffer->data(type));
+  }
+  int stride(webrtc::PlaneType type) const override {
+    return buffer_->stride(type);
+  }
+  rtc::scoped_refptr<webrtc::NativeHandle> native_handle() const override {
+    return nullptr;
+  }
+
+  friend class rtc::RefCountedObject<PooledI420Buffer>;
+  rtc::scoped_refptr<webrtc::I420Buffer> buffer_;
+};
+
+}  // namespace
+
+namespace webrtc {
+
+I420BufferPool::I420BufferPool() {
+  thread_checker_.DetachFromThread();
+}
+
+rtc::scoped_refptr<VideoFrameBuffer> I420BufferPool::CreateBuffer(int width,
+                                                                  int height) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  // Release buffers with wrong resolution.
+  for (auto it = buffers_.begin(); it != buffers_.end();) {
+    if ((*it)->width() != width || (*it)->height() != height)
+      it = buffers_.erase(it);
+    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->HasOneRef())
+      return new rtc::RefCountedObject<PooledI420Buffer>(buffer);
+  }
+  // Allocate new buffer.
+  buffers_.push_back(new rtc::RefCountedObject<I420Buffer>(width, height));
+  return new rtc::RefCountedObject<PooledI420Buffer>(buffers_.back());
+}
+
+}  // namespace webrtc

webrtc/common_video/i420_buffer_pool_unittest.cc

@@ -0,0 +1,74 @@
+/*
+ *  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 <string>
+
+#include "testing/gtest/include/gtest/gtest.h"
+#include "webrtc/common_video/interface/i420_buffer_pool.h"
+
+namespace webrtc {
+
+TEST(TestI420BufferPool, SimpleFrameReuse) {
+  I420BufferPool pool;
+  rtc::scoped_refptr<VideoFrameBuffer> buffer = pool.CreateBuffer(16, 16);
+  EXPECT_EQ(16, buffer->width());
+  EXPECT_EQ(16, buffer->height());
+  // Extract non-refcounted pointers for testing.
+  const uint8_t* y_ptr = buffer->data(kYPlane);
+  const uint8_t* u_ptr = buffer->data(kUPlane);
+  const uint8_t* v_ptr = buffer->data(kVPlane);
+  // Release buffer so that it is returned to the pool.
+  buffer = nullptr;
+  // Check that the memory is resued.
+  buffer = pool.CreateBuffer(16, 16);
+  EXPECT_EQ(y_ptr, buffer->data(kYPlane));
+  EXPECT_EQ(u_ptr, buffer->data(kUPlane));
+  EXPECT_EQ(v_ptr, buffer->data(kVPlane));
+  EXPECT_EQ(16, buffer->width());
+  EXPECT_EQ(16, buffer->height());
+}
+
+TEST(TestI420BufferPool, FailToReuse) {
+  I420BufferPool pool;
+  rtc::scoped_refptr<VideoFrameBuffer> buffer = pool.CreateBuffer(16, 16);
+  // Extract non-refcounted pointers for testing.
+  const uint8_t* u_ptr = buffer->data(kUPlane);
+  const uint8_t* v_ptr = buffer->data(kVPlane);
+  // Release buffer so that it is returned to the pool.
+  buffer = nullptr;
+  // Check that the pool doesn't try to reuse buffers of incorrect size.
+  buffer = pool.CreateBuffer(32, 16);
+  EXPECT_EQ(32, buffer->width());
+  EXPECT_EQ(16, buffer->height());
+  EXPECT_NE(u_ptr, buffer->data(kUPlane));
+  EXPECT_NE(v_ptr, buffer->data(kVPlane));
+}
+
+TEST(TestI420BufferPool, ExclusiveOwner) {
+  // Check that created buffers are exclusive so that they can be written to.
+  I420BufferPool pool;
+  rtc::scoped_refptr<VideoFrameBuffer> buffer = pool.CreateBuffer(16, 16);
+  EXPECT_TRUE(buffer->HasOneRef());
+}
+
+TEST(TestI420BufferPool, FrameValidAfterPoolDestruction) {
+  rtc::scoped_refptr<VideoFrameBuffer> buffer;
+  {
+    I420BufferPool pool;
+    buffer = pool.CreateBuffer(16, 16);
+  }
+  EXPECT_TRUE(buffer->HasOneRef());
+  EXPECT_EQ(16, buffer->width());
+  EXPECT_EQ(16, buffer->height());
+  // Try to trigger use-after-free errors by writing to y-plane.
+  memset(buffer->data(kYPlane), 0xA5, 16 * buffer->stride(kYPlane));
+}
+
+}  // namespace webrtc

webrtc/common_video/interface/i420_buffer_pool.h

@@ -0,0 +1,40 @@
+/*
+ *  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.
+ */
+
+#ifndef WEBRTC_COMMON_VIDEO_INTERFACE_I420_BUFFER_POOL_H_
+#define WEBRTC_COMMON_VIDEO_INTERFACE_I420_BUFFER_POOL_H_
+
+#include <list>
+
+#include "webrtc/base/thread_checker.h"
+#include "webrtc/common_video/interface/video_frame_buffer.h"
+
+namespace webrtc {
+
+// Simple buffer pool to avoid unnecessary allocations of I420Buffer objects.
+// The pool manages the memory of the I420Buffer returned from CreateBuffer.
+// When the I420Buffer is destructed, the memory is returned to the pool for use
+// by subsequent calls to CreateBuffer. If the resolution passed to CreateBuffer
+// changes, old buffers will be purged from the pool.
+class I420BufferPool {
+ public:
+  I420BufferPool();
+  // Returns a buffer from the pool, or creates a new buffer if no suitable
+  // buffer exists in the pool.
+  rtc::scoped_refptr<VideoFrameBuffer> CreateBuffer(int width, int height);
+
+ private:
+  rtc::ThreadChecker thread_checker_;
+  std::list<rtc::scoped_refptr<I420Buffer>> buffers_;
+};
+
+}  // namespace webrtc
+
+#endif  // WEBRTC_COMMON_VIDEO_INTERFACE_I420_BUFFER_POOL_H_

webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc

@@ -17,6 +17,7 @@
 
 // NOTE(ajm): Path provided by gyp.
 #include "libyuv/scale.h"  // NOLINT
+#include "libyuv/convert.h"  // NOLINT
 
 #include "webrtc/common.h"
 #include "webrtc/common_types.h"
@@ -1333,17 +1334,18 @@ int VP8DecoderImpl::ReturnFrame(const vpx_image_t* img,
   last_frame_width_ = img->d_w;
   last_frame_height_ = img->d_h;
   // Allocate memory for decoded image.
-  // TODO(mikhal): This does  a copy - need to SwapBuffers.
+  I420VideoFrame decoded_image(buffer_pool_.CreateBuffer(img->d_w, img->d_h),
-  decoded_image_.CreateFrame(img->planes[VPX_PLANE_Y],
+                               timestamp, 0, kVideoRotation_0);
-                             img->planes[VPX_PLANE_U],
+  libyuv::I420Copy(
-                             img->planes[VPX_PLANE_V],
+      img->planes[VPX_PLANE_Y], img->stride[VPX_PLANE_Y],
-                             img->d_w, img->d_h,
+      img->planes[VPX_PLANE_U], img->stride[VPX_PLANE_U],
-                             img->stride[VPX_PLANE_Y],
+      img->planes[VPX_PLANE_V], img->stride[VPX_PLANE_V],
-                             img->stride[VPX_PLANE_U],
+      decoded_image.buffer(kYPlane), decoded_image.stride(kYPlane),
-                             img->stride[VPX_PLANE_V]);
+      decoded_image.buffer(kUPlane), decoded_image.stride(kUPlane),
-  decoded_image_.set_timestamp(timestamp);
+      decoded_image.buffer(kVPlane), decoded_image.stride(kVPlane),
-  decoded_image_.set_ntp_time_ms(ntp_time_ms);
+      img->d_w, img->d_h);
-  int ret = decode_complete_callback_->Decoded(decoded_image_);
+  decoded_image.set_ntp_time_ms(ntp_time_ms);
+  int ret = decode_complete_callback_->Decoded(decoded_image);
   if (ret != 0)
     return ret;
 
@@ -1386,7 +1388,7 @@ VideoDecoder* VP8DecoderImpl::Copy() {
     assert(false);
     return NULL;
   }
-  if (decoded_image_.IsZeroSize()) {
+  if (last_frame_width_ == 0 || last_frame_height_ == 0) {
     // Nothing has been decoded before; cannot clone.
     return NULL;
   }
@@ -1409,13 +1411,13 @@ VideoDecoder* VP8DecoderImpl::Copy() {
     return NULL;
   }
   // Allocate memory for reference image copy
-  assert(decoded_image_.width() > 0);
+  assert(last_frame_width_ > 0);
-  assert(decoded_image_.height() > 0);
+  assert(last_frame_height_ > 0);
   assert(image_format_ > VPX_IMG_FMT_NONE);
   // Check if frame format has changed.
   if (ref_frame_ &&
-      (decoded_image_.width() != static_cast<int>(ref_frame_->img.d_w) ||
+      (last_frame_width_ != static_cast<int>(ref_frame_->img.d_w) ||
-          decoded_image_.height() != static_cast<int>(ref_frame_->img.d_h) ||
+          last_frame_height_ != static_cast<int>(ref_frame_->img.d_h) ||
           image_format_ != ref_frame_->img.fmt)) {
     vpx_img_free(&ref_frame_->img);
     delete ref_frame_;
@@ -1430,7 +1432,7 @@ VideoDecoder* VP8DecoderImpl::Copy() {
     // for the y plane, but only half of it to the u and v planes.
     if (!vpx_img_alloc(&ref_frame_->img,
                        static_cast<vpx_img_fmt_t>(image_format_),
-                       decoded_image_.width(), decoded_image_.height(),
+                       last_frame_width_, last_frame_height_,
                        kVp832ByteAlign)) {
       assert(false);
       delete copy;

webrtc/modules/video_coding/codecs/vp8/vp8_impl.h

@@ -22,6 +22,7 @@
 #include "vpx/vp8cx.h"
 #include "vpx/vp8dx.h"
 
+#include "webrtc/common_video/interface/i420_buffer_pool.h"
 #include "webrtc/common_video/interface/i420_video_frame.h"
 #include "webrtc/modules/video_coding/codecs/interface/video_codec_interface.h"
 #include "webrtc/modules/video_coding/codecs/vp8/include/vp8.h"
@@ -154,7 +155,7 @@ class VP8DecoderImpl : public VP8Decoder {
                   uint32_t timeStamp,
                   int64_t ntp_time_ms);
 
-  I420VideoFrame decoded_image_;
+  I420BufferPool buffer_pool_;
   DecodedImageCallback* decode_complete_callback_;
   bool inited_;
   bool feedback_mode_;