media: Remove JPEG YUV to RGB Color Mapping Assumption

Now that another VA-API image decoder is available, namely VaapiWebPDecoder, this CL removes the JPEG YUV to RGB color mapping assumption in ImageTransferCacheEntry::BuildFromHardwareDecodedImage(). Bug: 988860 Test: media_unittests, cc_unittests Change-Id: Ife799854cea3ceb3ff0a226830b2fabeaca03bf6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1726795 Commit-Queue: Gil Dekel <gildekel@chromium.org> Reviewed-by: Andres Calderon Jaramillo <andrescj@chromium.org> Reviewed-by: Miguel Casas <mcasas@chromium.org> Reviewed-by: Khushal <khushalsagar@chromium.org> Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org> Cr-Commit-Position: refs/heads/master@{#685892}
chromium · Aug 11, 2019 · 00a1445 · 00a1445
1 parent 86d16dd
commit 00a1445
Show file tree

Hide file tree

Showing 15 changed files with 66 additions and 21 deletions.
diff --git a/cc/paint/image_transfer_cache_entry.cc b/cc/paint/image_transfer_cache_entry.cc
@@ -16,7 +16,6 @@
 #include "cc/paint/paint_op_writer.h"
 #include "third_party/skia/include/core/SkColorSpace.h"
 #include "third_party/skia/include/core/SkImage.h"
-#include "third_party/skia/include/core/SkImageInfo.h"
 #include "third_party/skia/include/core/SkPixmap.h"
 #include "third_party/skia/include/core/SkYUVAIndex.h"
 #include "third_party/skia/include/gpu/GrBackendSurface.h"
@@ -326,10 +325,10 @@ bool ServiceImageTransferCacheEntry::BuildFromHardwareDecodedImage(
     GrContext* context,
     std::vector<sk_sp<SkImage>> plane_images,
     YUVDecodeFormat plane_images_format,
+    SkYUVColorSpace yuv_color_space,
     size_t buffer_byte_size,
     bool needs_mips) {
   context_ = context;
-  yuv_color_space_ = SkYUVColorSpace::kJPEG_SkYUVColorSpace;
 
   // 1) Generate mipmap chains if requested.
   if (needs_mips) {
@@ -344,13 +343,14 @@ bool ServiceImageTransferCacheEntry::BuildFromHardwareDecodedImage(
   }
   plane_images_ = std::move(plane_images);
   plane_images_format_ = plane_images_format;
+  yuv_color_space_ = yuv_color_space;
 
   // 2) Create a SkImage backed by |plane_images|.
   // TODO(andrescj): support embedded color profiles for hardware decodes and
   // pass the color space to MakeYUVImageFromUploadedPlanes.
-  image_ = MakeYUVImageFromUploadedPlanes(
-      context_, plane_images_, plane_images_format_, yuv_color_space_,
-      SkColorSpace::MakeSRGB());
+  image_ = MakeYUVImageFromUploadedPlanes(context_, plane_images_,
+                                          plane_images_format_, yuv_color_space,
+                                          SkColorSpace::MakeSRGB());
   if (!image_)
     return false;
 
@@ -454,9 +454,9 @@ bool ServiceImageTransferCacheEntry::Deserialize(
       // unit tests.
       plane_images_.push_back(std::move(plane));
     }
-
+    DCHECK(yuv_color_space_.has_value());
     image_ = MakeYUVImageFromUploadedPlanes(
-        context_, plane_images_, plane_images_format_, yuv_color_space_,
+        context_, plane_images_, plane_images_format_, yuv_color_space_.value(),
         decoded_color_space);
     return !!image_;
   }
@@ -570,7 +570,7 @@ void ServiceImageTransferCacheEntry::EnsureMips() {
 
   if (is_yuv()) {
     DCHECK(image_);
-    DCHECK_LE(yuv_color_space_, SkYUVColorSpace::kLastEnum_SkYUVColorSpace);
+    DCHECK(yuv_color_space_.has_value());
     DCHECK_NE(YUVDecodeFormat::kUnknown, plane_images_format_);
     DCHECK_EQ(NumberOfPlanesForYUVDecodeFormat(plane_images_format_),
               plane_images_.size());
@@ -590,7 +590,7 @@ void ServiceImageTransferCacheEntry::EnsureMips() {
     }
     mipped_planes.clear();
     image_ = MakeYUVImageFromUploadedPlanes(
-        context_, plane_images_, plane_images_format_, yuv_color_space_,
+        context_, plane_images_, plane_images_format_, yuv_color_space_.value(),
         image_->refColorSpace() /* image_color_space */);
     has_mips_ = true;
     return;

diff --git a/cc/paint/image_transfer_cache_entry.h b/cc/paint/image_transfer_cache_entry.h
@@ -12,7 +12,9 @@
 
 #include "base/atomic_sequence_num.h"
 #include "base/containers/span.h"
+#include "base/optional.h"
 #include "cc/paint/transfer_cache_entry.h"
+#include "third_party/skia/include/core/SkImageInfo.h"
 #include "third_party/skia/include/core/SkRefCnt.h"
 #include "third_party/skia/include/core/SkYUVASizeInfo.h"
 
@@ -106,13 +108,15 @@ class CC_PAINT_EXPORT ServiceImageTransferCacheEntry
   //
   // - The backing textures don't have mipmaps. We will generate the mipmaps if
   //   |needs_mips| is true.
-  // - The conversion from YUV to RGB will be performed assuming a JPEG image.
+  // - The conversion from YUV to RGB will be performed according to
+  //   |yuv_color_space|.
   // - The colorspace of the resulting RGB image is sRGB.
   //
   // Returns true if the entry can be built, false otherwise.
   bool BuildFromHardwareDecodedImage(GrContext* context,
                                      std::vector<sk_sp<SkImage>> plane_images,
                                      YUVDecodeFormat plane_images_format,
+                                     SkYUVColorSpace yuv_color_space,
                                      size_t buffer_byte_size,
                                      bool needs_mips);
 
@@ -148,7 +152,7 @@ class CC_PAINT_EXPORT ServiceImageTransferCacheEntry
   YUVDecodeFormat plane_images_format_ = YUVDecodeFormat::kUnknown;
   std::vector<size_t> plane_sizes_;
   sk_sp<SkImage> image_;
-  SkYUVColorSpace yuv_color_space_;
+  base::Optional<SkYUVColorSpace> yuv_color_space_;
   bool has_mips_ = false;
   size_t size_ = 0;
   bool fits_on_gpu_ = false;

diff --git a/cc/paint/image_transfer_cache_entry_unittest.cc b/cc/paint/image_transfer_cache_entry_unittest.cc
@@ -37,6 +37,9 @@
 namespace cc {
 namespace {
 
+constexpr SkYUVColorSpace kJpegYUVColorSpace =
+    SkYUVColorSpace::kJPEG_SkYUVColorSpace;
+
 void MarkTextureAsReleased(SkImage::ReleaseContext context) {
   auto* released = static_cast<bool*>(context);
   DCHECK(!*released);
@@ -206,8 +209,8 @@ TEST_P(ImageTransferCacheEntryTest, HardwareDecodedNoMipsAtCreation) {
   auto entry(std::make_unique<ServiceImageTransferCacheEntry>());
   EXPECT_TRUE(entry->BuildFromHardwareDecodedImage(
       gr_context(), std::move(plane_images),
-      GetParam() /* plane_images_format */, 0u /* buffer_byte_size */,
-      false /* needs_mips */));
+      GetParam() /* plane_images_format */, kJpegYUVColorSpace,
+      0u /* buffer_byte_size */, false /* needs_mips */));
 
   // We didn't request generating mipmap chains, so the textures we created
   // above should stay alive until after the cache entry is deleted.
@@ -238,8 +241,8 @@ TEST_P(ImageTransferCacheEntryTest, HardwareDecodedMipsAtCreation) {
   auto entry(std::make_unique<ServiceImageTransferCacheEntry>());
   EXPECT_TRUE(entry->BuildFromHardwareDecodedImage(
       gr_context(), std::move(plane_images),
-      GetParam() /* plane_images_format */, 0u /* buffer_byte_size */,
-      true /* needs_mips */));
+      GetParam() /* plane_images_format */, kJpegYUVColorSpace,
+      0u /* buffer_byte_size */, true /* needs_mips */));
 
   // We requested generating mipmap chains at creation time, so the textures we
   // created above should be released by now.
@@ -275,8 +278,8 @@ TEST_P(ImageTransferCacheEntryTest, HardwareDecodedMipsAfterCreation) {
   auto entry(std::make_unique<ServiceImageTransferCacheEntry>());
   EXPECT_TRUE(entry->BuildFromHardwareDecodedImage(
       gr_context(), std::move(plane_images),
-      GetParam() /* plane_images_format */, 0u /* buffer_byte_size */,
-      false /* needs_mips */));
+      GetParam() /* plane_images_format */, kJpegYUVColorSpace,
+      0u /* buffer_byte_size */, false /* needs_mips */));
 
   // We didn't request generating mip chains, so the textures we created above
   // should stay alive for now.

diff --git a/gpu/command_buffer/service/service_transfer_cache.cc b/gpu/command_buffer/service/service_transfer_cache.cc
@@ -267,6 +267,7 @@ bool ServiceTransferCache::CreateLockedHardwareDecodedImageEntry(
     GrContext* context,
     std::vector<sk_sp<SkImage>> plane_images,
     cc::YUVDecodeFormat plane_images_format,
+    SkYUVColorSpace yuv_color_space,
     size_t buffer_byte_size,
     bool needs_mips) {
   EntryKey key(decoder_id, cc::TransferCacheEntryType::kImage, entry_id);
@@ -276,9 +277,9 @@ bool ServiceTransferCache::CreateLockedHardwareDecodedImageEntry(
 
   // Create the service-side image transfer cache entry.
   auto entry = std::make_unique<cc::ServiceImageTransferCacheEntry>();
-  if (!entry->BuildFromHardwareDecodedImage(context, std::move(plane_images),
-                                            plane_images_format,
-                                            buffer_byte_size, needs_mips)) {
+  if (!entry->BuildFromHardwareDecodedImage(
+          context, std::move(plane_images), plane_images_format,
+          yuv_color_space, buffer_byte_size, needs_mips)) {
     return false;
   }
 

diff --git a/gpu/command_buffer/service/service_transfer_cache.h b/gpu/command_buffer/service/service_transfer_cache.h
@@ -19,6 +19,7 @@
 #include "gpu/command_buffer/common/discardable_handle.h"
 #include "gpu/command_buffer/service/context_group.h"
 #include "gpu/gpu_gles2_export.h"
+#include "third_party/skia/include/core/SkImageInfo.h"
 #include "third_party/skia/include/core/SkRefCnt.h"
 
 class GrContext;
@@ -74,6 +75,7 @@ class GPU_GLES2_EXPORT ServiceTransferCache
       GrContext* context,
       std::vector<sk_sp<SkImage>> plane_images,
       cc::YUVDecodeFormat plane_images_format,
+      SkYUVColorSpace yuv_color_space,
       size_t buffer_byte_size,
       bool needs_mips);
 

diff --git a/gpu/ipc/service/image_decode_accelerator_stub.cc b/gpu/ipc/service/image_decode_accelerator_stub.cc
@@ -438,6 +438,7 @@ void ImageDecodeAcceleratorStub::ProcessCompletedDecode(
                  completed_decode->buffer_format == gfx::BufferFormat::YVU_420
                      ? cc::YUVDecodeFormat::kYVU3
                      : cc::YUVDecodeFormat::kYUV2,
+                 completed_decode->yuv_color_space,
                  completed_decode->buffer_byte_size, params.needs_mips)) {
       DLOG(ERROR) << "Could not create and insert the transfer cache entry";
       OnError();

diff --git a/gpu/ipc/service/image_decode_accelerator_worker.h b/gpu/ipc/service/image_decode_accelerator_worker.h
@@ -14,6 +14,7 @@
 #include "base/callback.h"
 #include "base/containers/span.h"
 #include "gpu/config/gpu_info.h"
+#include "third_party/skia/include/core/SkImageInfo.h"
 #include "ui/gfx/buffer_types.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/gpu_memory_buffer.h"
@@ -32,6 +33,7 @@ class ImageDecodeAcceleratorWorker {
     gfx::Size visible_size;
     gfx::BufferFormat buffer_format;
     size_t buffer_byte_size;
+    SkYUVColorSpace yuv_color_space;
   };
 
   using CompletedDecodeCB =

diff --git a/media/gpu/vaapi/BUILD.gn b/media/gpu/vaapi/BUILD.gn
@@ -97,6 +97,10 @@ source_set("vaapi") {
     "//ui/gfx/geometry",
   ]
 
+  public_deps = [
+    "//skia",
+  ]
+
   if (is_chromeos) {
     sources += [
       "vaapi_jpeg_encode_accelerator.cc",

diff --git a/media/gpu/vaapi/vaapi_image_decode_accelerator_worker.cc b/media/gpu/vaapi/vaapi_image_decode_accelerator_worker.cc
@@ -111,6 +111,7 @@ void DecodeTask(
   result->visible_size = exported_pixmap->pixmap->GetBufferSize();
   result->buffer_format = exported_pixmap->pixmap->GetBufferFormat();
   result->buffer_byte_size = exported_pixmap->byte_size;
+  result->yuv_color_space = decoder->GetYUVColorSpace();
   std::move(scoped_decode_callback).Run(std::move(result));
 }
 

diff --git a/media/gpu/vaapi/vaapi_image_decode_accelerator_worker_unittest.cc b/media/gpu/vaapi/vaapi_image_decode_accelerator_worker_unittest.cc
@@ -15,6 +15,7 @@
 
 #include "base/bind.h"
 #include "base/containers/span.h"
+#include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/scoped_refptr.h"
@@ -24,6 +25,7 @@
 #include "media/gpu/vaapi/vaapi_image_decode_accelerator_worker.h"
 #include "media/gpu/vaapi/vaapi_image_decoder.h"
 #include "media/gpu/vaapi/vaapi_wrapper.h"
+#include "third_party/skia/include/core/SkImageInfo.h"
 #include "ui/gfx/buffer_types.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/linux/native_pixmap_dmabuf.h"
@@ -92,6 +94,17 @@ class MockVaapiImageDecoder : public VaapiImageDecoder {
   ~MockVaapiImageDecoder() override = default;
 
   gpu::ImageDecodeAcceleratorType GetType() const override { return type_; }
+  SkYUVColorSpace GetYUVColorSpace() const override {
+    switch (type_) {
+      case gpu::ImageDecodeAcceleratorType::kJpeg:
+        return SkYUVColorSpace::kJPEG_SkYUVColorSpace;
+      case gpu::ImageDecodeAcceleratorType::kWebP:
+        return SkYUVColorSpace::kRec601_SkYUVColorSpace;
+      case gpu::ImageDecodeAcceleratorType::kUnknown:
+        NOTREACHED();
+        return SkYUVColorSpace::kIdentity_SkYUVColorSpace;
+    }
+  }
 
   gpu::ImageDecodeAcceleratorSupportedProfile GetSupportedProfile()
       const override {

diff --git a/media/gpu/vaapi/vaapi_image_decoder.h b/media/gpu/vaapi/vaapi_image_decoder.h
@@ -10,12 +10,12 @@
 
 #include <memory>
 
-
 #include "base/callback_forward.h"
 #include "base/containers/span.h"
 #include "base/macros.h"
 #include "base/memory/scoped_refptr.h"
 #include "gpu/config/gpu_info.h"
+#include "third_party/skia/include/core/SkImageInfo.h"
 
 namespace gfx {
 class NativePixmapDmaBuf;
@@ -78,6 +78,10 @@ class VaapiImageDecoder {
   // Returns the type of image supported by this decoder.
   virtual gpu::ImageDecodeAcceleratorType GetType() const = 0;
 
+  // Returns the type of mapping needed to convert the NativePixmapDmaBuf
+  // returned by ExportAsNativePixmapDmaBuf() from YUV to RGB.
+  virtual SkYUVColorSpace GetYUVColorSpace() const = 0;
+
   // Returns the image profile supported by this decoder.
   virtual gpu::ImageDecodeAcceleratorSupportedProfile GetSupportedProfile()
       const;

diff --git a/media/gpu/vaapi/vaapi_jpeg_decoder.cc b/media/gpu/vaapi/vaapi_jpeg_decoder.cc
@@ -318,6 +318,10 @@ gpu::ImageDecodeAcceleratorType VaapiJpegDecoder::GetType() const {
   return gpu::ImageDecodeAcceleratorType::kJpeg;
 }
 
+SkYUVColorSpace VaapiJpegDecoder::GetYUVColorSpace() const {
+  return SkYUVColorSpace::kJPEG_SkYUVColorSpace;
+}
+
 std::unique_ptr<ScopedVAImage> VaapiJpegDecoder::GetImage(
     uint32_t preferred_image_fourcc,
     VaapiImageDecodeStatus* status) {

diff --git a/media/gpu/vaapi/vaapi_jpeg_decoder.h b/media/gpu/vaapi/vaapi_jpeg_decoder.h
@@ -29,6 +29,7 @@ class VaapiJpegDecoder : public VaapiImageDecoder {
 
   // VaapiImageDecoder implementation.
   gpu::ImageDecodeAcceleratorType GetType() const override;
+  SkYUVColorSpace GetYUVColorSpace() const override;
 
   // Get the decoded data from the last Decode() call as a ScopedVAImage. The
   // VAImage's format will be either |preferred_image_fourcc| if the conversion

diff --git a/media/gpu/vaapi/vaapi_webp_decoder.cc b/media/gpu/vaapi/vaapi_webp_decoder.cc
@@ -84,6 +84,10 @@ gpu::ImageDecodeAcceleratorType VaapiWebPDecoder::GetType() const {
   return gpu::ImageDecodeAcceleratorType::kWebP;
 }
 
+SkYUVColorSpace VaapiWebPDecoder::GetYUVColorSpace() const {
+  return SkYUVColorSpace::kRec601_SkYUVColorSpace;
+}
+
 VaapiImageDecodeStatus VaapiWebPDecoder::AllocateVASurfaceAndSubmitVABuffers(
     base::span<const uint8_t> encoded_image) {
   DCHECK(vaapi_wrapper_);

diff --git a/media/gpu/vaapi/vaapi_webp_decoder.h b/media/gpu/vaapi/vaapi_webp_decoder.h
@@ -19,6 +19,7 @@ class VaapiWebPDecoder : public VaapiImageDecoder {
 
   // VaapiImageDecoder implementation.
   gpu::ImageDecodeAcceleratorType GetType() const override;
+  SkYUVColorSpace GetYUVColorSpace() const override;
 
  private:
   // VaapiImageDecoder implementation.