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 c074af98857be59b59f895bd53b380b75ca746ea..b50fc97e4523d72237d714d28da255738872759f 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,16 +97,62 @@ class ImageDecodeTaskImpl : public TileTask {
   DISALLOW_COPY_AND_ASSIGN(ImageDecodeTaskImpl);
 };
 
+SkMatrix GetMipMapScale(const SoftwareImageDecodeController::ImageKey& key) {

[vmpstr, 2016/04/21 20:11:30]

Can you add a comment here to describe what the function does and what it
returns?

Should this be return SkSize and then the caller builds a matrix out of it if
they want?

Maybe GetMipMapScaleAdjustment or something?

[cblume, 2016/04/22 01:40:14]

Done.

+  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;
+      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 SkMatrix::MakeScale(x_scale, y_scale);
+    }
+
+    if (mip_height == 1 && mip_width == 1) {
+      // We have reached the final mip level
+      break;
+    }
+  }
+
+  return SkMatrix::MakeScale(0, 0);
+}
+
 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())

[vmpstr, 2016/04/21 20:11:30]

I'd kind of prefer if we keep this type of logic:

if (foo)
  return blah;
...
...

instead of

if (!foo) {
  ...
  ...
}
return blah;


but it's really up to you.

[cblume, 2016/04/22 01:40:14]

Done.

-    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());
+  float x_scale = 1.f;
+  float y_scale = 1.f;
+
+  if (!key.can_use_original_decode()) {
+    if (key.filter_quality() == kMedium_SkFilterQuality) {
+      SkMatrix mipmap_scale = GetMipMapScale(key);
+      x_scale = mipmap_scale.getScaleX();
+      y_scale = mipmap_scale.getScaleY();
+    } else {
+      x_scale = key.target_size().width() /
+                static_cast<float>(key.src_rect().width());
+      y_scale = key.target_size().height() /
+                static_cast<float>(key.src_rect().height());
+    }
+  }
   return SkSize::Make(x_scale, y_scale);
 }
 
@@ -122,7 +169,6 @@ SkImageInfo CreateImageInfo(size_t width,
 }
 
 }  // namespace
-

[vmpstr, 2016/04/21 20:11:31]

keep whitespace

[cblume, 2016/04/22 01:40:14]

Done.

 SoftwareImageDecodeController::SoftwareImageDecodeController(
     ResourceFormat format)
     : decoded_images_(ImageMRUCache::NO_AUTO_EVICT),
@@ -173,25 +219,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, prepare_tiles_id));
-      }
-      *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.
@@ -263,7 +290,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());
@@ -294,20 +320,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);
 
@@ -364,6 +376,21 @@ void SoftwareImageDecodeController::DecodeImage(const ImageKey& key,
 }
 
 std::unique_ptr<SoftwareImageDecodeController::DecodedImage>
+SoftwareImageDecodeController::DecodeImageMediumQuality(const ImageKey& key,
+                                                        const SkImage& image) {
+  SkMatrix mipmap_scale = GetMipMapScale(key);
+  if (mipmap_scale.getScaleX() == 0 || mipmap_scale.getScaleY() == 0) {

[vmpstr, 2016/04/21 20:11:30]

This should be doing an epsilon comparison since it's a float?

[cblume, 2016/04/22 01:40:14]

This started a good conversation in my cube.
Our consensus is that we don't need to compare against epsilon. But I want to
provide the thought that got there:

I actually changed it to -1 since close to 0 is a possible scale. I wanted to
get away from any possibly-good value.

-1 can be represented perfectly in floating point, no matter the size of the
float. There must always be at least 1 sign bit and 1 mantissa bit.

However, there were other things discussed which are important to note. First,
there were no operations to get to -1. It was a literal. If there were
operations, we would need the epsilon.

Additionally, there is a case where the number couldn't be perfectly represented
like say comparing -1.3f == -1.3f. The compiler will generate sort of literal.
But it is possible that the compiler generates say a 80-bit literal. When the
original literal was stored into a float, it will involve rounding from the
80-bits.

Now then, all that said, didn't we just approve std::optional? In general I
dislike invalid values...

+    return nullptr;
+  }
+
+  gfx::Rect src_rect = key.src_rect();
+  DrawImage mip_image(&image, gfx::RectToSkIRect(src_rect),

[vmpstr, 2016/04/21 20:11:31]

nit: key.src_rect() directly here

[cblume, 2016/04/22 01:40:14]

Done.

+                      kMedium_SkFilterQuality, mipmap_scale);
+  auto mip_key = ImageKey::FromDrawImage(mip_image);

[vmpstr, 2016/04/21 20:11:30]

Interesting, so you create a new DrawImage that is at the exact mipmap level and
then use kMedium_SkFilterQuality but other than that it's going down the same
path that high filter quality would go... That's pretty cool!

Do we have guarantees that the mip size is exactly the same as what skia would
pick? That is to say, would there still be a bilerp to adjust the size by one
pixel or so maybe?

[cblume, 2016/04/22 01:40:14]

I asked the Skia team about if it runs a bilerp if I requested the exact scale
of the mipmap. I was told it would not.

Unfortunately, I do not have a good way to guarantee that the mip size is
exactly the same as the one used by Skia. That would require Skia exporting such
information. So I cannot DCHECK it or the likes. Right now, I just happen to
know that medium quality is mipmaps and I happen to know Skia uses SkMipMap. And
I happen to know SkMipMap fairly well.

A better solution would probably be to export SkMipMap, but that requires
exporting other dependencies as well. SkMipMap allows access to each mip level,
including that mip level's size.

+  return GetScaledImageDecode(mip_key, image);
+}
+
+std::unique_ptr<SoftwareImageDecodeController::DecodedImage>
 SoftwareImageDecodeController::DecodeImageInternal(
     const ImageKey& key,
     const DrawImage& draw_image) {
@@ -379,8 +406,7 @@ SoftwareImageDecodeController::DecodeImageInternal(
     case kLow_SkFilterQuality:
       return GetOriginalImageDecode(key, *image);
     case kMedium_SkFilterQuality:
-      NOTIMPLEMENTED();
-      return nullptr;
+      return DecodeImageMediumQuality(key, *image);

[vmpstr, 2016/04/21 20:11:30]

To be consistent, can you have this GetMediumQualityImageDecode()

Or something like GetMipMap...Decode? I don't know :)

[cblume, 2016/04/22 01:40:14]

Done.

     case kHigh_SkFilterQuality:
       return GetScaledImageDecode(key, *image);
     default:
@@ -399,9 +425,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);
 }
 
@@ -577,8 +600,6 @@ SoftwareImageDecodeController::GetScaledImageDecode(const ImageKey& key,
   }
   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());

[vmpstr, 2016/04/21 20:11:31]

Can you add a

DCHECK(key.filter_quality() == kHigh_SkFilterQuality || k.filter_quality() ==
kMedium_SkFilterQuality)?

[cblume, 2016/04/22 01:40:14]

Done.

   {
     TRACE_EVENT0("disabled-by-default-cc.debug",
                  "SoftwareImageDecodeController::ScaleImage - scale pixels");
@@ -605,7 +626,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())
@@ -672,11 +693,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_);