Revert "gpu: Avoid texture uploads for hardware-accelerated image dec…

…odes." This reverts commit 9dc2f05. Reason for revert: Causing linux 32-bit debug to fail all cc_unittests and viz_unittests: https://ci.chromium.org/p/chromium/builders/ci/Linux%20Tests%20%28dbg%29%281%29%2832%29/58651 Original change's description: > gpu: Avoid texture uploads for hardware-accelerated image decodes. > > This CL adds support for avoiding texture uploads for hardware > accelerated image decodes. Specifically: > > - The gpu::ImageDecodeAcceleratorWorker interface is modified so that > implementations can return the decoded image in the form of a > GpuMemoryBufferHandle. In Chrome OS, this would refer to a dmabuf. For > now, we only support YUV 4:2:0 results. > > - The gpu::ImageDecodeAcceleratorStub is modified so that it decomposes > the GpuMemoryBufferHandle coming from the hardware decoder into separate > gl::GLImages (one per plane). Each image is attached to a texture and a > SkImage is created out of each texture. These 3 SkImages are used to > create the service-side transfer cache entry which builds a single YUV > SkImage. This allows us to specify the matrix used for converting from > YUV to RGB (assumed to be the JPEG conversion matrix for now). In an > earlier prototype, I had tried to create a single GLImage and attach it > to a single texture, but the YUV-to-RGB conversion was giving wrong > results, so I decided to go the Skia route. > > - The cc::ServiceImageTransferCacheEntry is modified so that it doesn't > do texture uploads for hardware decodes. Instead, it just takes the > 3 SkImages built in the previous step plus enough data to build the > final YUV SkImage. > > Bug: 868400 > Test: the ImageDecodeAcceleratorStub unittests pass. > Change-Id: I6f435707c6b2d4368613b55399b4d6d185a84c70 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1590530 > Commit-Queue: Andres Calderon Jaramillo <andrescj@chromium.org> > Reviewed-by: Antoine Labour <piman@chromium.org> > Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org> > Reviewed-by: Khushal <khushalsagar@chromium.org> > Cr-Commit-Position: refs/heads/master@{#660130} TBR=sunnyps@chromium.org,khushalsagar@chromium.org,piman@chromium.org,andrescj@chromium.org Change-Id: I60b7a80d6f0ba462afd3771f9b6d6abcd686edb2 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 868400 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1614777 Reviewed-by: Brandon Tolsch <btolsch@chromium.org> Commit-Queue: Brandon Tolsch <btolsch@chromium.org> Cr-Commit-Position: refs/heads/master@{#660244}
Denger-Network · May 16, 2019 · 0d46cf6 · 0d46cf6
1 parent 36498b2
commit 0d46cf6
Show file tree

Hide file tree

Showing 18 changed files with 140 additions and 448 deletions.
diff --git a/cc/paint/image_transfer_cache_entry.cc b/cc/paint/image_transfer_cache_entry.cc
@@ -5,6 +5,7 @@
 #include "cc/paint/image_transfer_cache_entry.h"
 
 #include <utility>
+#include <vector>
 
 #include "base/bind_helpers.h"
 #include "base/logging.h"
@@ -15,8 +16,6 @@
 #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"
 #include "third_party/skia/include/gpu/GrContext.h"
 #include "third_party/skia/include/gpu/GrTypes.h"
 
@@ -144,67 +143,29 @@ ServiceImageTransferCacheEntry::ServiceImageTransferCacheEntry(
 ServiceImageTransferCacheEntry& ServiceImageTransferCacheEntry::operator=(
     ServiceImageTransferCacheEntry&& other) = default;
 
-bool ServiceImageTransferCacheEntry::BuildFromHardwareDecodedImage(
+bool ServiceImageTransferCacheEntry::BuildFromDecodedData(
     GrContext* context,
-    std::vector<sk_sp<SkImage>> plane_images,
-    size_t buffer_byte_size,
+    base::span<const uint8_t> decoded_image,
+    size_t row_bytes,
+    const SkImageInfo& image_info,
     bool needs_mips,
     sk_sp<SkColorSpace> target_color_space) {
   context_ = context;
-
-  // 1) Extract the planar textures from |plane_images|.
-  std::vector<GrBackendTexture> plane_backend_textures(3u);
-  DCHECK_EQ(3u, plane_images.size());
-  for (size_t plane = 0; plane < 3u; plane++) {
-    plane_backend_textures[plane] = plane_images[plane]->getBackendTexture(
-        true /* flushPendingGrContextIO */);
-    if (!plane_backend_textures[plane].isValid()) {
-      DLOG(ERROR) << "Invalid backend texture found";
-      return false;
-    }
-    if (needs_mips) {
-      // TODO(andrescj): generate mipmaps when requested. This will require some
-      // resource management: we either let Skia own the new textures or we take
-      // ownership and delete them in |destroy_callback|.
-      NOTIMPLEMENTED();
-    }
-  }
-  plane_images_ = std::move(plane_images);
-
-  // 2) Create a SkImage backed by the YUV textures extracted above. There are
-  //    two assumptions here:
-  //
-  //    - SkYUVColorSpace::kJPEG_SkYUVColorSpace is used for the YUV-to-RGB
-  //      matrix.
-  //    - The color space of the resulting image is sRGB.
-  //
-  // TODO(andrescj): support other YUV-to-RGB conversions and embedded color
-  // profiles.
-  SkYUVAIndex plane_indices[] = {
-      SkYUVAIndex{0, SkColorChannel::kR}, SkYUVAIndex{1, SkColorChannel::kR},
-      SkYUVAIndex{2, SkColorChannel::kR}, SkYUVAIndex{-1, SkColorChannel::kR}};
-  image_ = SkImage::MakeFromYUVATextures(
-      context_, SkYUVColorSpace::kJPEG_SkYUVColorSpace,
-      plane_backend_textures.data(), plane_indices,
-      plane_images_[0]->dimensions(), kTopLeft_GrSurfaceOrigin);
-  if (!image_) {
-    DLOG(ERROR) << "Could not create YUV SkImage";
+  has_mips_ = needs_mips;
+  size_ = image_info.computeByteSize(row_bytes);
+  if (size_ == SIZE_MAX)
     return false;
-  }
+  DCHECK_LE(size_, decoded_image.size());
 
-  // 3) Perform color space conversion if necessary.
-  if (target_color_space)
-    image_ = image_->makeColorSpace(target_color_space);
-  if (!image_) {
-    DLOG(ERROR) << "Could not do color space conversion";
+  uint32_t width;
+  uint32_t height;
+  if (!base::CheckedNumeric<int>(image_info.width()).AssignIfValid(&width) ||
+      !base::CheckedNumeric<int>(image_info.height()).AssignIfValid(&height)) {
     return false;
   }
 
-  // 4) Fill out the rest of the information.
-  has_mips_ = false;
-  size_ = buffer_byte_size;
-  fits_on_gpu_ = true;
-  return true;
+  return MakeSkImage(SkPixmap(image_info, decoded_image.data(), row_bytes),
+                     width, height, target_color_space);
 }
 
 size_t ServiceImageTransferCacheEntry::CachedSize() const {
@@ -315,14 +276,6 @@ void ServiceImageTransferCacheEntry::EnsureMips() {
   if (has_mips_)
     return;
 
-  if (!plane_images_.empty()) {
-    // TODO(andrescj): generate mipmaps for hardware-accelerated decodes when
-    // requested. This will require some resource management: we either let Skia
-    // own the new textures or we take ownership and delete them in
-    // |destroy_callback_|.
-    NOTIMPLEMENTED();
-  }
-
   has_mips_ = true;
   // TODO(ericrk): consider adding in the DeleteSkImageAndPreventCaching
   // optimization from GpuImageDecodeCache where we forcefully remove the

diff --git a/cc/paint/image_transfer_cache_entry.h b/cc/paint/image_transfer_cache_entry.h
@@ -8,8 +8,6 @@
 #include <stddef.h>
 #include <stdint.h>
 
-#include <vector>
-
 #include "base/atomic_sequence_num.h"
 #include "base/containers/span.h"
 #include "cc/paint/transfer_cache_entry.h"
@@ -18,6 +16,7 @@
 class GrContext;
 class SkColorSpace;
 class SkImage;
+struct SkImageInfo;
 class SkPixmap;
 
 namespace cc {
@@ -61,35 +60,21 @@ class CC_PAINT_EXPORT ServiceImageTransferCacheEntry
   ServiceImageTransferCacheEntry& operator=(
       ServiceImageTransferCacheEntry&& other);
 
-  // Populates this entry using the result of a hardware decode. The assumption
-  // is that |plane_images| are backed by textures that are in turn backed by a
-  // buffer (dmabuf in Chrome OS) containing the planes of the decoded image.
-  // |buffer_byte_size| is the size of the buffer. We assume the following:
-  //
-  // - |plane_images| represents a YUV 4:2:0 triplanar image.
-  // - 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 colorspace of the resulting RGB image is sRGB. We will convert from
-  //   this to |target_color_space| (if non-null).
-  //
-  // Returns true if the entry can be built, false otherwise.
-  //
-  // TODO(andrescj): actually generate the mipmaps when |needs_mips| is true.
-  bool BuildFromHardwareDecodedImage(GrContext* context,
-                                     std::vector<sk_sp<SkImage>> plane_images,
-                                     size_t buffer_byte_size,
-                                     bool needs_mips,
-                                     sk_sp<SkColorSpace> target_color_space);
+  // Populates this entry using |decoded_image| described by |row_bytes| and
+  // |image_info|. The image is uploaded to the GPU if its dimensions are both
+  // at most |context_|->maxTextureSize().
+  bool BuildFromDecodedData(GrContext* context,
+                            base::span<const uint8_t> decoded_image,
+                            size_t row_bytes,
+                            const SkImageInfo& image_info,
+                            bool needs_mips,
+                            sk_sp<SkColorSpace> target_color_space);
 
   // ServiceTransferCacheEntry implementation:
   size_t CachedSize() const final;
   bool Deserialize(GrContext* context, base::span<const uint8_t> data) final;
 
   bool fits_on_gpu() const { return fits_on_gpu_; }
-  const std::vector<sk_sp<SkImage>>& plane_images() const {
-    return plane_images_;
-  }
   const sk_sp<SkImage>& image() const { return image_; }
 
   // Ensures the cached image has mips.
@@ -102,7 +87,6 @@ class CC_PAINT_EXPORT ServiceImageTransferCacheEntry
                    sk_sp<SkColorSpace> target_color_space);
 
   GrContext* context_ = nullptr;
-  std::vector<sk_sp<SkImage>> plane_images_;
   sk_sp<SkImage> image_;
   bool has_mips_ = false;
   size_t size_ = 0;

diff --git a/gpu/BUILD.gn b/gpu/BUILD.gn
@@ -468,14 +468,9 @@ test("gpu_unittests") {
     "ipc/service/gpu_channel_test_common.cc",
     "ipc/service/gpu_channel_test_common.h",
     "ipc/service/gpu_channel_unittest.cc",
+    "ipc/service/image_decode_accelerator_stub_unittest.cc",
   ]
 
-  if (is_chromeos) {
-    # Image decode acceleration with hardware is only supported in Chrome OS.
-    # The intention is to run this test in the linux-chromeos build.
-    sources += [ "ipc/service/image_decode_accelerator_stub_unittest.cc" ]
-  }
-
   if (use_dawn) {
     sources += [ "command_buffer/service/webgpu_decoder_unittest.cc" ]
   }

diff --git a/gpu/command_buffer/service/raster_decoder.cc b/gpu/command_buffer/service/raster_decoder.cc
@@ -689,7 +689,10 @@ RasterDecoderImpl::RasterDecoderImpl(
   DCHECK(shared_context_state_);
 }
 
-RasterDecoderImpl::~RasterDecoderImpl() = default;
+RasterDecoderImpl::~RasterDecoderImpl() {
+  if (supports_oop_raster_)
+    transfer_cache()->DeleteAllEntriesForDecoder(raster_decoder_id_);
+}
 
 base::WeakPtr<DecoderContext> RasterDecoderImpl::AsWeakPtr() {
   return weak_ptr_factory_.GetWeakPtr();
@@ -762,10 +765,6 @@ void RasterDecoderImpl::Destroy(bool have_context) {
   DCHECK(!have_context || shared_context_state_->context()->IsCurrent(nullptr));
 
   if (have_context) {
-    if (supports_oop_raster_) {
-      transfer_cache()->DeleteAllEntriesForDecoder(raster_decoder_id_);
-    }
-
     if (copy_tex_image_blit_.get()) {
       copy_tex_image_blit_->Destroy();
       copy_tex_image_blit_.reset();

diff --git a/gpu/command_buffer/service/raster_decoder_unittest.cc b/gpu/command_buffer/service/raster_decoder_unittest.cc
@@ -289,7 +289,6 @@ class RasterDecoderOOPTest : public testing::Test, DecoderClient {
     context_state_->InitializeGL(GpuPreferences(), feature_info);
   }
   void TearDown() override {
-    context_state_->MakeCurrent(nullptr);
     context_state_ = nullptr;
     gl::init::ShutdownGL(false);
   }

diff --git a/gpu/command_buffer/service/raster_decoder_unittest_base.cc b/gpu/command_buffer/service/raster_decoder_unittest_base.cc
@@ -278,8 +278,6 @@ void RasterDecoderTestBase::ResetDecoder() {
   for (auto& image : shared_images_)
     image->OnContextLost();
   shared_images_.clear();
-  context_->GLContextStub::MakeCurrent(surface_.get());
-  shared_context_state_.reset();
   ::gl::MockGLInterface::SetGLInterface(nullptr);
   gl_.reset();
   gl::init::ShutdownGL(false);

diff --git a/gpu/command_buffer/service/service_transfer_cache.cc b/gpu/command_buffer/service/service_transfer_cache.cc
@@ -40,10 +40,6 @@ void DumpMemoryForImageTransferCacheEntry(
 
   // Alias the image entry to its skia counterpart, taking ownership of the
   // memory and preventing double counting.
-  //
-  // TODO(andrescj): if entry->image() is backed by multiple textures,
-  // getBackendTexture() would end up flattening them which is undesirable:
-  // figure out how to report memory usage for those cases.
   DCHECK(entry->image());
   GrBackendTexture image_backend_texture =
       entry->image()->getBackendTexture(false /* flushPendingGrContextIO */);
@@ -211,25 +207,27 @@ void ServiceTransferCache::DeleteAllEntriesForDecoder(int decoder_id) {
   }
 }
 
-bool ServiceTransferCache::CreateLockedHardwareDecodedImageEntry(
+bool ServiceTransferCache::CreateLockedImageEntry(
     int decoder_id,
     uint32_t entry_id,
     ServiceDiscardableHandle handle,
     GrContext* context,
-    std::vector<sk_sp<SkImage>> plane_images,
-    size_t buffer_byte_size,
+    base::span<const uint8_t> decoded_image,
+    size_t row_bytes,
+    const SkImageInfo& image_info,
     bool needs_mips,
     sk_sp<SkColorSpace> target_color_space) {
   EntryKey key(decoder_id, cc::TransferCacheEntryType::kImage, entry_id);
   auto found = entries_.Peek(key);
   if (found != entries_.end())
     return false;
 
-  // Create the service-side image transfer cache entry.
+  // Create the service-side image transfer cache entry. Note that this involves
+  // uploading the image if it fits in GPU memory.
   auto entry = std::make_unique<cc::ServiceImageTransferCacheEntry>();
-  if (!entry->BuildFromHardwareDecodedImage(context, std::move(plane_images),
-                                            buffer_byte_size, needs_mips,
-                                            std::move(target_color_space))) {
+  if (!entry->BuildFromDecodedData(context, decoded_image, row_bytes,
+                                   image_info, needs_mips,
+                                   target_color_space)) {
     return false;
   }
 

diff --git a/gpu/command_buffer/service/service_transfer_cache.h b/gpu/command_buffer/service/service_transfer_cache.h
@@ -9,7 +9,6 @@
 #include <stdint.h>
 
 #include <memory>
-#include <vector>
 
 #include "base/containers/mru_cache.h"
 #include "base/containers/span.h"
@@ -22,7 +21,7 @@
 
 class GrContext;
 class SkColorSpace;
-class SkImage;
+struct SkImageInfo;
 
 namespace gpu {
 
@@ -61,21 +60,22 @@ class GPU_GLES2_EXPORT ServiceTransferCache
   cc::ServiceTransferCacheEntry* GetEntry(const EntryKey& key);
   void DeleteAllEntriesForDecoder(int decoder_id);
 
-  // Creates an image transfer cache entry using |plane_images| (refer to
-  // ServiceImageTransferCacheEntry::BuildFromHardwareDecodedImage() for
-  // details). |decoder_id| and |entry_id| are used for creating the
-  // ServiceTransferCache::EntryKey (assuming cc::TransferCacheEntryType:kImage
-  // for the type). Returns true if the entry could be created and inserted;
-  // false otherwise.
-  bool CreateLockedHardwareDecodedImageEntry(
-      int decoder_id,
-      uint32_t entry_id,
-      ServiceDiscardableHandle handle,
-      GrContext* context,
-      std::vector<sk_sp<SkImage>> plane_images,
-      size_t buffer_byte_size,
-      bool needs_mips,
-      sk_sp<SkColorSpace> target_color_space);
+  // Creates an image transfer cache entry using the decoded data in
+  // |decoded_image|. The |context| will be used to upload the image (if it's
+  // determined to fit in the GPU). |row_bytes| is the stride, and |image_info|
+  // describes the decoded data. |decoder_id| and |entry_id| are used for
+  // creating the ServiceTransferCache::EntryKey (assuming
+  // cc::TransferCacheEntryType:kImage for the type). Returns true if the entry
+  // could be created and inserted; false otherwise.
+  bool CreateLockedImageEntry(int decoder_id,
+                              uint32_t entry_id,
+                              ServiceDiscardableHandle handle,
+                              GrContext* context,
+                              base::span<const uint8_t> decoded_image,
+                              size_t row_bytes,
+                              const SkImageInfo& image_info,
+                              bool needs_mips,
+                              sk_sp<SkColorSpace> target_color_space);
 
   void PurgeMemory(
       base::MemoryPressureListener::MemoryPressureLevel memory_pressure_level);

diff --git a/gpu/command_buffer/service/shared_context_state.cc b/gpu/command_buffer/service/shared_context_state.cc
@@ -75,24 +75,8 @@ SharedContextState::SharedContextState(
 }
 
 SharedContextState::~SharedContextState() {
-  // Delete the transfer cache first: that way, destruction callbacks for image
-  // entries can use *|this| to make the context current and do GPU clean up.
-  // The context should be current so that texture deletes that result from
-  // destroying the cache happen in the right context (unless the context is
-  // lost in which case we don't delete the textures).
-  DCHECK(IsCurrent(nullptr) || context_lost_);
-  transfer_cache_.reset();
-
-  // We should have the last ref on this GrContext to ensure we're not holding
-  // onto any skia objects using this context. Note that some tests don't run
-  // InitializeGrContext(), so |owned_gr_context_| is not expected to be
-  // initialized.
-  DCHECK(!owned_gr_context_ || owned_gr_context_->unique());
-
-  // Delete the GrContext. This will either do cleanup if the context is
-  // current, or the GrContext was already abandoned if the GLContext was lost.
-  owned_gr_context_.reset();
-
+  if (gr_context_)
+    gr_context_->abandonContext();
   if (context_->IsCurrent(nullptr))
     context_->ReleaseCurrent(nullptr);
   base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider(
@@ -236,7 +220,6 @@ bool SharedContextState::MakeCurrent(gl::GLSurface* surface) {
 }
 
 void SharedContextState::MarkContextLost() {
-  DCHECK(GrContextIsGL());
   if (!context_lost_) {
     scoped_refptr<SharedContextState> prevent_last_ref_drop = this;
     context_lost_ = true;
@@ -254,8 +237,6 @@ void SharedContextState::MarkContextLost() {
 bool SharedContextState::IsCurrent(gl::GLSurface* surface) {
   if (!GrContextIsGL())
     return true;
-  if (context_lost_)
-    return false;
   return context_->IsCurrent(surface);
 }
 

diff --git a/gpu/ipc/in_process_command_buffer.cc b/gpu/ipc/in_process_command_buffer.cc
@@ -782,8 +782,6 @@ bool InProcessCommandBuffer::DestroyOnGpuThread() {
   gl_share_group_ = nullptr;
   context_group_ = nullptr;
   task_sequence_ = nullptr;
-  if (context_state_)
-    context_state_->MakeCurrent(nullptr);
   context_state_ = nullptr;
   return true;
 }