Delete IsMutable and IsExclusive methods.

This affects the webrtc::VideoFrameBuffer and cricket::VideoFrame
classes.

To make this work, VideoFrameFactory is changed to use an
I420BufferPool rather than a plain VideoFrame to cache allocated
frames.

The I420BufferPool is reorganized to return an I420Buffer,
rather than a proxy object.

BUG=webrtc:5921, webrtc:5682

Review-Url: https://codereview.webrtc.org/2009193002
Cr-Commit-Position: refs/heads/master@{#12919}

webrtc/common_video/i420_buffer_pool.cc

@@ -12,60 +12,11 @@
 
 #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
-
 namespace webrtc {
 
 I420BufferPool::I420BufferPool(bool zero_initialize)
     : zero_initialize_(zero_initialize) {
-  Release();
+  thread_checker_.DetachFromThread();
 }
 
 void I420BufferPool::Release() {
@@ -73,8 +24,8 @@ void I420BufferPool::Release() {
   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();) {
@@ -84,22 +35,21 @@ rtc::scoped_refptr<VideoFrameBuffer> I420BufferPool::CreateBuffer(int width,
       ++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 the buffer is in use, the ref count will be >= 2, one from the list we
+    // 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->HasOneRef())
+      return buffer;
   }
   // Allocate new buffer.
-  rtc::scoped_refptr<I420Buffer> buffer = new rtc::RefCountedObject<I420Buffer>(
-      width, height);
+  rtc::scoped_refptr<PooledI420Buffer> buffer =
+      new PooledI420Buffer(width, height);
   if (zero_initialize_)
     buffer->InitializeData();
   buffers_.push_back(buffer);
-  return new rtc::RefCountedObject<PooledI420Buffer>(buffers_.back());
+  return buffer;
 }
 
 }  // namespace webrtc

webrtc/common_video/i420_buffer_pool_unittest.cc

@@ -51,20 +51,12 @@ TEST(TestI420BufferPool, FailToReuse) {
   EXPECT_NE(v_ptr, buffer->DataV());
 }
 
-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->IsMutable());
-}
-
 TEST(TestI420BufferPool, FrameValidAfterPoolDestruction) {
   rtc::scoped_refptr<VideoFrameBuffer> buffer;
   {
     I420BufferPool pool;
     buffer = pool.CreateBuffer(16, 16);
   }
-  EXPECT_TRUE(buffer->IsMutable());
   EXPECT_EQ(16, buffer->width());
   EXPECT_EQ(16, buffer->height());
   // Try to trigger use-after-free errors by writing to y-plane.

webrtc/common_video/i420_video_frame_unittest.cc

@@ -200,18 +200,6 @@ TEST(TestVideoFrame, CopyBuffer) {
   EXPECT_LE(kSizeUv, frame2.allocated_size(kVPlane));
 }
 
-TEST(TestVideoFrame, ReuseAllocation) {
-  VideoFrame frame;
-  frame.CreateEmptyFrame(640, 320, 640, 320, 320);
-  const uint8_t* y = frame.video_frame_buffer()->DataY();
-  const uint8_t* u = frame.video_frame_buffer()->DataU();
-  const uint8_t* v = frame.video_frame_buffer()->DataV();
-  frame.CreateEmptyFrame(640, 320, 640, 320, 320);
-  EXPECT_EQ(y, frame.video_frame_buffer()->DataY());
-  EXPECT_EQ(u, frame.video_frame_buffer()->DataU());
-  EXPECT_EQ(v, frame.video_frame_buffer()->DataV());
-}
-
 TEST(TestVideoFrame, FailToReuseAllocation) {
   VideoFrame frame1;
   frame1.CreateEmptyFrame(640, 320, 640, 320, 320);

webrtc/common_video/include/i420_buffer_pool.h

@@ -30,14 +30,18 @@ class 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);
+  rtc::scoped_refptr<I420Buffer> CreateBuffer(int width, int height);
   // Clears buffers_ and detaches the thread checker so that it can be reused
   // later from another thread.
   void Release();
 
  private:
+  // Explicitly use a RefCountedObject to get access to HasOneRef,
+  // needed by the pool to check exclusive access.
+  using PooledI420Buffer = rtc::RefCountedObject<I420Buffer>;
+
   rtc::ThreadChecker thread_checker_;
-  std::list<rtc::scoped_refptr<I420Buffer>> buffers_;
+  std::list<rtc::scoped_refptr<PooledI420Buffer>> buffers_;
   // If true, newly allocated buffers are zero-initialized. Note that recycled
   // buffers are not zero'd before reuse. This is required of buffers used by
   // FFmpeg according to http://crbug.com/390941, which only requires it for the

webrtc/common_video/include/video_frame_buffer.h

@@ -33,18 +33,6 @@ enum PlaneType {
 // not contain any frame metadata such as rotation, timestamp, pixel_width, etc.
 class VideoFrameBuffer : public rtc::RefCountInterface {
  public:
-  // Returns true if the caller is exclusive owner, and allowed to
-  // call MutableData.
-
-  // TODO(nisse): Delete default implementation when subclasses in
-  // Chrome are updated.
-  virtual bool IsMutable() { return false; }
-
-  // Underlying refcount access, used to implement IsMutable.
-  // TODO(nisse): Demote to protected, as soon as Chrome is changed to
-  // use IsMutable.
-  virtual bool HasOneRef() const = 0;
-
   // The resolution of the frame in pixels. For formats where some planes are
   // subsampled, this is the highest-resolution plane.
   virtual int width() const = 0;
@@ -67,7 +55,8 @@ class VideoFrameBuffer : public rtc::RefCountInterface {
   // TODO(nisse): Delete after all users are updated.
   virtual const uint8_t* data(PlaneType type) const;
 
-  // Non-const data access is allowed only if HasOneRef() is true.
+  // TODO(nisse): Move MutableData methods to the I420Buffer subclass.
+  // Non-const data access.
   virtual uint8_t* MutableDataY();
   virtual uint8_t* MutableDataU();
   virtual uint8_t* MutableDataV();
@@ -105,9 +94,7 @@ class I420Buffer : public VideoFrameBuffer {
   const uint8_t* DataY() const override;
   const uint8_t* DataU() const override;
   const uint8_t* DataV() const override;
-  // Non-const data access is only allowed if IsMutable() is true, to protect
-  // against unexpected overwrites.
-  bool IsMutable() override;
+
   uint8_t* MutableDataY() override;
   uint8_t* MutableDataU() override;
   uint8_t* MutableDataV() override;
@@ -152,7 +139,6 @@ class NativeHandleBuffer : public VideoFrameBuffer {
   int StrideV() const override;
 
   void* native_handle() const override;
-  bool IsMutable() override;
 
  protected:
   void* native_handle_;
@@ -174,8 +160,6 @@ class WrappedI420Buffer : public webrtc::VideoFrameBuffer {
   int width() const override;
   int height() const override;
 
-  bool IsMutable() override;
-
   const uint8_t* DataY() const override;
   const uint8_t* DataU() const override;
   const uint8_t* DataV() const override;

webrtc/common_video/video_frame.cc

@@ -66,18 +66,7 @@ void VideoFrame::CreateEmptyFrame(int width,
   render_time_ms_ = 0;
   rotation_ = kVideoRotation_0;
 
-  // Check if it's safe to reuse allocation.
-  if (video_frame_buffer_ && video_frame_buffer_->IsMutable() &&
-      !video_frame_buffer_->native_handle() &&
-      width == video_frame_buffer_->width() &&
-      height == video_frame_buffer_->height() &&
-      stride_y == video_frame_buffer_->StrideY() &&
-      stride_u == video_frame_buffer_->StrideU() &&
-      stride_v == video_frame_buffer_->StrideV()) {
-    return;
-  }
-
-  // Need to allocate new buffer.
+  // Allocate a new buffer.
   video_frame_buffer_ = new rtc::RefCountedObject<I420Buffer>(
       width, height, stride_y, stride_u, stride_v);
 }

webrtc/common_video/video_frame_buffer.cc

@@ -154,20 +154,13 @@ const uint8_t* I420Buffer::DataV() const {
   return data_.get() + stride_y_ * height_ + stride_u_ * ((height_ + 1) / 2);
 }
 
-bool I420Buffer::IsMutable() {
-  return HasOneRef();
-}
-
 uint8_t* I420Buffer::MutableDataY() {
-  RTC_DCHECK(IsMutable());
   return const_cast<uint8_t*>(DataY());
 }
 uint8_t* I420Buffer::MutableDataU() {
-  RTC_DCHECK(IsMutable());
   return const_cast<uint8_t*>(DataU());
 }
 uint8_t* I420Buffer::MutableDataV() {
-  RTC_DCHECK(IsMutable());
   return const_cast<uint8_t*>(DataV());
 }
 
@@ -216,10 +209,6 @@ NativeHandleBuffer::NativeHandleBuffer(void* native_handle,
   RTC_DCHECK_GT(height, 0);
 }
 
-bool NativeHandleBuffer::IsMutable() {
-  return false;
-}
-
 int NativeHandleBuffer::width() const {
   return width_;
 }
@@ -282,11 +271,6 @@ WrappedI420Buffer::~WrappedI420Buffer() {
   no_longer_used_cb_();
 }
 
-// Data owned by creator; never mutable.
-bool WrappedI420Buffer::IsMutable() {
-  return false;
-}
-
 int WrappedI420Buffer::width() const {
   return width_;
 }

webrtc/media/base/videoframe.cc

@@ -148,17 +148,6 @@ void VideoFrame::StretchToFrame(VideoFrame* dst,
   dst->set_rotation(rotation());
 }
 
-VideoFrame* VideoFrame::Stretch(size_t dst_width, size_t dst_height,
-                                bool interpolate, bool vert_crop) const {
-  VideoFrame* dest = CreateEmptyFrame(static_cast<int>(dst_width),
-                                      static_cast<int>(dst_height),
-                                      GetTimeStamp());
-  if (dest) {
-    StretchToFrame(dest, interpolate, vert_crop);
-  }
-  return dest;
-}
-
 bool VideoFrame::SetToBlack() {
   return libyuv::I420Rect(video_frame_buffer()->MutableDataY(),
                           video_frame_buffer()->StrideY(),

webrtc/media/base/videoframe.h

@@ -56,10 +56,6 @@ class VideoFrame {
   // frame buffer.
   virtual VideoFrame *Copy() const = 0;
 
-  // Since VideoFrame supports shallow copy and the internal frame buffer might
-  // be shared, this function can be used to check exclusive ownership.
-  virtual bool IsExclusive() const = 0;
-
   // Return a copy of frame which has its pending rotation applied. The
   // ownership of the returned frame is held by this frame.
   virtual const VideoFrame* GetCopyWithRotationApplied() const = 0;
@@ -95,13 +91,6 @@ class VideoFrame {
   virtual void StretchToFrame(VideoFrame *target, bool interpolate,
                               bool crop) const;
 
-  // Stretches the frame to the given size, creating a new VideoFrame object to
-  // hold it. The parameter "interpolate" controls whether to interpolate or
-  // just take the nearest-point. The parameter "crop" controls whether to crop
-  // this frame to the aspect ratio of the given dimensions before stretching.
-  virtual VideoFrame *Stretch(size_t w, size_t h, bool interpolate,
-                              bool crop) const;
-
   // Sets the video frame to black.
   virtual bool SetToBlack();
 
@@ -128,6 +117,7 @@ class VideoFrame {
   virtual VideoFrame* CreateEmptyFrame(int w,
                                        int h,
                                        int64_t timestamp_us) const = 0;
+
   virtual void set_rotation(webrtc::VideoRotation rotation) = 0;
 };
 

webrtc/media/base/videoframefactory.cc

@@ -39,23 +39,13 @@ VideoFrame* VideoFrameFactory::CreateAliasedFrame(
     std::swap(output_width, output_height);
   }
 
-  // Create and stretch the output frame if it has not been created yet, is
-  // still in use by others, or its size is not same as the expected.
-  if (!output_frame_ || !output_frame_->IsExclusive() ||
-      output_frame_->width() != output_width ||
-      output_frame_->height() != output_height) {
-    output_frame_.reset(
-        cropped_input_frame->Stretch(output_width, output_height, true, true));
-    if (!output_frame_) {
-      LOG(LS_WARNING) << "Failed to stretch frame to " << output_width << "x"
-                      << output_height;
-      return NULL;
-    }
-  } else {
-    cropped_input_frame->StretchToFrame(output_frame_.get(), true, true);
-    output_frame_->SetTimeStamp(cropped_input_frame->GetTimeStamp());
-  }
-  return output_frame_->Copy();
+  std::unique_ptr<VideoFrame> output_frame(new WebRtcVideoFrame(
+      pool_.CreateBuffer(output_width, output_height),
+      cropped_input_frame->rotation(),
+      cropped_input_frame->timestamp_us()));
+
+  cropped_input_frame->StretchToFrame(output_frame.get(), true, true);
+  return output_frame.release();
 }
 
 }  // namespace cricket

webrtc/media/base/videoframefactory.h

@@ -13,6 +13,7 @@
 
 #include <memory>
 
+#include "webrtc/common_video/include/i420_buffer_pool.h"
 #include "webrtc/media/base/videoframe.h"
 
 namespace cricket {
@@ -52,9 +53,9 @@ class VideoFrameFactory {
   bool apply_rotation_;
 
  private:
-  // An internal frame buffer to avoid reallocations. It is mutable because it
+  // An internal pool to avoid reallocations. It is mutable because it
   // does not affect behaviour, only performance.
-  mutable std::unique_ptr<VideoFrame> output_frame_;
+  mutable webrtc::I420BufferPool pool_;
 };
 
 }  // namespace cricket

webrtc/media/engine/webrtcvideoframe.cc

@@ -79,10 +79,6 @@ int WebRtcVideoFrame::height() const {
   return video_frame_buffer_ ? video_frame_buffer_->height() : 0;
 }
 
-bool WebRtcVideoFrame::IsExclusive() const {
-  return video_frame_buffer_->IsMutable();
-}
-
 const rtc::scoped_refptr<webrtc::VideoFrameBuffer>&
 WebRtcVideoFrame::video_frame_buffer() const {
   return video_frame_buffer_;

webrtc/media/engine/webrtcvideoframe.h

@@ -78,7 +78,7 @@ class WebRtcVideoFrame : public VideoFrame {
   webrtc::VideoRotation rotation() const override { return rotation_; }
 
   VideoFrame* Copy() const override;
-  bool IsExclusive() const override;
+
   size_t ConvertToRgbBuffer(uint32_t to_fourcc,
                             uint8_t* buffer,
                             size_t size,