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 89 matching lines @@
       key.target_size().width() / static_cast<float>(key.src_rect().width());
   float y_scale =
       key.target_size().height() / static_cast<float>(key.src_rect().height());
   return SkSize::Make(x_scale, y_scale);
 }
 
 SkFilterQuality GetDecodedFilterQuality(const ImageDecodeControllerKey& key) {
   return std::min(key.filter_quality(), kLow_SkFilterQuality);
 }
 
-SkColorType SkColorTypeForDecoding(ResourceFormat format) {

[vmpstr, 2016/03/21 20:53:40]

I think you kind of lost this patch in the merge?

[cblume, 2016/03/21 23:01:03]

Whoops. Fixed.

-  // Use kN32_SkColorType if there is no corresponding SkColorType.
-  switch (format) {
-    case RGBA_4444:
-      return kARGB_4444_SkColorType;
-    case RGBA_8888:
-    case BGRA_8888:
-      return kN32_SkColorType;
-    case ALPHA_8:
-      return kAlpha_8_SkColorType;
-    case RGB_565:
-      return kRGB_565_SkColorType;
-    case LUMINANCE_8:
-      return kGray_8_SkColorType;
-    case ETC1:
-    case RED_8:
-    case LUMINANCE_F16:
-      return kN32_SkColorType;
-  }
-  NOTREACHED();
-  return kN32_SkColorType;
-}
-
-SkImageInfo CreateImageInfo(size_t width,
-                            size_t height,
-                            ResourceFormat format) {
-  return SkImageInfo::Make(width, height, SkColorTypeForDecoding(format),
-                           kPremul_SkAlphaType);
-}
-
 }  // namespace
 
 SoftwareImageDecodeController::SoftwareImageDecodeController(
     ResourceFormat format)
     : decoded_images_(ImageMRUCache::NO_AUTO_EVICT),
       at_raster_decoded_images_(ImageMRUCache::NO_AUTO_EVICT),
       locked_images_budget_(kLockedMemoryLimitBytes),
       format_(format) {}
 
 SoftwareImageDecodeController::~SoftwareImageDecodeController() {
@@ skipping 208 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::DecodeImageNoneLowQuality(const ImageKey& key,

[vmpstr, 2016/03/21 20:53:40]

Maybe we can just skip this function and use GetOriginalDecode? Wdyt?

[cblume, 2016/03/21 23:01:04]

Done.

+                                                         const SkImage& image) {
+  DCHECK(key.can_use_original_decode());
+
+  return AttemptToUseOriginalImage(key, image);
+}
+
+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()) {

[vmpstr, 2016/03/21 20:53:40]

nit: Usually for single line if and a single line statement, braces are
optional.

[cblume, 2016/03/21 23:01:04]

Done.

+    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
+    case kLow_SkFilterQuality:
+      return DecodeImageNoneLowQuality(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 50 matching lines @@
   // Now we know that we don't have a locked image, and we seem to be the first
   // 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);
+    const SkImage* image = draw_image.image();
+    if (!image) {

[vmpstr, 2016/03/21 20:53:40]

When would this be the case?

+      return DecodedDrawImage(nullptr, kNone_SkFilterQuality);

[vmpstr, 2016/03/21 20:53:40]

If this is a case that we need to handle, then it needs to be handled earlier.
The problem with returning here is that the unlock object will be destroyed
first, which will first acquire the mutex and then the lock on line 418 will be
destroyed which will unlock it again. That's kind of just mutex churn :)

[cblume, 2016/03/21 23:01:03]

I think you are right, this won't ever be the case.
I needed to call draw_image.image(). From there I likely thought "I better check
for null before I dereference the pointer." I failed to consider the mutex.

+    }
+    scoped_decoded_image = AttemptToUseOriginalImage(key, *image);
     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(
+    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::AttemptToUseOriginalImage(const ImageKey& key,

[vmpstr, 2016/03/21 20:53:40]

nit: Can we call this "GetOriginalImageDecode" or something like that?

[cblume, 2016/03/21 23:01:03]

Done.

+                                                         const SkImage& image) {
+  TRACE_EVENT1("disabled-by-default-cc.debug",
+               "SoftwareImageDecodeController::AttemptToUseOriginalImage",
+               "key", key.ToString());
+  if (!key.can_use_original_decode()) {

[vmpstr, 2016/03/21 20:53:40]

Can we make this a DCHECK? Whatever code is calling this should only call it if
key.can_use_original_decode is true. 

[cblume, 2016/03/21 23:01:04]

Done.

+    return nullptr;
+  } else {
+    SkImageInfo decoded_info =
+        SkImageInfo::MakeN32Premul(image.width(), image.height());
+    scoped_ptr<base::DiscardableMemory> decoded_pixels;
+    {
+      TRACE_EVENT0("disabled-by-default-cc.debug",
+                   "SoftwareImageDecodeController::AttemptToUseOriginalImage - "
+                   "allocate decoded pixels");
+      decoded_pixels =
+          base::DiscardableMemoryAllocator::GetInstance()
+              ->AllocateLockedDiscardableMemory(decoded_info.minRowBytes() *
+                                                decoded_info.height());
+    }
+    {
+      TRACE_EVENT0("disabled-by-default-cc.debug",
+                   "SoftwareImageDecodeController::AttemptToUseOriginalImage - "
+                   "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,
+                                                     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 = SkImageInfo::MakeN32Premul(
+      key.target_size().width(), key.target_size().height());
+  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 SoftwareImageDecodeController::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 281 matching lines @@
 void SoftwareImageDecodeController::MemoryBudget::ResetUsage() {
   current_usage_bytes_ = 0;
 }
 
 size_t SoftwareImageDecodeController::MemoryBudget::GetCurrentUsageSafe()
     const {
   return current_usage_bytes_.ValueOrDie();
 }
 
 }  // namespace cc