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-Original-Commit-Position: refs/heads/master@{#12404}
Cr-Mirrored-From: https://chromium.googlesource.com/external/webrtc
Cr-Mirrored-Commit: 47fe34c2bd38927d78925174390a9e07efc3752f
diff --git a/common_video/i420_buffer_pool.cc b/common_video/i420_buffer_pool.cc
index 82a1079..d00ab78 100644
--- a/common_video/i420_buffer_pool.cc
+++ b/common_video/i420_buffer_pool.cc
@@ -29,10 +29,11 @@
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<uint8_t*>(buffer_->data(type));
}
int stride(webrtc::PlaneType type) const override {
@@ -80,7 +81,7 @@
// 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<PooledI420Buffer>(buffer);
}
// Allocate new buffer.
diff --git a/common_video/i420_buffer_pool_unittest.cc b/common_video/i420_buffer_pool_unittest.cc
index b030ee7..273b72d 100644
--- a/common_video/i420_buffer_pool_unittest.cc
+++ b/common_video/i420_buffer_pool_unittest.cc
@@ -55,7 +55,7 @@
// 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());
+ EXPECT_TRUE(buffer->IsMutable());
}
TEST(TestI420BufferPool, FrameValidAfterPoolDestruction) {
@@ -64,7 +64,7 @@
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/common_video/include/video_frame_buffer.h b/common_video/include/video_frame_buffer.h
index 9cf57a4..e78a1d6 100644
--- a/common_video/include/video_frame_buffer.h
+++ b/common_video/include/video_frame_buffer.h
@@ -33,7 +33,16 @@
// 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 @@
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 @@
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 @@
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/common_video/video_frame.cc b/common_video/video_frame.cc
index 28e4631..3f817c0 100644
--- a/common_video/video_frame.cc
+++ b/common_video/video_frame.cc
@@ -65,7 +65,7 @@
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/common_video/video_frame_buffer.cc b/common_video/video_frame_buffer.cc
index 6f49e8a..6ce806e 100644
--- a/common_video/video_frame_buffer.cc
+++ b/common_video/video_frame_buffer.cc
@@ -89,8 +89,12 @@
}
}
+bool I420Buffer::IsMutable() {
+ return HasOneRef();
+}
+
uint8_t* I420Buffer::MutableData(PlaneType type) {
- RTC_DCHECK(HasOneRef());
+ RTC_DCHECK(IsMutable());
return const_cast<uint8_t*>(
static_cast<const VideoFrameBuffer*>(this)->data(type));
}
@@ -144,6 +148,10 @@
RTC_DCHECK_GT(height, 0);
}
+bool NativeHandleBuffer::IsMutable() {
+ return false;
+}
+
int NativeHandleBuffer::width() const {
return width_;
}
@@ -190,6 +198,11 @@
no_longer_used_cb_();
}
+// Data owned by creator; never mutable.
+bool WrappedI420Buffer::IsMutable() {
+ return false;
+}
+
int WrappedI420Buffer::width() const {
return width_;
}
diff --git a/media/engine/webrtcvideoframe.cc b/media/engine/webrtcvideoframe.cc
index 9c18235..9983db9 100644
--- a/media/engine/webrtcvideoframe.cc
+++ b/media/engine/webrtcvideoframe.cc
@@ -119,7 +119,7 @@
}
bool WebRtcVideoFrame::IsExclusive() const {
- return video_frame_buffer_->HasOneRef();
+ return video_frame_buffer_->IsMutable();
}
void* WebRtcVideoFrame::GetNativeHandle() const {
