Loading/Creating images for mutiple scale factors on the fly

ui/gfx/image/image_skia.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/gfx/image/image_skia.h"
 
+#include <algorithm>
+#include <cmath>
 #include <limits>
-#include <cmath>
 
 #include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "ui/gfx/image/image_skia_source.h"
 #include "ui/gfx/size.h"
 #include "ui/gfx/skia_util.h"
 
 namespace gfx {
+namespace {
+
+// static
+gfx::ImageSkiaRep& NullImageRep() {
+  CR_DEFINE_STATIC_LOCAL(ImageSkiaRep, null_image_rep, ());
+  return null_image_rep;
+}
+
+}  // namespace
 
 namespace internal {
+namespace {
+
+class Matcher {
+ public:
+  explicit Matcher(ui::ScaleFactor scale_factor) : scale_factor_(scale_factor) {
+  }
+
+  bool operator()(const ImageSkiaRep& rep) const {
+    return rep.scale_factor() == scale_factor_;
+  }
+
+ private:
+  ui::ScaleFactor scale_factor_;
+};
+
+}  // namespace
 
 // A helper class such that ImageSkia can be cheaply copied. ImageSkia holds a
 // refptr instance of ImageSkiaStorage, which in turn holds all of ImageSkia's
 // information.
 class ImageSkiaStorage : public base::RefCounted<ImageSkiaStorage> {
  public:
-  ImageSkiaStorage() {
+  ImageSkiaStorage(ImageSkiaSource* source, const gfx::Size& size)
+      : source_(source),
+        size_(size) {
   }
 
   std::vector<gfx::ImageSkiaRep>& image_reps() { return image_reps_; }
 
-  void set_size(const gfx::Size& size) { size_ = size; }
   const gfx::Size& size() const { return size_; }
 
+  // Returns the iterator of the image rep whose density best matches
+  // |scale_factor|. If the image for the |scale_factor| doesn't exist
+  // in the storage and |storage| is set, it fetches new image by calling
+  // |ImageSkiaSource::GetImageForScale|. If the source returns the
+  // image with different scale factor (if the image doesn't exist in
+  // resource, for example), it will fallback to closest image rep.
+  std::vector<ImageSkiaRep>::iterator FindRepresentation(
+      ui::ScaleFactor scale_factor, bool fetch_new_image) const {
+    ImageSkiaStorage* non_const = const_cast<ImageSkiaStorage*>(this);
+
+    float scale = ui::GetScaleFactorScale(scale_factor);
+    ImageSkia::ImageSkiaReps::iterator closest_iter =
+        non_const->image_reps().end();
+    ImageSkia::ImageSkiaReps::iterator exact_iter =
+        non_const->image_reps().end();
+    float smallest_diff = std::numeric_limits<float>::max();
+    for (ImageSkia::ImageSkiaReps::iterator it =
+             non_const->image_reps().begin();
+         it < image_reps_.end(); ++it) {
+      if (it->GetScale() == scale) {
+        // found exact match
+        fetch_new_image = false;
+        if (it->is_null())
+          continue;
+        exact_iter = it;
+        break;
+      }
+      float diff = std::abs(it->GetScale() - scale);
+      if (diff < smallest_diff && !it->is_null()) {
+        closest_iter = it;
+        smallest_diff = diff;
+      }
+    }
+
+    if (fetch_new_image && source_.get()) {
+      ImageSkiaRep image = source_->GetImageForScale(scale_factor);
+
+      // If the source returned the new image, store it.
+      if (!image.is_null() &&
+          std::find_if(image_reps_.begin(), image_reps_.end(),
+                       Matcher(image.scale_factor())) == image_reps_.end()) {
+        non_const->image_reps().push_back(image);
+      }
+
+      // If the result image's scale factor isn't same as the expected
+      // scale factor, create null ImageSkiaRep with the |scale_factor|
+      // so that the next lookup will fallback to the closest scale.
+      if (image.is_null() || image.scale_factor() != scale_factor) {
+        non_const->image_reps().push_back(
+            ImageSkiaRep(SkBitmap(), scale_factor));
+      }
+
+      // image_reps_ must have the exact much now, so find again.
+      return FindRepresentation(scale_factor, false);
+    }
+    return exact_iter != image_reps_.end() ? exact_iter : closest_iter;
+  }

[pkotwicz, 2012/07/01 22:22:23]

This code is ok. However, I would rather that if the image representation at the
desired scale factor is not available that it would be generated via a low
quality scale as I did here: http://codereview.chromium.org/10575039/

When a user does something like this
gfx::Canvas canvas(image.size(), ui::SCALE_FACTOR_200P, false);
SkBitmapOperations(c.ExtractImageRep(),
image1.GetRepresentation(ui::SCALE_FACTOR_200P))
It is much simpler if this call always succeeds.
It can fail if image1 only has a representation for ui::SCALE_FACTOR_100P.
The reason why I think it makes sense to do a low quality scale in
GetRepresentation is that we are just moving the time at which the scale is
applied from Canvas::DrawImageInt to ImageSkia::GetRepresentation.
The benefit is a cleaner API
I am ok if you do this in a separate CL though

[oshima, 2012/07/02 16:43:34]

ImageSkia::GetRepresentation doesn't guarantee that it will
return the image rep for given scale factor, and the caller must either 
do CHECK/DCHECK or handle it gracefully.

If scaling is what a user wants, we can provide such option as an utility
source, and
daisy chain them. Here is pseudo code:

class AutoScaleSoruce : public gfx::ImagSkiaSource {
 public:
  ImageSkiaRep GetImageForScale(ui::ScaleFactor scale_factor) {
    ImageSkiaRep rep = source_->GetImageForScale(scale_factor);
    if (rep.scale_factor() != scale_factor) {
      rep = ScaleImageSkiaRep(rep, cale_factor);
    return rep;
  }
  scoped_ptr<ImageSkiaSource> source_;
}

new ImageSkia(new AutoScaleSoruce(new YourImageSkiaSource());

Otherwise, it should fail early (either programmatically, or visually) so that
we can detect such issue sooner than later.

[pkotwicz, 2012/07/02 17:13:53]

I think there are two aspects.
1) We currently sometimes only have 1x images. (This is a bug because we are
missing assets). It might be useful to scale the images directly in the resource
bundle.
2) I don't know of any situation where the user would not want scaling. From
implementing the tab strip, I ran into a number of places where we were missing
2x assets. So I had to be really careful about which scale factor I requested
assets which went through SkBitmapOperations. However, I am fine if you want to
tackle this problem in a separate patch.
Btw, we currently already support HDA. (Though I should write some code to make
this slightly more explicit) So we already are not scaling for HDA code.

[pkotwicz, 2012/07/02 17:26:21]

Btw, it might be sufficient to scale the assets which do not have 2x versions to
2x. (Only do 1)

[oshima, 2012/07/02 17:26:47]

grit will scale if necessary. Even if it doesn't 1x image will be scaled in
canvas
appropriately, right?
We don't want to scale in most case and want to use correct 2x images instead.
This is transition issue and we should fix it in right way. I will handle these
issues
when I convert them.

+
  private:
   ~ImageSkiaStorage() {
   }
 
   // Vector of bitmaps and their associated scale factor.
   std::vector<gfx::ImageSkiaRep> image_reps_;
 
+  scoped_ptr<ImageSkiaSource> source_;
+
   // Size of the image in DIP.
-  gfx::Size size_;
+  const gfx::Size size_;
 
   friend class base::RefCounted<ImageSkiaStorage>;
 };
 
 }  // internal
 
 ImageSkia::ImageSkia() : storage_(NULL) {
 }
 
+ImageSkia::ImageSkia(ImageSkiaSource* source, const gfx::Size& size)
+    : storage_(new internal::ImageSkiaStorage(source, size)) {
+}
+
 ImageSkia::ImageSkia(const SkBitmap& bitmap) {
   Init(ImageSkiaRep(bitmap));
 }
 
 ImageSkia::ImageSkia(const ImageSkiaRep& image_rep) {
   Init(image_rep);
 }
 
 ImageSkia::ImageSkia(const ImageSkia& other) : storage_(other.storage_) {
 }
@@ skipping 37 matching lines @@
     Init(image_rep);
   else
     storage_->image_reps().push_back(image_rep);
 }
 
 void ImageSkia::RemoveRepresentation(ui::ScaleFactor scale_factor) {
   if (isNull())
     return;
 
   ImageSkiaReps& image_reps = storage_->image_reps();
-  ImageSkiaReps::iterator it = FindRepresentation(scale_factor);
+  ImageSkiaReps::iterator it =
+      storage_->FindRepresentation(scale_factor, false);
   if (it != image_reps.end() && it->scale_factor() == scale_factor)
     image_reps.erase(it);
 }
 
 bool ImageSkia::HasRepresentation(ui::ScaleFactor scale_factor) {
   if (isNull())
     return false;
 
-  ImageSkiaReps::iterator it = FindRepresentation(scale_factor);
+  ImageSkiaReps::iterator it =
+      storage_->FindRepresentation(scale_factor, false);
   return (it != storage_->image_reps().end() &&
           it->scale_factor() == scale_factor);
 }
 
 const ImageSkiaRep& ImageSkia::GetRepresentation(
     ui::ScaleFactor scale_factor) const {
   if (isNull())
     return NullImageRep();
 
-  ImageSkiaReps::iterator it = FindRepresentation(scale_factor);
+  ImageSkiaReps::iterator it = storage_->FindRepresentation(scale_factor, true);
   if (it == storage_->image_reps().end())
     return NullImageRep();
 
   return *it;
 }
 
 bool ImageSkia::empty() const {
   return isNull() || storage_->size().IsEmpty();
 }
 
 int ImageSkia::width() const {
   return isNull() ? 0 : storage_->size().width();
 }
 
+gfx::Size ImageSkia::size() const {
+  return gfx::Size(width(), height());
+}
+
 int ImageSkia::height() const {
   return isNull() ? 0 : storage_->size().height();
 }
 
 bool ImageSkia::extractSubset(ImageSkia* dst, const SkIRect& subset) const {
   if (isNull())
     return false;
   ImageSkia image;
   ImageSkiaReps& image_reps = storage_->image_reps();
   for (ImageSkiaReps::iterator it = image_reps.begin();
@@ skipping 15 matching lines @@
     return false;
 
   *dst = image;
   return true;
 }
 
 std::vector<ImageSkiaRep> ImageSkia::image_reps() const {
   if (isNull())
     return std::vector<ImageSkiaRep>();
 
   return storage_->image_reps();

[pkotwicz, 2012/07/01 22:22:23]

What happens here when you have an ImageSkiaSource() ?
I think you should rename this to cached_image_reps() to make it clear as to
what this method does.
I think you should either
1) Remove / change the use of image_reps() in the code base as this code will
not do what is expected when when being called on an ImageSkia with an
ImageSkiaSource (I think this is doable)
2) Create an image_reps() method which generates image reps for all possible
scale factors

[oshima, 2012/07/02 16:43:34]

The semantics of this api is and has been "return the image reps that ImageSkia
currently have".
It's never been "returns all images for available scale factors". I understand
that there are
such use cases, and the goal of this change is to eliminate such use case. Once
we convert
them, the use of this api should be rare, and such code must know what they're
doing.
Do you have valid use case of 2)?

[pkotwicz, 2012/07/02 17:13:53]

A couple places where we might want to use 2
- Converting between image types (though it might be possible to use 1)
- When we want to save the contents of an ImageSkia to a file. One example is in
web_app_mac

[oshima, 2012/07/02 17:26:47]

Are both cases for mac? We have no plan to use ImageSkiaSource for mac as it has
its own way to handle multiple resources.

[pkotwicz, 2012/07/02 18:05:12]

We will most likely be using ImageSkiaSource for mac as mac uses shared code
which uses SkBitmaps. I don't know of any place where we would call image_reps()
from an ImageSkia which would have been constructed with an ImageSkiaSource.
However, it would be nice for ImageSkia to have a robust API.

[oshima, 2012/07/02 18:19:30]

Let's revisit if we find such case then.
As I mentioned, there is no guarantee that image_reps() will return
images for possible scale factors (it's simply impossible), even now
without this change. Such code must know what they're doing.

Actually, I wonder if we really need this API. I looked at the current usage in
mac,
and looks like we can replace them with "get supported scale factors +
GetRepresentation"?

 }
 
 const SkBitmap* ImageSkia::bitmap() const {
   if (isNull()) {
     // Callers expect a ImageSkiaRep even if it is |isNull()|.
     // TODO(pkotwicz): Fix this.
     return &NullImageRep().sk_bitmap();
   }
 
   return &storage_->image_reps()[0].sk_bitmap();
 }
 
 void ImageSkia::Init(const ImageSkiaRep& image_rep) {
   // TODO(pkotwicz): The image should be null whenever image rep is null.
   if (image_rep.sk_bitmap().empty()) {
     storage_ = NULL;
     return;
   }
-  storage_ = new internal::ImageSkiaStorage();
-  storage_->set_size(gfx::Size(image_rep.GetWidth(), image_rep.GetHeight()));
+  storage_ = new internal::ImageSkiaStorage(
+      NULL, gfx::Size(image_rep.GetWidth(), image_rep.GetHeight()));
   storage_->image_reps().push_back(image_rep);
 }
 
-// static
-ImageSkiaRep& ImageSkia::NullImageRep() {
-  CR_DEFINE_STATIC_LOCAL(ImageSkiaRep, null_image_rep, ());
-  return null_image_rep;
-}
-
-std::vector<ImageSkiaRep>::iterator ImageSkia::FindRepresentation(
-    ui::ScaleFactor scale_factor) const {
-  DCHECK(!isNull());
-
-  float scale = ui::GetScaleFactorScale(scale_factor);
-  ImageSkiaReps& image_reps = storage_->image_reps();
-  ImageSkiaReps::iterator closest_iter = image_reps.end();
-  float smallest_diff = std::numeric_limits<float>::max();
-  for (ImageSkiaReps::iterator it = image_reps.begin();
-       it < image_reps.end();
-       ++it) {
-    float diff = std::abs(it->GetScale() - scale);
-    if (diff < smallest_diff) {
-      closest_iter = it;
-      smallest_diff = diff;
-    }
-  }
-
-  return closest_iter;
-}
-
 }  // namespace gfx