From 97ed2a4b70eb3f78bb2e1f51a14bbdfeb1783502 Mon Sep 17 00:00:00 2001 From: "magjed@webrtc.org" Date: Mon, 2 Mar 2015 17:33:26 +0000 Subject: [PATCH] I420VideoFrame: Remove function ResetSize This is a partial reland of https://webrtc-codereview.appspot.com/39939004/. The original CL was reverted because ViECapturer use ResetSize/IsZeroSize on |captured_frame_| as a check to make sure each captured frame is only delivered once. Removing ResetSize introduced a race condition where a captured frame could be delivered multiple times. I have fixed this problem in this CL by replacing ResetSize with scoped_ptr::release. BUG=4352 R=perkj@webrtc.org, tommi@webrtc.org Review URL: https://webrtc-codereview.appspot.com/39359004 Cr-Commit-Position: refs/heads/master@{#8561} git-svn-id: http://webrtc.googlecode.com/svn/trunk@8561 4adac7df-926f-26a2-2b94-8c16560cd09d --- webrtc/common_video/i420_video_frame.cc | 6 ------ webrtc/common_video/i420_video_frame_unittest.cc | 8 -------- webrtc/common_video/interface/texture_video_frame.h | 1 - webrtc/common_video/texture_video_frame.cc | 4 ---- .../main/source/video_sender_unittest.cc | 6 +++--- webrtc/modules/video_render/video_render_frames.cc | 1 - webrtc/video_engine/vie_capturer.cc | 13 +------------ webrtc/video_engine/vie_capturer_unittest.cc | 4 ---- webrtc/video_frame.h | 4 ---- 9 files changed, 4 insertions(+), 43 deletions(-) diff --git a/webrtc/common_video/i420_video_frame.cc b/webrtc/common_video/i420_video_frame.cc index 829d5a7d49..d95697032f 100644 --- a/webrtc/common_video/i420_video_frame.cc +++ b/webrtc/common_video/i420_video_frame.cc @@ -156,12 +156,6 @@ bool I420VideoFrame::IsZeroSize() const { v_plane_.IsZeroSize()); } -void I420VideoFrame::ResetSize() { - y_plane_.ResetSize(); - u_plane_.ResetSize(); - v_plane_.ResetSize(); -} - void* I420VideoFrame::native_handle() const { return NULL; } int I420VideoFrame::CheckDimensions(int width, int height, diff --git a/webrtc/common_video/i420_video_frame_unittest.cc b/webrtc/common_video/i420_video_frame_unittest.cc index 1679df8567..47017efe10 100644 --- a/webrtc/common_video/i420_video_frame_unittest.cc +++ b/webrtc/common_video/i420_video_frame_unittest.cc @@ -71,14 +71,6 @@ TEST(TestI420VideoFrame, SizeAllocation) { frame.allocated_size(kVPlane)); } -TEST(TestI420VideoFrame, ResetSize) { - I420VideoFrame frame; - EXPECT_EQ(0, frame. CreateEmptyFrame(10, 10, 12, 14, 220)); - EXPECT_FALSE(frame.IsZeroSize()); - frame.ResetSize(); - EXPECT_TRUE(frame.IsZeroSize()); -} - TEST(TestI420VideoFrame, CopyFrame) { uint32_t timestamp = 1; int64_t ntp_time_ms = 2; diff --git a/webrtc/common_video/interface/texture_video_frame.h b/webrtc/common_video/interface/texture_video_frame.h index 9a1fee0bc4..225a0ec042 100644 --- a/webrtc/common_video/interface/texture_video_frame.h +++ b/webrtc/common_video/interface/texture_video_frame.h @@ -68,7 +68,6 @@ class TextureVideoFrame : public I420VideoFrame { virtual int allocated_size(PlaneType type) const OVERRIDE; virtual int stride(PlaneType type) const OVERRIDE; virtual bool IsZeroSize() const OVERRIDE; - virtual void ResetSize() OVERRIDE; virtual void* native_handle() const OVERRIDE; protected: diff --git a/webrtc/common_video/texture_video_frame.cc b/webrtc/common_video/texture_video_frame.cc index a03e096319..7772f128b6 100644 --- a/webrtc/common_video/texture_video_frame.cc +++ b/webrtc/common_video/texture_video_frame.cc @@ -107,10 +107,6 @@ bool TextureVideoFrame::IsZeroSize() const { return true; } -void TextureVideoFrame::ResetSize() { - assert(false); // Should not be called. -} - void* TextureVideoFrame::native_handle() const { return handle_.get(); } int TextureVideoFrame::CheckDimensions( diff --git a/webrtc/modules/video_coding/main/source/video_sender_unittest.cc b/webrtc/modules/video_coding/main/source/video_sender_unittest.cc index a59d0c095c..6666e4b786 100644 --- a/webrtc/modules/video_coding/main/source/video_sender_unittest.cc +++ b/webrtc/modules/video_coding/main/source/video_sender_unittest.cc @@ -71,12 +71,12 @@ MATCHER_P(MatchesVp8StreamInfo, expected, "") { class EmptyFrameGenerator : public FrameGenerator { public: virtual I420VideoFrame* NextFrame() OVERRIDE { - frame_.ResetSize(); - return &frame_; + frame_.reset(new I420VideoFrame()); + return frame_.get(); } private: - I420VideoFrame frame_; + rtc::scoped_ptr frame_; }; class PacketizationCallback : public VCMPacketizationCallback { diff --git a/webrtc/modules/video_render/video_render_frames.cc b/webrtc/modules/video_render/video_render_frames.cc index d790877e31..3df4ea40b1 100644 --- a/webrtc/modules/video_render/video_render_frames.cc +++ b/webrtc/modules/video_render/video_render_frames.cc @@ -125,7 +125,6 @@ I420VideoFrame* VideoRenderFrames::FrameToRender() { int32_t VideoRenderFrames::ReturnFrame(I420VideoFrame* old_frame) { // No need to reuse texture frames because they do not allocate memory. if (old_frame->native_handle() == NULL) { - old_frame->ResetSize(); old_frame->set_timestamp(0); old_frame->set_render_time_ms(0); empty_frames_.push_back(old_frame); diff --git a/webrtc/video_engine/vie_capturer.cc b/webrtc/video_engine/vie_capturer.cc index a2f0fd5f75..9d41a40c91 100644 --- a/webrtc/video_engine/vie_capturer.cc +++ b/webrtc/video_engine/vie_capturer.cc @@ -610,18 +610,7 @@ bool ViECapturer::SwapCapturedAndDeliverFrameIfAvailable() { if (captured_frame_ == NULL) return false; - if (captured_frame_->native_handle() != NULL) { - deliver_frame_.reset(captured_frame_.release()); - return true; - } - - if (captured_frame_->IsZeroSize()) - return false; - - if (deliver_frame_ == NULL) - deliver_frame_.reset(new I420VideoFrame()); - deliver_frame_->SwapFrame(captured_frame_.get()); - captured_frame_->ResetSize(); + deliver_frame_.reset(captured_frame_.release()); return true; } diff --git a/webrtc/video_engine/vie_capturer_unittest.cc b/webrtc/video_engine/vie_capturer_unittest.cc index ca1f25fcec..bb61c4c3a5 100644 --- a/webrtc/video_engine/vie_capturer_unittest.cc +++ b/webrtc/video_engine/vie_capturer_unittest.cc @@ -161,10 +161,6 @@ TEST_F(ViECapturerTest, TestI420Frames) { // Make sure the buffer is swapped and not copied. for (int i = 0; i < kNumFrame; ++i) EXPECT_EQ(ybuffer_pointers[i], output_frame_ybuffers_[i]); - // The pipeline should be filled with frames with allocated buffers. Check - // the last input frame has the same allocated size after swapping. - EXPECT_EQ(input_frames_.back()->allocated_size(kYPlane), - copied_input_frames.back()->allocated_size(kYPlane)); } TEST_F(ViECapturerTest, TestI420FrameAfterTextureFrame) { diff --git a/webrtc/video_frame.h b/webrtc/video_frame.h index 451dd3286a..3f673cd3a9 100644 --- a/webrtc/video_frame.h +++ b/webrtc/video_frame.h @@ -157,10 +157,6 @@ class I420VideoFrame { // Return true if underlying plane buffers are of zero size, false if not. virtual bool IsZeroSize() const; - // Reset underlying plane buffers sizes to 0. This function doesn't - // clear memory. - virtual void ResetSize(); - // Return the handle of the underlying video frame. This is used when the // frame is backed by a texture. The object should be destroyed when it is no // longer in use, so the underlying resource can be freed.