Refactor SoftwareImageDecodeController

cc/tiles/software_image_decode_controller.cc

 // Copyright 2015 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.
 
 #include "cc/tiles/software_image_decode_controller.h"
 
 #include <stdint.h>
 
 #include <functional>
 
@@ skipping 350 matching lines @@
   if (decoded_images_ref_counts_.find(key) ==
       decoded_images_ref_counts_.end()) {
     decoded_image->Unlock();
   }
 
   decoded_images_.Put(key, std::move(decoded_image));
   SanityCheckState(__LINE__, true);
 }
 
 scoped_ptr<SoftwareImageDecodeController::DecodedImage>
+SoftwareImageDecodeController::DecodeImageMediumQuality(const ImageKey& key,
+                                                        const SkImage& image) {
+  NOTIMPLEMENTED();
+  return nullptr;
+}
+
+scoped_ptr<SoftwareImageDecodeController::DecodedImage>
+SoftwareImageDecodeController::DecodeImageHighQuality(const ImageKey& key,
+                                                      const SkImage& image) {
+  // If we get here, that means we couldn't use the original sized decode for
+  // whatever reason. However, in all cases we do need an original decode to
+  // either do a scale or to extract a subrect from the image.
+  auto decoded_image_result = DecodeImageOrUseCache(key, image);
+  if (!decoded_image_result.decoded_pixmap_.addr())
+    return nullptr;
+
+  // Now we have a decoded_pixmap which represents the src_rect at the
+  // original scale. All we need to do is scale it.
+  return ScaleImage(key, decoded_image_result);
+}
+
+scoped_ptr<SoftwareImageDecodeController::DecodedImage>
 SoftwareImageDecodeController::DecodeImageInternal(
     const ImageKey& key,
     const DrawImage& draw_image) {
   TRACE_EVENT1("disabled-by-default-cc.debug",
                "SoftwareImageDecodeController::DecodeImageInternal", "key",
                key.ToString());
   const SkImage* image = draw_image.image();
-
-  // If we can use the original decode, then we don't need to do scaling. We can
-  // just read pixels into the final memory.
-  if (key.can_use_original_decode()) {
-    SkImageInfo decoded_info =
-        CreateImageInfo(image->width(), image->height(), format_);
-    scoped_ptr<base::DiscardableMemory> decoded_pixels;
-    {
-      TRACE_EVENT0(
-          "disabled-by-default-cc.debug",
-          "SoftwareImageDecodeController::DecodeImageInternal - allocate "
-          "decoded pixels");
-      decoded_pixels =
-          base::DiscardableMemoryAllocator::GetInstance()
-              ->AllocateLockedDiscardableMemory(decoded_info.minRowBytes() *
-                                                decoded_info.height());
-    }
-    {
-      TRACE_EVENT0(
-          "disabled-by-default-cc.debug",
-          "SoftwareImageDecodeController::DecodeImageInternal - read pixels");
-      bool result = image->readPixels(decoded_info, decoded_pixels->data(),
-                                      decoded_info.minRowBytes(), 0, 0,
-                                      SkImage::kDisallow_CachingHint);
-
-      if (!result) {
-        decoded_pixels->Unlock();
-        return nullptr;
-      }
-    }
-
-    return make_scoped_ptr(new DecodedImage(
-        decoded_info, std::move(decoded_pixels), SkSize::Make(0, 0)));
-  }
-
-  // If we get here, that means we couldn't use the original sized decode for
-  // whatever reason. However, in all cases we do need an original decode to
-  // either do a scale or to extract a subrect from the image. So, what we can
-  // do is construct a key that would require a full sized decode, then get that
-  // decode via GetDecodedImageForDrawInternal(), use it, and unref it. This
-  // ensures that if the original sized decode is already available in any of
-  // the caches, we reuse that. We also ensure that all the proper locking takes
-  // place. If, on the other hand, the decode was not available,
-  // GetDecodedImageForDrawInternal() would decode the image, and unreffing it
-  // later ensures that we will store the discardable memory unlocked in the
-  // cache to be used by future requests.
-  gfx::Rect full_image_rect(image->width(), image->height());
-  DrawImage original_size_draw_image(image, gfx::RectToSkIRect(full_image_rect),
-                                     kNone_SkFilterQuality, SkMatrix::I());
-  ImageKey original_size_key =
-      ImageKey::FromDrawImage(original_size_draw_image);
-  // Sanity checks.
-  DCHECK(original_size_key.can_use_original_decode());
-  DCHECK(full_image_rect.size() == original_size_key.target_size());
-
-  auto decoded_draw_image = GetDecodedImageForDrawInternal(
-      original_size_key, original_size_draw_image);
-  if (!decoded_draw_image.image()) {
-    DrawWithImageFinished(original_size_draw_image, decoded_draw_image);
+  if (!image) {
     return nullptr;
   }
 
-  SkPixmap decoded_pixmap;
-  bool result = decoded_draw_image.image()->peekPixels(&decoded_pixmap);
-  DCHECK(result);
-  if (key.src_rect() != full_image_rect) {
-    result = decoded_pixmap.extractSubset(&decoded_pixmap,
-                                          gfx::RectToSkIRect(key.src_rect()));
-    DCHECK(result);
+  switch (key.filter_quality()) {
+    case kNone_SkFilterQuality:
+    // fall through

[vmpstr, 2016/03/31 19:54:32]

I don't think you really need this comment. That is, "

case A:
case B:
  ...

" is pretty clear.

If you want to keep it, then it has to be a complete sentence. 

[cblume, 2016/04/09 06:48:54]

Done.

+    case kLow_SkFilterQuality:
+      return GetOriginalImageDecode(key, *image);
+    case kMedium_SkFilterQuality:
+      return DecodeImageMediumQuality(key, *image);
+    case kHigh_SkFilterQuality:
+      return DecodeImageHighQuality(key, *image);
+    default:
+      NOTREACHED();
+      return nullptr;
   }
-
-  // Now we have a decoded_pixmap which represents the src_rect at the
-  // original scale. All we need to do is scale it.
-  DCHECK(!key.target_size().IsEmpty());
-  SkImageInfo scaled_info = CreateImageInfo(
-      key.target_size().width(), key.target_size().height(), format_);
-  scoped_ptr<base::DiscardableMemory> scaled_pixels;
-  {
-    TRACE_EVENT0(
-        "disabled-by-default-cc.debug",
-        "SoftwareImageDecodeController::DecodeImageInternal - allocate "
-        "scaled pixels");
-    scaled_pixels = base::DiscardableMemoryAllocator::GetInstance()
-                        ->AllocateLockedDiscardableMemory(
-                            scaled_info.minRowBytes() * scaled_info.height());
-  }
-  SkPixmap scaled_pixmap(scaled_info, scaled_pixels->data(),
-                         scaled_info.minRowBytes());
-  // TODO(vmpstr): Start handling more than just high filter quality.
-  DCHECK_EQ(kHigh_SkFilterQuality, key.filter_quality());
-  {
-    TRACE_EVENT0(
-        "disabled-by-default-cc.debug",
-        "SoftwareImageDecodeController::DecodeImageInternal - scale pixels");
-    bool result =
-        decoded_pixmap.scalePixels(scaled_pixmap, key.filter_quality());
-    DCHECK(result);
-  }
-
-  // Release the original sized decode. Any other intermediate result to release
-  // would be the subrect memory. However, that's in a scoped_ptr and will be
-  // deleted automatically when we return.
-  DrawWithImageFinished(original_size_draw_image, decoded_draw_image);
-
-  return make_scoped_ptr(
-      new DecodedImage(scaled_info, std::move(scaled_pixels),
-                       SkSize::Make(-key.src_rect().x(), -key.src_rect().y())));
 }
 
 DecodedDrawImage SoftwareImageDecodeController::GetDecodedImageForDraw(
     const DrawImage& draw_image) {
   ImageKey key = ImageKey::FromDrawImage(draw_image);
   TRACE_EVENT1("disabled-by-default-cc.debug",
                "SoftwareImageDecodeController::GetDecodedImageForDraw", "key",
                key.ToString());
   // If the target size is empty, we can skip this image draw.
   if (key.target_size().IsEmpty())
@@ skipping 51 matching lines @@
   // thread encountering this image (that might not be true, since other threads
   // might be decoding it already). This means that we need to decode the image
   // assuming we can't lock the one we found in the cache.
   bool check_at_raster_cache = false;
   if (!decoded_image || !decoded_image->Lock()) {
     // Note that we have to release the lock, since this lock is also accessed
     // on the compositor thread. This means holding on to the lock might stall
     // the compositor thread for the duration of the decode!
     base::AutoUnlock unlock(lock_);
     scoped_decoded_image = DecodeImageInternal(key, draw_image);
+

[vmpstr, 2016/03/31 19:54:32]

unnecessary change

[cblume, 2016/04/09 06:48:54]

Done.

     decoded_image = scoped_decoded_image.get();
 
     // Skip the image if we couldn't decode it.
     if (!decoded_image)
       return DecodedDrawImage(nullptr, kNone_SkFilterQuality);
     check_at_raster_cache = true;
   }
 
   DCHECK(decoded_image == scoped_decoded_image.get());
 
@@ skipping 20 matching lines @@
   DCHECK(decoded_image->is_locked());
   RefAtRasterImage(key);
   SanityCheckState(__LINE__, true);
   auto decoded_draw_image =
       DecodedDrawImage(decoded_image->image(), decoded_image->src_rect_offset(),
                        GetScaleAdjustment(key), GetDecodedFilterQuality(key));
   decoded_draw_image.set_at_raster_decode(true);
   return decoded_draw_image;
 }
 
+SoftwareImageDecodeController::DecodedImageResult::DecodedImageResult(

[vmpstr, 2016/03/31 19:54:32]

Move this function to be after all other SoftwareImageDecodeController functions
are defined. 

[cblume, 2016/04/09 06:48:54]

Done.

+    SkPixmap decoded_pixmap,
+    DrawImage original_size_draw_image,
+    DecodedDrawImage decoded_draw_image)
+    : decoded_pixmap_(decoded_pixmap),
+      original_size_draw_image_(original_size_draw_image),
+      decoded_draw_image_(decoded_draw_image) {}
+
+scoped_ptr<SoftwareImageDecodeController::DecodedImage>
+SoftwareImageDecodeController::GetOriginalImageDecode(const ImageKey& key,
+                                                      const SkImage& image) {
+  TRACE_EVENT1("disabled-by-default-cc.debug",
+               "SoftwareImageDecodeController::GetOriginalImageDecode", "key",
+               key.ToString());
+  DCHECK(key.can_use_original_decode());
+  SkImageInfo decoded_info =
+      CreateImageInfo(image.width(), image.height(), format_);
+  scoped_ptr<base::DiscardableMemory> decoded_pixels;
+  {
+    TRACE_EVENT0("disabled-by-default-cc.debug",
+                 "SoftwareImageDecodeController::GetOriginalImageDecode - "
+                 "allocate decoded pixels");
+    decoded_pixels =
+        base::DiscardableMemoryAllocator::GetInstance()
+            ->AllocateLockedDiscardableMemory(decoded_info.minRowBytes() *
+                                              decoded_info.height());
+  }
+  {
+    TRACE_EVENT0("disabled-by-default-cc.debug",
+                 "SoftwareImageDecodeController::GetOriginalImageDecode - "
+                 "read pixels");
+    bool result = image.readPixels(decoded_info, decoded_pixels->data(),
+                                   decoded_info.minRowBytes(), 0, 0,
+                                   SkImage::kDisallow_CachingHint);
+
+    if (!result) {
+      decoded_pixels->Unlock();
+      return nullptr;
+    }
+  }
+
+  return make_scoped_ptr(new DecodedImage(
+      decoded_info, std::move(decoded_pixels), SkSize::Make(0, 0)));
+}
+
+SoftwareImageDecodeController::DecodedImageResult
+SoftwareImageDecodeController::DecodeImageOrUseCache(const ImageKey& key,

[vmpstr, 2016/03/31 19:54:32]

I'm still a bit confused about DecodeImageOrUseCache vs
GetDecodedImageForDrawInternal... or similar things.

Can you explain the general flow of the code as it stands now?

[cblume, 2016/04/09 06:48:54]

DecodeImageInternal's old body was split into 3 functions:
- GetOriginalImageDecode,
- DecodeImageOrUseCache, and
- ScaleImage

The qualities which require no scaling only called GetOriginalImageDecode.
The qualities which required scaling would first call DecodeImageOrUseCache and
then pass the results to ScaleImage.

Vlad and I talked about it and feel that a more natural split would be:
- GetOriginalImageDecode, and
- GetScaledImageDecode

+                                                     const SkImage& image) {
+  // Construct a key to use in GetDecodedImageForDrawInternal().
+  // This allows us to reuse an image in any cache if available.
+  gfx::Rect full_image_rect(image.width(), image.height());
+  DrawImage original_size_draw_image(&image,
+                                     gfx::RectToSkIRect(full_image_rect),
+                                     kNone_SkFilterQuality, SkMatrix::I());
+  ImageKey original_size_key =
+      ImageKey::FromDrawImage(original_size_draw_image);
+  // Sanity checks.
+  DCHECK(original_size_key.can_use_original_decode());
+  DCHECK(full_image_rect.size() == original_size_key.target_size());
+
+  auto decoded_draw_image = GetDecodedImageForDrawInternal(
+      original_size_key, original_size_draw_image);
+  if (!decoded_draw_image.image()) {
+    DrawWithImageFinished(original_size_draw_image, decoded_draw_image);
+    return DecodedImageResult(SkPixmap(), DrawImage(),
+                              DecodedDrawImage(nullptr, kNone_SkFilterQuality));
+  }
+
+  SkPixmap decoded_pixmap;
+  bool result = decoded_draw_image.image()->peekPixels(&decoded_pixmap);
+  DCHECK(result);
+  if (key.src_rect() != full_image_rect) {
+    result = decoded_pixmap.extractSubset(&decoded_pixmap,
+                                          gfx::RectToSkIRect(key.src_rect()));
+    DCHECK(result);
+  }
+
+  return DecodedImageResult(decoded_pixmap, original_size_draw_image,
+                            decoded_draw_image);
+}
+
+scoped_ptr<SoftwareImageDecodeController::DecodedImage>
+SoftwareImageDecodeController::ScaleImage(
+    const ImageKey& key,
+    const DecodedImageResult& decoded_image_result) {
+  DCHECK(!key.target_size().IsEmpty());
+  SkImageInfo scaled_info = CreateImageInfo(
+      key.target_size().width(), key.target_size().height(), format_);
+  scoped_ptr<base::DiscardableMemory> scaled_pixels;
+  {
+    TRACE_EVENT0(
+        "disabled-by-default-cc.debug",
+        "SoftwareImageDecodeController::ScaleImage - allocate scaled pixels");
+    scaled_pixels = base::DiscardableMemoryAllocator::GetInstance()
+                        ->AllocateLockedDiscardableMemory(
+                            scaled_info.minRowBytes() * scaled_info.height());
+  }
+  SkPixmap scaled_pixmap(scaled_info, scaled_pixels->data(),
+                         scaled_info.minRowBytes());
+  // TODO(vmpstr): Start handling more than just high filter quality.
+  DCHECK_EQ(kHigh_SkFilterQuality, key.filter_quality());
+  {
+    TRACE_EVENT0("disabled-by-default-cc.debug",
+                 "SoftwareImageDecodeController::ScaleImage - scale pixels");
+    bool result = decoded_image_result.decoded_pixmap_.scalePixels(
+        scaled_pixmap, key.filter_quality());
+    DCHECK(result);
+  }
+
+  // Release the original sized decode. Any other intermediate result to release
+  // would be the subrect memory. However, that's in a scoped_ptr and will be
+  // deleted automatically when we return.
+  DrawWithImageFinished(decoded_image_result.original_size_draw_image_,
+                        decoded_image_result.decoded_draw_image_);
+
+  return make_scoped_ptr(
+      new DecodedImage(scaled_info, std::move(scaled_pixels),
+                       SkSize::Make(-key.src_rect().x(), -key.src_rect().y())));
+}
+
 void SoftwareImageDecodeController::DrawWithImageFinished(
     const DrawImage& image,
     const DecodedDrawImage& decoded_image) {
   TRACE_EVENT1("disabled-by-default-cc.debug",
                "SoftwareImageDecodeController::DrawWithImageFinished", "key",
                ImageKey::FromDrawImage(image).ToString());
   ImageKey key = ImageKey::FromDrawImage(image);
   if (!decoded_image.image() || !CanHandleImage(key))
     return;
 
@@ skipping 284 matching lines @@
 void SoftwareImageDecodeController::MemoryBudget::ResetUsage() {
   current_usage_bytes_ = 0;
 }
 
 size_t SoftwareImageDecodeController::MemoryBudget::GetCurrentUsageSafe()
     const {
   return current_usage_bytes_.ValueOrDie();
 }
 
 }  // namespace cc