From 00983572b0a65e9c9f65e8fc9e0628cb1399aa28 Mon Sep 17 00:00:00 2001 From: Per Date: Fri, 4 Nov 2016 08:57:26 +0100 Subject: [PATCH] Replace Check for too many pending frames in I420_buffer_pool with returning nullptr. Added histograms for when this happens in VP8Impl. BUG=chromium:542522 R=tommi@webrtc.org Review URL: https://codereview.webrtc.org/2474783005 . Cr-Commit-Position: refs/heads/master@{#14930} --- webrtc/common_video/i420_buffer_pool.cc | 13 +++++++------ webrtc/common_video/i420_buffer_pool_unittest.cc | 7 +++++++ webrtc/common_video/include/i420_buffer_pool.h | 16 +++++++++++----- .../modules/video_coding/codecs/vp8/vp8_impl.cc | 10 +++++++++- 4 files changed, 34 insertions(+), 12 deletions(-) diff --git a/webrtc/common_video/i420_buffer_pool.cc b/webrtc/common_video/i420_buffer_pool.cc index 630955aff9..aa9c32ecd0 100644 --- a/webrtc/common_video/i420_buffer_pool.cc +++ b/webrtc/common_video/i420_buffer_pool.cc @@ -14,10 +14,10 @@ namespace webrtc { -const size_t I420BufferPool::kMaxNumberOfFramesBeforeCrash = 300; - -I420BufferPool::I420BufferPool(bool zero_initialize) - : zero_initialize_(zero_initialize) {} +I420BufferPool::I420BufferPool(bool zero_initialize, + size_t max_number_of_buffers) + : zero_initialize_(zero_initialize), + max_number_of_buffers_(max_number_of_buffers) {} void I420BufferPool::Release() { buffers_.clear(); @@ -26,8 +26,6 @@ void I420BufferPool::Release() { rtc::scoped_refptr I420BufferPool::CreateBuffer(int width, int height) { RTC_DCHECK_RUNS_SERIALIZED(&race_checker_); - RTC_CHECK_LT(buffers_.size(), kMaxNumberOfFramesBeforeCrash) - << "I420BufferPool too big."; // Release buffers with wrong resolution. for (auto it = buffers_.begin(); it != buffers_.end();) { if ((*it)->width() != width || (*it)->height() != height) @@ -44,6 +42,9 @@ rtc::scoped_refptr I420BufferPool::CreateBuffer(int width, if (buffer->HasOneRef()) return buffer; } + + if (buffers_.size() >= max_number_of_buffers_) + return nullptr; // Allocate new buffer. rtc::scoped_refptr buffer = new PooledI420Buffer(width, height); diff --git a/webrtc/common_video/i420_buffer_pool_unittest.cc b/webrtc/common_video/i420_buffer_pool_unittest.cc index d5c7c9bbd8..a4dda0c60e 100644 --- a/webrtc/common_video/i420_buffer_pool_unittest.cc +++ b/webrtc/common_video/i420_buffer_pool_unittest.cc @@ -63,4 +63,11 @@ TEST(TestI420BufferPool, FrameValidAfterPoolDestruction) { memset(buffer->MutableDataY(), 0xA5, 16 * buffer->StrideY()); } +TEST(TestI420BufferPool, MaxNumberOfBuffers) { + I420BufferPool pool(false, 1); + rtc::scoped_refptr buffer1 = pool.CreateBuffer(16, 16); + EXPECT_NE(nullptr, buffer1.get()); + EXPECT_EQ(nullptr, pool.CreateBuffer(16, 16).get()); +} + } // namespace webrtc diff --git a/webrtc/common_video/include/i420_buffer_pool.h b/webrtc/common_video/include/i420_buffer_pool.h index 6359c21c8f..53950945c2 100644 --- a/webrtc/common_video/include/i420_buffer_pool.h +++ b/webrtc/common_video/include/i420_buffer_pool.h @@ -12,6 +12,7 @@ #define WEBRTC_COMMON_VIDEO_INCLUDE_I420_BUFFER_POOL_H_ #include +#include #include "webrtc/base/race_checker.h" #include "webrtc/common_video/include/video_frame_buffer.h" @@ -27,18 +28,21 @@ namespace webrtc { // are created. This is to prevent memory leaks where frames are not returned. class I420BufferPool { public: - I420BufferPool() : I420BufferPool(false) {} - explicit I420BufferPool(bool zero_initialize); + I420BufferPool() + : I420BufferPool(false, std::numeric_limits::max()) {} + explicit I420BufferPool(bool zero_initialize) + : I420BufferPool(zero_initialize, std::numeric_limits::max()) {} + I420BufferPool(bool zero_initialze, size_t max_number_of_buffers); - // Returns a buffer from the pool, or creates a new buffer if no suitable - // buffer exists in the pool. + // Returns a buffer from the pool. If no suitable buffer exist in the pool + // and there are less than |max_number_of_buffers| pending, a buffer is + // created. Returns null otherwise. rtc::scoped_refptr CreateBuffer(int width, int height); // Clears buffers_ and detaches the thread checker so that it can be reused // later from another thread. void Release(); private: - static const size_t kMaxNumberOfFramesBeforeCrash; // Explicitly use a RefCountedObject to get access to HasOneRef, // needed by the pool to check exclusive access. using PooledI420Buffer = rtc::RefCountedObject; @@ -51,6 +55,8 @@ class I420BufferPool { // initial allocation (as shown by FFmpeg's own buffer allocation code). It // has to do with "Use-of-uninitialized-value" on "Linux_msan_chrome". bool zero_initialize_; + // Max number of buffers this pool can have pending. + size_t max_number_of_buffers_; }; } // namespace webrtc diff --git a/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc b/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc index 523c19c257..0e70c06cf4 100644 --- a/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc +++ b/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc @@ -31,6 +31,7 @@ #include "webrtc/modules/video_coding/codecs/vp8/temporal_layers.h" #include "webrtc/modules/video_coding/utility/simulcast_rate_allocator.h" #include "webrtc/system_wrappers/include/clock.h" +#include "webrtc/system_wrappers/include/metrics.h" namespace webrtc { namespace { @@ -1055,7 +1056,8 @@ int VP8EncoderImpl::RegisterEncodeCompleteCallback( } VP8DecoderImpl::VP8DecoderImpl() - : decode_complete_callback_(NULL), + : buffer_pool_(false, 300 /* max_number_of_buffers*/), + decode_complete_callback_(NULL), inited_(false), feedback_mode_(false), decoder_(NULL), @@ -1270,6 +1272,12 @@ int VP8DecoderImpl::ReturnFrame(const vpx_image_t* img, // Allocate memory for decoded image. rtc::scoped_refptr buffer = buffer_pool_.CreateBuffer(img->d_w, img->d_h); + if (!buffer.get()) { + // Pool has too many pending frames. + RTC_HISTOGRAM_BOOLEAN("WebRTC.Video.VP8DecoderImpl.TooManyPendingFrames", + 1); + return WEBRTC_VIDEO_CODEC_NO_OUTPUT; + } libyuv::I420Copy(img->planes[VPX_PLANE_Y], img->stride[VPX_PLANE_Y], img->planes[VPX_PLANE_U], img->stride[VPX_PLANE_U],