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 <noahric@chromium.org>
Reviewed-by: Magnus Flodman <mflodman@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#27551}

common_video/i420_buffer_pool.cc

@@ -31,14 +31,27 @@ void I420BufferPool::Release() {
 
 rtc::scoped_refptr<I420Buffer> 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<I420Buffer> 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<PooledI420Buffer>& buffer : buffers_) {
     // If the buffer is in use, the ref count will be >= 2, one from the list we
@@ -53,7 +66,7 @@ rtc::scoped_refptr<I420Buffer> I420BufferPool::CreateBuffer(int width,
     return nullptr;
   // Allocate new buffer.
   rtc::scoped_refptr<PooledI420Buffer> buffer =
-      new PooledI420Buffer(width, height);
+      new PooledI420Buffer(width, height, stride_y, stride_u, stride_v);
   if (zero_initialize_)
     buffer->InitializeData();
   buffers_.push_back(buffer);

common_video/i420_buffer_pool_unittest.cc

@@ -21,7 +21,7 @@ namespace webrtc {
 
 TEST(TestI420BufferPool, SimpleFrameReuse) {
   I420BufferPool pool;
-  rtc::scoped_refptr<I420BufferInterface> 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<I420BufferInterface> 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<I420BufferInterface> buffer1 = pool.CreateBuffer(16, 16);
+  auto buffer1 = pool.CreateBuffer(16, 16);
   EXPECT_NE(nullptr, buffer1.get());
   EXPECT_EQ(nullptr, pool.CreateBuffer(16, 16).get());
 }

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<I420Buffer> CreateBuffer(int width, int height);
+
+  // Returns a buffer from the pool with the explicitly specified stride.
+  rtc::scoped_refptr<I420Buffer> 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();