Fix rounding rules for skia operations to work with non-integer scaling factors.

ui/gfx/image/image_skia_operations.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_operations.h"
 
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "skia/ext/image_operations.h"
 #include "ui/base/layout.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/image/canvas_image_source.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/image/image_skia_rep.h"
 #include "ui/gfx/image/image_skia_source.h"
 #include "ui/gfx/insets.h"
+#include "ui/gfx/point.h"
+#include "ui/gfx/point_conversions.h"
 #include "ui/gfx/rect.h"
 #include "ui/gfx/rect_conversions.h"
 #include "ui/gfx/size.h"
 #include "ui/gfx/size_conversions.h"
 #include "ui/gfx/skbitmap_operations.h"
 #include "ui/gfx/skia_util.h"
 
 namespace gfx {
 namespace {
 
+gfx::Rect DIPToPixelBounds(gfx::Rect dip_bounds, float scale) {
+  return ToEnclosingRect(ScaleRect(dip_bounds, scale));
+}
+
+gfx::Size DIPToPixelSize(gfx::Size dip_size, float scale) {
+  return ToCeiledSize(ScaleSize(dip_size, scale));
+}
+
+ImageSkiaRep CropBitmap(ImageSkiaRep& source, gfx::Rect& bounds) {

[pkotwicz, 2013/03/13 04:03:48]

Nit: Rename to CropImageSkiaRep
     Rename |bounds| to |bounds_in_pixel|.

[kevers, 2013/03/13 14:36:59]

Done.

+  SkBitmap result;
+  bool success = source.sk_bitmap().extractSubset(&result,
+                                                  RectToSkIRect(bounds));
+  DCHECK(success);
+  return ImageSkiaRep(result, source.scale_factor());
+}
+
 // Returns an image rep for the ImageSkiaSource to return to visually indicate
 // an error.
 ImageSkiaRep GetErrorImageRep(ui::ScaleFactor scale_factor,
                               const gfx::Size& pixel_size) {
   SkBitmap bitmap;
   bitmap.setConfig(
       SkBitmap::kARGB_8888_Config, pixel_size.width(), pixel_size.height());
   bitmap.allocPixels();
   bitmap.eraseColor(SK_ColorRED);
   return gfx::ImageSkiaRep(bitmap, scale_factor);
@@ skipping 10 matching lines @@
         second_(second),
         source_name_(source_name) {
   }
   virtual ~BinaryImageSource() {
   }
 
   // gfx::ImageSkiaSource overrides:
   virtual ImageSkiaRep GetImageForScale(ui::ScaleFactor scale_factor) OVERRIDE {
     ImageSkiaRep first_rep = first_.GetRepresentation(scale_factor);
     ImageSkiaRep second_rep = second_.GetRepresentation(scale_factor);
-    if (first_rep.pixel_size() != second_rep.pixel_size()) {
+    gfx::Size first_size = first_rep.pixel_size();
+    gfx::Size second_size = second_rep.pixel_size();
+    // Allow for maximum of 1 pixel mismatch due to rounding errors.

[pkotwicz, 2013/03/13 04:03:48]

Nit: "due to rounding errors because of a fractional scale factor".
You can also simplify this code by using ImageSkiaRep::pixel_width() and
ImageSkiaRep::pixel_height().

[kevers, 2013/03/13 14:36:59]

Done.

+    int dw = first_size.width() - second_size.width();
+    int dh = first_size.height() - second_size.height();
+    if (abs(dw) > 1 || abs(dh) > 1) {
       DCHECK_NE(first_rep.scale_factor(), second_rep.scale_factor());
       if (first_rep.scale_factor() == second_rep.scale_factor()) {
         LOG(ERROR) << "ImageSkiaRep size mismatch in " << source_name_;
         return GetErrorImageRep(first_rep.scale_factor(),
                                 first_rep.pixel_size());
       }
       first_rep = first_.GetRepresentation(ui::SCALE_FACTOR_100P);
       second_rep = second_.GetRepresentation(ui::SCALE_FACTOR_100P);
-      DCHECK_EQ(first_rep.pixel_width(), second_rep.pixel_width());
-      DCHECK_EQ(first_rep.pixel_height(), second_rep.pixel_height());
-      if (first_rep.pixel_size() != second_rep.pixel_size()) {
+      first_size = first_rep.pixel_size();
+      second_size = second_rep.pixel_size();
+      dw = first_size.width() - second_size.width();
+      dh = first_size.height() - second_size.height();

[pkotwicz, 2013/03/13 04:03:48]

Use ImageSkiaRep::pixel_width() and ImageSkiaRep::pixel_height() here too :)

[kevers, 2013/03/13 14:36:59]

Done.

+      DCHECK_LE(abs(dw), 1);
+      DCHECK_LE(abs(dh), 1);
+      if (abs(dw) > 1 || abs(dh) > 1) {
         LOG(ERROR) << "ImageSkiaRep size mismatch in " << source_name_;
         return GetErrorImageRep(first_rep.scale_factor(),
                                 first_rep.pixel_size());
       }
     } else {
       DCHECK_EQ(first_rep.scale_factor(), second_rep.scale_factor());
     }
+    if (dw || dh) {
+      gfx::Rect bounds = IntersectRects(Rect(first_size), Rect(second_size));
+      first_rep = CropBitmap(first_rep, bounds);
+      second_rep = CropBitmap(second_rep, bounds);
+    }
     return CreateImageSkiaRep(first_rep, second_rep);
   }
 
   // Creates a final image from two ImageSkiaReps. The pixel size of
   // the two images are guaranteed to be the same.
   virtual ImageSkiaRep CreateImageSkiaRep(
       const ImageSkiaRep& first_rep,
       const ImageSkiaRep& second_rep) const = 0;
 
  private:
@@ skipping 123 matching lines @@
         dst_h_(dst_h) {
   }
 
   virtual ~TiledImageSource() {
   }
 
   // gfx::ImageSkiaSource overrides:
   virtual ImageSkiaRep GetImageForScale(ui::ScaleFactor scale_factor) OVERRIDE {
     ImageSkiaRep source_rep = source_.GetRepresentation(scale_factor);
     float scale = ui::GetScaleFactorScale(source_rep.scale_factor());
+    gfx::Rect bounds = DIPToPixelBounds(gfx::Rect(src_x_, src_y_, dst_w_,
+                                                  dst_h_), scale);

[pkotwicz, 2013/03/13 04:03:48]

ToEnclosingRect() does something different than using
Point::ToFlooredPoint() and Size::ToCeiledSize() that you were doing in patch
set #1. Is this change intentional?

[kevers, 2013/03/13 14:36:59]

The size resulting from using ToEnclosingRect is a pixel larger in width and
height than using ToFlooredPoint + ToCeiledSize if both the top-left and
bottom-right corners are not integer aligned after floating point scaling.  This
change is intentional.

     return ImageSkiaRep(
         SkBitmapOperations::CreateTiledBitmap(
             source_rep.sk_bitmap(),
-            src_x_ * scale, src_y_ * scale, dst_w_ * scale, dst_h_ * scale),
+            bounds.x(), bounds.y(), bounds.width(), bounds.height()),
         source_rep.scale_factor());
   }
 
  private:
   const ImageSkia source_;
   const int src_x_;
   const int src_y_;
   const int dst_w_;
   const int dst_h_;
 
@@ skipping 20 matching lines @@
   }
 
  private:
   const gfx::ImageSkia image_;
   const color_utils::HSL hsl_shift_;
   DISALLOW_COPY_AND_ASSIGN(HSLImageSource);
 };
 
 // ImageSkiaSource which uses SkBitmapOperations::CreateButtonBackground
 // to generate image reps for the target image.
-class ButtonImageSource: public gfx::ImageSkiaSource {
+class ButtonImageSource: public BinaryImageSource {
  public:
   ButtonImageSource(SkColor color,
                     const ImageSkia& image,
                     const ImageSkia& mask)
-      : color_(color),
-        image_(image),
-        mask_(mask) {
+      : BinaryImageSource(image, mask, "ButtonImageSource"),
+        color_(color) {
   }
 
   virtual ~ButtonImageSource() {
   }
 
-  // gfx::ImageSkiaSource overrides:
-  virtual ImageSkiaRep GetImageForScale(ui::ScaleFactor scale_factor) OVERRIDE {
-    ImageSkiaRep image_rep = image_.GetRepresentation(scale_factor);
-    ImageSkiaRep mask_rep = mask_.GetRepresentation(scale_factor);
-    if (image_rep.scale_factor() != mask_rep.scale_factor()) {
-      image_rep = image_.GetRepresentation(ui::SCALE_FACTOR_100P);
-      mask_rep = mask_.GetRepresentation(ui::SCALE_FACTOR_100P);
-    }
+  // BinaryImageSource overrides:
+  virtual ImageSkiaRep CreateImageSkiaRep(
+      const ImageSkiaRep& first_rep,
+      const ImageSkiaRep& second_rep) const OVERRIDE {
     return gfx::ImageSkiaRep(
         SkBitmapOperations::CreateButtonBackground(color_,
-              image_rep.sk_bitmap(), mask_rep.sk_bitmap()),
-          image_rep.scale_factor());
+              first_rep.sk_bitmap(), second_rep.sk_bitmap()),
+          first_rep.scale_factor());
   }
 
  private:
   const SkColor color_;
-  const ImageSkia image_;
-  const ImageSkia mask_;
 
   DISALLOW_COPY_AND_ASSIGN(ButtonImageSource);
 };
 
 // ImageSkiaSource which uses SkBitmap::extractSubset to generate image reps
 // for the target image.
 class ExtractSubsetImageSource: public gfx::ImageSkiaSource {
  public:
   ExtractSubsetImageSource(const gfx::ImageSkia& image,
                            const gfx::Rect& subset_bounds)
       : image_(image),
         subset_bounds_(subset_bounds) {
   }
 
   virtual ~ExtractSubsetImageSource() {
   }
 
   // gfx::ImageSkiaSource overrides:
   virtual ImageSkiaRep GetImageForScale(ui::ScaleFactor scale_factor) OVERRIDE {
     ImageSkiaRep image_rep = image_.GetRepresentation(scale_factor);
     float scale_to_pixel = ui::GetScaleFactorScale(image_rep.scale_factor());
-    SkIRect subset_bounds_in_pixel = RectToSkIRect(ToFlooredRectDeprecated(
-        gfx::ScaleRect(subset_bounds_, scale_to_pixel)));
-    SkBitmap dst;
-    bool success = image_rep.sk_bitmap().extractSubset(&dst,
-                                                       subset_bounds_in_pixel);
-    DCHECK(success);
-    return gfx::ImageSkiaRep(dst, image_rep.scale_factor());
+    Rect subset_bounds_in_pixel = DIPToPixelBounds(subset_bounds_,
+                                                   scale_to_pixel);
+    return CropBitmap(image_rep, subset_bounds_in_pixel);
   }
 
  private:
   const gfx::ImageSkia image_;
   const gfx::Rect subset_bounds_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtractSubsetImageSource);
 };
 
 // ResizeSource resizes relevant image reps in |source| to |target_dip_size|
@@ skipping 10 matching lines @@
   virtual ~ResizeSource() {}
 
   // gfx::ImageSkiaSource overrides:
   virtual ImageSkiaRep GetImageForScale(ui::ScaleFactor scale_factor) OVERRIDE {
     const ImageSkiaRep& image_rep = source_.GetRepresentation(scale_factor);
     if (image_rep.GetWidth() == target_dip_size_.width() &&
         image_rep.GetHeight() == target_dip_size_.height())
       return image_rep;
 
     const float scale = ui::GetScaleFactorScale(scale_factor);
-    const Size target_pixel_size = gfx::ToFlooredSize(
-        gfx::ScaleSize(target_dip_size_, scale));
+    const Size target_pixel_size = DIPToPixelSize(target_dip_size_, scale);
     const SkBitmap resized = skia::ImageOperations::Resize(
         image_rep.sk_bitmap(),
         resize_method_,
         target_pixel_size.width(),
         target_pixel_size.height());
     return ImageSkiaRep(resized, scale_factor);
   }
 
  private:
   const ImageSkia source_;
@@ skipping 181 matching lines @@
     return ImageSkia();
 
   return ImageSkia(new RotatedSource(source, rotation),
       SkBitmapOperations::ROTATION_180_CW == rotation ?
           source.size() :
           gfx::Size(source.height(), source.width()));
 
 }
 
 }  // namespace gfx