From 5a477a0bc6fd3657eb7173d2c8a2e1a01a88dbb3 Mon Sep 17 00:00:00 2001 From: "pbos@webrtc.org" Date: Wed, 18 Mar 2015 14:11:39 +0000 Subject: [PATCH] DCHECK frame parameters instead of return codes. We should never be creating video frames without width/height. If these DCHECKs fire we should be fixing the calling code instead. BUG=4359 R=magjed@webrtc.org, tommi@webrtc.org Review URL: https://webrtc-codereview.appspot.com/46639004 Cr-Commit-Position: refs/heads/master@{#8779} git-svn-id: http://webrtc.googlecode.com/svn/trunk@8779 4adac7df-926f-26a2-2b94-8c16560cd09d --- webrtc/common_video/i420_video_frame.cc | 20 ++++++++++++------- .../common_video/i420_video_frame_unittest.cc | 8 -------- .../unit_test/video_processing_unittest.cc | 20 ------------------- webrtc/video_frame.h | 3 +++ 4 files changed, 16 insertions(+), 35 deletions(-) diff --git a/webrtc/common_video/i420_video_frame.cc b/webrtc/common_video/i420_video_frame.cc index 05b72cf629..995565cd3b 100644 --- a/webrtc/common_video/i420_video_frame.cc +++ b/webrtc/common_video/i420_video_frame.cc @@ -47,15 +47,20 @@ I420VideoFrame::I420VideoFrame(NativeHandle* handle, ntp_time_ms_(0), render_time_ms_(render_time_ms), rotation_(kVideoRotation_0) { + DCHECK(handle != nullptr); + DCHECK_GT(width, 0); + DCHECK_GT(height, 0); } int I420VideoFrame::CreateEmptyFrame(int width, int height, int stride_y, int stride_u, int stride_v) { const int half_width = (width + 1) / 2; - if (width <= 0 || height <= 0 || stride_y < width || stride_u < half_width || - stride_v < half_width) { - return -1; - } + DCHECK_GT(width, 0); + DCHECK_GT(height, 0); + DCHECK_GE(stride_y, width); + DCHECK_GE(stride_u, half_width); + DCHECK_GE(stride_v, half_width); + // Creating empty frame - reset all values. timestamp_ = 0; ntp_time_ms_ = 0; @@ -105,8 +110,7 @@ int I420VideoFrame::CreateFrame(const uint8_t* buffer_y, const int expected_size_y = height * stride_y; const int expected_size_u = half_height * stride_u; const int expected_size_v = half_height * stride_v; - if (CreateEmptyFrame(width, height, stride_y, stride_u, stride_v) < 0) - return -1; + CreateEmptyFrame(width, height, stride_y, stride_u, stride_v); memcpy(buffer(kYPlane), buffer_y, expected_size_y); memcpy(buffer(kUPlane), buffer_u, expected_size_u); memcpy(buffer(kVPlane), buffer_v, expected_size_v); @@ -145,7 +149,9 @@ void I420VideoFrame::ShallowCopy(const I420VideoFrame& videoFrame) { I420VideoFrame* I420VideoFrame::CloneFrame() const { rtc::scoped_ptr new_frame(new I420VideoFrame()); if (new_frame->CopyFrame(*this) == -1) { - // CopyFrame failed. + // TODO(pbos): Make void, not war. + // CopyFrame failed this shouldn't happen. + RTC_NOTREACHED(); return NULL; } return new_frame.release(); diff --git a/webrtc/common_video/i420_video_frame_unittest.cc b/webrtc/common_video/i420_video_frame_unittest.cc index e9e117eae3..2208a871c5 100644 --- a/webrtc/common_video/i420_video_frame_unittest.cc +++ b/webrtc/common_video/i420_video_frame_unittest.cc @@ -42,16 +42,8 @@ int ExpectedSize(int plane_stride, int image_height, PlaneType type); TEST(TestI420VideoFrame, InitialValues) { I420VideoFrame frame; - // Invalid arguments - one call for each variable. EXPECT_TRUE(frame.IsZeroSize()); EXPECT_EQ(kVideoRotation_0, frame.rotation()); - EXPECT_EQ(-1, frame.CreateEmptyFrame(0, 10, 10, 14, 14)); - EXPECT_EQ(-1, frame.CreateEmptyFrame(10, -1, 10, 90, 14)); - EXPECT_EQ(-1, frame.CreateEmptyFrame(10, 10, 0, 14, 18)); - EXPECT_EQ(-1, frame.CreateEmptyFrame(10, 10, 10, -2, 13)); - EXPECT_EQ(-1, frame.CreateEmptyFrame(10, 10, 10, 14, 0)); - EXPECT_EQ(0, frame.CreateEmptyFrame(10, 10, 10, 14, 90)); - EXPECT_FALSE(frame.IsZeroSize()); } TEST(TestI420VideoFrame, WidthHeightValues) { diff --git a/webrtc/modules/video_processing/main/test/unit_test/video_processing_unittest.cc b/webrtc/modules/video_processing/main/test/unit_test/video_processing_unittest.cc index 60a2e41c4c..8d3fcd6531 100644 --- a/webrtc/modules/video_processing/main/test/unit_test/video_processing_unittest.cc +++ b/webrtc/modules/video_processing/main/test/unit_test/video_processing_unittest.cc @@ -114,26 +114,6 @@ TEST_F(VideoProcessingModuleTest, HandleBadStats) { EXPECT_EQ(-3, vpm_->BrightnessDetection(video_frame_, stats)); } -TEST_F(VideoProcessingModuleTest, HandleBadSize) { - VideoProcessingModule::FrameStats stats; - - I420VideoFrame bad_frame; - bad_frame.CreateEmptyFrame(width_, 0, width_, (width_ + 1) / 2, - (width_ + 1) / 2); - EXPECT_EQ(-3, vpm_->GetFrameStats(&stats, bad_frame)); - - EXPECT_EQ(-1, vpm_->ColorEnhancement(&bad_frame)); - - EXPECT_EQ(-1, vpm_->Deflickering(&bad_frame, &stats)); - - EXPECT_EQ(-3, vpm_->BrightnessDetection(bad_frame, stats)); - - EXPECT_EQ(VPM_PARAMETER_ERROR, vpm_->SetTargetResolution(0,0,0)); - - I420VideoFrame *out_frame = NULL; - EXPECT_EQ(VPM_PARAMETER_ERROR, vpm_->PreprocessFrame(bad_frame, &out_frame)); -} - TEST_F(VideoProcessingModuleTest, IdenticalResultsAfterReset) { I420VideoFrame video_frame2; VideoProcessingModule::FrameStats stats; diff --git a/webrtc/video_frame.h b/webrtc/video_frame.h index c99dc63f52..67699a49eb 100644 --- a/webrtc/video_frame.h +++ b/webrtc/video_frame.h @@ -32,6 +32,9 @@ class I420VideoFrame { uint32_t timestamp, int64_t render_time_ms); + // TODO(pbos): Make all create/copy functions void, they should not be able to + // fail (which should be DCHECK/CHECKed instead). + // CreateEmptyFrame: Sets frame dimensions and allocates buffers based // on set dimensions - height and plane stride. // If required size is bigger than the allocated one, new buffers of adequate