From f09e7b8a4f521447ea56e3e8c5ff2f6826feacf2 Mon Sep 17 00:00:00 2001 From: "magjed@webrtc.org" Date: Wed, 25 Feb 2015 14:49:48 +0000 Subject: [PATCH] WebRtcVideoFrame: DCHECK exclusive ownership for non-const pixel access Add some const safety by DCHECK(HasOneRef()) in non-const GetYPlane. This CL also replaces all incorrect non-const calls with const calls for pixel data access in cricket::VideoFrame. It's easy to call the non-const version of e.g. GetYPlane by mistake, even if only const-access is needed. For example: const scoped_ptr foo; const uint8_t* y = foo->GetYPlane(); will actually call the non-const version of GetYPlane. R=mflodman@webrtc.org, perkj@webrtc.org Review URL: https://webrtc-codereview.appspot.com/39079004 Cr-Commit-Position: refs/heads/master@{#8507} git-svn-id: http://webrtc.googlecode.com/svn/trunk@8507 4adac7df-926f-26a2-2b94-8c16560cd09d --- talk/app/webrtc/objc/RTCI420Frame.mm | 9 +++++--- talk/media/base/videoadapter_unittest.cc | 3 ++- talk/media/base/videoframe.cc | 2 +- talk/media/base/videoframe.h | 4 ++-- talk/media/base/videoframe_unittest.h | 9 ++++---- talk/media/webrtc/webrtcvideoframe.cc | 21 ++++++++++++------- .../test/video_capture_unittest.cc | 7 ++++--- webrtc/video_engine/vie_capturer_unittest.cc | 9 ++++---- 8 files changed, 39 insertions(+), 25 deletions(-) diff --git a/talk/app/webrtc/objc/RTCI420Frame.mm b/talk/app/webrtc/objc/RTCI420Frame.mm index adbf93c009..c1a9cc0d45 100644 --- a/talk/app/webrtc/objc/RTCI420Frame.mm +++ b/talk/app/webrtc/objc/RTCI420Frame.mm @@ -55,15 +55,18 @@ } - (const uint8_t*)yPlane { - return _videoFrame->GetYPlane(); + const cricket::VideoFrame* const_frame = _videoFrame.get(); + return const_frame->GetYPlane(); } - (const uint8_t*)uPlane { - return _videoFrame->GetUPlane(); + const cricket::VideoFrame* const_frame = _videoFrame.get(); + return const_frame->GetUPlane(); } - (const uint8_t*)vPlane { - return _videoFrame->GetVPlane(); + const cricket::VideoFrame* const_frame = _videoFrame.get(); + return const_frame->GetVPlane(); } - (NSInteger)yPitch { diff --git a/talk/media/base/videoadapter_unittest.cc b/talk/media/base/videoadapter_unittest.cc index 518c18329f..995660591e 100755 --- a/talk/media/base/videoadapter_unittest.cc +++ b/talk/media/base/videoadapter_unittest.cc @@ -419,7 +419,8 @@ TEST_F(VideoAdapterTest, BlackOutput) { EXPECT_TRUE_WAIT(!capturer_->IsRunning() || listener_->GetStats().captured_frames >= 10, kWaitTimeout); // Verify that the output frame is not black. - rtc::scoped_ptr adapted_frame(listener_->CopyAdaptedFrame()); + rtc::scoped_ptr adapted_frame( + listener_->CopyAdaptedFrame()); EXPECT_NE(16, *adapted_frame->GetYPlane()); EXPECT_NE(128, *adapted_frame->GetUPlane()); EXPECT_NE(128, *adapted_frame->GetVPlane()); diff --git a/talk/media/base/videoframe.cc b/talk/media/base/videoframe.cc index 304d65b24a..77417a2769 100644 --- a/talk/media/base/videoframe.cc +++ b/talk/media/base/videoframe.cc @@ -41,7 +41,7 @@ namespace cricket { #define ROUNDTO2(v) (v & ~1) rtc::StreamResult VideoFrame::Write(rtc::StreamInterface* stream, - int* error) { + int* error) const { rtc::StreamResult result = rtc::SR_SUCCESS; const uint8* src_y = GetYPlane(); const uint8* src_u = GetUPlane(); diff --git a/talk/media/base/videoframe.h b/talk/media/base/videoframe.h index 8d39f7120f..56621ad5a4 100644 --- a/talk/media/base/videoframe.h +++ b/talk/media/base/videoframe.h @@ -159,8 +159,8 @@ class VideoFrame { // See webrtc/base/stream.h for a description of StreamResult and error. // Error may be NULL. If a non-success value is returned from // StreamInterface::Write(), we immediately return with that value. - virtual rtc::StreamResult Write(rtc::StreamInterface *stream, - int *error); + virtual rtc::StreamResult Write(rtc::StreamInterface* stream, + int* error) const; // Converts the I420 data to RGB of a certain type such as ARGB and ABGR. // Returns the frame's actual size, regardless of whether it was written or diff --git a/talk/media/base/videoframe_unittest.h b/talk/media/base/videoframe_unittest.h index 3bf0043e32..17f83c6e87 100644 --- a/talk/media/base/videoframe_unittest.h +++ b/talk/media/base/videoframe_unittest.h @@ -2007,13 +2007,14 @@ class VideoFrameTest : public testing::Test { void CopyIsRef() { rtc::scoped_ptr source(new T); - rtc::scoped_ptr target; + rtc::scoped_ptr target; ASSERT_TRUE(LoadFrameNoRepeat(source.get())); target.reset(source->Copy()); EXPECT_TRUE(IsEqual(*source, *target, 0)); - EXPECT_EQ(source->GetYPlane(), target->GetYPlane()); - EXPECT_EQ(source->GetUPlane(), target->GetUPlane()); - EXPECT_EQ(source->GetVPlane(), target->GetVPlane()); + const T* const_source = source.get(); + EXPECT_EQ(const_source->GetYPlane(), target->GetYPlane()); + EXPECT_EQ(const_source->GetUPlane(), target->GetUPlane()); + EXPECT_EQ(const_source->GetVPlane(), target->GetVPlane()); } void MakeExclusive() { diff --git a/talk/media/webrtc/webrtcvideoframe.cc b/talk/media/webrtc/webrtcvideoframe.cc index adb35aab0d..9764f31af5 100644 --- a/talk/media/webrtc/webrtcvideoframe.cc +++ b/talk/media/webrtc/webrtcvideoframe.cc @@ -207,11 +207,13 @@ const uint8* WebRtcVideoFrame::GetVPlane() const { } uint8* WebRtcVideoFrame::GetYPlane() { + DCHECK(video_buffer_->HasOneRef()); uint8_t* buffer = frame()->Buffer(); return buffer; } uint8* WebRtcVideoFrame::GetUPlane() { + DCHECK(video_buffer_->HasOneRef()); uint8_t* buffer = frame()->Buffer(); if (buffer) { buffer += (frame()->Width() * frame()->Height()); @@ -220,6 +222,7 @@ uint8* WebRtcVideoFrame::GetUPlane() { } uint8* WebRtcVideoFrame::GetVPlane() { + DCHECK(video_buffer_->HasOneRef()); uint8_t* buffer = frame()->Buffer(); if (buffer) { int uv_size = static_cast(GetChromaSize()); @@ -321,13 +324,17 @@ bool WebRtcVideoFrame::Reset(uint32 format, new_height = dw; } - size_t desired_size = SizeOf(new_width, new_height); - rtc::scoped_refptr video_buffer( - new RefCountedBuffer(desired_size)); - // Since the libyuv::ConvertToI420 will handle the rotation, so the - // new frame's rotation should always be 0. - Attach(video_buffer.get(), desired_size, new_width, new_height, pixel_width, - pixel_height, elapsed_time, time_stamp, webrtc::kVideoRotation_0); + // Release reference in |video_buffer| at the end of this scope, so that + // |video_buffer_| becomes the sole owner. + { + size_t desired_size = SizeOf(new_width, new_height); + rtc::scoped_refptr video_buffer( + new RefCountedBuffer(desired_size)); + // Since the libyuv::ConvertToI420 will handle the rotation, so the + // new frame's rotation should always be 0. + Attach(video_buffer.get(), desired_size, new_width, new_height, pixel_width, + pixel_height, elapsed_time, time_stamp, webrtc::kVideoRotation_0); + } int horiz_crop = ((w - dw) / 2) & ~1; // ARGB on Windows has negative height. diff --git a/webrtc/modules/video_capture/test/video_capture_unittest.cc b/webrtc/modules/video_capture/test/video_capture_unittest.cc index c2cb750196..ba2dbd6ddf 100644 --- a/webrtc/modules/video_capture/test/video_capture_unittest.cc +++ b/webrtc/modules/video_capture/test/video_capture_unittest.cc @@ -514,9 +514,10 @@ TEST_F(VideoCaptureExternalTest, DISABLED_TestExternalCaptureI420) { int uv_width = kTestWidth / 2; int y_rows = kTestHeight; int uv_rows = kTestHeight / 2; - unsigned char* y_plane = test_frame_.buffer(webrtc::kYPlane); - unsigned char* u_plane = test_frame_.buffer(webrtc::kUPlane); - unsigned char* v_plane = test_frame_.buffer(webrtc::kVPlane); + const webrtc::I420VideoFrame& const_test_frame = test_frame_; + const unsigned char* y_plane = const_test_frame.buffer(webrtc::kYPlane); + const unsigned char* u_plane = const_test_frame.buffer(webrtc::kUPlane); + const unsigned char* v_plane = const_test_frame.buffer(webrtc::kVPlane); // Copy Y unsigned char* current_pointer = aligned_test_frame.buffer(webrtc::kYPlane); for (int i = 0; i < y_rows; ++i) { diff --git a/webrtc/video_engine/vie_capturer_unittest.cc b/webrtc/video_engine/vie_capturer_unittest.cc index 92ef786994..7011390d7e 100644 --- a/webrtc/video_engine/vie_capturer_unittest.cc +++ b/webrtc/video_engine/vie_capturer_unittest.cc @@ -94,7 +94,7 @@ class ViECapturerTest : public ::testing::Test { data_callback_->OnIncomingCapturedFrame(0, *frame); } - void AddOutputFrame(I420VideoFrame* frame) { + void AddOutputFrame(const I420VideoFrame* frame) { if (frame->native_handle() == NULL) output_frame_ybuffers_.push_back(frame->buffer(kYPlane)); // Clone the frames because ViECapturer owns the frames. @@ -126,7 +126,7 @@ class ViECapturerTest : public ::testing::Test { // The pointers of Y plane buffers of output frames. This is used to verify // the frame are swapped and not copied. - std::vector output_frame_ybuffers_; + std::vector output_frame_ybuffers_; }; TEST_F(ViECapturerTest, TestTextureFrames) { @@ -145,10 +145,11 @@ TEST_F(ViECapturerTest, TestTextureFrames) { TEST_F(ViECapturerTest, TestI420Frames) { const int kNumFrame = 4; ScopedVector copied_input_frames; - std::vector ybuffer_pointers; + std::vector ybuffer_pointers; for (int i = 0; i < kNumFrame; ++i) { input_frames_.push_back(CreateI420VideoFrame(static_cast(i + 1))); - ybuffer_pointers.push_back(input_frames_[i]->buffer(kYPlane)); + const I420VideoFrame* const_input_frame = input_frames_[i]; + ybuffer_pointers.push_back(const_input_frame->buffer(kYPlane)); // Copy input frames because the buffer data will be swapped. copied_input_frames.push_back(input_frames_[i]->CloneFrame()); AddInputFrame(input_frames_[i]);