From 47fe34c2bd38927d78925174390a9e07efc3752f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Mon, 18 Apr 2016 13:02:59 +0200 Subject: [PATCH] Introduce an IsMutable method on VideoFrameBuffer. Unlike HasOneRef, it can be overridden to always return false in immutable subclasses. I'm also investigating overiding it in PooledI420Buffer, to directly inherit I420Buffer but ignore the reference from the pool. Still unclear if that will work out. BUG=webrtc:5682 Committed: https://crrev.com/6bd10f2c1ac912cbe5addd880e559d59274c60e6 Cr-Commit-Position: refs/heads/master@{#12365} R=magjed@webrtc.org, pbos@webrtc.org, perkj@webrtc.org Review URL: https://codereview.webrtc.org/1881933004 . Cr-Commit-Position: refs/heads/master@{#12404} --- webrtc/common_video/i420_buffer_pool.cc | 5 +++-- webrtc/common_video/i420_buffer_pool_unittest.cc | 4 ++-- webrtc/common_video/include/video_frame_buffer.h | 15 ++++++++++++++- webrtc/common_video/video_frame.cc | 2 +- webrtc/common_video/video_frame_buffer.cc | 15 ++++++++++++++- webrtc/media/engine/webrtcvideoframe.cc | 2 +- 6 files changed, 35 insertions(+), 8 deletions(-) diff --git a/webrtc/common_video/i420_buffer_pool.cc b/webrtc/common_video/i420_buffer_pool.cc index 82a10797b3..d00ab781aa 100644 --- a/webrtc/common_video/i420_buffer_pool.cc +++ b/webrtc/common_video/i420_buffer_pool.cc @@ -29,10 +29,11 @@ class PooledI420Buffer : public webrtc::VideoFrameBuffer { const uint8_t* data(webrtc::PlaneType type) const override { return buffer_->data(type); } + bool IsMutable() { return HasOneRef(); } uint8_t* MutableData(webrtc::PlaneType type) override { // Make the HasOneRef() check here instead of in |buffer_|, because the pool // also has a reference to |buffer_|. - RTC_DCHECK(HasOneRef()); + RTC_DCHECK(IsMutable()); return const_cast(buffer_->data(type)); } int stride(webrtc::PlaneType type) const override { @@ -80,7 +81,7 @@ rtc::scoped_refptr I420BufferPool::CreateBuffer(int width, // 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()) + if (buffer->IsMutable()) return new rtc::RefCountedObject(buffer); } // Allocate new buffer. diff --git a/webrtc/common_video/i420_buffer_pool_unittest.cc b/webrtc/common_video/i420_buffer_pool_unittest.cc index b030ee774a..273b72dc91 100644 --- a/webrtc/common_video/i420_buffer_pool_unittest.cc +++ b/webrtc/common_video/i420_buffer_pool_unittest.cc @@ -55,7 +55,7 @@ TEST(TestI420BufferPool, ExclusiveOwner) { // Check that created buffers are exclusive so that they can be written to. I420BufferPool pool; rtc::scoped_refptr buffer = pool.CreateBuffer(16, 16); - EXPECT_TRUE(buffer->HasOneRef()); + EXPECT_TRUE(buffer->IsMutable()); } TEST(TestI420BufferPool, FrameValidAfterPoolDestruction) { @@ -64,7 +64,7 @@ TEST(TestI420BufferPool, FrameValidAfterPoolDestruction) { I420BufferPool pool; buffer = pool.CreateBuffer(16, 16); } - EXPECT_TRUE(buffer->HasOneRef()); + 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. diff --git a/webrtc/common_video/include/video_frame_buffer.h b/webrtc/common_video/include/video_frame_buffer.h index 9cf57a4359..e78a1d6968 100644 --- a/webrtc/common_video/include/video_frame_buffer.h +++ b/webrtc/common_video/include/video_frame_buffer.h @@ -33,7 +33,16 @@ enum PlaneType { // not contain any frame metadata such as rotation, timestamp, pixel_width, etc. class VideoFrameBuffer : public rtc::RefCountInterface { public: - // Returns true if this buffer has a single exclusive owner. + // 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 @@ -76,6 +85,7 @@ class I420Buffer : public VideoFrameBuffer { const uint8_t* data(PlaneType type) const override; // Non-const data access is only allowed if HasOneRef() is true to protect // against unexpected overwrites. + bool IsMutable() override; uint8_t* MutableData(PlaneType type) override; int stride(PlaneType type) const override; void* native_handle() const override; @@ -110,6 +120,7 @@ class NativeHandleBuffer : public VideoFrameBuffer { const uint8_t* data(PlaneType type) const override; int stride(PlaneType type) const override; void* native_handle() const override; + bool IsMutable() override; protected: void* native_handle_; @@ -131,6 +142,8 @@ class WrappedI420Buffer : public webrtc::VideoFrameBuffer { int width() const override; int height() const override; + bool IsMutable() override; + const uint8_t* data(PlaneType type) const override; int stride(PlaneType type) const override; diff --git a/webrtc/common_video/video_frame.cc b/webrtc/common_video/video_frame.cc index 28e463112c..3f817c0e8f 100644 --- a/webrtc/common_video/video_frame.cc +++ b/webrtc/common_video/video_frame.cc @@ -65,7 +65,7 @@ void VideoFrame::CreateEmptyFrame(int width, rotation_ = kVideoRotation_0; // Check if it's safe to reuse allocation. - if (video_frame_buffer_ && video_frame_buffer_->HasOneRef() && + 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 == stride(kYPlane) && diff --git a/webrtc/common_video/video_frame_buffer.cc b/webrtc/common_video/video_frame_buffer.cc index 6f49e8aef9..6ce806e35e 100644 --- a/webrtc/common_video/video_frame_buffer.cc +++ b/webrtc/common_video/video_frame_buffer.cc @@ -89,8 +89,12 @@ const uint8_t* I420Buffer::data(PlaneType type) const { } } +bool I420Buffer::IsMutable() { + return HasOneRef(); +} + uint8_t* I420Buffer::MutableData(PlaneType type) { - RTC_DCHECK(HasOneRef()); + RTC_DCHECK(IsMutable()); return const_cast( static_cast(this)->data(type)); } @@ -144,6 +148,10 @@ NativeHandleBuffer::NativeHandleBuffer(void* native_handle, RTC_DCHECK_GT(height, 0); } +bool NativeHandleBuffer::IsMutable() { + return false; +} + int NativeHandleBuffer::width() const { return width_; } @@ -190,6 +198,11 @@ WrappedI420Buffer::~WrappedI420Buffer() { no_longer_used_cb_(); } +// Data owned by creator; never mutable. +bool WrappedI420Buffer::IsMutable() { + return false; +} + int WrappedI420Buffer::width() const { return width_; } diff --git a/webrtc/media/engine/webrtcvideoframe.cc b/webrtc/media/engine/webrtcvideoframe.cc index 9c18235bb6..9983db9a7e 100644 --- a/webrtc/media/engine/webrtcvideoframe.cc +++ b/webrtc/media/engine/webrtcvideoframe.cc @@ -119,7 +119,7 @@ int32_t WebRtcVideoFrame::GetVPitch() const { } bool WebRtcVideoFrame::IsExclusive() const { - return video_frame_buffer_->HasOneRef(); + return video_frame_buffer_->IsMutable(); } void* WebRtcVideoFrame::GetNativeHandle() const {