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 d1e7f8083c497b3cd526ee36975cda02d00501d3..5f93e98c018f5aeb54180540397052a9a0d044da 100644
--- a/cc/tiles/software_image_decode_controller.cc
+++ b/cc/tiles/software_image_decode_controller.cc
@@ -368,6 +368,28 @@ void SoftwareImageDecodeController::DecodeImage(const ImageKey& key,
 }
 
 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) {
@@ -375,113 +397,23 @@ SoftwareImageDecodeController::DecodeImageInternal(
                "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);
-  }
-
-  // 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());
+  switch (key.filter_quality()) {
+    case kNone_SkFilterQuality:
+    // fall through
+    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;
   }
-  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(
@@ -552,7 +484,18 @@ 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 (key.can_use_original_decode()) {

[ericrk, 2016/03/24 16:06:59]

If we unify the two by calling DecodeImageInternal, |can_use_original_decode| is
only used in dchecks, so I think we can just remove this?

[ericrk, 2016/03/24 16:06:59]

Can't we just call DecodeImageInternal here? Seems like the same logic is in
both places?

[cblume, 2016/03/24 18:45:10]

I began removing it and noticed GetScaleAdjustment was using it to return a
SkSize of 1.f, 1.f. I would imagine a floating point divide of x/x would return
1.f but I wonder if it happens to be just slightly off and causing a problem?
(Otherwise, why have that shortcut?)

Similarly, inside ImageDecodeControllerKey's constructor there is a special
branch for if we can use the original decode. But I don't think it is required.

So I made these changes and believe they should work.

[cblume, 2016/03/24 18:45:10]

Done.

[cblume, 2016/03/24 18:46:49]

Running the tests, the low quality test and none quality test are failing on
EXPECT_TRUE(decoded_draw_image.is_scale_adjustment_identity());

So it looks like the floating point division of x/x isn't within our epsilon.

+      scoped_decoded_image = GetOriginalImageDecode(key, *image);
+    } else {
+      auto decoded_image_result = DecodeImageOrUseCache(key, *image);
+      if (!decoded_image_result.decoded_pixmap_.addr()) {
+        scoped_decoded_image = nullptr;
+      } else {
+        scoped_decoded_image = ScaleImage(key, decoded_image_result);
+      }
+    }
+
     decoded_image = scoped_decoded_image.get();
 
     // Skip the image if we couldn't decode it.
@@ -593,6 +536,126 @@ 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::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,
+                                                     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) {