Make our HSL shifting match Photoshop's.

Also clean up a bunch of PMColor code - skia_utils should operate on SkColors and PMColors should only ever be used by SkBitmaps and ImageOperations.

BUG=16687

Review URL: http://codereview.chromium.org/149663

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@20788 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 46 insertions, 69 deletions

diff --git a/skia/ext/image_operations.cc b/skia/ext/image_operations.cc
index cb4a29c..ff16fe5 100644
--- a/skia/ext/image_operations.cc
+++ b/skia/ext/image_operations.cc
@@ -17,6 +17,7 @@
 #include "base/time.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkColorPriv.h"
+#include "third_party/skia/include/core/SkUnPreMultiply.h"
 #include "skia/ext/convolver.h"
 
 namespace skia {
@@ -540,49 +541,9 @@ SkBitmap ImageOperations::CreateHSLShiftedBitmap(const SkBitmap& bitmap,
     SkPMColor* tinted_pixels = shifted.getAddr32(0, y);
 
     for (int x = 0; x < bitmap.width(); x++) {
-      // Convert the color of this pixel to HSL.
-      SkPMColor color = pixels[x];
-      int alpha = SkColorGetA(color);
-      if (alpha != 255) {
-        // We have to normalize the colors as they're pre-multiplied.
-        double r = SkColorGetR(color) / static_cast<double>(alpha);
-        double g = SkColorGetG(color) / static_cast<double>(alpha);
-        double b = SkColorGetB(color) / static_cast<double>(alpha);
-        color = SkColorSetARGB(255,
-                               static_cast<int>(r * 255.0),
-                               static_cast<int>(g * 255.0),
-                               static_cast<int>(b * 255.0));
-      }
-
-      HSL pixel_hsl = { 0, 0, 0 };
-      SkColorToHSL(color, pixel_hsl);
-
-      // Replace the hue with the tint's hue.
-      if (hsl_shift.h >= 0)
-        pixel_hsl.h = hsl_shift.h;
-
-      // Change the saturation.
-      if (hsl_shift.s >= 0) {
-        if (hsl_shift.s <= 0.5) {
-          pixel_hsl.s *= hsl_shift.s * 2.0;
-        } else {
-          pixel_hsl.s = pixel_hsl.s + (1.0 - pixel_hsl.s) *
-              ((hsl_shift.s - 0.5) * 2.0);
-        }
-      }
-
-      // Change the lightness.
-      if (hsl_shift.l >= 0) {
-        if (hsl_shift.l <= 0.5) {
-          pixel_hsl.l *= hsl_shift.l * 2.0;
-        } else {
-          pixel_hsl.l = pixel_hsl.l + (1.0 - pixel_hsl.l) *
-              ((hsl_shift.l - 0.5) * 2.0);
-        }
-      }
-
-      // Convert back to RGB.
-      tinted_pixels[x] = HSLToSKColor(alpha, pixel_hsl);
+      SkColor color = SkUnPreMultiply::PMColorToColor(pixels[x]);
+      SkColor shifted = HSLShift(color, hsl_shift);
+      tinted_pixels[x] = SkPreMultiplyColor(shifted);
     }
   }
 
diff --git a/skia/ext/skia_utils.cc b/skia/ext/skia_utils.cc
index 5a1226e..84a003b 100644
--- a/skia/ext/skia_utils.cc
+++ b/skia/ext/skia_utils.cc
@@ -38,11 +38,10 @@ static inline double calcHue(double temp1, double temp2, double hueVal) {
   return temp1;
 }
 
-SkPMColor HSLToSKColor(U8CPU alpha, HSL hsl) {
+SkColor HSLToSkColor(U8CPU alpha, HSL hsl) {
   double hue = hsl.h;
   double saturation = hsl.s;
   double lightness = hsl.l;
-  double scaleFactor = 256.0;
 
   // If there's no color, we don't care about hue and can do everything based
   // on brightness.
@@ -56,8 +55,7 @@ SkPMColor HSLToSKColor(U8CPU alpha, HSL hsl) {
     else
       light = SkDoubleToFixed(lightness) >> 8;
 
-    unsigned greyValue = SkAlphaMul(light, alpha);
-    return SkColorSetARGB(alpha, greyValue, greyValue, greyValue);
+    return SkColorSetARGB(alpha, light, light, light);
   }
 
   double temp2 = (lightness < 0.5) ?
@@ -70,12 +68,12 @@ SkPMColor HSLToSKColor(U8CPU alpha, HSL hsl) {
   double bh = calcHue(temp1, temp2, hue - 1.0 / 3.0);
 
   return SkColorSetARGB(alpha,
-      SkAlphaMul(static_cast<int>(rh * scaleFactor), alpha),
-      SkAlphaMul(static_cast<int>(gh * scaleFactor), alpha),
-      SkAlphaMul(static_cast<int>(bh * scaleFactor), alpha));
+      static_cast<int>(rh * 255),
+      static_cast<int>(gh * 255),
+      static_cast<int>(bh * 255));
 }
 
-void SkColorToHSL(SkPMColor c, HSL& hsl) {
+void SkColorToHSL(SkColor c, HSL& hsl) {
   double r = SkColorGetR(c) / 255.0;
   double g = SkColorGetG(c) / 255.0;
   double b = SkColorGetB(c) / 255.0;
@@ -119,7 +117,11 @@ void SkColorToHSL(SkPMColor c, HSL& hsl) {
   hsl.l = l;
 }
 
-SkColor HSLShift(HSL hsl, HSL shift) {
+SkColor HSLShift(SkColor color, HSL shift) {
+  HSL hsl;
+  int alpha = SkColorGetA(color);
+  SkColorToHSL(color, hsl);
+
   // Replace the hue with the tint's hue.
   if (shift.h >= 0)
     hsl.h = shift.h;
@@ -134,17 +136,33 @@ SkColor HSLShift(HSL hsl, HSL shift) {
     }
   }
 
-  // Change the lightness.
+  SkColor result = HSLToSkColor(alpha, hsl);
+
+  // Lightness shifts in the style of popular image editors aren't
+  // actually represented in HSL - the L value does have some effect
+  // on saturation.
   if (shift.l >= 0) {
+    double r = static_cast<double>SkColorGetR(result);
+    double g = static_cast<double>SkColorGetG(result);
+    double b = static_cast<double>SkColorGetB(result);
+
     if (shift.l <= 0.5) {
-      hsl.l *= shift.l * 2.0;
+      r *= (shift.l * 2.0);
+      g *= (shift.l * 2.0);
+      b *= (shift.l * 2.0);
     } else {
-      hsl.l = hsl.l + (1.0 - hsl.l) *
-        ((shift.l - 0.5) * 2.0);
+      r = (r + (255.0 - r) * ((shift.l - 0.5) * 2.0));
+      g = (g + (255.0 - g) * ((shift.l - 0.5) * 2.0));
+      b = (b + (255.0 - b) * ((shift.l - 0.5) * 2.0));
     }
-  }
 
-  return skia::HSLToSKColor(0xff, hsl);
+    return SkColorSetARGB(alpha, 
+                          static_cast<int>(r),
+                          static_cast<int>(g),
+                          static_cast<int>(b));
+  } else {
+    return result;
+  }
 }
 
 
diff --git a/skia/ext/skia_utils.h b/skia/ext/skia_utils.h
index f9707ab..769d9d4 100644
--- a/skia/ext/skia_utils.h
+++ b/skia/ext/skia_utils.h
@@ -27,15 +27,14 @@ SkShader* CreateGradientShader(int start_point,
                                SkColor start_color,
                                SkColor end_color);
 
-// Convert a premultiplied SkColor to a HSL value.
-void SkColorToHSL(SkPMColor c, HSL& hsl);
+// Convert an SkColor to a HSL value.
+void SkColorToHSL(SkColor c, HSL& hsl);
 
-// Convert a HSL color to a premultiplied SkColor.
-SkPMColor HSLToSKColor(U8CPU alpha, HSL hsl);
+// Convert a HSL color to an SkColor.
+SkColor HSLToSkColor(U8CPU alpha, HSL hsl);
 
-// Shift an HSL value. The shift values are in the range of 0-1,
-// with the option to specify -1 for 'no change'. The shift values are
-// defined as:
+// HSL-Shift an SkColor. The shift values are in the range of 0-1, with the 
+// option to specify -1 for 'no change'. The shift values are defined as:
 // hsl_shift[0] (hue): The absolute hue value - 0 and 1 map
 //    to 0 and 360 on the hue color wheel (red).
 // hsl_shift[1] (saturation): A saturation shift, with the
@@ -48,7 +47,7 @@ SkPMColor HSLToSKColor(U8CPU alpha, HSL hsl);
 //    0 = remove all lightness (make all pixels black).
 //    0.5 = leave unchanged.
 //    1 = full lightness (make all pixels white).
-SkColor HSLShift(skia::HSL hsl, skia::HSL shift);
+SkColor HSLShift(SkColor color, skia::HSL shift);
 
 }  // namespace skia
 
diff --git a/skia/ext/skia_utils_unittest.cc b/skia/ext/skia_utils_unittest.cc
index 21a6c8d..3b590b4 100644
--- a/skia/ext/skia_utils_unittest.cc
+++ b/skia/ext/skia_utils_unittest.cc
@@ -29,12 +29,11 @@ TEST(SkiaUtils, SkColorToHSLGrey) {
 }
 
 TEST(SkiaUtils, HSLToSkColorWithAlpha) {
-  // Premultiplied alpha - this is full red.
-  SkColor red = SkColorSetARGB(128, 128, 0, 0);
+  SkColor red = SkColorSetARGB(128, 255, 0, 0);
 
   skia::HSL hsl = { 0, 1, 0.5 };
 
-  SkColor result = skia::HSLToSKColor(128, hsl);
+  SkColor result = skia::HSLToSkColor(128, hsl);
   EXPECT_EQ(SkColorGetA(red), SkColorGetA(result));
   EXPECT_EQ(SkColorGetR(red), SkColorGetR(result));
   EXPECT_EQ(SkColorGetG(red), SkColorGetG(result));