VAAPI-JDA: Re-extract lower level decode code.

This CL effectively reverts the refactoring in
https://chromium-review.googlesource.com/c/chromium/src/+/1185218/. It
extracts the lower level decode code so that it can be used (later) to
implement a gpu::ImageDecodeAcceleratorWorker that decodes JPEGs using
the VAAPI.

A VaapiJpegDecoder class is created to contain this lower level code. In
a follow-up CL, this class is expanded to be responsible for dealing
with the VaapiWrapper and surface creation.

This opportunity is used to fix some includes and build dependencies.
Additionally, some code health fixes are done, the most significant
being getting rid of memset in the Fill*() methods.

Bug: 924310
Test: the VaapiJpegDecoderTest unit tests pass on a nocturne.
Change-Id: Idf207e9f81385611e84069e229a0e59eec2479ec
Reviewed-on: https://chromium-review.googlesource.com/c/1424401Reviewed-by: Dan Sanders <sandersd@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Commit-Queue: Andres Calderon Jaramillo <andrescj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#626162}

media/gpu/BUILD.gn

@@ -564,7 +564,7 @@ if (is_chromeos || is_linux) {
       "//media/test/data/peach_pi-41x23.jpg",
     ]
     if (use_vaapi) {
-      deps += [ "//media/gpu/vaapi:jpeg_decode_accelerator_unit_test" ]
+      deps += [ "//media/gpu/vaapi:jpeg_decoder_unit_test" ]
       data += [ "//media/test/data/pixel-1280x720.jpg" ]
     }
     if (use_x11) {

media/gpu/vaapi/BUILD.gn

@@ -41,6 +41,8 @@ source_set("vaapi") {
     "vaapi_h264_accelerator.h",
     "vaapi_jpeg_decode_accelerator.cc",
     "vaapi_jpeg_decode_accelerator.h",
+    "vaapi_jpeg_decoder.cc",
+    "vaapi_jpeg_decoder.h",
     "vaapi_jpeg_encode_accelerator.cc",
     "vaapi_jpeg_encode_accelerator.h",
     "vaapi_jpeg_encoder.cc",
@@ -69,10 +71,12 @@ source_set("vaapi") {
 
   deps = [
     ":libva_stubs",
+    "//base",
     "//gpu/ipc/service",
     "//media",
     "//media/gpu:common",
     "//third_party/libyuv",
+    "//ui/gfx/geometry",
   ]
 
   if (is_linux) {
@@ -127,19 +131,18 @@ source_set("unit_test") {
   ]
 }
 
-source_set("jpeg_decode_accelerator_unit_test") {
+source_set("jpeg_decoder_unit_test") {
   testonly = true
   sources = [
-    "vaapi_jpeg_decode_accelerator_unittest.cc",
+    "vaapi_jpeg_decoder_unittest.cc",
   ]
   deps = [
     ":vaapi",
+    "//base",
     "//base/test:test_support",
-    "//gpu:test_support",
+    "//media",
     "//media:test_support",
-    "//media/gpu:common",
-    "//testing/gmock",
     "//testing/gtest",
-    "//ui/gfx:test_support",
+    "//ui/gfx/geometry",
   ]
 }

media/gpu/vaapi/vaapi_jpeg_decode_accelerator.h

@@ -10,27 +10,28 @@
 #include <memory>
 
 #include "base/macros.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
-#include "base/single_thread_task_runner.h"
 #include "base/threading/thread.h"
 #include "media/gpu/media_gpu_export.h"
 #include "media/video/jpeg_decode_accelerator.h"
+#include "ui/gfx/geometry/size.h"
 
-// These data types are defined in va/va.h using typedef, reproduced here.
-typedef struct _VAImageFormat VAImageFormat;
-typedef unsigned int VASurfaceID;
+// This data type is defined in va/va.h using typedef, reproduced here.
+// TODO(andrescj): remove this once VaapiJpegDecoder is responsible for
+// obtaining the VAImage.
+using VASurfaceID = unsigned int;
+
+namespace base {
+class SingleThreadTaskRunner;
+}
 
 namespace media {
 
 class BitstreamBuffer;
-struct JpegParseResult;
 class UnalignedSharedMemory;
 class VaapiWrapper;
-class VaapiJpegDecodeAcceleratorTest;
-
-// Alternative notation for the VA_FOURCC_YUY2 format, <va/va.h> doesn't provide
-// this specific packing/ordering.
-constexpr uint32_t VA_FOURCC_YUYV = 0x56595559;
+class VideoFrame;
 
 // Class to provide JPEG decode acceleration for Intel systems with hardware
 // support for it, and on which libva is available.
@@ -54,8 +55,6 @@ class MEDIA_GPU_EXPORT VaapiJpegDecodeAccelerator
   bool IsSupported() override;
 
  private:
-  friend class VaapiJpegDecodeAcceleratorTest;
-
   // Notifies the client that an error has occurred and decoding cannot
   // continue. The client is notified on the |task_runner_|, i.e., the thread in
   // which |*this| was created.
@@ -78,16 +77,6 @@ class MEDIA_GPU_EXPORT VaapiJpegDecodeAccelerator
                      int32_t input_buffer_id,
                      const scoped_refptr<VideoFrame>& video_frame);
 
-  // Decodes a JPEG picture. It will fill VA-API parameters and call
-  // corresponding VA-API methods according to the JPEG |parse_result|. Decoded
-  // data will be outputted to the given |va_surface|. Returns false on failure.
-  // |vaapi_wrapper| should be initialized in kDecode mode with
-  // VAProfileJPEGBaseline profile. |va_surface| should be created with size at
-  // least as large as the picture size.
-  static bool DoDecode(VaapiWrapper* vaapi_wrapper,
-                       const JpegParseResult& parse_result,
-                       VASurfaceID va_surface);
-
   // ChildThread's task runner.
   const scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
 

media/gpu/vaapi/vaapi_jpeg_decoder.h

+// Copyright 2019 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_VAAPI_VAAPI_JPEG_DECODER_H_
+#define MEDIA_GPU_VAAPI_VAAPI_JPEG_DECODER_H_
+
+#include <stdint.h>
+
+// These data types are defined in va/va.h using typedef, reproduced here.
+// TODO(andrescj): revisit this once VaSurfaceFormatToImageFormat() and
+// VaSurfaceFormatForJpeg() are moved to the anonymous namespace in the .cc
+// file.
+using VAImageFormat = struct _VAImageFormat;
+using VASurfaceID = unsigned int;
+
+namespace media {
+
+struct JpegFrameHeader;
+struct JpegParseResult;
+class VaapiWrapper;
+
+// Convert the specified surface format to the associated output image format.
+bool VaSurfaceFormatToImageFormat(uint32_t va_rt_format,
+                                  VAImageFormat* va_image_format);
+
+unsigned int VaSurfaceFormatForJpeg(const JpegFrameHeader& frame_header);
+
+class VaapiJpegDecoder {
+ public:
+  VaapiJpegDecoder() = default;
+  virtual ~VaapiJpegDecoder() = default;
+
+  // Decodes a JPEG picture. It will fill VA-API parameters and call
+  // corresponding VA-API methods according to the JPEG |parse_result|. Decoded
+  // data will be outputted to the given |va_surface|. Returns false on failure.
+  // |vaapi_wrapper| should be initialized in kDecode mode with
+  // VAProfileJPEGBaseline profile. |va_surface| should be created with size at
+  // least as large as the picture size.
+  static bool DoDecode(VaapiWrapper* vaapi_wrapper,
+                       const JpegParseResult& parse_result,
+                       VASurfaceID va_surface);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_GPU_VAAPI_VAAPI_JPEG_DECODER_H_

media/gpu/vaapi/vaapi_jpeg_decode_accelerator_unittest.cc → media/gpu/vaapi/vaapi_jpeg_decoder_unittest.cc

@@ -3,7 +3,10 @@
 // found in the LICENSE file.
 
 #include <stdint.h>
-#include <string.h>
+
+#include <memory>
+#include <string>
+#include <vector>
 
 #include <va/va.h>
 
@@ -11,21 +14,26 @@
 // See http://code.google.com/p/googletest/issues/detail?id=371
 #include "testing/gtest/include/gtest/gtest.h"
 
-#include "base/at_exit.h"
 #include "base/bind.h"
+#include "base/callback.h"
 #include "base/command_line.h"
+#include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/md5.h"
-#include "base/path_service.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/strings/string_piece.h"
+#include "base/synchronization/lock.h"
 #include "base/test/gtest_util.h"
+#include "base/thread_annotations.h"
 #include "media/base/test_data_util.h"
 #include "media/base/video_frame.h"
+#include "media/base/video_types.h"
 #include "media/filters/jpeg_parser.h"
-#include "media/gpu/vaapi/vaapi_jpeg_decode_accelerator.h"
+#include "media/gpu/vaapi/vaapi_jpeg_decoder.h"
 #include "media/gpu/vaapi/vaapi_utils.h"
 #include "media/gpu/vaapi/vaapi_wrapper.h"
+#include "ui/gfx/geometry/size.h"
 
 namespace media {
 namespace {
@@ -34,12 +42,17 @@ constexpr const char* kTestFilename = "pixel-1280x720.jpg";
 constexpr const char* kExpectedMd5SumI420 = "6e9e1716073c9a9a1282e3f0e0dab743";
 constexpr const char* kExpectedMd5SumYUYV = "ff313a6aedbc4e157561e5c2d5c2e079";
 
-constexpr VAImageFormat kImageFormatI420 = {.fourcc = VA_FOURCC_I420,
+constexpr VAImageFormat kImageFormatI420 = {
+    .fourcc = VA_FOURCC_I420,
     .byte_order = VA_LSB_FIRST,
-                                            .bits_per_pixel = 12};
-constexpr VAImageFormat kImageFormatYUYV = {.fourcc = VA_FOURCC_YUYV,
+    .bits_per_pixel = 12,
+};
+
+constexpr VAImageFormat kImageFormatYUYV = {
+    .fourcc = VA_FOURCC('Y', 'U', 'Y', 'V'),
     .byte_order = VA_LSB_FIRST,
-                                            .bits_per_pixel = 16};
+    .bits_per_pixel = 16,
+};
 
 void LogOnError() {
   LOG(FATAL) << "Oh noes! Decoder failed";
@@ -67,14 +80,13 @@ VAImageFormat GetVAImageFormat() {
 
 }  // namespace
 
-class VaapiJpegDecodeAcceleratorTest : public ::testing::Test {
+class VaapiJpegDecoderTest : public ::testing::Test {
  protected:
-  VaapiJpegDecodeAcceleratorTest() {
+  VaapiJpegDecoderTest() {
     const base::CommandLine* cmd_line = base::CommandLine::ForCurrentProcess();
-    if (cmd_line && cmd_line->HasSwitch("test_data_path")) {
+    if (cmd_line && cmd_line->HasSwitch("test_data_path"))
       test_data_path_ = cmd_line->GetSwitchValueASCII("test_data_path");
   }
-  }
 
   void SetUp() override {
     base::RepeatingClosure report_error_cb = base::BindRepeating(&LogOnError);
@@ -118,7 +130,7 @@ class VaapiJpegDecodeAcceleratorTest : public ::testing::Test {
 // is not found, treat the file as being relative to the test file directory.
 // This is either a custom test data path provided by --test_data_path, or the
 // default test data path (//media/test/data).
-base::FilePath VaapiJpegDecodeAcceleratorTest::FindTestDataFilePath(
+base::FilePath VaapiJpegDecoderTest::FindTestDataFilePath(
     const std::string& file_name) {
   const base::FilePath file_path = base::FilePath(file_name);
   if (base::PathExists(file_path))
@@ -128,7 +140,7 @@ base::FilePath VaapiJpegDecodeAcceleratorTest::FindTestDataFilePath(
   return GetTestDataFilePath(file_name);
 }
 
-bool VaapiJpegDecodeAcceleratorTest::VerifyDecode(
+bool VaapiJpegDecoderTest::VerifyDecode(
     const JpegParseResult& parse_result) const {
   gfx::Size size(parse_result.frame_header.coded_width,
                  parse_result.frame_header.coded_height);
@@ -179,14 +191,13 @@ bool VaapiJpegDecodeAcceleratorTest::VerifyDecode(
   return true;
 }
 
-bool VaapiJpegDecodeAcceleratorTest::Decode(VaapiWrapper* vaapi_wrapper,
+bool VaapiJpegDecoderTest::Decode(VaapiWrapper* vaapi_wrapper,
                                   const JpegParseResult& parse_result,
                                   VASurfaceID va_surface) const {
-  return VaapiJpegDecodeAccelerator::DoDecode(vaapi_wrapper, parse_result,
-                                              va_surface);
+  return VaapiJpegDecoder::DoDecode(vaapi_wrapper, parse_result, va_surface);
 }
 
-TEST_F(VaapiJpegDecodeAcceleratorTest, DecodeSuccess) {
+TEST_F(VaapiJpegDecoderTest, DecodeSuccess) {
   JpegParseResult parse_result;
   ASSERT_TRUE(
       ParseJpegPicture(reinterpret_cast<const uint8_t*>(jpeg_data_.data()),
@@ -195,7 +206,7 @@ TEST_F(VaapiJpegDecodeAcceleratorTest, DecodeSuccess) {
   EXPECT_TRUE(VerifyDecode(parse_result));
 }
 
-TEST_F(VaapiJpegDecodeAcceleratorTest, DecodeFail) {
+TEST_F(VaapiJpegDecoderTest, DecodeFail) {
   JpegParseResult parse_result;
   ASSERT_TRUE(
       ParseJpegPicture(reinterpret_cast<const uint8_t*>(jpeg_data_.data()),
@@ -216,7 +227,7 @@ TEST_F(VaapiJpegDecodeAcceleratorTest, DecodeFail) {
 }
 
 // This test exercises the usual ScopedVAImage lifetime.
-TEST_F(VaapiJpegDecodeAcceleratorTest, ScopedVAImage) {
+TEST_F(VaapiJpegDecoderTest, ScopedVAImage) {
   std::vector<VASurfaceID> va_surfaces;
   const gfx::Size coded_size(64, 64);
   ASSERT_TRUE(wrapper_->CreateContextAndSurfaces(VA_RT_FORMAT_YUV420,
@@ -242,7 +253,7 @@ TEST_F(VaapiJpegDecodeAcceleratorTest, ScopedVAImage) {
 }
 
 // This test exercises creation of a ScopedVAImage with a bad VASurfaceID.
-TEST_F(VaapiJpegDecodeAcceleratorTest, BadScopedVAImage) {
+TEST_F(VaapiJpegDecoderTest, BadScopedVAImage) {
   const std::vector<VASurfaceID> va_surfaces = {VA_INVALID_ID};
   const gfx::Size coded_size(64, 64);
 
@@ -265,7 +276,7 @@ TEST_F(VaapiJpegDecodeAcceleratorTest, BadScopedVAImage) {
 }
 
 // This test exercises creation of a ScopedVABufferMapping with bad VABufferIDs.
-TEST_F(VaapiJpegDecodeAcceleratorTest, BadScopedVABufferMapping) {
+TEST_F(VaapiJpegDecoderTest, BadScopedVABufferMapping) {
   base::AutoLock auto_lock(*GetVaapiWrapperLock());
 
   // A ScopedVABufferMapping with a VA_INVALID_ID VABufferID is DCHECK()ed.

media/gpu/vaapi/vaapi_wrapper.h

@@ -223,7 +223,7 @@ class MEDIA_GPU_EXPORT VaapiWrapper
 
  private:
   friend class base::RefCountedThreadSafe<VaapiWrapper>;
-  friend class VaapiJpegDecodeAcceleratorTest;
+  friend class VaapiJpegDecoderTest;
 
   bool Initialize(CodecMode mode, VAProfile va_profile);
   void Deinitialize();