Add medium image quality to software predecode.

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 86d1bb2c5749430e57378a3a8b49f2c00e14fcde..aa67c498296506f27c55321fdf03703018eccebb 100644
--- a/cc/tiles/software_image_decode_controller.cc
+++ b/cc/tiles/software_image_decode_controller.cc
@@ -6,6 +6,7 @@
 
 #include <stdint.h>
 
+#include <algorithm>
 #include <functional>
 
 #include "base/format_macros.h"
@@ -96,17 +97,66 @@ class ImageDecodeTaskImpl : public TileTask {
   DISALLOW_COPY_AND_ASSIGN(ImageDecodeTaskImpl);
 };
 
+// Most images are scaled from the source image's size to the target size.
+// But in the case of mipmaps, we are scaling from the mip level which is
+// larger than we need.
+// This function gets the scale of the mip level which will be used.
+SkSize GetMipMapScaleAdjustment(const gfx::Size& src_size,
+                                const gfx::Size& target_size) {
+  int src_height = src_size.height();
+  int src_width = src_size.width();
+  int target_height = target_size.height();
+  int target_width = target_size.width();

[vmpstr, 2016/05/02 19:58:26]

nit: you can early out if target_width or target_height are 0 (return (-1, -1),
and then NOTREACHED at the end). This works as well though.

[cblume, 2016/05/02 23:04:09]

I had tried this but still needed a final return, or else I would get an error
about control possibly reaching the end of the function. It does offer us an
early out though. But that is favoring the rare error case and introducing a
branch for all the good cases.

I'll go ahead and add it so you can take a look and see if you like it more.

+
+  int next_mip_height = src_height;
+  int next_mip_width = src_width;
+  for (int current_mip_level = 0;; current_mip_level++) {
+    int mip_height = next_mip_height;
+    int mip_width = next_mip_width;
+
+    next_mip_height = std::max(1, src_height / (1 << (current_mip_level + 1)));
+    next_mip_width = std::max(1, src_width / (1 << (current_mip_level + 1)));
+
+    // Check if an axis on the next mip level would be smaller than the target.
+    // If so, use the current mip level.
+    // This effectively always uses the larger image and always scales down.
+    if (next_mip_height < target_height || next_mip_width < target_width) {
+      SkScalar y_scale = static_cast<float>(mip_height) / src_height;
+      SkScalar x_scale = static_cast<float>(mip_width) / src_width;
+
+      return SkSize::Make(x_scale, y_scale);
+    }
+
+    if (mip_height == 1 && mip_width == 1) {
+      // We have reached the final mip level
+      if (target_height == 1 && target_width == 1) {

[vmpstr, 2016/05/02 19:58:26]

This is kind of meh... Do you still need this if you early out before?

[cblume, 2016/05/02 23:04:09]

This is for the final mip level case (say 1x1), not the 0 height/width error
case.

It is tricky. I previously was doing L123 "if (next_mip_height <
target_height..." and no check on this line. It would get to the final mip
level, see we haven't hit our target yet, and return an error. So it wasn't
handling 1x1.

My first attempt to handle the 1x1 case was to change L123 to <=. The problem
with that is say we have a 500x500 image that we want to present at 250x250.
Using the first mip level (not the base level) gives us exactly what we want.
But this would use the 500x500 image, instead of the 250x250 image.

So I arrived at this solution. I think it actually handles the 1x1 case pretty
well (or 1xM or Nx1). Only after we've scaled all the way down does it consider
this branch.

An alternative would be know in advance how many mip levels we need. Then we
don't need a special case for the final mip level. My original design had this.
We didn't like it because it relied on some bit manipulation math.

+        SkScalar y_scale = static_cast<float>(mip_height) / src_height;
+        SkScalar x_scale = static_cast<float>(mip_width) / src_width;
+
+        return SkSize::Make(x_scale, y_scale);
+      }
+
+      break;
+    }
+  }
+
+  return SkSize::Make(-1.f, -1.f);
+}
+
 SkSize GetScaleAdjustment(const ImageDecodeControllerKey& key) {
   // If the requested filter quality did not require scale, then the adjustment
   // is identity.
-  if (key.can_use_original_decode())
+  if (key.can_use_original_decode()) {
     return SkSize::Make(1.f, 1.f);
-
-  float x_scale =
-      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);
+  } else if (key.filter_quality() == kMedium_SkFilterQuality) {
+    return GetMipMapScaleAdjustment(key.src_rect().size(), key.target_size());
+  } else {
+    float x_scale =
+        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) {
@@ -188,25 +238,6 @@ bool SoftwareImageDecodeController::GetTaskForImageAndRef(
     return false;
   }
 
-  // If we're not going to do a scale, we will just create a task to preroll the
-  // image the first time we see it. This doesn't need to account for memory.
-  // TODO(vmpstr): We can also lock the original sized image, in which case it
-  // does require memory bookkeeping.
-  if (!CanHandleImage(key)) {
-    base::AutoLock lock(lock_);
-    if (prerolled_images_.count(key.image_id()) == 0) {
-      scoped_refptr<TileTask>& existing_task = pending_image_tasks_[key];
-      if (!existing_task) {
-        existing_task = make_scoped_refptr(
-            new ImageDecodeTaskImpl(this, key, image, tracing_info));
-      }
-      *task = existing_task;
-    } else {
-      *task = nullptr;
-    }
-    return false;
-  }
-
   base::AutoLock lock(lock_);
 
   // If we already have the image in cache, then we can return it.
@@ -289,7 +320,6 @@ void SoftwareImageDecodeController::UnrefImage(const DrawImage& image) {
   //       it yet (or failed to decode it).
   //   2b. Unlock the image but keep it in list.
   const ImageKey& key = ImageKey::FromDrawImage(image);
-  DCHECK(CanHandleImage(key)) << key.ToString();
   TRACE_EVENT1("disabled-by-default-cc.debug",
                "SoftwareImageDecodeController::UnrefImage", "key",
                key.ToString());
@@ -320,20 +350,6 @@ void SoftwareImageDecodeController::DecodeImage(const ImageKey& key,
                                                 const DrawImage& image) {
   TRACE_EVENT1("cc", "SoftwareImageDecodeController::DecodeImage", "key",
                key.ToString());
-  if (!CanHandleImage(key)) {
-    image.image()->preroll();
-
-    base::AutoLock lock(lock_);
-    prerolled_images_.insert(key.image_id());
-    // Erase the pending task from the queue, since the task won't be doing
-    // anything useful after this function terminates. Since we don't preroll
-    // images twice, this is actually not necessary but it behaves similar to
-    // the other code path: when this function finishes, the task isn't in the
-    // pending_image_tasks_ list.
-    pending_image_tasks_.erase(key);
-    return;
-  }
-
   base::AutoLock lock(lock_);
   AutoRemoveKeyFromTaskMap remove_key_from_task_map(&pending_image_tasks_, key);
 
@@ -390,6 +406,25 @@ void SoftwareImageDecodeController::DecodeImage(const ImageKey& key,
 }
 
 std::unique_ptr<SoftwareImageDecodeController::DecodedImage>
+SoftwareImageDecodeController::GetMediumQualityImageDecode(
+    const ImageKey& key,
+    sk_sp<const SkImage> image) {
+  SkSize mipmap_scale =
+      GetMipMapScaleAdjustment(key.src_rect().size(), key.target_size());
+  DCHECK(mipmap_scale.width() > 0.f && mipmap_scale.height() > 0.f);
+
+  if (mipmap_scale.width() == 1.f && mipmap_scale.height() == 1.f) {
+    return GetOriginalImageDecode(key, std::move(image));
+  } else {
+    DrawImage mip_image(
+        image, gfx::RectToSkIRect(key.src_rect()), kMedium_SkFilterQuality,
+        SkMatrix::MakeScale(mipmap_scale.width(), mipmap_scale.height()));
+    auto mip_key = ImageKey::FromDrawImage(mip_image);

[vmpstr, 2016/05/02 19:58:26]

I'm a bit confused: wouldn't the passed key already have the right
target_width/height?

[cblume, 2016/05/02 23:04:09]

Good catch. After our change to ::FromDrawImage, this always already had the
right scale. This is about to get CLEAN yo!

+    return GetScaledImageDecode(mip_key, std::move(image));
+  }
+}
+
+std::unique_ptr<SoftwareImageDecodeController::DecodedImage>
 SoftwareImageDecodeController::DecodeImageInternal(
     const ImageKey& key,
     const DrawImage& draw_image) {
@@ -405,8 +440,7 @@ SoftwareImageDecodeController::DecodeImageInternal(
     case kLow_SkFilterQuality:
       return GetOriginalImageDecode(key, std::move(image));
     case kMedium_SkFilterQuality:
-      NOTIMPLEMENTED();
-      return nullptr;
+      return GetMediumQualityImageDecode(key, std::move(image));
     case kHigh_SkFilterQuality:
       return GetScaledImageDecode(key, std::move(image));
     default:
@@ -425,9 +459,6 @@ DecodedDrawImage SoftwareImageDecodeController::GetDecodedImageForDraw(
   if (key.target_size().IsEmpty())
     return DecodedDrawImage(nullptr, kNone_SkFilterQuality);
 
-  if (!CanHandleImage(key))
-    return DecodedDrawImage(draw_image.image(), draw_image.filter_quality());
-
   return GetDecodedImageForDrawInternal(key, draw_image);
 }
 
@@ -605,8 +636,8 @@ SoftwareImageDecodeController::GetScaledImageDecode(
   }
   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());
+  DCHECK(key.filter_quality() == kHigh_SkFilterQuality ||
+         key.filter_quality() == kMedium_SkFilterQuality);
   {
     TRACE_EVENT0("disabled-by-default-cc.debug",
                  "SoftwareImageDecodeController::ScaleImage - scale pixels");
@@ -633,7 +664,7 @@ void SoftwareImageDecodeController::DrawWithImageFinished(
                "SoftwareImageDecodeController::DrawWithImageFinished", "key",
                ImageKey::FromDrawImage(image).ToString());
   ImageKey key = ImageKey::FromDrawImage(image);
-  if (!decoded_image.image() || !CanHandleImage(key))
+  if (!decoded_image.image())
     return;
 
   if (decoded_image.is_at_raster_decode())
@@ -700,11 +731,6 @@ void SoftwareImageDecodeController::UnrefAtRasterImage(const ImageKey& key) {
   }
 }
 
-bool SoftwareImageDecodeController::CanHandleImage(const ImageKey& key) {
-  // TODO(vmpstr): Start handling medium filter quality as well.
-  return key.filter_quality() != kMedium_SkFilterQuality;
-}
-
 void SoftwareImageDecodeController::ReduceCacheUsage() {
   TRACE_EVENT0("cc", "SoftwareImageDecodeController::ReduceCacheUsage");
   base::AutoLock lock(lock_);
@@ -856,6 +882,13 @@ ImageDecodeControllerKey ImageDecodeControllerKey::FromDrawImage(
   if (can_use_original_decode && !target_size.IsEmpty())

[vmpstr, 2016/05/02 19:58:26]

Should the below condition also check if target_size is empty?

[cblume, 2016/05/02 23:04:09]

Done.

     target_size = gfx::Size(image.image()->width(), image.image()->height());
 
+  if (quality == kMedium_SkFilterQuality) {
+    SkSize mip_target_size =
+        GetMipMapScaleAdjustment(src_rect.size(), target_size);
+    target_size.set_width(src_rect.width() * mip_target_size.width());
+    target_size.set_height(src_rect.height() * mip_target_size.height());
+  }
+
   return ImageDecodeControllerKey(image.image()->uniqueID(), src_rect,
                                   target_size, quality,
                                   can_use_original_decode);