VaVDA: extract surface lifetime into an interface

This CL extracts CreateSurface() and SurfaceReady() into a new
interface: DecodeSurfaceHandler.  This extraction reduces the
coupling surface (always good) and allows e.g. easier testing in
subsequent CLs.

VaapiVideoDecodeAccelerator is made to implement the said
templatised interface and teaches the Vaapi*Accelerator classes
to use it (instead of the whole VaVDA).

Partial diagram before: https://goo.gl/LBxH3h, after: https://goo.gl/E6sDrc.

Test: v_d_a_unittests and simplechrome on nautilus
Bug: 875005
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: Id376a6a25e664b0b6669b202062ebd7179346c50
Reviewed-on: https://chromium-review.googlesource.com/1178340
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586680}

media/gpu/BUILD.gn

@@ -327,6 +327,7 @@ source_set("common") {
   ]
   if (use_v4l2_codec || use_vaapi) {
     sources += [
+      "decode_surface_handler.h",
       "vp8_decoder.cc",
       "vp8_decoder.h",
       "vp8_picture.cc",

media/gpu/decode_surface_handler.h

+// Copyright 2018 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.
+
+#ifndef MEDIA_GPU_DECODE_SURFACE_HANDLER_H_
+#define MEDIA_GPU_DECODE_SURFACE_HANDLER_H_
+
+#include "base/memory/scoped_refptr.h"
+
+namespace gfx {
+class Rect;
+}
+
+namespace media {
+
+class VideoColorSpace;
+
+// Interface representing {V4L2,Vaapi}DecodeSurface operations, i.e. a client
+// gets a T to work with by calling CreateSurface() and returns it when finished
+// by calling SurfaceReady(). Class T has to be ref-counted. No assumptions are
+// made about threading.
+template <class T>
+class DecodeSurfaceHandler {
+ public:
+  DecodeSurfaceHandler() = default;
+  virtual ~DecodeSurfaceHandler() = default;
+
+  // Returns a T for decoding into, if available, or nullptr.
+  virtual scoped_refptr<T> CreateSurface() = 0;
+
+  // Called by the client to indicate that |dec_surface| is ready to be
+  // outputted. This can actually be called before decode is finished in
+  // hardware; this method must guarantee that |dec_surface|s are processed in
+  // the same order as SurfaceReady is called. (On Intel, this order doesn't
+  // need to be explicitly maintained since the driver will enforce it, together
+  // with any necessary dependencies).
+  virtual void SurfaceReady(const scoped_refptr<T>& dec_surface,
+                            int32_t bitstream_id,
+                            const gfx::Rect& visible_rect,
+                            const VideoColorSpace& color_space) = 0;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(DecodeSurfaceHandler);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_GPU_DECODE_SURFACE_HANDLER_H_

media/gpu/vaapi/vaapi_h264_accelerator.cc

@@ -4,9 +4,11 @@
 
 #include "media/gpu/vaapi/vaapi_h264_accelerator.h"
 
+#include <va/va.h>
+
+#include "media/gpu/decode_surface_handler.h"
 #include "media/gpu/h264_dpb.h"
 #include "media/gpu/vaapi/vaapi_common.h"
-#include "media/gpu/vaapi/vaapi_video_decode_accelerator.h"
 #include "media/gpu/vaapi/vaapi_wrapper.h"
 
 #define ARRAY_MEMCPY_CHECKED(to, from)                               \
@@ -39,7 +41,7 @@ static constexpr uint8_t kZigzagScan8x8[64] = {
 }  // namespace
 
 VaapiH264Accelerator::VaapiH264Accelerator(
-    VaapiVideoDecodeAccelerator* vaapi_dec,
+    DecodeSurfaceHandler<VASurface>* vaapi_dec,
     scoped_refptr<VaapiWrapper> vaapi_wrapper)
     : vaapi_wrapper_(vaapi_wrapper), vaapi_dec_(vaapi_dec) {
   DCHECK(vaapi_wrapper_);
@@ -54,7 +56,7 @@ VaapiH264Accelerator::~VaapiH264Accelerator() {
 
 scoped_refptr<H264Picture> VaapiH264Accelerator::CreateH264Picture() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  const auto va_surface = vaapi_dec_->CreateVASurface();
+  const auto va_surface = vaapi_dec_->CreateSurface();
   if (!va_surface)
     return nullptr;
 
@@ -306,9 +308,9 @@ bool VaapiH264Accelerator::OutputPicture(
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   const VaapiH264Picture* vaapi_pic = pic->AsVaapiH264Picture();
-  vaapi_dec_->VASurfaceReady(vaapi_pic->va_surface(), vaapi_pic->bitstream_id(),
-                             vaapi_pic->visible_rect(),
-                             vaapi_pic->get_colorspace());
+  vaapi_dec_->SurfaceReady(vaapi_pic->va_surface(), vaapi_pic->bitstream_id(),
+                           vaapi_pic->visible_rect(),
+                           vaapi_pic->get_colorspace());
   return true;
 }
 

media/gpu/vaapi/vaapi_h264_accelerator.h

@@ -5,21 +5,23 @@
 #ifndef MEDIA_GPU_VAAPI_VAAPI_H264_ACCELERATOR_H_
 #define MEDIA_GPU_VAAPI_VAAPI_H264_ACCELERATOR_H_
 
-#include <va/va.h>
-
 #include "base/sequence_checker.h"
 #include "media/filters/vp9_parser.h"
 #include "media/gpu/h264_decoder.h"
 
+// Verbatim from va/va.h, where typedef is used.
+typedef struct _VAPictureH264 VAPictureH264;
+
 namespace media {
 
+template <class T> class DecodeSurfaceHandler;
 class H264Picture;
-class VaapiVideoDecodeAccelerator;
+class VASurface;
 class VaapiWrapper;
 
 class VaapiH264Accelerator : public H264Decoder::H264Accelerator {
  public:
-  VaapiH264Accelerator(VaapiVideoDecodeAccelerator* vaapi_dec,
+  VaapiH264Accelerator(DecodeSurfaceHandler<VASurface>* vaapi_dec,
                        const scoped_refptr<VaapiWrapper> vaapi_wrapper);
   ~VaapiH264Accelerator() override;
 
@@ -51,7 +53,7 @@ class VaapiH264Accelerator : public H264Decoder::H264Accelerator {
                              int num_pics);
 
   const scoped_refptr<VaapiWrapper> vaapi_wrapper_;
-  VaapiVideoDecodeAccelerator* vaapi_dec_;
+  DecodeSurfaceHandler<VASurface>* vaapi_dec_;
 
   SEQUENCE_CHECKER(sequence_checker_);
 

media/gpu/vaapi/vaapi_video_decode_accelerator.cc

@@ -915,16 +915,16 @@ bool VaapiVideoDecodeAccelerator::TryToSetupDecodeOnSeparateThread(
   return false;
 }
 
-void VaapiVideoDecodeAccelerator::VASurfaceReady(
-    const scoped_refptr<VASurface>& va_surface,
+void VaapiVideoDecodeAccelerator::SurfaceReady(
+    const scoped_refptr<VASurface>& dec_surface,
     int32_t bitstream_id,
     const gfx::Rect& visible_rect,
     const VideoColorSpace& color_space) {
   if (!task_runner_->BelongsToCurrentThread()) {
     task_runner_->PostTask(
         FROM_HERE,
-        base::Bind(&VaapiVideoDecodeAccelerator::VASurfaceReady, weak_this_,
-                   va_surface, bitstream_id, visible_rect, color_space));
+        base::Bind(&VaapiVideoDecodeAccelerator::SurfaceReady, weak_this_,
+                   dec_surface, bitstream_id, visible_rect, color_space));
     return;
   }
 
@@ -939,12 +939,11 @@ void VaapiVideoDecodeAccelerator::VASurfaceReady(
 
   pending_output_cbs_.push(
       base::Bind(&VaapiVideoDecodeAccelerator::OutputPicture, weak_this_,
-                 va_surface, bitstream_id, visible_rect, color_space));
-
+                 dec_surface, bitstream_id, visible_rect, color_space));
   TryOutputPicture();
 }
 
-scoped_refptr<VASurface> VaapiVideoDecodeAccelerator::CreateVASurface() {
+scoped_refptr<VASurface> VaapiVideoDecodeAccelerator::CreateSurface() {
   DCHECK(decoder_thread_task_runner_->BelongsToCurrentThread());
   base::AutoLock auto_lock(lock_);
 

media/gpu/vaapi/vaapi_video_decode_accelerator.h

@@ -27,6 +27,7 @@
 #include "base/synchronization/lock.h"
 #include "base/threading/thread.h"
 #include "media/base/bitstream_buffer.h"
+#include "media/gpu/decode_surface_handler.h"
 #include "media/gpu/gpu_video_decode_accelerator_helpers.h"
 #include "media/gpu/media_gpu_export.h"
 #include "media/gpu/vaapi/vaapi_picture_factory.h"
@@ -52,7 +53,8 @@ class VaapiPicture;
 // stopped during |this->Destroy()|, so any tasks posted to the decoder thread
 // can assume |*this| is still alive.  See |weak_this_| below for more details.
 class MEDIA_GPU_EXPORT VaapiVideoDecodeAccelerator
-    : public VideoDecodeAccelerator {
+    : public VideoDecodeAccelerator,
+      public DecodeSurfaceHandler<VASurface> {
  public:
   VaapiVideoDecodeAccelerator(
       const MakeGLContextCurrentCallback& make_context_current_cb,
@@ -81,23 +83,12 @@ class MEDIA_GPU_EXPORT VaapiVideoDecodeAccelerator
 
   static VideoDecodeAccelerator::SupportedProfiles GetSupportedProfiles();
 
-  //
-  // Below methods are used by accelerator implementations.
-  //
-  // The |visible_rect| area of |va_surface| associated with |bitstream_id| is
-  // ready to be outputted once decode is finished. This can be called before
-  // decode is actually done in hardware, and this method is responsible for
-  // maintaining the ordering, i.e. the surfaces have to be outputted in the
-  // same order as VASurfaceReady is called.  On Intel, we don't have to
-  // explicitly maintain the ordering however, as the driver will maintain
-  // ordering, as well as dependencies, and will process each submitted command
-  // in order, and run each command only if its dependencies are ready.
-  void VASurfaceReady(const scoped_refptr<VASurface>& va_surface,
-                      int32_t bitstream_id,
-                      const gfx::Rect& visible_rect,
-                      const VideoColorSpace& color_space);
-  // Returns a new VASurface for decoding into, or nullptr if not available.
-  scoped_refptr<VASurface> CreateVASurface();
+  // DecodeSurfaceHandler implementation.
+  scoped_refptr<VASurface> CreateSurface() override;
+  void SurfaceReady(const scoped_refptr<VASurface>& va_surface,
+                    int32_t bitstream_id,
+                    const gfx::Rect& visible_rect,
+                    const VideoColorSpace& color_space) override;
 
  private:
   friend class VaapiVideoDecodeAcceleratorTest;

media/gpu/vaapi/vaapi_vp8_accelerator.cc

@@ -5,8 +5,8 @@
 #include "media/gpu/vaapi/vaapi_vp8_accelerator.h"
 
 #include "base/numerics/ranges.h"
+#include "media/gpu/decode_surface_handler.h"
 #include "media/gpu/vaapi/vaapi_common.h"
-#include "media/gpu/vaapi/vaapi_video_decode_accelerator.h"
 #include "media/gpu/vaapi/vaapi_wrapper.h"
 #include "media/gpu/vp8_picture.h"
 
@@ -20,7 +20,7 @@
 namespace media {
 
 VaapiVP8Accelerator::VaapiVP8Accelerator(
-    VaapiVideoDecodeAccelerator* vaapi_dec,
+    DecodeSurfaceHandler<VASurface>* vaapi_dec,
     scoped_refptr<VaapiWrapper> vaapi_wrapper)
     : vaapi_wrapper_(vaapi_wrapper), vaapi_dec_(vaapi_dec) {
   DCHECK(vaapi_wrapper_);
@@ -35,7 +35,7 @@ VaapiVP8Accelerator::~VaapiVP8Accelerator() {
 
 scoped_refptr<VP8Picture> VaapiVP8Accelerator::CreateVP8Picture() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  const auto va_surface = vaapi_dec_->CreateVASurface();
+  const auto va_surface = vaapi_dec_->CreateSurface();
   if (!va_surface)
     return nullptr;
 
@@ -229,9 +229,9 @@ bool VaapiVP8Accelerator::OutputPicture(const scoped_refptr<VP8Picture>& pic) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   const VaapiVP8Picture* vaapi_pic = pic->AsVaapiVP8Picture();
-  vaapi_dec_->VASurfaceReady(vaapi_pic->va_surface(), vaapi_pic->bitstream_id(),
-                             vaapi_pic->visible_rect(),
-                             vaapi_pic->get_colorspace());
+  vaapi_dec_->SurfaceReady(vaapi_pic->va_surface(), vaapi_pic->bitstream_id(),
+                           vaapi_pic->visible_rect(),
+                           vaapi_pic->get_colorspace());
   return true;
 }
 

media/gpu/vaapi/vaapi_vp8_accelerator.h

@@ -11,13 +11,14 @@
 
 namespace media {
 
+template <class T> class DecodeSurfaceHandler;
+class VASurface;
 class VP8Picture;
-class VaapiVideoDecodeAccelerator;
 class VaapiWrapper;
 
 class VaapiVP8Accelerator : public VP8Decoder::VP8Accelerator {
  public:
-  VaapiVP8Accelerator(VaapiVideoDecodeAccelerator* vaapi_dec,
+  VaapiVP8Accelerator(DecodeSurfaceHandler<VASurface>* vaapi_dec,
                       scoped_refptr<VaapiWrapper> vaapi_wrapper);
   ~VaapiVP8Accelerator() override;
 
@@ -29,7 +30,7 @@ class VaapiVP8Accelerator : public VP8Decoder::VP8Accelerator {
 
  private:
   const scoped_refptr<VaapiWrapper> vaapi_wrapper_;
-  VaapiVideoDecodeAccelerator* vaapi_dec_;
+  DecodeSurfaceHandler<VASurface>* vaapi_dec_;
 
   SEQUENCE_CHECKER(sequence_checker_);
 

media/gpu/vaapi/vaapi_vp9_accelerator.cc

@@ -4,8 +4,8 @@
 
 #include "media/gpu/vaapi/vaapi_vp9_accelerator.h"
 
+#include "media/gpu/decode_surface_handler.h"
 #include "media/gpu/vaapi/vaapi_common.h"
-#include "media/gpu/vaapi/vaapi_video_decode_accelerator.h"
 #include "media/gpu/vaapi/vaapi_wrapper.h"
 #include "media/gpu/vp9_picture.h"
 
@@ -19,7 +19,7 @@
 namespace media {
 
 VaapiVP9Accelerator::VaapiVP9Accelerator(
-    VaapiVideoDecodeAccelerator* vaapi_dec,
+    DecodeSurfaceHandler<VASurface>* vaapi_dec,
     scoped_refptr<VaapiWrapper> vaapi_wrapper)
     : vaapi_wrapper_(vaapi_wrapper), vaapi_dec_(vaapi_dec) {
   DCHECK(vaapi_wrapper_);
@@ -34,8 +34,7 @@ VaapiVP9Accelerator::~VaapiVP9Accelerator() {
 
 scoped_refptr<VP9Picture> VaapiVP9Accelerator::CreateVP9Picture() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
-  const auto va_surface = vaapi_dec_->CreateVASurface();
+  const auto va_surface = vaapi_dec_->CreateSurface();
   if (!va_surface)
     return nullptr;
 
@@ -166,9 +165,9 @@ bool VaapiVP9Accelerator::OutputPicture(const scoped_refptr<VP9Picture>& pic) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   const VaapiVP9Picture* vaapi_pic = pic->AsVaapiVP9Picture();
-  vaapi_dec_->VASurfaceReady(vaapi_pic->va_surface(), vaapi_pic->bitstream_id(),
-                             vaapi_pic->visible_rect(),
-                             vaapi_pic->get_colorspace());
+  vaapi_dec_->SurfaceReady(vaapi_pic->va_surface(), vaapi_pic->bitstream_id(),
+                           vaapi_pic->visible_rect(),
+                           vaapi_pic->get_colorspace());
   return true;
 }
 

media/gpu/vaapi/vaapi_vp9_accelerator.h

@@ -11,13 +11,14 @@
 
 namespace media {
 
+template <class T> class DecodeSurfaceHandler;
+class VASurface;
 class VP9Picture;
-class VaapiVideoDecodeAccelerator;
 class VaapiWrapper;
 
 class VaapiVP9Accelerator : public VP9Decoder::VP9Accelerator {
  public:
-  VaapiVP9Accelerator(VaapiVideoDecodeAccelerator* vaapi_dec,
+  VaapiVP9Accelerator(DecodeSurfaceHandler<VASurface>* vaapi_dec,
                       scoped_refptr<VaapiWrapper> vaapi_wrapper);
   ~VaapiVP9Accelerator() override;
 
@@ -36,7 +37,7 @@ class VaapiVP9Accelerator : public VP9Decoder::VP9Accelerator {
 
  private:
   const scoped_refptr<VaapiWrapper> vaapi_wrapper_;
-  VaapiVideoDecodeAccelerator* vaapi_dec_;
+  DecodeSurfaceHandler<VASurface>* vaapi_dec_;
 
   SEQUENCE_CHECKER(sequence_checker_);