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..ab51ae5c41cf59ecc5ca8c7b2650af1aaec547d6 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,65 @@ 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 SoftwareImageDecodeController::ImageKey& key) {
+  gfx::Rect src_rect = key.src_rect();
+  int src_height = src_rect.height();
+  int src_width = src_rect.width();
+
+  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 <= key.target_size().height() ||
+        next_mip_width <= key.target_size().width()) {
+      SkScalar y_scale = 1.f;

[vmpstr, 2016/04/29 19:09:41]

nit: i'd just do the math below unconditionally, but up to you.

[cblume, 2016/05/01 01:03:37]

I thought I had run into a situation where this caused a not-identity case
earlier.
But it is passing all the tests now, which check for identity when it should be.
So perhaps my non-identity problem was just my bad code.

+      SkScalar x_scale = 1.f;
+      if (current_mip_level != 0) {
+        y_scale = static_cast<float>(mip_height) / src_height;
+        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
+      break;
+    }
+  }
+
+  return SkSize::Make(-1.f, -1.f);

[vmpstr, 2016/04/29 19:09:40]

Should this be a NOTREACHED? Is this code called with arguments that fall in
here?

[cblume, 2016/05/01 01:03:37]

I changed it to NOTREACHED but in our unit test of scale 0.001 it actually hits
this. The requested scale is smaller than an image can actually store. (IE 0
height or width.) So I took the NOTREACHED back out and added the error checking
for if it was a bad call.

Should I instead change it to be NOTREACHED and check the params before making
the call? I think that might be how we normally do it on Chrome.

+}
+
 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 {

[vmpstr, 2016/04/29 19:09:40]

nit:

} else if (...) {
} else {
}

[cblume, 2016/05/01 01:03:37]

Done.

+    if (key.filter_quality() == kMedium_SkFilterQuality) {
+      return GetMipMapScaleAdjustment(key);
+    } 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 +237,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 +319,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 +349,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 +405,28 @@ 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);
+  // -1 represents an invalid scale.
+  // So add 1 and compare to epsilon.

[vmpstr, 2016/04/29 19:09:40]

Comment is still wrong :)

[cblume, 2016/05/01 01:03:37]

Oops. I could have sworn I fixed that.

+  if (mipmap_scale.width() <= 0.f || mipmap_scale.height() <= 0.f) {

[vmpstr, 2016/04/29 19:09:40]

Can this be a DCHECK instead? That is, are there cases where we call this
function with an image that causes a <= 0 scale?

[cblume, 2016/05/01 01:03:37]

This is similar to the NOTREACHED thing above.
When the requested scale turns the height or width into 0, we get this bad case
and need to actually check for it.

I could instead check early if the requested height or width would be 0.

[cblume, 2016/05/01 22:51:27]

I switched this over to a DCHECK since it is separate form the ::FromImageKey
and may not ever get a scale that produces a 0 height or width. It seems to
work. Please double check me on this.

+    return nullptr;
+  }
+
+  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);
+    return GetScaledImageDecode(mip_key, std::move(image));
+  }
+}
+
+std::unique_ptr<SoftwareImageDecodeController::DecodedImage>
 SoftwareImageDecodeController::DecodeImageInternal(
     const ImageKey& key,
     const DrawImage& draw_image) {
@@ -405,8 +442,7 @@ SoftwareImageDecodeController::DecodeImageInternal(
     case kLow_SkFilterQuality:
       return GetOriginalImageDecode(key, std::move(image));
     case kMedium_SkFilterQuality:
-      NOTIMPLEMENTED();
-      return nullptr;
+      return GetMediumQualityImageDecode(key, std::move(image));

[vmpstr, 2016/04/29 19:09:40]

I'm a bit concerned about the fact that key and key generation didn't change
here.

Would this mean that if we have an image I that appears at medium quality in two
places at scale 0.4 and 0.45, then we'd cache it twice:

key (I, 0.4) -> mip at scale 0.5
key (I, 0.45) -> mip at scale 0.5

That seems wasteful since it's the same mip. Or am I missing something?

[cblume, 2016/05/01 01:03:37]

You are completely correct. This should be fixed now.

     case kHigh_SkFilterQuality:
       return GetScaledImageDecode(key, std::move(image));
     default:
@@ -425,9 +461,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 +638,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 +666,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 +733,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_);