gpu: Fix OOP-R memory reporting for hw decodes.

This CL fixes the service-side transfer cache to report memory correctly
for accelerated image decodes. Prior to this CL, the service transfer
cache reported multiple planes for hardware decodes (because they were
treated as regular YUV decodes). The problem is that the driver does not
report per-plane sizes, so we report the whole buffer size for the Y
plane and 0 for the other planes. This CL changes that so that hardware
image decodes report a single 'dma_buf' memory dump with the whole
buffer size. This would be of the form:

  gpu/transfer_cache/cache_0x123/gpu/entry_0x567/dma_buf

However, this only happens for unmipped decodes. When mips are
generated, Skia owns the mipped textures, and since we flush pending
Skia work before generating the YUV SkImage, we assume that the original
unmipped textures are deleted. Therefore, we treat this situation as a
regular YUV decode and we don't report the dma_buf dump.

Bug: 995156
Test: added a lot of unit test coverage.
Change-Id: Ie4990ada28fcbc573e9d85fb0a51a7095ed39cce
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1933199Reviewed-by: Stephen White <senorblanco@chromium.org>
Reviewed-by: Khushal <khushalsagar@chromium.org>
Reviewed-by: Eric Karl <ericrk@chromium.org>
Commit-Queue: Andres Calderon Jaramillo <andrescj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#736154}

cc/paint/image_transfer_cache_entry.cc

@@ -332,9 +332,12 @@ bool ServiceImageTransferCacheEntry::BuildFromHardwareDecodedImage(
     size_t buffer_byte_size,
     bool needs_mips) {
   context_ = context;
+  size_ = buffer_byte_size;
 
   // 1) Generate mipmap chains if requested.
   if (needs_mips) {
+    DCHECK(plane_sizes_.empty());
+    base::CheckedNumeric<size_t> safe_total_size(0u);
     for (size_t plane = 0; plane < plane_images.size(); plane++) {
       plane_images[plane] =
           plane_images[plane]->makeTextureImage(context_, GrMipMapped::kYes);
@@ -342,6 +345,13 @@ bool ServiceImageTransferCacheEntry::BuildFromHardwareDecodedImage(
         DLOG(ERROR) << "Could not generate mipmap chain for plane " << plane;
         return false;
       }
+      plane_sizes_.push_back(
+          GrContext::ComputeImageSize(plane_images[plane], GrMipMapped::kYes));
+      safe_total_size += plane_sizes_.back();
+    }
+    if (!safe_total_size.AssignIfValid(&size_)) {
+      DLOG(ERROR) << "Could not calculate the total image size";
+      return false;
     }
   }
   plane_images_ = std::move(plane_images);
@@ -359,7 +369,6 @@ bool ServiceImageTransferCacheEntry::BuildFromHardwareDecodedImage(
 
   // 3) Fill out the rest of the information.
   has_mips_ = needs_mips;
-  size_ = buffer_byte_size;
   fits_on_gpu_ = true;
   return true;
 }
@@ -590,7 +599,12 @@ void ServiceImageTransferCacheEntry::EnsureMips() {
     DCHECK_NE(YUVDecodeFormat::kUnknown, plane_images_format_);
     DCHECK_EQ(NumberOfPlanesForYUVDecodeFormat(plane_images_format_),
               plane_images_.size());
+
+    // We first do all the work with local variables. Then, if everything
+    // succeeds, we update the object's state. That way, we don't leave it in an
+    // inconsistent state if one step of mip generation fails.
     std::vector<sk_sp<SkImage>> mipped_planes;
+    std::vector<size_t> mipped_plane_sizes;
     for (size_t plane = 0; plane < plane_images_.size(); plane++) {
       DCHECK(plane_images_.at(plane));
       sk_sp<SkImage> mipped_plane = plane_images_.at(plane)->makeTextureImage(
@@ -598,25 +612,33 @@ void ServiceImageTransferCacheEntry::EnsureMips() {
       if (!mipped_plane)
         return;
       mipped_planes.push_back(std::move(mipped_plane));
+      mipped_plane_sizes.push_back(
+          GrContext::ComputeImageSize(mipped_planes.back(), GrMipMapped::kYes));
     }
-    // Keeping a separate vector for the planes as mips are added means that we
-    // are consistent: either all planes have mips or none do.
-    for (size_t plane = 0; plane < mipped_planes.size(); plane++) {
-      plane_images_.at(plane) = std::move(mipped_planes.at(plane));
-    }
-    mipped_planes.clear();
-    image_ = MakeYUVImageFromUploadedPlanes(
-        context_, plane_images_, plane_images_format_, yuv_color_space_.value(),
+    sk_sp<SkImage> mipped_image = MakeYUVImageFromUploadedPlanes(
+        context_, mipped_planes, plane_images_format_, yuv_color_space_.value(),
         image_->refColorSpace() /* image_color_space */);
+    if (!mipped_image)
+      return;
+    // Note that we cannot update |size_| because the transfer cache keeps track
+    // of a total size that is not updated after EnsureMips(). The original size
+    // is used when the image is deleted from the cache.
+    plane_images_ = std::move(mipped_planes);
+    plane_sizes_ = std::move(mipped_plane_sizes);
+    image_ = std::move(mipped_image);
     has_mips_ = true;
     return;
   }
 
-  has_mips_ = true;
   // TODO(ericrk): consider adding in the DeleteSkImageAndPreventCaching
   // optimization from GpuImageDecodeCache where we forcefully remove the
   // intermediate from Skia's cache.
-  image_ = image_->makeTextureImage(context_, GrMipMapped::kYes);
+  sk_sp<SkImage> mipped_image =
+      image_->makeTextureImage(context_, GrMipMapped::kYes);
+  if (!mipped_image)
+    return;
+  image_ = std::move(mipped_image);
+  has_mips_ = true;
 }
 
 }  // namespace cc

gpu/command_buffer/service/service_transfer_cache.cc

@@ -75,20 +75,26 @@ void DumpMemoryForYUVImageTransferCacheEntry(
   DCHECK(entry->is_yuv());
 
   std::vector<size_t> plane_sizes = entry->GetPlaneCachedSizes();
+  if (plane_sizes.empty()) {
+    // This entry corresponds to an unmipped hardware decoded image.
+    MemoryAllocatorDump* dump = pmd->CreateAllocatorDump(
+        dump_base_name + base::StringPrintf("/dma_buf"));
+    dump->AddScalar(MemoryAllocatorDump::kNameSize,
+                    MemoryAllocatorDump::kUnitsBytes, entry->CachedSize());
+    // We don't need to establish shared ownership of the dump with Skia: the
+    // reason is that Skia doesn't own the textures for hardware decoded images,
+    // so it won't count them in its memory dump (because
+    // SkiaGpuTraceMemoryDump::shouldDumpWrappedObjects() returns false).
+    return;
+  }
+
   for (size_t i = 0u; i < entry->num_planes(); ++i) {
     MemoryAllocatorDump* dump = pmd->CreateAllocatorDump(
         dump_base_name +
         base::StringPrintf("/plane_%0u", base::checked_cast<uint32_t>(i)));
-    if (plane_sizes.empty()) {
-      // Hardware-decoded image case.
-      dump->AddScalar(MemoryAllocatorDump::kNameSize,
-                      MemoryAllocatorDump::kUnitsBytes,
-                      (i == SkYUVAIndex::kY_Index) ? entry->CachedSize() : 0u);
-    } else {
-      DCHECK_EQ(plane_sizes.size(), entry->num_planes());
-      dump->AddScalar(MemoryAllocatorDump::kNameSize,
-                      MemoryAllocatorDump::kUnitsBytes, plane_sizes.at(i));
-    }
+    DCHECK_EQ(plane_sizes.size(), entry->num_planes());
+    dump->AddScalar(MemoryAllocatorDump::kNameSize,
+                    MemoryAllocatorDump::kUnitsBytes, plane_sizes.at(i));
 
     // If entry->image() is backed by multiple textures,
     // getBackendTexture() would end up flattening them to RGB, which is

gpu/ipc/service/DEPS

@@ -15,3 +15,9 @@ include_rules = [
   "+ui/platform_window",
   "+media/gpu/android/texture_owner.h",
 ]
+
+specific_include_rules = {
+  "image_decode_accelerator_stub_unittest\.cc": [
+    "+skia/ext/skia_memory_dump_provider.h",
+  ],
+}

gpu/ipc/service/gpu_channel_test_common.cc

@@ -7,6 +7,7 @@
 #include "base/memory/unsafe_shared_memory_region.h"
 #include "base/test/test_simple_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/trace_event/memory_dump_manager.h"
 #include "gpu/command_buffer/common/activity_flags.h"
 #include "gpu/command_buffer/service/scheduler.h"
 #include "gpu/command_buffer/service/shared_image_manager.h"
@@ -62,7 +63,9 @@ GpuChannelTestCommon::GpuChannelTestCommon(bool use_stub_bindings)
 GpuChannelTestCommon::GpuChannelTestCommon(
     std::vector<int32_t> enabled_workarounds,
     bool use_stub_bindings)
-    : task_runner_(new base::TestSimpleTaskRunner),
+    : memory_dump_manager_(
+          base::trace_event::MemoryDumpManager::CreateInstanceForTesting()),
+      task_runner_(new base::TestSimpleTaskRunner),
       io_task_runner_(new base::TestSimpleTaskRunner),
       sync_point_manager_(new SyncPointManager()),
       shared_image_manager_(new SharedImageManager(false /* thread_safe */)),

gpu/ipc/service/gpu_channel_test_common.h

@@ -15,6 +15,9 @@
 
 namespace base {
 class TestSimpleTaskRunner;
+namespace trace_event {
+class MemoryDumpManager;
+}  // namespace trace_event
 }  // namespace base
 
 namespace IPC {
@@ -52,6 +55,7 @@ class GpuChannelTestCommon : public testing::Test {
   base::UnsafeSharedMemoryRegion GetSharedMemoryRegion();
 
  private:
+  std::unique_ptr<base::trace_event::MemoryDumpManager> memory_dump_manager_;
   IPC::TestSink sink_;
   scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
   scoped_refptr<base::TestSimpleTaskRunner> io_task_runner_;