media/gpu/linux: Don't purge stale frames in PlatformVideoFramePool.

Currently we always set a "just-enough" frame number. In usual usage
there is no stale frames. This CL remove the frame purging logic to
simplify the code and reduce the periodically cleanup time.

BUG=none
TEST=media_unittests --gtest_filter=PlatformVideoFramePoolTest
TEST=Run video_decode_accelerator_tests on Kevin

Change-Id: I1542c97ed2a492776bc6ec7a2e1d48fdc12467a2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1786706
Commit-Queue: Chih-Yu Huang <akahuang@chromium.org>
Reviewed-by: David Staessens <dstaessens@chromium.org>
Cr-Commit-Position: refs/heads/master@{#695060}

media/gpu/linux/platform_video_frame_pool.cc

@@ -9,7 +9,6 @@
 #include "base/logging.h"
 #include "base/optional.h"
 #include "base/task/post_task.h"
-#include "base/time/default_tick_clock.h"
 #include "media/gpu/linux/platform_video_frame_utils.h"
 #include "media/gpu/macros.h"
 
@@ -17,13 +16,6 @@ namespace media {
 
 namespace {
 
-// The lifespan for stale frames. If a frame is not used for 10 seconds, then
-// drop the frame to reduce memory usage.
-constexpr base::TimeDelta kStaleFrameLimit = base::TimeDelta::FromSeconds(10);
-
-// The default maximum number of frames.
-constexpr size_t kDefaultMaxNumFrames = 64;
-
 // The default method to create frames.
 scoped_refptr<VideoFrame> DefaultCreateFrame(VideoPixelFormat format,
                                              const gfx::Size& coded_size,
@@ -43,30 +35,11 @@ PlatformVideoFramePool::DmabufId GetDmabufId(const VideoFrame& frame) {
 
 }  // namespace
 
-struct PlatformVideoFramePool::FrameEntry {
-  base::TimeTicks last_use_time;
-  scoped_refptr<VideoFrame> frame;
-
-  FrameEntry(base::TimeTicks time, scoped_refptr<VideoFrame> frame)
-      : last_use_time(time), frame(std::move(frame)) {}
-  FrameEntry(const FrameEntry& other) {
-    last_use_time = other.last_use_time;
-    frame = other.frame;
-  }
-  ~FrameEntry() = default;
-};
-
 PlatformVideoFramePool::PlatformVideoFramePool()
-    : PlatformVideoFramePool(base::BindRepeating(&DefaultCreateFrame),
-                             base::DefaultTickClock::GetInstance()) {}
-
-PlatformVideoFramePool::PlatformVideoFramePool(
-    CreateFrameCB cb,
-    const base::TickClock* tick_clock)
-    : create_frame_cb_(std::move(cb)),
-      tick_clock_(tick_clock),
-      max_num_frames_(kDefaultMaxNumFrames),
-      weak_this_factory_(this) {
+    : PlatformVideoFramePool(base::BindRepeating(&DefaultCreateFrame)) {}
+
+PlatformVideoFramePool::PlatformVideoFramePool(CreateFrameCB cb)
+    : create_frame_cb_(std::move(cb)) {
   DVLOGF(4);
   weak_this_ = weak_this_factory_.GetWeakPtr();
 }
@@ -111,7 +84,7 @@ scoped_refptr<VideoFrame> PlatformVideoFramePool::GetFrame() {
   }
 
   DCHECK(!free_frames_.empty());
-  scoped_refptr<VideoFrame> origin_frame = std::move(free_frames_.back().frame);
+  scoped_refptr<VideoFrame> origin_frame = std::move(free_frames_.back());
   free_frames_.pop_back();
   DCHECK_EQ(origin_frame->format(), format);
   DCHECK_EQ(origin_frame->coded_size(), coded_size);
@@ -130,34 +103,6 @@ scoped_refptr<VideoFrame> PlatformVideoFramePool::GetFrame() {
   return wrapped_frame;
 }
 
-void PlatformVideoFramePool::set_parent_task_runner(
-    scoped_refptr<base::SequencedTaskRunner> parent_task_runner) {
-  DCHECK(!parent_task_runner_);
-
-  parent_task_runner_ = std::move(parent_task_runner);
-  parent_task_runner_->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(&PlatformVideoFramePool::PurgeStaleFrames, weak_this_),
-      kStaleFrameLimit);
-}
-
-void PlatformVideoFramePool::PurgeStaleFrames() {
-  DCHECK(parent_task_runner_->RunsTasksInCurrentSequence());
-  DVLOGF(4);
-  base::AutoLock auto_lock(lock_);
-
-  const base::TimeTicks now = tick_clock_->NowTicks();
-  while (!free_frames_.empty() &&
-         now - free_frames_.front().last_use_time > kStaleFrameLimit) {
-    free_frames_.pop_front();
-  }
-
-  parent_task_runner_->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(&PlatformVideoFramePool::PurgeStaleFrames, weak_this_),
-      kStaleFrameLimit);
-}
-
 void PlatformVideoFramePool::SetMaxNumFrames(size_t max_num_frames) {
   DVLOGF(4);
   base::AutoLock auto_lock(lock_);
@@ -276,7 +221,7 @@ void PlatformVideoFramePool::InsertFreeFrame_Locked(
   lock_.AssertAcquired();
 
   if (GetTotalNumFrames_Locked() < max_num_frames_)
-    free_frames_.push_back({tick_clock_->NowTicks(), std::move(frame)});
+    free_frames_.push_back(std::move(frame));
 }
 
 size_t PlatformVideoFramePool::GetTotalNumFrames_Locked() const {

media/gpu/linux/platform_video_frame_pool.h

@@ -21,10 +21,6 @@
 #include "media/gpu/linux/dmabuf_video_frame_pool.h"
 #include "media/gpu/media_gpu_export.h"
 
-namespace base {
-class TickClock;
-}
-
 namespace media {
 
 // Simple VideoFrame pool used to avoid unnecessarily allocating and destroying
@@ -35,8 +31,7 @@ namespace media {
 // PlatformVideoFramePool object. Before calling GetFrame(), the client should
 // call NegotiateFrameFormat(). If the parameters passed to
 // NegotiateFrameFormat() are changed, then the memory used by frames with the
-// old parameter values will be purged from the pool. Frames which are not used
-// for a certain period will be purged.
+// old parameter values will be purged from the pool.
 class MEDIA_GPU_EXPORT PlatformVideoFramePool : public DmabufVideoFramePool {
  public:
   using DmabufId = const std::vector<base::ScopedFD>*;
@@ -45,8 +40,6 @@ class MEDIA_GPU_EXPORT PlatformVideoFramePool : public DmabufVideoFramePool {
   ~PlatformVideoFramePool() override;
 
   // VideoFramePoolBase Implementation.
-  void set_parent_task_runner(
-      scoped_refptr<base::SequencedTaskRunner> parent_task_runner) override;
   void SetMaxNumFrames(size_t max_num_frames) override;
   base::Optional<VideoFrameLayout> NegotiateFrameFormat(
       const VideoFrameLayout& layout,
@@ -67,12 +60,9 @@ class MEDIA_GPU_EXPORT PlatformVideoFramePool : public DmabufVideoFramePool {
       const gfx::Size& natural_size,
       base::TimeDelta timestamp)>;
 
-  // Used to store free frame and the last used time.
-  struct FrameEntry;
-
-  // Allows injection of create frame callback and base::SimpleTestClock.
-  // This is used to test the behavior of the video frame pool.
-  PlatformVideoFramePool(CreateFrameCB cb, const base::TickClock* tick_clock);
+  // 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();
@@ -105,9 +95,6 @@ class MEDIA_GPU_EXPORT PlatformVideoFramePool : public DmabufVideoFramePool {
   // The function used to allocate new frames.
   const CreateFrameCB create_frame_cb_;
 
-  // |tick_clock_| is always a DefaultTickClock outside of testing.
-  const base::TickClock* tick_clock_;
-
   // Lock to protect all data members.
   // Every public method and OnFrameReleased() should acquire this lock.
   base::Lock lock_;
@@ -121,12 +108,13 @@ class MEDIA_GPU_EXPORT PlatformVideoFramePool : public DmabufVideoFramePool {
 
   // The pool of free frames. The layout of all the frames in |free_frames_|
   // should be the same as |format_| and |coded_size_|.
-  base::circular_deque<FrameEntry> free_frames_ GUARDED_BY(lock_);
+  base::circular_deque<scoped_refptr<VideoFrame>> free_frames_
+      GUARDED_BY(lock_);
   // Mapping from the unique_id of the wrapped frame to the original frame.
   std::map<DmabufId, VideoFrame*> frames_in_use_ GUARDED_BY(lock_);
 
   // The maximum number of frames created by the pool.
-  size_t max_num_frames_ GUARDED_BY(lock_);
+  size_t max_num_frames_ GUARDED_BY(lock_) = 0;
 
   // Callback which is called when the pool is not exhausted.
   base::OnceClosure frame_available_cb_ GUARDED_BY(lock_);
@@ -134,7 +122,7 @@ class MEDIA_GPU_EXPORT PlatformVideoFramePool : public DmabufVideoFramePool {
   // The weak pointer of this, bound at |parent_task_runner_|.
   // Used at the VideoFrame destruction callback.
   base::WeakPtr<PlatformVideoFramePool> weak_this_;
-  base::WeakPtrFactory<PlatformVideoFramePool> weak_this_factory_;
+  base::WeakPtrFactory<PlatformVideoFramePool> weak_this_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(PlatformVideoFramePool);
 };

media/gpu/linux/platform_video_frame_pool_unittest.cc

@@ -11,7 +11,6 @@
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_file.h"
-#include "base/test/simple_test_tick_clock.h"
 #include "base/test/task_environment.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "media/gpu/linux/platform_video_frame_pool.h"
@@ -56,8 +55,9 @@ class PlatformVideoFramePoolTest
   PlatformVideoFramePoolTest()
       : task_environment_(base::test::TaskEnvironment::TimeSource::MOCK_TIME) {
     pool_.reset(new PlatformVideoFramePool(
-        base::BindRepeating(&CreateDmabufVideoFrame), &test_clock_));
+        base::BindRepeating(&CreateDmabufVideoFrame)));
     pool_->set_parent_task_runner(base::ThreadTaskRunnerHandle::Get());
+    pool_->SetMaxNumFrames(10);
   }
 
   void SetFrameFormat(VideoPixelFormat format) {
@@ -90,7 +90,6 @@ class PlatformVideoFramePoolTest
 
  protected:
   base::test::TaskEnvironment task_environment_;
-  base::SimpleTestTickClock test_clock_;
   std::unique_ptr<PlatformVideoFramePool,
                   std::default_delete<DmabufVideoFramePool>>
       pool_;
@@ -163,28 +162,6 @@ TEST_F(PlatformVideoFramePoolTest, FormatChange) {
   CheckPoolSize(0u);
 }
 
-TEST_F(PlatformVideoFramePoolTest, StaleFramesAreExpired) {
-  SetFrameFormat(PIXEL_FORMAT_I420);
-  scoped_refptr<VideoFrame> frame_1 = GetFrame(10);
-  scoped_refptr<VideoFrame> frame_2 = GetFrame(10);
-  EXPECT_NE(frame_1.get(), frame_2.get());
-  CheckPoolSize(0u);
-
-  // Drop frame and verify that resources are still available for reuse.
-  frame_1 = nullptr;
-  task_environment_.RunUntilIdle();
-  CheckPoolSize(1u);
-
-  // Advance clock far enough to hit stale timer; ensure only frame_1 has its
-  // resources released.
-  base::TimeDelta time_forward = base::TimeDelta::FromMinutes(1);
-  test_clock_.Advance(time_forward);
-  task_environment_.FastForwardBy(time_forward);
-  frame_2 = nullptr;
-  task_environment_.RunUntilIdle();
-  CheckPoolSize(1u);
-}
-
 TEST_F(PlatformVideoFramePoolTest, UnwrapVideoFrame) {
   SetFrameFormat(PIXEL_FORMAT_I420);
   scoped_refptr<VideoFrame> frame_1 = GetFrame(10);

media/gpu/vaapi/vaapi_video_decoder.cc

@@ -523,8 +523,7 @@ void VaapiVideoDecoder::ChangeFrameResolutionTask() {
       GfxBufferFormatToVideoPixelFormat(GetBufferFormat());
   frame_layout_ = VideoFrameLayout::Create(format, pic_size);
   DCHECK(frame_layout_);
-  frame_pool_->NegotiateFrameFormat(*frame_layout_, visible_rect,
-                                    natural_size);
+  frame_pool_->NegotiateFrameFormat(*frame_layout_, visible_rect, natural_size);
   frame_pool_->SetMaxNumFrames(decoder_->GetRequiredNumOfPictures());
 
   // All pending decode operations will be completed before triggering a