From dad23d06aa1c679fc2dbbabeab64f293e886b8e3 Mon Sep 17 00:00:00 2001 From: guidou Date: Thu, 14 Apr 2016 09:34:57 -0700 Subject: [PATCH] Revert of Introduce an IsMutable method on VideoFrameBuffer. (patchset #1 id:1 of https://codereview.webrtc.org/1881933004/ ) Reason for revert: This is breaking all FYI bots. The new virtual method is not implemented on the Chromium side yet. Original issue's description: > 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} TBR=magjed@webrtc.org,perkj@webrtc.org,pbos@webrtc.org,nisse@webrtc.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=webrtc:5682 Review URL: https://codereview.webrtc.org/1885943004 Cr-Commit-Position: refs/heads/master@{#12366} --- 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 | 12 +----------- webrtc/common_video/video_frame.cc | 2 +- webrtc/common_video/video_frame_buffer.cc | 15 +-------------- webrtc/media/engine/webrtcvideoframe.cc | 2 +- 6 files changed, 8 insertions(+), 32 deletions(-) diff --git a/webrtc/common_video/i420_buffer_pool.cc b/webrtc/common_video/i420_buffer_pool.cc index d00ab781aa..82a10797b3 100644 --- a/webrtc/common_video/i420_buffer_pool.cc +++ b/webrtc/common_video/i420_buffer_pool.cc @@ -29,11 +29,10 @@ 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(IsMutable()); + RTC_DCHECK(HasOneRef()); return const_cast(buffer_->data(type)); } int stride(webrtc::PlaneType type) const override { @@ -81,7 +80,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->IsMutable()) + if (buffer->HasOneRef()) 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 273b72dc91..b030ee774a 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->IsMutable()); + EXPECT_TRUE(buffer->HasOneRef()); } TEST(TestI420BufferPool, FrameValidAfterPoolDestruction) { @@ -64,7 +64,7 @@ TEST(TestI420BufferPool, FrameValidAfterPoolDestruction) { I420BufferPool pool; buffer = pool.CreateBuffer(16, 16); } - EXPECT_TRUE(buffer->IsMutable()); + 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. diff --git a/webrtc/common_video/include/video_frame_buffer.h b/webrtc/common_video/include/video_frame_buffer.h index f1b78492ba..9cf57a4359 100644 --- a/webrtc/common_video/include/video_frame_buffer.h +++ b/webrtc/common_video/include/video_frame_buffer.h @@ -33,13 +33,7 @@ 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. - virtual bool IsMutable() = 0; - - // Underlying refcount access, used to implement IsMutable. - // TODO(nisse): Demote to protected, as soon as Chrome is changed to - // use IsMutable. + // Returns true if this buffer has a single exclusive owner. virtual bool HasOneRef() const = 0; // The resolution of the frame in pixels. For formats where some planes are @@ -82,7 +76,6 @@ 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; @@ -117,7 +110,6 @@ 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_; @@ -139,8 +131,6 @@ 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 533fa2e8ba..a30f658ea0 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_->IsMutable() && + if (video_frame_buffer_ && video_frame_buffer_->HasOneRef() && !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 6ce806e35e..6f49e8aef9 100644 --- a/webrtc/common_video/video_frame_buffer.cc +++ b/webrtc/common_video/video_frame_buffer.cc @@ -89,12 +89,8 @@ const uint8_t* I420Buffer::data(PlaneType type) const { } } -bool I420Buffer::IsMutable() { - return HasOneRef(); -} - uint8_t* I420Buffer::MutableData(PlaneType type) { - RTC_DCHECK(IsMutable()); + RTC_DCHECK(HasOneRef()); return const_cast( static_cast(this)->data(type)); } @@ -148,10 +144,6 @@ NativeHandleBuffer::NativeHandleBuffer(void* native_handle, RTC_DCHECK_GT(height, 0); } -bool NativeHandleBuffer::IsMutable() { - return false; -} - int NativeHandleBuffer::width() const { return width_; } @@ -198,11 +190,6 @@ 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 9983db9a7e..9c18235bb6 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_->IsMutable(); + return video_frame_buffer_->HasOneRef(); } void* WebRtcVideoFrame::GetNativeHandle() const {