PlatformVideoFramePoolTest: connect the parameterized test cases

This CL teaches PlatformVideoFramePoolTest to use the parameterization
defined inside (ToT did not use it). It also removes the surprising
(unnecessary?) explicit unique_ptr deleter, using PlatformVideoFramePool
directly, and also takes the chance to remove the private ctor in that
class, since anyway the Test is a friend and can access the internal
members (albeit we lose a const).

Test: media_unittests on a target_os="chromeos" build
Bug: 1040291
Change-Id: I660e579b34a0ebea213f3c4a9b66b64a17043f3b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1992762
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Andres Calderon Jaramillo <andrescj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#729787}

media/gpu/chromeos/platform_video_frame_pool.cc

@@ -40,12 +40,6 @@ PlatformVideoFramePool::PlatformVideoFramePool(
   weak_this_ = weak_this_factory_.GetWeakPtr();
 }
 
-PlatformVideoFramePool::PlatformVideoFramePool(CreateFrameCB cb)
-    : create_frame_cb_(std::move(cb)) {
-  DVLOGF(4);
-  weak_this_ = weak_this_factory_.GetWeakPtr();
-}
-
 PlatformVideoFramePool::~PlatformVideoFramePool() {
   if (parent_task_runner_)
     DCHECK(parent_task_runner_->RunsTasksInCurrentSequence());
@@ -214,9 +208,8 @@ void PlatformVideoFramePool::OnFrameReleased(
   DCHECK(it != frames_in_use_.end());
   frames_in_use_.erase(it);
 
-  if (IsSameFormat_Locked(origin_frame->format(), origin_frame->coded_size())) {
+  if (IsSameFormat_Locked(origin_frame->format(), origin_frame->coded_size()))
     InsertFreeFrame_Locked(std::move(origin_frame));
-  }
 
   if (frame_available_cb_ && !IsExhausted_Locked())
     std::move(frame_available_cb_).Run();

media/gpu/chromeos/platform_video_frame_pool.h

@@ -43,7 +43,7 @@ class MEDIA_GPU_EXPORT PlatformVideoFramePool : public DmabufVideoFramePool {
       gpu::GpuMemoryBufferFactory* gpu_memory_buffer_factory);
   ~PlatformVideoFramePool() override;
 
-  // VideoFramePoolBase Implementation.
+  // DmabufVideoFramePool implementation.
   base::Optional<GpuBufferLayout> RequestFrames(const Fourcc& fourcc,
                                                 const gfx::Size& coded_size,
                                                 const gfx::Rect& visible_rect,
@@ -61,18 +61,6 @@ class MEDIA_GPU_EXPORT PlatformVideoFramePool : public DmabufVideoFramePool {
  private:
   friend class PlatformVideoFramePoolTest;
 
-  using CreateFrameCB = base::RepeatingCallback<scoped_refptr<VideoFrame>(
-      gpu::GpuMemoryBufferFactory* gpu_memory_buffer_factory,
-      VideoPixelFormat format,
-      const gfx::Size& coded_size,
-      const gfx::Rect& visible_rect,
-      const gfx::Size& natural_size,
-      base::TimeDelta timestamp)>;
-
-  // Allows injection of create frame callback. This is used to test the
-  // behavior of the video frame pool.
-  PlatformVideoFramePool(CreateFrameCB cb);
-
   // Returns the number of frames in the pool for testing purposes.
   size_t GetPoolSizeForTesting();
 
@@ -99,10 +87,17 @@ class MEDIA_GPU_EXPORT PlatformVideoFramePool : public DmabufVideoFramePool {
   bool IsExhausted_Locked() EXCLUSIVE_LOCKS_REQUIRED(lock_);
 
   // The function used to allocate new frames.
-  const CreateFrameCB create_frame_cb_;
+  using CreateFrameCB = base::RepeatingCallback<scoped_refptr<VideoFrame>(
+      gpu::GpuMemoryBufferFactory* gpu_memory_buffer_factory,
+      VideoPixelFormat format,
+      const gfx::Size& coded_size,
+      const gfx::Rect& visible_rect,
+      const gfx::Size& natural_size,
+      base::TimeDelta timestamp)>;
+  CreateFrameCB create_frame_cb_;
 
   // Lock to protect all data members.
-  // Every public method and OnFrameReleased() should acquire this lock.
+  // Every public method and OnFrameReleased() acquire this lock.
   base::Lock lock_;
 
   // Used to allocate the video frame GpuMemoryBuffers, passed directly to

media/gpu/chromeos/platform_video_frame_pool_unittest.cc

@@ -57,9 +57,9 @@ class PlatformVideoFramePoolTest
   using DmabufId = DmabufVideoFramePool::DmabufId;
 
   PlatformVideoFramePoolTest()
-      : task_environment_(base::test::TaskEnvironment::TimeSource::MOCK_TIME) {
-    pool_.reset(new PlatformVideoFramePool(
-        base::BindRepeating(&CreateDmabufVideoFrame)));
+      : task_environment_(base::test::TaskEnvironment::TimeSource::MOCK_TIME),
+        pool_(new PlatformVideoFramePool(nullptr)) {
+    pool_->create_frame_cb_ = base::BindRepeating(&CreateDmabufVideoFrame);
     pool_->set_parent_task_runner(base::ThreadTaskRunnerHandle::Get());
   }
 
@@ -94,9 +94,7 @@ class PlatformVideoFramePoolTest
 
  protected:
   base::test::TaskEnvironment task_environment_;
-  std::unique_ptr<PlatformVideoFramePool,
-                  std::default_delete<DmabufVideoFramePool>>
-      pool_;
+  std::unique_ptr<PlatformVideoFramePool> pool_;
 
   base::Optional<GpuBufferLayout> layout_;
   gfx::Rect visible_rect_;
@@ -106,11 +104,14 @@ class PlatformVideoFramePoolTest
 INSTANTIATE_TEST_SUITE_P(All,
                          PlatformVideoFramePoolTest,
                          testing::Values(PIXEL_FORMAT_I420,
+                                         PIXEL_FORMAT_YV12,
                                          PIXEL_FORMAT_NV12,
                                          PIXEL_FORMAT_ARGB));
 
-TEST_F(PlatformVideoFramePoolTest, SingleFrameReuse) {
-  RequestFrames(Fourcc(Fourcc::YV12));
+TEST_P(PlatformVideoFramePoolTest, SingleFrameReuse) {
+  const auto fourcc = Fourcc::FromVideoPixelFormat(GetParam());
+  ASSERT_TRUE(fourcc.has_value());
+  RequestFrames(fourcc.value());
   scoped_refptr<VideoFrame> frame = GetFrame(10);
   DmabufId id = DmabufVideoFramePool::GetDmabufId(*frame);
 
@@ -123,8 +124,10 @@ TEST_F(PlatformVideoFramePoolTest, SingleFrameReuse) {
   EXPECT_EQ(id, DmabufVideoFramePool::GetDmabufId(*new_frame));
 }
 
-TEST_F(PlatformVideoFramePoolTest, MultipleFrameReuse) {
-  RequestFrames(Fourcc(Fourcc::YV12));
+TEST_P(PlatformVideoFramePoolTest, MultipleFrameReuse) {
+  const auto fourcc = Fourcc::FromVideoPixelFormat(GetParam());
+  ASSERT_TRUE(fourcc.has_value());
+  RequestFrames(fourcc.value());
   scoped_refptr<VideoFrame> frame1 = GetFrame(10);
   scoped_refptr<VideoFrame> frame2 = GetFrame(20);
   DmabufId id1 = DmabufVideoFramePool::GetDmabufId(*frame1);
@@ -146,8 +149,10 @@ TEST_F(PlatformVideoFramePoolTest, MultipleFrameReuse) {
   CheckPoolSize(2u);
 }
 
-TEST_F(PlatformVideoFramePoolTest, FormatChange) {
-  RequestFrames(Fourcc(Fourcc::YV12));
+TEST_P(PlatformVideoFramePoolTest, RequestFramesWithDifferentFourcc) {
+  const auto fourcc = Fourcc::FromVideoPixelFormat(GetParam());
+  ASSERT_TRUE(fourcc.has_value());
+  RequestFrames(fourcc.value());
   scoped_refptr<VideoFrame> frame_a = GetFrame(10);
   scoped_refptr<VideoFrame> frame_b = GetFrame(10);
 
@@ -161,13 +166,17 @@ TEST_F(PlatformVideoFramePoolTest, FormatChange) {
 
   // Verify that requesting a frame with a different format causes the pool
   // to get drained.
-  RequestFrames(Fourcc(Fourcc::NV12));
+  const Fourcc different_fourcc(Fourcc::NV21);
+  ASSERT_NE(fourcc, different_fourcc);
+  RequestFrames(different_fourcc);
   scoped_refptr<VideoFrame> new_frame = GetFrame(10);
   CheckPoolSize(0u);
 }
 
-TEST_F(PlatformVideoFramePoolTest, UnwrapVideoFrame) {
-  RequestFrames(Fourcc(Fourcc::YV12));
+TEST_P(PlatformVideoFramePoolTest, UnwrapVideoFrame) {
+  const auto fourcc = Fourcc::FromVideoPixelFormat(GetParam());
+  ASSERT_TRUE(fourcc.has_value());
+  RequestFrames(fourcc.value());
   scoped_refptr<VideoFrame> frame_1 = GetFrame(10);
   scoped_refptr<VideoFrame> frame_2 = VideoFrame::WrapVideoFrame(
       frame_1, frame_1->format(), frame_1->visible_rect(),
@@ -180,8 +189,10 @@ TEST_F(PlatformVideoFramePoolTest, UnwrapVideoFrame) {
   EXPECT_FALSE(frame_1->IsSameDmaBufsAs(*frame_3));
 }
 
-TEST_F(PlatformVideoFramePoolTest, FormatNotChange) {
-  RequestFrames(Fourcc(Fourcc::YV12));
+TEST_P(PlatformVideoFramePoolTest, RequestFramesWithSameFourcc) {
+  const auto fourcc = Fourcc::FromVideoPixelFormat(GetParam());
+  ASSERT_TRUE(fourcc.has_value());
+  RequestFrames(fourcc.value());
   scoped_refptr<VideoFrame> frame1 = GetFrame(10);
   DmabufId id1 = DmabufVideoFramePool::GetDmabufId(*frame1);
 
@@ -190,7 +201,7 @@ TEST_F(PlatformVideoFramePoolTest, FormatNotChange) {
   task_environment_.RunUntilIdle();
 
   // Request frame with the same format. The pool should not request new frames.
-  RequestFrames(Fourcc(Fourcc::YV12));
+  RequestFrames(fourcc.value());
 
   scoped_refptr<VideoFrame> frame2 = GetFrame(20);
   DmabufId id2 = DmabufVideoFramePool::GetDmabufId(*frame2);