Abort any pending copies upon decoder Reset() and pool shutdown.

Clients don't care about these copies once they destroyed the pool;
we also don't want to waste resources copying these unused frames
after Reset().

BUG=801245, 820685, 820944
TEST=updated tests

Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I15ccf0c5a00b14e87522f923c4ec9c93416d9342
Reviewed-on: https://chromium-review.googlesource.com/958042
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542577}

media/filters/gpu_memory_buffer_decoder_wrapper.cc

@@ -78,6 +78,7 @@ void GpuMemoryBufferDecoderWrapper::Reset(const base::Closure& reset_cb) {
 
   // Abort any in-flight copies and stop any new ones from being started until
   // the next Decode() call (which will not occur until Reset() completes).
+  gmb_pool_->Abort();
   abort_copies_ = true;
   pending_copies_ = 0u;
   copy_factory_.InvalidateWeakPtrs();

media/filters/gpu_memory_buffer_decoder_wrapper_unittest.cc

@@ -27,10 +27,12 @@ namespace media {
 
 class GpuMemoryBufferDecoderWrapperTest : public testing::Test {
  public:
-  GpuMemoryBufferDecoderWrapperTest() : config_(TestVideoConfig::Normal()) {
-    decoder_ = new testing::StrictMock<MockVideoDecoder>();
+  GpuMemoryBufferDecoderWrapperTest()
+      : config_(TestVideoConfig::Normal()),
+        decoder_(new testing::StrictMock<MockVideoDecoder>()),
+        mock_pool_(new MockGpuMemoryBufferVideoFramePool(&frame_ready_cbs_)) {
     gmb_decoder_ = std::make_unique<GpuMemoryBufferDecoderWrapper>(
-        std::make_unique<MockGpuMemoryBufferVideoFramePool>(&frame_ready_cbs_),
+        std::unique_ptr<MockGpuMemoryBufferVideoFramePool>(mock_pool_),
         std::unique_ptr<VideoDecoder>(decoder_));
   }
 
@@ -133,15 +135,16 @@ class GpuMemoryBufferDecoderWrapperTest : public testing::Test {
     std::move(frame_ready_cbs_[2]).Run();
   }
 
+  const VideoDecoderConfig config_;
+
   base::TestMessageLoop message_loop_;
   std::vector<base::OnceClosure> frame_ready_cbs_;
 
   std::unique_ptr<GpuMemoryBufferDecoderWrapper> gmb_decoder_;
 
   // Owned by |gmb_decoder_|.
-  testing::StrictMock<MockVideoDecoder>* decoder_ = nullptr;
-
-  const VideoDecoderConfig config_;
+  testing::StrictMock<MockVideoDecoder>* decoder_;
+  MockGpuMemoryBufferVideoFramePool* mock_pool_;
 
   MOCK_METHOD1(OnInitDone, void(bool));
   MOCK_METHOD1(OnDecodeDone, void(DecodeStatus));
@@ -169,6 +172,7 @@ TEST_F(GpuMemoryBufferDecoderWrapperTest, InitializeReset) {
 
   EXPECT_CALL(*this, OnResetDone());
   EXPECT_CALL(*decoder_, Reset(_)).WillOnce(RunCallback<0>());
+  EXPECT_CALL(*mock_pool_, Abort());
   gmb_decoder_->Reset(base::BindRepeating(
       &GpuMemoryBufferDecoderWrapperTest::OnResetDone, base::Unretained(this)));
 
@@ -234,6 +238,7 @@ TEST_F(GpuMemoryBufferDecoderWrapperTest, ResetAbortsCopies) {
     // run after anything that might currently be on the task runner queue.
     EXPECT_CALL(*this, OnResetDone()).WillOnce(RunClosure(loop.QuitClosure()));
     EXPECT_CALL(*decoder_, Reset(_)).WillOnce(RunCallback<0>());
+    EXPECT_CALL(*mock_pool_, Abort());
     gmb_decoder_->Reset(BindToCurrentLoop(
         base::BindRepeating(&GpuMemoryBufferDecoderWrapperTest::OnResetDone,
                             base::Unretained(this))));
@@ -308,6 +313,7 @@ TEST_F(GpuMemoryBufferDecoderWrapperTest,
                           base::Unretained(this)));
 
   // Technically the status here should be ABORTED, but it doesn't matter.
+  EXPECT_CALL(*mock_pool_, Abort());
   EXPECT_CALL(*this, OnDecodeDone(DecodeStatus::OK));
   EXPECT_CALL(*decoder_, Reset(_)).WillOnce(RunCallback<0>());
   EXPECT_CALL(*this, OnResetDone()).WillOnce(RunClosure(loop.QuitClosure()));
@@ -361,6 +367,7 @@ TEST_F(GpuMemoryBufferDecoderWrapperTest,
       base::BindRepeating(&GpuMemoryBufferDecoderWrapperTest::OnDecodeDone,
                           base::Unretained(this)));
 
+  EXPECT_CALL(*mock_pool_, Abort());
   EXPECT_CALL(*decoder_, Reset(_))
       .WillOnce(
           DoAll(RunClosure(base::BindRepeating(decode_cb, DecodeStatus::OK)),

media/video/gpu_memory_buffer_video_frame_pool.cc

@@ -72,6 +72,9 @@ class GpuMemoryBufferVideoFramePool::PoolImpl
   bool OnMemoryDump(const base::trace_event::MemoryDumpArgs& args,
                     base::trace_event::ProcessMemoryDump* pmd) override;
 
+  // Aborts any pending copies.
+  void Abort();
+
   // Shuts down the frame pool and releases all frames in |frames_|.
   // Once this is called frames will no longer be inserted back into
   // |frames_|.
@@ -625,6 +628,16 @@ bool GpuMemoryBufferVideoFramePool::PoolImpl::OnMemoryDump(
   return true;
 }
 
+void GpuMemoryBufferVideoFramePool::PoolImpl::Abort() {
+  DCHECK(media_task_runner_->BelongsToCurrentThread());
+  // Abort any pending copy requests. If one is already in flight, we can't do
+  // anything about it.
+  if (frame_copy_requests_.size() <= 1u)
+    return;
+  frame_copy_requests_.erase(frame_copy_requests_.begin() + 1,
+                             frame_copy_requests_.end());
+}
+
 void GpuMemoryBufferVideoFramePool::PoolImpl::OnCopiesDone(
     const scoped_refptr<VideoFrame>& video_frame,
     FrameResources* frame_resources) {
@@ -903,6 +916,9 @@ GpuMemoryBufferVideoFramePool::PoolImpl::~PoolImpl() {
 }
 
 void GpuMemoryBufferVideoFramePool::PoolImpl::Shutdown() {
+  // Clients don't care about copies once shutdown has started, so abort them.
+  Abort();
+
   // Delete all the resources on the media thread.
   in_shutdown_ = true;
   for (auto* frame_resources : resources_pool_) {
@@ -1065,6 +1081,10 @@ void GpuMemoryBufferVideoFramePool::MaybeCreateHardwareFrame(
   pool_impl_->CreateHardwareFrame(video_frame, std::move(frame_ready_cb));
 }
 
+void GpuMemoryBufferVideoFramePool::Abort() {
+  pool_impl_->Abort();
+}
+
 void GpuMemoryBufferVideoFramePool::SetTickClockForTesting(
     base::TickClock* tick_clock) {
   pool_impl_->SetTickClockForTesting(tick_clock);

media/video/gpu_memory_buffer_video_frame_pool.h

@@ -25,11 +25,12 @@ class GpuVideoAcceleratorFactories;
 // The pool recycles resources to a void unnecessarily allocating and
 // destroying textures, images and GpuMemoryBuffer that could result
 // in a round trip to the browser/GPU process.
-// NOTE: Destroying the pool will not immediately invalidate outstanding video
-// frames. GPU memory buffers will be kept alive by video frames indirectly
-// referencing them. Video frames themselves are ref-counted and will be
-// released when they are no longer needed, potentially after the pool is
-// destroyed.
+//
+// NOTE: While destroying the pool will abort any uncompleted copies, it will
+// not immediately invalidate outstanding video frames. GPU memory buffers will
+// be kept alive by video frames indirectly referencing them. Video frames
+// themselves are ref-counted and will be released when they are no longer
+// needed, potentially after the pool is destroyed.
 class MEDIA_EXPORT GpuMemoryBufferVideoFramePool {
  public:
   GpuMemoryBufferVideoFramePool();
@@ -55,6 +56,10 @@ class MEDIA_EXPORT GpuMemoryBufferVideoFramePool {
       const scoped_refptr<VideoFrame>& video_frame,
       FrameReadyCB frame_ready_cb);
 
+  // Aborts any pending copies. Previously provided |frame_ready_cb| callbacks
+  // may still be called if the copy has already started.
+  virtual void Abort();
+
   // Allows injection of a base::SimpleTestClock for testing.
   void SetTickClockForTesting(base::TickClock* tick_clock);
 

media/video/gpu_memory_buffer_video_frame_pool_unittest.cc

@@ -440,4 +440,29 @@ TEST_F(GpuMemoryBufferVideoFramePoolTest, AtMostOneCopyInFlight) {
   RunUntilIdle();
 }
 
+// Test that Abort() stops any pending copies.
+TEST_F(GpuMemoryBufferVideoFramePoolTest, AbortCopies) {
+  scoped_refptr<VideoFrame> software_frame_1 = CreateTestYUVVideoFrame(10);
+  scoped_refptr<VideoFrame> frame_1;
+  gpu_memory_buffer_pool_->MaybeCreateHardwareFrame(
+      software_frame_1,
+      base::BindOnce(MaybeCreateHardwareFrameCallback, &frame_1));
+
+  scoped_refptr<VideoFrame> software_frame_2 = CreateTestYUVVideoFrame(10);
+  scoped_refptr<VideoFrame> frame_2;
+  gpu_memory_buffer_pool_->MaybeCreateHardwareFrame(
+      software_frame_2,
+      base::BindOnce(MaybeCreateHardwareFrameCallback, &frame_2));
+
+  media_task_runner_->RunUntilIdle();
+  EXPECT_GE(1u, copy_task_runner_->NumPendingTasks());
+  copy_task_runner_->RunUntilIdle();
+
+  gpu_memory_buffer_pool_->Abort();
+  media_task_runner_->RunUntilIdle();
+  EXPECT_EQ(0u, copy_task_runner_->NumPendingTasks());
+  RunUntilIdle();
+  ASSERT_FALSE(frame_2);
+}
+
 }  // namespace media

media/video/mock_gpu_memory_buffer_video_frame_pool.cc

@@ -8,6 +8,12 @@
 
 namespace media {
 
+MockGpuMemoryBufferVideoFramePool::MockGpuMemoryBufferVideoFramePool(
+    std::vector<base::OnceClosure>* frame_ready_cbs)
+    : frame_ready_cbs_(frame_ready_cbs) {}
+
+MockGpuMemoryBufferVideoFramePool::~MockGpuMemoryBufferVideoFramePool() {}
+
 void MockGpuMemoryBufferVideoFramePool::MaybeCreateHardwareFrame(
     const scoped_refptr<VideoFrame>& video_frame,
     FrameReadyCB frame_ready_cb) {

media/video/mock_gpu_memory_buffer_video_frame_pool.h

@@ -7,18 +7,19 @@
 
 #include "base/callback.h"
 #include "media/video/gpu_memory_buffer_video_frame_pool.h"
+#include "testing/gmock/include/gmock/gmock.h"
 
 namespace media {
 
 class MockGpuMemoryBufferVideoFramePool : public GpuMemoryBufferVideoFramePool {
  public:
-  MockGpuMemoryBufferVideoFramePool(
-      std::vector<base::OnceClosure>* frame_ready_cbs)
-      : frame_ready_cbs_(frame_ready_cbs) {}
-  ~MockGpuMemoryBufferVideoFramePool() override = default;
+  explicit MockGpuMemoryBufferVideoFramePool(
+      std::vector<base::OnceClosure>* frame_ready_cbs);
+  ~MockGpuMemoryBufferVideoFramePool() override;
 
   void MaybeCreateHardwareFrame(const scoped_refptr<VideoFrame>& video_frame,
                                 FrameReadyCB frame_ready_cb) override;
+  MOCK_METHOD0(Abort, void());
 
  private:
   std::vector<base::OnceClosure>* frame_ready_cbs_;