Fix various SkBitmap::copyTo() bugs

I believe that the best fix is to move this entire function into the
framework and call SkPixmap::readPixels() from there.  Given that,
we have already branched, I think this is the simplest fix.

(1) We never added F16 support.  Enable F16.  This involves assigning
    color spaces to src and dst pixmaps so that the conversion makes
    sense.
(2) SkPixmap::readPixels() does not support parametric transfer
    functions.  Fortunately, the framework never uses copyTo() to
    do color space conversions.  We should at least support the
    case where the src color space matches the dst color space.

Bug: skia:
Change-Id: I129b7c02249eed34a9ec4aa0ca736aadccc19777
Reviewed-on: https://skia-review.googlesource.com/11387
Commit-Queue: Matt Sarett <msarett@google.com>
Reviewed-by: Mike Klein <mtklein@chromium.org>

diff --git a/src/core/SkBitmap.cpp b/src/core/SkBitmap.cpp
index 1516505..0e22e30 100644 (file)
--- a/src/core/SkBitmap.cpp
+++ b/src/core/SkBitmap.cpp
@@ -675,6 +675,7 @@ bool SkBitmap::canCopyTo(SkColorType dstCT) const {
         case kRGB_565_SkColorType:
         case kRGBA_8888_SkColorType:
         case kBGRA_8888_SkColorType:
+        case kRGBA_F16_SkColorType:
             break;
         case kGray_8_SkColorType:
             if (!sameConfigs) {
@@ -735,14 +736,36 @@ bool SkBitmap::copyTo(SkBitmap* dst, SkColorType dstColorType, Allocator* alloc)
         return false;
     }
     SkPixmap srcPM = srcUnlocker.pixmap();
-    if (kRGB_565_SkColorType == dstColorType && kOpaque_SkAlphaType != srcPM.alphaType()) {
-        // copyTo() is not strict on alpha type.  Here we set the src to opaque to allow
-        // the call to readPixels() to succeed and preserve this lenient behavior.
-        srcPM = SkPixmap(srcPM.info().makeAlphaType(kOpaque_SkAlphaType), srcPM.addr(),
-                         srcPM.rowBytes(), srcPM.ctable());
+
+    // Various Android specific compatibility modes.
+    // TODO:
+    // Move the logic of this entire function into the framework, then call readPixels() directly.
+    SkImageInfo dstInfo = srcPM.info().makeColorType(dstColorType);
+    switch (dstColorType) {
+        case kRGB_565_SkColorType:
+            // copyTo() is not strict on alpha type.  Here we set the src to opaque to allow
+            // the call to readPixels() to succeed and preserve this lenient behavior.
+            if (kOpaque_SkAlphaType != srcPM.alphaType()) {
+                srcPM = SkPixmap(srcPM.info().makeAlphaType(kOpaque_SkAlphaType), srcPM.addr(),
+                                 srcPM.rowBytes(), srcPM.ctable());
+                dstInfo = dstInfo.makeAlphaType(kOpaque_SkAlphaType);
+            }
+            break;
+        case kRGBA_F16_SkColorType:
+            // The caller does not have an opportunity to pass a dst color space.  Assume that
+            // they want linear sRGB.
+            dstInfo = dstInfo.makeColorSpace(SkColorSpace::MakeSRGBLinear());
+
+            if (!srcPM.colorSpace()) {
+                // We can't do a sane conversion to F16 without a dst color space.  Guess sRGB
+                // in this case.
+                srcPM.setColorSpace(SkColorSpace::MakeSRGB());
+            }
+            break;
+        default:
+            break;
     }
 
-    const SkImageInfo dstInfo = srcPM.info().makeColorType(dstColorType);
     SkBitmap tmpDst;
     if (!tmpDst.setInfo(dstInfo)) {
         return false;
@@ -762,7 +785,22 @@ bool SkBitmap::copyTo(SkBitmap* dst, SkColorType dstColorType, Allocator* alloc)
         return false;
     }
 
-    if (!srcPM.readPixels(dstUnlocker.pixmap())) {
+    SkPixmap dstPM = dstUnlocker.pixmap();
+
+    // We can't do a sane conversion from F16 without a src color space.  Guess sRGB in this case.
+    if (kRGBA_F16_SkColorType == srcPM.colorType() && !dstPM.colorSpace()) {
+        dstPM.setColorSpace(SkColorSpace::MakeSRGB());
+    }
+
+    // readPixels does not yet support color spaces with parametric transfer functions.  This
+    // works around that restriction when the color spaces are equal.
+    if (kRGBA_F16_SkColorType != dstColorType && kRGBA_F16_SkColorType != srcPM.colorType() &&
+            dstPM.colorSpace() == srcPM.colorSpace()) {
+        dstPM.setColorSpace(nullptr);
+        srcPM.setColorSpace(nullptr);
+    }
+
+    if (!srcPM.readPixels(dstPM)) {
         return false;
     }
 

diff --git a/tests/BitmapCopyTest.cpp b/tests/BitmapCopyTest.cpp
index 4578bbb..71b9a60 100644 (file)
--- a/tests/BitmapCopyTest.cpp
+++ b/tests/BitmapCopyTest.cpp
@@ -14,16 +14,35 @@ static const char* boolStr(bool value) {
     return value ? "true" : "false";
 }
 
-// these are in the same order as the SkColorType enum
-static const char* gColorTypeName[] = {
-    "None", "A8", "565", "4444", "RGBA", "BGRA", "Index8"
-};
+static const char* color_type_name(SkColorType colorType) {
+    switch (colorType) {
+        case kUnknown_SkColorType:
+            return "None";
+        case kAlpha_8_SkColorType:
+            return "A8";
+        case kRGB_565_SkColorType:
+            return "565";
+        case kARGB_4444_SkColorType:
+            return "4444";
+        case kRGBA_8888_SkColorType:
+            return "RGBA";
+        case kBGRA_8888_SkColorType:
+            return "BGRA";
+        case kIndex_8_SkColorType:
+            return "Index8";
+        case kGray_8_SkColorType:
+            return "Gray8";
+        case kRGBA_F16_SkColorType:
+            return "F16";
+    }
+    return "";
+}
 
 static void report_opaqueness(skiatest::Reporter* reporter, const SkBitmap& src,
                               const SkBitmap& dst) {
     ERRORF(reporter, "src %s opaque:%d, dst %s opaque:%d",
-           gColorTypeName[src.colorType()], src.isOpaque(),
-           gColorTypeName[dst.colorType()], dst.isOpaque());
+           color_type_name(src.colorType()), src.isOpaque(),
+           color_type_name(dst.colorType()), dst.isOpaque());
 }
 
 static bool canHaveAlpha(SkColorType ct) {
@@ -168,7 +187,7 @@ static void reportCopyVerification(const SkBitmap& bm1, const SkBitmap& bm2,
         uint32_t p2 = getPixel(coords[i]->fX, coords[i]->fY, bm2);
 //        SkDebugf("[%d] (%d %d) p1=%x p2=%x\n", i, coords[i]->fX, coords[i]->fY, p1, p2);
         if (p1 != p2) {
-            ERRORF(reporter, "%s [colortype = %s]", msg, gColorTypeName[bm1.colorType()]);
+            ERRORF(reporter, "%s [colortype = %s]", msg, color_type_name(bm1.colorType()));
             break;
         }
     }
@@ -181,12 +200,13 @@ static void writeCoordPixels(SkBitmap& bm, const Coordinates& coords) {
 }
 
 static const Pair gPairs[] = {
-    { kUnknown_SkColorType,     "000000"  },
-    { kAlpha_8_SkColorType,     "010000"  },
-    { kIndex_8_SkColorType,     "010111"  },
-    { kRGB_565_SkColorType,     "010101"  },
-    { kARGB_4444_SkColorType,   "010111"  },
-    { kN32_SkColorType,         "010111"  },
+    { kUnknown_SkColorType,     "0000000"  },
+    { kAlpha_8_SkColorType,     "0100000"  },
+    { kIndex_8_SkColorType,     "0101111"  },
+    { kRGB_565_SkColorType,     "0101011"  },
+    { kARGB_4444_SkColorType,   "0101111"  },
+    { kN32_SkColorType,         "0101111"  },
+    { kRGBA_F16_SkColorType,    "0101011"  },
 };
 
 static const int W = 20;
@@ -199,8 +219,13 @@ static void setup_src_bitmaps(SkBitmap* srcOpaque, SkBitmap* srcPremul,
         ctable = init_ctable();
     }
 
-    srcOpaque->allocPixels(SkImageInfo::Make(W, H, ct, kOpaque_SkAlphaType), ctable);
-    srcPremul->allocPixels(SkImageInfo::Make(W, H, ct, kPremul_SkAlphaType), ctable);
+    sk_sp<SkColorSpace> colorSpace = nullptr;
+    if (kRGBA_F16_SkColorType == ct) {
+        colorSpace = SkColorSpace::MakeSRGBLinear();
+    }
+
+    srcOpaque->allocPixels(SkImageInfo::Make(W, H, ct, kOpaque_SkAlphaType, colorSpace), ctable);
+    srcPremul->allocPixels(SkImageInfo::Make(W, H, ct, kPremul_SkAlphaType, colorSpace), ctable);
     init_src(*srcOpaque);
     init_src(*srcPremul);
 }
@@ -278,14 +303,16 @@ DEF_TEST(BitmapCopy, reporter) {
             bool expected = gPairs[i].fValid[j] != '0';
             if (success != expected) {
                 ERRORF(reporter, "SkBitmap::copyTo from %s to %s. expected %s "
-                       "returned %s", gColorTypeName[i], gColorTypeName[j],
+                       "returned %s", color_type_name(gPairs[i].fColorType),
+                       color_type_name(gPairs[j].fColorType),
                        boolStr(expected), boolStr(success));
             }
 
             bool canSucceed = srcPremul.canCopyTo(gPairs[j].fColorType);
             if (success != canSucceed) {
                 ERRORF(reporter, "SkBitmap::copyTo from %s to %s. returned %s "
-                       "canCopyTo %s", gColorTypeName[i], gColorTypeName[j],
+                       "canCopyTo %s", color_type_name(gPairs[i].fColorType),
+                       color_type_name(gPairs[j].fColorType),
                        boolStr(success), boolStr(canSucceed));
             }
 
@@ -334,7 +361,7 @@ DEF_TEST(BitmapCopy, reporter) {
             int64_t safeSize = tstSafeSize.computeSafeSize64();
             if (safeSize < 0) {
                 ERRORF(reporter, "getSafeSize64() negative: %s",
-                       gColorTypeName[tstSafeSize.colorType()]);
+                       color_type_name(tstSafeSize.colorType()));
             }
             bool sizeFail = false;
             // Compare against hand-computed values.
@@ -367,7 +394,7 @@ DEF_TEST(BitmapCopy, reporter) {
             }
             if (sizeFail) {
                 ERRORF(reporter, "computeSafeSize64() wrong size: %s",
-                       gColorTypeName[tstSafeSize.colorType()]);
+                       color_type_name(tstSafeSize.colorType()));
             }
 
             int subW = 2;
@@ -629,3 +656,19 @@ DEF_TEST(BitmapReadPixels, reporter) {
     }
 }
 
+DEF_TEST(BitmapCopy_ColorSpaceMatch, r) {
+    // We should support matching color spaces, even if they are parametric.
+    SkColorSpaceTransferFn fn;
+    fn.fA = 1.f; fn.fB = 0.f; fn.fC = 0.f; fn.fD = 0.f; fn.fF = 0.f; fn.fG = 1.8f;
+    sk_sp<SkColorSpace> cs = SkColorSpace::MakeRGB(fn, SkColorSpace::kRec2020_Gamut);
+
+    SkImageInfo info = SkImageInfo::MakeN32Premul(1, 1, cs);
+    SkBitmap bitmap;
+    bitmap.allocPixels(info);
+    bitmap.eraseColor(0);
+
+    SkBitmap copy;
+    bool success = bitmap.copyTo(&copy, kN32_SkColorType, nullptr);
+    REPORTER_ASSERT(r, success);
+    REPORTER_ASSERT(r, cs.get() == copy.colorSpace());
+}