From Video Capture, abolish OnFrameInfo and enable resolution changes

content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc

Index: content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc
diff --git a/content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc b/content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc
index 30509a34c75f9c478b6582b7d2d4f76239a420c5..71a86e2b2da901f9c7740846461cd1de0be6bb5d 100644
--- a/content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc
+++ b/content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc
@@ -16,27 +16,51 @@
 
 namespace content {
 
-TEST(VideoCaptureBufferPoolTest, BufferPool) {
-  const gfx::Size size = gfx::Size(640, 480);
+class VideoCaptureBufferPoolTest : public testing::Test {
+ protected:
+  VideoCaptureBufferPoolTest()
+      : expected_dropped_id_(0),
+        pool_(new VideoCaptureBufferPool(3)) {}
+
+  void ExpectDroppedId(int expected_dropped_id) {
+    expected_dropped_id_ = expected_dropped_id;
+  }
+
+  scoped_refptr<media::VideoFrame> ReserveI420VideoFrame(
+      const gfx::Size& size) {
+    int buffer_id_to_drop = ~expected_dropped_id_;

[Ami GONE FROM CHROMIUM, 2013/10/04 00:24:15]

o rly ~ ?

[ncarter (slow), 2013/10/16 02:08:40]

It's what I intended. What should I use instead? I just want to initialize it to
something other than the target value. Flipping the bits yields the MOST
DIFFERENT VALUE POSSIBLE :)

[Ami GONE FROM CHROMIUM, 2013/10/17 20:31:45]

I'm not faulting your MAXIMIZATION, which is indeed optimal, merely its
readability (~ might be the rarest C operator in the chromium codebase).
Maybe just add an explanatory comment?

+    scoped_refptr<media::VideoFrame> frame =
+        pool_->ReserveI420VideoFrame(size, 0, &buffer_id_to_drop);
+    EXPECT_EQ(expected_dropped_id_, buffer_id_to_drop)
+        << "Unexpected buffer reallocation result.";
+    return frame;
+  }
+
+  int expected_dropped_id_;
+  scoped_refptr<VideoCaptureBufferPool> pool_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(VideoCaptureBufferPoolTest);
+};
+
+TEST_F(VideoCaptureBufferPoolTest, BufferPool) {
+  const gfx::Size size_lo = gfx::Size(640, 480);
+  const gfx::Size size_hi = gfx::Size(1024, 768);
   scoped_refptr<media::VideoFrame> non_pool_frame =
-      media::VideoFrame::CreateFrame(media::VideoFrame::YV12, size,
-                                     gfx::Rect(size), size, base::TimeDelta());
-  scoped_refptr<VideoCaptureBufferPool> pool = new VideoCaptureBufferPool(
-      media::VideoFrame::AllocationSize(media::VideoFrame::I420, size), 3);
+      media::VideoFrame::CreateFrame(media::VideoFrame::YV12, size_lo,
+                                     gfx::Rect(size_lo), size_lo,
+                                     base::TimeDelta());
 
-  ASSERT_EQ(460800u, pool->GetMemorySize());
-  ASSERT_TRUE(pool->Allocate());
+  // Reallocation won't happen for the first part of the test.
+  ExpectDroppedId(VideoCaptureBufferPool::kInvalidId);
 
-  scoped_refptr<media::VideoFrame> frame1 =
-      pool->ReserveI420VideoFrame(size, 0);
+  scoped_refptr<media::VideoFrame> frame1 = ReserveI420VideoFrame(size_lo);
   ASSERT_TRUE(NULL != frame1.get());
-  ASSERT_EQ(size, frame1->coded_size());
-  scoped_refptr<media::VideoFrame> frame2 =
-      pool->ReserveI420VideoFrame(size, 0);
+  ASSERT_EQ(size_lo, frame1->coded_size());
+  scoped_refptr<media::VideoFrame> frame2 = ReserveI420VideoFrame(size_lo);
   ASSERT_TRUE(NULL != frame2.get());
-  ASSERT_EQ(size, frame2->coded_size());
-  scoped_refptr<media::VideoFrame> frame3 =
-      pool->ReserveI420VideoFrame(size, 0);
+  ASSERT_EQ(size_lo, frame2->coded_size());
+  scoped_refptr<media::VideoFrame> frame3 = ReserveI420VideoFrame(size_lo);
   ASSERT_TRUE(NULL != frame3.get());
 
   // Touch the memory.
@@ -45,112 +69,110 @@ TEST(VideoCaptureBufferPoolTest, BufferPool) {
   media::FillYUV(frame3.get(), 0x77, 0x88, 0x99);
 
   // Fourth frame should fail.
-  ASSERT_EQ(NULL, pool->ReserveI420VideoFrame(size, 0).get())
-      << "Pool should be empty";
+  ASSERT_FALSE(ReserveI420VideoFrame(size_lo)) << "Pool should be empty";
 
   // Release 1st frame and retry; this should succeed.
   frame1 = NULL;
-  scoped_refptr<media::VideoFrame> frame4 =
-      pool->ReserveI420VideoFrame(size, 0);
+  scoped_refptr<media::VideoFrame> frame4 = ReserveI420VideoFrame(size_lo);
   ASSERT_TRUE(NULL != frame4.get());
 
-  ASSERT_EQ(NULL, pool->ReserveI420VideoFrame(size, 0).get())
-      << "Pool should be empty";
+  ASSERT_FALSE(ReserveI420VideoFrame(size_lo)) << "Pool should be empty";
+  ASSERT_FALSE(ReserveI420VideoFrame(size_hi)) << "Pool should be empty";
 
   // Validate the IDs
   int buffer_id2 =
-      pool->RecognizeReservedBuffer(frame2->shared_memory_handle());
-  ASSERT_LE(0, buffer_id2);
+      pool_->RecognizeReservedBuffer(frame2->shared_memory_handle());
+  ASSERT_EQ(1, buffer_id2);
   int buffer_id3 =
-      pool->RecognizeReservedBuffer(frame3->shared_memory_handle());
-  ASSERT_LE(0, buffer_id3);
+      pool_->RecognizeReservedBuffer(frame3->shared_memory_handle());
+  base::SharedMemoryHandle memory_handle3 = frame3->shared_memory_handle();
+  ASSERT_EQ(2, buffer_id3);
   int buffer_id4 =
-      pool->RecognizeReservedBuffer(frame4->shared_memory_handle());
-  ASSERT_LE(0, buffer_id4);
+      pool_->RecognizeReservedBuffer(frame4->shared_memory_handle());
+  ASSERT_EQ(0, buffer_id4);
   int buffer_id_non_pool =
-      pool->RecognizeReservedBuffer(non_pool_frame->shared_memory_handle());
-  ASSERT_GT(0, buffer_id_non_pool);
-
-  ASSERT_FALSE(pool->IsAnyBufferHeldForConsumers());
+      pool_->RecognizeReservedBuffer(non_pool_frame->shared_memory_handle());
+  ASSERT_EQ(VideoCaptureBufferPool::kInvalidId, buffer_id_non_pool);
 
   // Deliver a frame.
-  pool->HoldForConsumers(buffer_id3, 2);
+  pool_->HoldForConsumers(buffer_id3, 2);
+
+  ASSERT_FALSE(ReserveI420VideoFrame(size_lo)) << "Pool should be empty";
 
-  ASSERT_TRUE(pool->IsAnyBufferHeldForConsumers());
-  ASSERT_EQ(NULL, pool->ReserveI420VideoFrame(size, 0).get())
-      << "Pool should be empty";
   frame3 = NULL;   // Old producer releases frame. Should be a noop.
-  ASSERT_TRUE(pool->IsAnyBufferHeldForConsumers());
-  ASSERT_EQ(NULL, pool->ReserveI420VideoFrame(size, 0).get())
-      << "Pool should be empty";
+  ASSERT_FALSE(ReserveI420VideoFrame(size_lo)) << "Pool should be empty";
+  ASSERT_FALSE(ReserveI420VideoFrame(size_hi)) << "Pool should be empty";
+
   frame2 = NULL;  // Active producer releases frame. Should free a frame.
-  buffer_id2 = 0;
 
-  ASSERT_TRUE(pool->IsAnyBufferHeldForConsumers());
-  frame1 = pool->ReserveI420VideoFrame(size, 0);
+  frame1 = ReserveI420VideoFrame(size_lo);
   ASSERT_TRUE(NULL != frame1.get());
-  ASSERT_EQ(NULL, pool->ReserveI420VideoFrame(size, 0).get())
-      << "Pool should be empty";
-  ASSERT_TRUE(pool->IsAnyBufferHeldForConsumers());
+  ASSERT_FALSE(ReserveI420VideoFrame(size_lo)) << "Pool should be empty";
 
   // First consumer finishes.
-  pool->RelinquishConsumerHold(buffer_id3, 1);
-  ASSERT_EQ(NULL, pool->ReserveI420VideoFrame(size, 0).get())
-      << "Pool should be empty";
-  ASSERT_TRUE(pool->IsAnyBufferHeldForConsumers());
+  pool_->RelinquishConsumerHold(buffer_id3, 1);
+  ASSERT_FALSE(ReserveI420VideoFrame(size_lo)) << "Pool should be empty";
 
   // Second consumer finishes. This should free that frame.
-  pool->RelinquishConsumerHold(buffer_id3, 1);
-  ASSERT_FALSE(pool->IsAnyBufferHeldForConsumers());
-  frame3 = pool->ReserveI420VideoFrame(size, 0);
+  pool_->RelinquishConsumerHold(buffer_id3, 1);
+  frame3 = ReserveI420VideoFrame(size_lo);
   ASSERT_TRUE(NULL != frame3.get());
-  ASSERT_FALSE(pool->IsAnyBufferHeldForConsumers());
-  ASSERT_EQ(NULL, pool->ReserveI420VideoFrame(size, 0).get())
-      << "Pool should be empty";
+  ASSERT_EQ(buffer_id3,
+            pool_->RecognizeReservedBuffer(frame3->shared_memory_handle()))
+      << "Buffer ID should be reused.";
+  ASSERT_EQ(memory_handle3, frame3->shared_memory_handle());
+  ASSERT_FALSE(ReserveI420VideoFrame(size_lo)) << "Pool should be empty";
 
   // Now deliver & consume frame1, but don't release the VideoFrame.
   int buffer_id1 =
-      pool->RecognizeReservedBuffer(frame1->shared_memory_handle());
-  ASSERT_LE(0, buffer_id1);
-  pool->HoldForConsumers(buffer_id1, 5);
-  ASSERT_TRUE(pool->IsAnyBufferHeldForConsumers());
-  pool->RelinquishConsumerHold(buffer_id1, 5);
-  ASSERT_FALSE(pool->IsAnyBufferHeldForConsumers());
+      pool_->RecognizeReservedBuffer(frame1->shared_memory_handle());
+  ASSERT_EQ(1, buffer_id1);
+  pool_->HoldForConsumers(buffer_id1, 5);
+  pool_->RelinquishConsumerHold(buffer_id1, 5);
 
   // Even though the consumer is done with the buffer at |buffer_id1|, it cannot
   // be re-allocated to the producer, because |frame1| still references it. But
   // when |frame1| goes away, we should be able to re-reserve the buffer (and
   // the ID ought to be the same).
-  ASSERT_EQ(NULL, pool->ReserveI420VideoFrame(size, 0).get())
-      << "Pool should be empty";
+  ASSERT_FALSE(ReserveI420VideoFrame(size_lo)) << "Pool should be empty";
   frame1 = NULL;  // Should free the frame.
-  frame2 = pool->ReserveI420VideoFrame(size, 0);
+  frame2 = ReserveI420VideoFrame(size_lo);
   ASSERT_TRUE(NULL != frame2.get());
   ASSERT_EQ(buffer_id1,
-            pool->RecognizeReservedBuffer(frame2->shared_memory_handle()));
-  ASSERT_EQ(NULL, pool->ReserveI420VideoFrame(size, 0).get())
-      << "Pool should be empty";
+            pool_->RecognizeReservedBuffer(frame2->shared_memory_handle()));
+  buffer_id2 = buffer_id1;
+  ASSERT_FALSE(ReserveI420VideoFrame(size_lo)) << "Pool should be empty";
 
-  // For good measure, do one more cycle of free/realloc without delivery, now
-  // that this buffer has been through the consumer-hold cycle.
+  // Now try reallocation with different resolutions. We expect reallocation
+  // to occur only when the old buffer is too small.
   frame2 = NULL;
-  frame1 = pool->ReserveI420VideoFrame(size, 0);
-  ASSERT_TRUE(NULL != frame1.get());
-  ASSERT_EQ(buffer_id1,
-            pool->RecognizeReservedBuffer(frame1->shared_memory_handle()));
-  ASSERT_EQ(NULL, pool->ReserveI420VideoFrame(size, 0).get())
-      << "Pool should be empty";
+  ExpectDroppedId(buffer_id2);
+  frame2 = ReserveI420VideoFrame(size_hi);
+  ASSERT_TRUE(NULL != frame2.get());
+  ASSERT_TRUE(frame2->coded_size() == size_hi);
+  ASSERT_EQ(3, pool_->RecognizeReservedBuffer(frame2->shared_memory_handle()));
+  base::SharedMemoryHandle memory_handle_hi = frame2->shared_memory_handle();
+  frame2 = NULL;  // Frees it.
+  ExpectDroppedId(VideoCaptureBufferPool::kInvalidId);
+  frame2 = ReserveI420VideoFrame(size_lo);
+  base::SharedMemoryHandle memory_handle_lo = frame2->shared_memory_handle();
+  ASSERT_EQ(memory_handle_hi, memory_handle_lo)
+      << "Decrease in resolution should not reallocate buffer";
+  ASSERT_TRUE(NULL != frame2.get());
+  ASSERT_TRUE(frame2->coded_size() == size_lo);
+  ASSERT_EQ(3, pool_->RecognizeReservedBuffer(frame2->shared_memory_handle()));
+  ASSERT_FALSE(ReserveI420VideoFrame(size_lo)) << "Pool should be empty";
 
-  // Tear down the pool, writing into the frames. The VideoFrame should
+  // Tear down the pool_, writing into the frames. The VideoFrame should
   // preserve the lifetime of the underlying memory.
   frame3 = NULL;
-  pool = NULL;
+  pool_ = NULL;
 
   // Touch the memory.
-  media::FillYUV(frame1.get(), 0x11, 0x22, 0x33);
+  media::FillYUV(frame2.get(), 0x11, 0x22, 0x33);
   media::FillYUV(frame4.get(), 0x44, 0x55, 0x66);
 
-  frame1 = NULL;
+  frame2 = NULL;
 
   media::FillYUV(frame4.get(), 0x44, 0x55, 0x66);
   frame4 = NULL;