From 3318f984cd7f51d24da4726665c05f5f06f82e6d Mon Sep 17 00:00:00 2001 From: Magnus Jedvert Date: Wed, 26 Aug 2015 16:06:21 +0200 Subject: [PATCH] VideoFrameBuffer: Make non-const data access explicit VideoFrameBuffer currently has two overloaded data() functions for pixel access, one for const and one for non-const. Unfortunately, it will default to the non-const version, even when 'const scoped_refptr&' is used. This is a problem, because many subclasses use RTC_NOTREACHED() in the non-const version. This CL makes the non-const version of data() explicit with a different, longer function name MutableData(). R=tommi@webrtc.org Review URL: https://codereview.webrtc.org/1304143003 . Cr-Commit-Position: refs/heads/master@{#9787} --- talk/media/webrtc/webrtcvideoframe.cc | 44 +++++++++---------- webrtc/common_video/i420_buffer_pool.cc | 10 ++--- .../common_video/i420_buffer_pool_unittest.cc | 2 +- .../interface/video_frame_buffer.h | 11 ++--- webrtc/common_video/video_frame.cc | 7 ++- webrtc/common_video/video_frame_buffer.cc | 25 ++++------- .../codecs/h264/h264_video_toolbox_decoder.cc | 6 +-- webrtc/test/fake_texture_frame.h | 6 +-- 8 files changed, 51 insertions(+), 60 deletions(-) diff --git a/talk/media/webrtc/webrtcvideoframe.cc b/talk/media/webrtc/webrtcvideoframe.cc index 055e24d3cd..e72ab144f0 100644 --- a/talk/media/webrtc/webrtcvideoframe.cc +++ b/talk/media/webrtc/webrtcvideoframe.cc @@ -117,35 +117,32 @@ size_t WebRtcVideoFrame::GetHeight() const { } const uint8* WebRtcVideoFrame::GetYPlane() const { - // Const cast to call the correct const-version of data. - const webrtc::VideoFrameBuffer* const_ptr = video_frame_buffer_.get(); - return const_ptr ? const_ptr->data(kYPlane) : nullptr; -} - -const uint8* WebRtcVideoFrame::GetUPlane() const { - // Const cast to call the correct const-version of data. - const webrtc::VideoFrameBuffer* const_ptr = video_frame_buffer_.get(); - return const_ptr ? const_ptr->data(kUPlane) : nullptr; -} - -const uint8* WebRtcVideoFrame::GetVPlane() const { - // Const cast to call the correct const-version of data. - const webrtc::VideoFrameBuffer* const_ptr = video_frame_buffer_.get(); - return const_ptr ? const_ptr->data(kVPlane) : nullptr; -} - -uint8* WebRtcVideoFrame::GetYPlane() { return video_frame_buffer_ ? video_frame_buffer_->data(kYPlane) : nullptr; } -uint8* WebRtcVideoFrame::GetUPlane() { +const uint8* WebRtcVideoFrame::GetUPlane() const { return video_frame_buffer_ ? video_frame_buffer_->data(kUPlane) : nullptr; } -uint8* WebRtcVideoFrame::GetVPlane() { +const uint8* WebRtcVideoFrame::GetVPlane() const { return video_frame_buffer_ ? video_frame_buffer_->data(kVPlane) : nullptr; } +uint8* WebRtcVideoFrame::GetYPlane() { + return video_frame_buffer_ ? video_frame_buffer_->MutableData(kYPlane) + : nullptr; +} + +uint8* WebRtcVideoFrame::GetUPlane() { + return video_frame_buffer_ ? video_frame_buffer_->MutableData(kUPlane) + : nullptr; +} + +uint8* WebRtcVideoFrame::GetVPlane() { + return video_frame_buffer_ ? video_frame_buffer_->MutableData(kVPlane) + : nullptr; +} + int32 WebRtcVideoFrame::GetYPitch() const { return video_frame_buffer_ ? video_frame_buffer_->stride(kYPlane) : 0; } @@ -192,9 +189,10 @@ bool WebRtcVideoFrame::MakeExclusive() { video_frame_buffer_->stride(kUPlane), video_frame_buffer_->stride(kVPlane)); - if (!CopyToPlanes(new_buffer->data(kYPlane), new_buffer->data(kUPlane), - new_buffer->data(kVPlane), new_buffer->stride(kYPlane), - new_buffer->stride(kUPlane), new_buffer->stride(kVPlane))) { + if (!CopyToPlanes( + new_buffer->MutableData(kYPlane), new_buffer->MutableData(kUPlane), + new_buffer->MutableData(kVPlane), new_buffer->stride(kYPlane), + new_buffer->stride(kUPlane), new_buffer->stride(kVPlane))) { return false; } diff --git a/webrtc/common_video/i420_buffer_pool.cc b/webrtc/common_video/i420_buffer_pool.cc index 4b8695824c..cb1f4d4022 100644 --- a/webrtc/common_video/i420_buffer_pool.cc +++ b/webrtc/common_video/i420_buffer_pool.cc @@ -27,13 +27,13 @@ class PooledI420Buffer : public webrtc::VideoFrameBuffer { 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); + return buffer_->data(type); } - uint8_t* data(webrtc::PlaneType type) { + 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_|. DCHECK(HasOneRef()); - const webrtc::I420Buffer* cbuffer = buffer_.get(); - return const_cast(cbuffer->data(type)); + return const_cast(buffer_->data(type)); } int stride(webrtc::PlaneType type) const override { return buffer_->stride(type); diff --git a/webrtc/common_video/i420_buffer_pool_unittest.cc b/webrtc/common_video/i420_buffer_pool_unittest.cc index 625160be11..a1596ebb09 100644 --- a/webrtc/common_video/i420_buffer_pool_unittest.cc +++ b/webrtc/common_video/i420_buffer_pool_unittest.cc @@ -68,7 +68,7 @@ TEST(TestI420BufferPool, FrameValidAfterPoolDestruction) { 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)); + memset(buffer->MutableData(kYPlane), 0xA5, 16 * buffer->stride(kYPlane)); } } // namespace webrtc diff --git a/webrtc/common_video/interface/video_frame_buffer.h b/webrtc/common_video/interface/video_frame_buffer.h index a7ac8d84d7..ed1df488c2 100644 --- a/webrtc/common_video/interface/video_frame_buffer.h +++ b/webrtc/common_video/interface/video_frame_buffer.h @@ -42,8 +42,9 @@ class VideoFrameBuffer : public rtc::RefCountInterface { // the VideoFrameBuffer object and must not be freed by the caller. virtual const uint8_t* data(PlaneType type) const = 0; - // Non-const data access is only allowed if |HasOneRef| is true. - virtual uint8_t* data(PlaneType type) = 0; + // Non-const data access is disallowed by default. You need to make sure you + // have exclusive access and a writable buffer before calling this function. + virtual uint8_t* MutableData(PlaneType type); // Returns the number of bytes between successive rows for a given plane. virtual int stride(PlaneType type) const = 0; @@ -69,7 +70,9 @@ class I420Buffer : public VideoFrameBuffer { int width() const override; int height() const override; const uint8_t* data(PlaneType type) const override; - uint8_t* data(PlaneType type) override; + // Non-const data access is only allowed if HasOneRef() is true to protect + // against unexpected overwrites. + uint8_t* MutableData(PlaneType type) override; int stride(PlaneType type) const override; void* native_handle() const override; rtc::scoped_refptr NativeToI420Buffer() override; @@ -97,7 +100,6 @@ class NativeHandleBuffer : public VideoFrameBuffer { int width() const override; int height() const override; const uint8_t* data(PlaneType type) const override; - uint8_t* data(PlaneType type) override; int stride(PlaneType type) const override; void* native_handle() const override; @@ -122,7 +124,6 @@ class WrappedI420Buffer : public webrtc::VideoFrameBuffer { int height() const override; const uint8_t* data(PlaneType type) const override; - uint8_t* data(PlaneType type) override; int stride(PlaneType type) const override; void* native_handle() const override; diff --git a/webrtc/common_video/video_frame.cc b/webrtc/common_video/video_frame.cc index 208a31b618..0ebb9835d5 100644 --- a/webrtc/common_video/video_frame.cc +++ b/webrtc/common_video/video_frame.cc @@ -152,13 +152,12 @@ void VideoFrame::Reset() { } uint8_t* VideoFrame::buffer(PlaneType type) { - return video_frame_buffer_ ? video_frame_buffer_->data(type) : nullptr; + return video_frame_buffer_ ? video_frame_buffer_->MutableData(type) + : nullptr; } const uint8_t* VideoFrame::buffer(PlaneType type) const { - // Const cast to call the correct const-version of data. - const VideoFrameBuffer* const_buffer = video_frame_buffer_.get(); - return const_buffer ? const_buffer->data(type) : nullptr; + return video_frame_buffer_ ? video_frame_buffer_->data(type) : nullptr; } int VideoFrame::allocated_size(PlaneType type) const { diff --git a/webrtc/common_video/video_frame_buffer.cc b/webrtc/common_video/video_frame_buffer.cc index cad33e8e97..4c15958041 100644 --- a/webrtc/common_video/video_frame_buffer.cc +++ b/webrtc/common_video/video_frame_buffer.cc @@ -24,6 +24,11 @@ static void NoLongerUsedCallback(rtc::scoped_refptr dummy) {} } // anonymous namespace +uint8_t* VideoFrameBuffer::MutableData(PlaneType type) { + RTC_NOTREACHED(); + return nullptr; +} + VideoFrameBuffer::~VideoFrameBuffer() {} I420Buffer::I420Buffer(int width, int height) @@ -76,7 +81,7 @@ const uint8_t* I420Buffer::data(PlaneType type) const { } } -uint8_t* I420Buffer::data(PlaneType type) { +uint8_t* I420Buffer::MutableData(PlaneType type) { DCHECK(HasOneRef()); return const_cast( static_cast(this)->data(type)); @@ -127,11 +132,6 @@ const uint8_t* NativeHandleBuffer::data(PlaneType type) const { return nullptr; } -uint8_t* NativeHandleBuffer::data(PlaneType type) { - RTC_NOTREACHED(); // Should not be called. - return nullptr; -} - int NativeHandleBuffer::stride(PlaneType type) const { RTC_NOTREACHED(); // Should not be called. return 0; @@ -187,11 +187,6 @@ const uint8_t* WrappedI420Buffer::data(PlaneType type) const { } } -uint8_t* WrappedI420Buffer::data(PlaneType type) { - RTC_NOTREACHED(); - return nullptr; -} - int WrappedI420Buffer::stride(PlaneType type) const { switch (type) { case kYPlane: @@ -232,13 +227,11 @@ rtc::scoped_refptr ShallowCenterCrop( const int offset_x = uv_offset_x * 2; const int offset_y = uv_offset_y * 2; - // Const cast to call the correct const-version of data(). - const VideoFrameBuffer* const_buffer(buffer.get()); - const uint8_t* y_plane = const_buffer->data(kYPlane) + + const uint8_t* y_plane = buffer->data(kYPlane) + buffer->stride(kYPlane) * offset_y + offset_x; - const uint8_t* u_plane = const_buffer->data(kUPlane) + + const uint8_t* u_plane = buffer->data(kUPlane) + buffer->stride(kUPlane) * uv_offset_y + uv_offset_x; - const uint8_t* v_plane = const_buffer->data(kVPlane) + + const uint8_t* v_plane = buffer->data(kVPlane) + buffer->stride(kVPlane) * uv_offset_y + uv_offset_x; return new rtc::RefCountedObject( cropped_width, cropped_height, diff --git a/webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc b/webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc index e905fd0199..c80ccbb73a 100644 --- a/webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc +++ b/webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc @@ -64,9 +64,9 @@ rtc::scoped_refptr VideoFrameBufferForPixelBuffer( int src_uv_stride = CVPixelBufferGetBytesPerRowOfPlane(pixel_buffer, 1); int ret = libyuv::NV12ToI420( src_y, src_y_stride, src_uv, src_uv_stride, - buffer->data(webrtc::kYPlane), buffer->stride(webrtc::kYPlane), - buffer->data(webrtc::kUPlane), buffer->stride(webrtc::kUPlane), - buffer->data(webrtc::kVPlane), buffer->stride(webrtc::kVPlane), + buffer->MutableData(webrtc::kYPlane), buffer->stride(webrtc::kYPlane), + buffer->MutableData(webrtc::kUPlane), buffer->stride(webrtc::kUPlane), + buffer->MutableData(webrtc::kVPlane), buffer->stride(webrtc::kVPlane), width, height); CVPixelBufferUnlockBaseAddress(pixel_buffer, kCVPixelBufferLock_ReadOnly); if (ret) { diff --git a/webrtc/test/fake_texture_frame.h b/webrtc/test/fake_texture_frame.h index 682e7b6bae..dc6abaf745 100644 --- a/webrtc/test/fake_texture_frame.h +++ b/webrtc/test/fake_texture_frame.h @@ -34,9 +34,9 @@ class FakeNativeHandleBuffer : public NativeHandleBuffer { new rtc::RefCountedObject(width_, height_)); int half_height = (height_ + 1) / 2; int half_width = (width_ + 1) / 2; - memset(buffer->data(kYPlane), 0, height_ * width_); - memset(buffer->data(kUPlane), 0, half_height * half_width); - memset(buffer->data(kVPlane), 0, half_height * half_width); + memset(buffer->MutableData(kYPlane), 0, height_ * width_); + memset(buffer->MutableData(kUPlane), 0, half_height * half_width); + memset(buffer->MutableData(kVPlane), 0, half_height * half_width); return buffer; } };