media/gpu/vaapiWrapper: Destroy pending buffers on failure of submitting buffers

If VaapiWrapper continues executing SubmitBuffer() even after the
previous SubmitBuffer() fails, VABuffers stored in a pending list
will be submitted to a driver by vaRenderPicture(). Some of the
submitted buffers might not be related to the current processing
and therefore the processing will fails.
We assume VaapiWrapper client stops using the VaapiWrapper and
destroys it if SubmitBuffer() fails. Although this should be
true, this CL makes sure that the pended VABuffers are not
used for the next processing by destroying them within
SubmitBuffer_Locked() on failure.

Bug: b:175897723
Test: vaapi_wrapper_unittest
Change-Id: I24eaa5f94a00f69d18c2a2f1bda707e4ccb21c87
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2612292
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Andres Calderon Jaramillo <andrescj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843408}

media/gpu/vaapi/BUILD.gn

@@ -222,6 +222,7 @@ source_set("unit_test") {
     "vaapi_image_decode_accelerator_worker_unittest.cc",
     "vaapi_video_decode_accelerator_unittest.cc",
     "vaapi_video_encode_accelerator_unittest.cc",
+    "vaapi_wrapper_unittest.cc",
     "vp9_encoder_unittest.cc",
     "vp9_temporal_layers_unittest.cc",
   ]

media/gpu/vaapi/vaapi_wrapper.cc

@@ -2809,10 +2809,12 @@ bool VaapiWrapper::SubmitBuffer_Locked(const VABufferDescriptor& va_buffer) {
   va_lock_->AssertAcquired();
 
   DCHECK(IsValidVABufferType(va_buffer.type));
-  DCHECK(va_buffer.data);
-
+  base::ScopedClosureRunner pending_buffers_destroyer_on_failure(base::BindOnce(
+      &VaapiWrapper::DestroyPendingBuffers_Locked, base::Unretained(this)));
   unsigned int va_buffer_size;
-  if (!base::CheckedNumeric<size_t>(va_buffer.size)
+  // We use a null |va_buffer|.data for testing: it signals that we want this
+  // SubmitBuffer_Locked() call to fail.
+  if (!va_buffer.data || !base::CheckedNumeric<size_t>(va_buffer.size)
                               .AssignIfValid(&va_buffer_size)) {
     return false;
   }
@@ -2831,6 +2833,7 @@ bool VaapiWrapper::SubmitBuffer_Locked(const VABufferDescriptor& va_buffer) {
     return false;
 
   pending_va_buffers_.push_back(buffer_id);
+  pending_buffers_destroyer_on_failure.ReplaceClosure(base::DoNothing());
   return true;
 }
 

media/gpu/vaapi/vaapi_wrapper.h

@@ -348,7 +348,8 @@ class MEDIA_GPU_EXPORT VaapiWrapper
   // allocated VABufferIDs stay alive until DestroyPendingBuffers_Locked(). Note
   // that this method does not submit the buffers for execution, they are simply
   // stored until ExecuteAndDestroyPendingBuffers()/Execute_Locked(). The
-  // ownership of |data| stays with the caller.
+  // ownership of |data| stays with the caller. On failure, all pending buffers
+  // are destroyed.
   bool SubmitBuffer(VABufferType va_buffer_type,
                     size_t size,
                     const void* data) WARN_UNUSED_RESULT;
@@ -470,6 +471,7 @@ class MEDIA_GPU_EXPORT VaapiWrapper
 
  private:
   friend class base::RefCountedThreadSafe<VaapiWrapper>;
+  friend class VaapiWrapperTest;
 
   FRIEND_TEST_ALL_PREFIXES(VaapiTest, LowQualityEncodingSetting);
   FRIEND_TEST_ALL_PREFIXES(VaapiUtilsTest, ScopedVAImage);
@@ -497,11 +499,13 @@ class MEDIA_GPU_EXPORT VaapiWrapper
                       const std::vector<VABufferID>& va_buffers)
       EXCLUSIVE_LOCKS_REQUIRED(va_lock_) WARN_UNUSED_RESULT;
 
-  void DestroyPendingBuffers_Locked() EXCLUSIVE_LOCKS_REQUIRED(va_lock_);
+  virtual void DestroyPendingBuffers_Locked()
+      EXCLUSIVE_LOCKS_REQUIRED(va_lock_);
 
   // Requests libva to allocate a new VABufferID of type |va_buffer.type|, then
-  // maps-and-copies |va_buffer.size| contents of |va_buffer.data| to it.
-  bool SubmitBuffer_Locked(const VABufferDescriptor& va_buffer)
+  // maps-and-copies |va_buffer.size| contents of |va_buffer.data| to it. If a
+  // failure occurs, calls DestroyPendingBuffers_Locked() and returns false.
+  virtual bool SubmitBuffer_Locked(const VABufferDescriptor& va_buffer)
       EXCLUSIVE_LOCKS_REQUIRED(va_lock_) WARN_UNUSED_RESULT;
 
   // Maps |va_buffer_id| and, if successful, copies the contents of |va_buffer|

media/gpu/vaapi/vaapi_wrapper_unittest.cc

+// Copyright 2021 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <va/va.h>
+
+#include "media/gpu/vaapi/vaapi_wrapper.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using ::testing::_;
+using ::testing::Invoke;
+using ::testing::Return;
+
+namespace media {
+namespace {
+VaapiWrapper::VABufferDescriptor CreateVABufferDescriptor() {
+  constexpr static char kData[] = "vaBufferData";
+  return VaapiWrapper::VABufferDescriptor{VAProcPipelineParameterBufferType,
+                                          sizeof(kData), kData};
+}
+
+class MockVaapiWrapper : public VaapiWrapper {
+ public:
+  MockVaapiWrapper() : VaapiWrapper(kVideoProcess) {}
+  MOCK_METHOD1(SubmitBuffer_Locked, bool(const VABufferDescriptor&));
+  MOCK_METHOD0(DestroyPendingBuffers_Locked, void());
+
+ protected:
+  ~MockVaapiWrapper() override = default;
+};
+}  // namespace
+
+class VaapiWrapperTest : public testing::Test {
+ public:
+  VaapiWrapperTest() = default;
+
+  void SetUp() override {
+    // Create a VaapiWrapper for testing.
+    mock_vaapi_wrapper_ = base::MakeRefCounted<MockVaapiWrapper>();
+    ASSERT_TRUE(mock_vaapi_wrapper_);
+
+    ON_CALL(*mock_vaapi_wrapper_, SubmitBuffer_Locked)
+        .WillByDefault(
+            Invoke(this, &VaapiWrapperTest::DefaultSubmitBuffer_Locked));
+    ON_CALL(*mock_vaapi_wrapper_, DestroyPendingBuffers_Locked)
+        .WillByDefault(Invoke(
+            this, &VaapiWrapperTest::DefaultDestroyPendingBuffers_Locked));
+  }
+  void TearDown() override {
+    // The VaapiWrapper destructor calls DestroyPendingBuffers_Locked(). Since
+    // MockVaapiWrapper is a derived class,
+    // MockVaapiWrapper::DestroyPendingBuffers_Locked() won't get called during
+    // destruction even though it's a virtual function. Instead,
+    // VaapiWrapper::DestroyPendingBuffers_Locked() will get called. Therefore,
+    // we need to clear |pending_va_buffers_| before this happens so that
+    // VaapiWrapper::DestroyPendingBuffers_Locked() doesn't call
+    // vaDestroyBuffer().
+    mock_vaapi_wrapper_->pending_va_buffers_.clear();
+    mock_vaapi_wrapper_.reset();
+  }
+
+  bool DefaultSubmitBuffer_Locked(
+      const VaapiWrapper::VABufferDescriptor& va_buffer)
+      EXCLUSIVE_LOCKS_REQUIRED(mock_vaapi_wrapper_->va_lock_) {
+    if (va_buffer.data) {
+      constexpr VABufferID kFakeBufferId = 1234;
+      mock_vaapi_wrapper_->pending_va_buffers_.push_back(kFakeBufferId);
+      return true;
+    }
+    // When |va_buffer|.data is null, the base method should return false and
+    // no libva calls should be made.
+    const bool submit_buffer_res =
+        (*mock_vaapi_wrapper_).VaapiWrapper::SubmitBuffer_Locked(va_buffer);
+    if (submit_buffer_res)
+      ADD_FAILURE();
+    return false;
+  }
+
+  void DefaultDestroyPendingBuffers_Locked()
+      EXCLUSIVE_LOCKS_REQUIRED(mock_vaapi_wrapper_->va_lock_) {
+    mock_vaapi_wrapper_->pending_va_buffers_.clear();
+  }
+
+  size_t GetPendingBuffersSize() const {
+    return mock_vaapi_wrapper_->pending_va_buffers_.size();
+  }
+
+ protected:
+  scoped_refptr<MockVaapiWrapper> mock_vaapi_wrapper_;
+};
+
+// This test ensures SubmitBuffer() calls SubmitBuffer_Locked().
+TEST_F(VaapiWrapperTest, SubmitBuffer) {
+  constexpr size_t kNumBuffers = 3;
+  auto va_buffer = CreateVABufferDescriptor();
+
+  EXPECT_CALL(*mock_vaapi_wrapper_, SubmitBuffer_Locked(_)).Times(kNumBuffers);
+  for (size_t i = 0; i < kNumBuffers; ++i) {
+    EXPECT_TRUE(mock_vaapi_wrapper_->SubmitBuffer(
+        va_buffer.type, va_buffer.size, va_buffer.data));
+  }
+  EXPECT_EQ(GetPendingBuffersSize(), kNumBuffers);
+}
+
+// This test ensures SubmitBuffers() calls SubmitBuffer_Locked() as many times
+// as the number of passed buffers.
+TEST_F(VaapiWrapperTest, SubmitBuffers) {
+  constexpr size_t kNumBuffers = 3;
+  auto va_buffer = CreateVABufferDescriptor();
+  std::vector<VaapiWrapper::VABufferDescriptor> buffers(kNumBuffers, va_buffer);
+
+  EXPECT_CALL(*mock_vaapi_wrapper_, SubmitBuffer_Locked(_)).Times(kNumBuffers);
+  EXPECT_TRUE(mock_vaapi_wrapper_->SubmitBuffers(buffers));
+  EXPECT_EQ(GetPendingBuffersSize(), kNumBuffers);
+}
+
+// This test ensures DestroyPendingBuffers_Locked() is executed on a failure of
+// SubmitBuffer().
+TEST_F(VaapiWrapperTest, FailOnSubmitBuffer) {
+  auto va_buffer = CreateVABufferDescriptor();
+
+  ::testing::InSequence s;
+  EXPECT_CALL(*mock_vaapi_wrapper_, SubmitBuffer_Locked(_)).Times(2);
+  EXPECT_CALL(*mock_vaapi_wrapper_, DestroyPendingBuffers_Locked);
+  EXPECT_TRUE(mock_vaapi_wrapper_->SubmitBuffer(va_buffer.type, va_buffer.size,
+                                                va_buffer.data));
+  EXPECT_FALSE(mock_vaapi_wrapper_->SubmitBuffer(va_buffer.type, va_buffer.size,
+                                                 /*data=*/nullptr));
+  EXPECT_EQ(GetPendingBuffersSize(), 0u);
+}
+
+// This test ensures DestroyPendingBuffers_Locked() is executed on a failure of
+// SubmitBuffers().
+TEST_F(VaapiWrapperTest, FailOnSubmitBuffers) {
+  constexpr size_t kNumBuffers = 3;
+  auto va_buffer = CreateVABufferDescriptor();
+  std::vector<VaapiWrapper::VABufferDescriptor> buffers(kNumBuffers, va_buffer);
+  // Set data to nullptr so that VaapiWrapper::SubmitBuffer_Locked() fails.
+  buffers[1].data = nullptr;
+
+  ::testing::InSequence s;
+  EXPECT_CALL(*mock_vaapi_wrapper_, SubmitBuffer_Locked(_))
+      .Times(kNumBuffers - 1);
+  EXPECT_CALL(*mock_vaapi_wrapper_, DestroyPendingBuffers_Locked);
+  EXPECT_FALSE(mock_vaapi_wrapper_->SubmitBuffers(buffers));
+  EXPECT_EQ(GetPendingBuffersSize(), 0u);
+}
+}  // namespace media