From 408a3c63d37bdb7e4f3d53a9891dc69c4765e11d Mon Sep 17 00:00:00 2001 From: Noah Richards Date: Mon, 1 Apr 2019 10:14:47 -0700 Subject: [PATCH] Add explicit stride options to I420BufferPool. Also fix tests that relied on memory allocation behaviors. These only worked by chance in the past because the allocated sizes of planes changed enough to put them in a different location in memory. But there's no easy/valid way to ensure memory *wasn't* re-used, and the test doesn't really care anyways (if I420BufferPool could re-use the buffer object but change the resolution/stride, it'd still be fine). Bug: None Change-Id: I28135d58d23f194a0142e5a037fa9d315af6b1c8 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/130821 Commit-Queue: Noah Richards Reviewed-by: Magnus Flodman Cr-Commit-Position: refs/heads/master@{#27551} --- common_video/i420_buffer_pool.cc | 19 ++++++-- common_video/i420_buffer_pool_unittest.cc | 55 +++++++++++++++++++---- common_video/include/i420_buffer_pool.h | 8 ++++ 3 files changed, 71 insertions(+), 11 deletions(-) diff --git a/common_video/i420_buffer_pool.cc b/common_video/i420_buffer_pool.cc index 2e6cdc83b5..e970419ba6 100644 --- a/common_video/i420_buffer_pool.cc +++ b/common_video/i420_buffer_pool.cc @@ -31,13 +31,26 @@ void I420BufferPool::Release() { rtc::scoped_refptr I420BufferPool::CreateBuffer(int width, int height) { + // Default stride_y is width, default uv stride is width / 2 (rounding up). + return CreateBuffer(width, height, width, (width + 1) / 2, (width + 1) / 2); +} + +rtc::scoped_refptr I420BufferPool::CreateBuffer(int width, + int height, + int stride_y, + int stride_u, + int stride_v) { RTC_DCHECK_RUNS_SERIALIZED(&race_checker_); // Release buffers with wrong resolution. for (auto it = buffers_.begin(); it != buffers_.end();) { - if ((*it)->width() != width || (*it)->height() != height) + const auto& buffer = *it; + if (buffer->width() != width || buffer->height() != height || + buffer->StrideY() != stride_y || buffer->StrideU() != stride_u || + buffer->StrideV() != stride_v) { it = buffers_.erase(it); - else + } else { ++it; + } } // Look for a free buffer. for (const rtc::scoped_refptr& buffer : buffers_) { @@ -53,7 +66,7 @@ rtc::scoped_refptr I420BufferPool::CreateBuffer(int width, return nullptr; // Allocate new buffer. rtc::scoped_refptr buffer = - new PooledI420Buffer(width, height); + new PooledI420Buffer(width, height, stride_y, stride_u, stride_v); if (zero_initialize_) buffer->InitializeData(); buffers_.push_back(buffer); diff --git a/common_video/i420_buffer_pool_unittest.cc b/common_video/i420_buffer_pool_unittest.cc index debe637f5b..230f340365 100644 --- a/common_video/i420_buffer_pool_unittest.cc +++ b/common_video/i420_buffer_pool_unittest.cc @@ -21,7 +21,7 @@ namespace webrtc { TEST(TestI420BufferPool, SimpleFrameReuse) { I420BufferPool pool; - rtc::scoped_refptr buffer = pool.CreateBuffer(16, 16); + auto buffer = pool.CreateBuffer(16, 16); EXPECT_EQ(16, buffer->width()); EXPECT_EQ(16, buffer->height()); // Extract non-refcounted pointers for testing. @@ -35,24 +35,63 @@ TEST(TestI420BufferPool, SimpleFrameReuse) { EXPECT_EQ(y_ptr, buffer->DataY()); EXPECT_EQ(u_ptr, buffer->DataU()); EXPECT_EQ(v_ptr, buffer->DataV()); - EXPECT_EQ(16, buffer->width()); - EXPECT_EQ(16, buffer->height()); } -TEST(TestI420BufferPool, FailToReuse) { +TEST(TestI420BufferPool, FrameReuseWithDefaultThenExplicitStride) { I420BufferPool pool; - rtc::scoped_refptr buffer = pool.CreateBuffer(16, 16); + auto buffer = pool.CreateBuffer(15, 16); + EXPECT_EQ(15, buffer->width()); + EXPECT_EQ(16, buffer->height()); + // The default Y stride is width and UV stride is halfwidth (rounded up). + ASSERT_EQ(15, buffer->StrideY()); + ASSERT_EQ(8, buffer->StrideU()); + ASSERT_EQ(8, buffer->StrideV()); // Extract non-refcounted pointers for testing. + const uint8_t* y_ptr = buffer->DataY(); const uint8_t* u_ptr = buffer->DataU(); const uint8_t* v_ptr = buffer->DataV(); // Release buffer so that it is returned to the pool. buffer = nullptr; + // Check that the memory is resued with explicit strides if they match the + // assumed default above. + buffer = pool.CreateBuffer(15, 16, 15, 8, 8); + EXPECT_EQ(y_ptr, buffer->DataY()); + EXPECT_EQ(u_ptr, buffer->DataU()); + EXPECT_EQ(v_ptr, buffer->DataV()); + EXPECT_EQ(15, buffer->width()); + EXPECT_EQ(16, buffer->height()); + EXPECT_EQ(15, buffer->StrideY()); + EXPECT_EQ(8, buffer->StrideU()); + EXPECT_EQ(8, buffer->StrideV()); +} + +TEST(TestI420BufferPool, FailToReuseWrongSize) { + // Set max frames to 1, just to make sure the first buffer is being released. + I420BufferPool pool(/*zero_initialize=*/false, 1); + auto buffer = pool.CreateBuffer(16, 16); + EXPECT_EQ(16, buffer->width()); + EXPECT_EQ(16, buffer->height()); + // Release buffer so that it is returned to the pool. + buffer = nullptr; // Check that the pool doesn't try to reuse buffers of incorrect size. buffer = pool.CreateBuffer(32, 16); + ASSERT_TRUE(buffer); EXPECT_EQ(32, buffer->width()); EXPECT_EQ(16, buffer->height()); - EXPECT_NE(u_ptr, buffer->DataU()); - EXPECT_NE(v_ptr, buffer->DataV()); +} + +TEST(TestI420BufferPool, FailToReuseWrongStride) { + // Set max frames to 1, just to make sure the first buffer is being released. + I420BufferPool pool(/*zero_initialize=*/false, 1); + auto buffer = pool.CreateBuffer(32, 32, 32, 16, 16); + // Make sure the stride was read correctly, for the rest of the test. + ASSERT_EQ(16, buffer->StrideU()); + ASSERT_EQ(16, buffer->StrideV()); + buffer = pool.CreateBuffer(32, 32, 32, 20, 20); + ASSERT_TRUE(buffer); + EXPECT_EQ(32, buffer->StrideY()); + EXPECT_EQ(20, buffer->StrideU()); + EXPECT_EQ(20, buffer->StrideV()); } TEST(TestI420BufferPool, FrameValidAfterPoolDestruction) { @@ -69,7 +108,7 @@ TEST(TestI420BufferPool, FrameValidAfterPoolDestruction) { TEST(TestI420BufferPool, MaxNumberOfBuffers) { I420BufferPool pool(false, 1); - rtc::scoped_refptr buffer1 = pool.CreateBuffer(16, 16); + auto buffer1 = pool.CreateBuffer(16, 16); EXPECT_NE(nullptr, buffer1.get()); EXPECT_EQ(nullptr, pool.CreateBuffer(16, 16).get()); } diff --git a/common_video/include/i420_buffer_pool.h b/common_video/include/i420_buffer_pool.h index 0beb3a58b4..d666c8b451 100644 --- a/common_video/include/i420_buffer_pool.h +++ b/common_video/include/i420_buffer_pool.h @@ -39,6 +39,14 @@ class I420BufferPool { // 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); + + // Returns a buffer from the pool with the explicitly specified stride. + rtc::scoped_refptr CreateBuffer(int width, + int height, + int stride_y, + int stride_u, + int stride_v); + // Clears buffers_ and detaches the thread checker so that it can be reused // later from another thread. void Release();