Refactor SoftwareImageDecodeController

cc/tiles/software_image_decode_controller.cc

Index: cc/tiles/software_image_decode_controller.cc
diff --git a/cc/tiles/software_image_decode_controller.cc b/cc/tiles/software_image_decode_controller.cc
index c61a0c6b5732448a0e6d9c85536183851e3f8849..d82835d9747c6be885902efa785f14055bf040bb 100644
--- a/cc/tiles/software_image_decode_controller.cc
+++ b/cc/tiles/software_image_decode_controller.cc
@@ -107,36 +107,6 @@ 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(
@@ -365,120 +335,62 @@ void SoftwareImageDecodeController::DecodeImage(const ImageKey& key,
 }
 
 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();
+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());
 
-  // 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 AttemptToUseOriginalImage(key, image);
+}
 
-    return make_scoped_ptr(new DecodedImage(
-        decoded_info, std::move(decoded_pixels), SkSize::Make(0, 0)));
-  }
+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. 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);
+  // 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;
   }
 
-  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);
-  }
 
   // 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);
-  }
+  return ScaleImage(key, decoded_image_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);
+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 (!image) {
+    return nullptr;
+  }
 
-  return make_scoped_ptr(
-      new DecodedImage(scaled_info, std::move(scaled_pixels),
-                       SkSize::Make(-key.src_rect().x(), -key.src_rect().y())));
+  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;
+  }
 }
 
 DecodedDrawImage SoftwareImageDecodeController::GetDecodedImageForDraw(
@@ -549,7 +461,11 @@ DecodedDrawImage SoftwareImageDecodeController::GetDecodedImageForDrawInternal(
     // 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.
@@ -590,6 +506,130 @@ DecodedDrawImage SoftwareImageDecodeController::GetDecodedImageForDrawInternal(
   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) {