commit efcc41805b43347444b83c1705d3d60c8d0caa70
author Brian Osman <brianosman@google.com> Thu Jan 05 10:41:39 2017 -0500
committer Skia Commit-Bot <skia-commit-bot@chromium.org> Thu Jan 05 18:07:55 2017 +0000
tree 705426a2eec1856364552d03c3648c319fe9e0bc
parent 08d57e6ae6510a7724119ab548485b4fcbd0a48d

Explicitly fail read/writePixels in invalid color space scenarios

It's not well defined what to do when moving from a nullptr color space to
a tagged destination (drawing, reading, writing, etc...). In these
scenarios, at least, we can choose to disallow the operation (rather than
produce an unexpected or inconsistent result).

BUG=skia:

Change-Id: I033b23c6f2bb00664efc8fdab1b3f52053d77695
Reviewed-on: https://skia-review.googlesource.com/6600
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Salomon <bsalomon@google.com>
Reviewed-by: Matt Sarett <msarett@google.com>

src/gpu/GrContext.cpp

diff --git a/src/gpu/GrContext.cpp b/src/gpu/GrContext.cpp
index 30f596f..310f455 100644
--- a/src/gpu/GrContext.cpp
+++ b/src/gpu/GrContext.cpp
@@ -278,6 +278,11 @@
         return false;
     }
 
+    // We don't allow writing to a color space tagged destination if the source isn't tagged
+    if (dstColorSpace && !srcColorSpace) {
+        return false;
+    }
+
     GrGpu::DrawPreference drawPreference = GrGpu::kNoDraw_DrawPreference;
     // Don't prefer to draw for the conversion (and thereby access a texture from the cache) when
     // we've already determined that there isn't a roundtrip preserving conversion processor pair.
@@ -427,6 +432,11 @@
         return false;
     }
 
+    // We don't allow reading to a color space tagged destination if the source isn't tagged
+    if (dstColorSpace && !srcColorSpace) {
+        return false;
+    }
+
     GrGpu::DrawPreference drawPreference = GrGpu::kNoDraw_DrawPreference;
     // Don't prefer to draw for the conversion (and thereby access a texture from the cache) when
     // we've already determined that there isn't a roundtrip preserving conversion processor pair.

tests/ImageTest.cpp

diff --git a/tests/ImageTest.cpp b/tests/ImageTest.cpp
index 161dff8..b2ac36d 100644
--- a/tests/ImageTest.cpp
+++ b/tests/ImageTest.cpp
@@ -45,8 +45,7 @@
     // see https://bug.skia.org/3965
     //REPORTER_ASSERT(reporter, a->isOpaque() == b->isOpaque());
 
-    // The codecs may have given us back F16, we can't read from F16 raster to N32, only S32.
-    SkImageInfo info = SkImageInfo::MakeS32(widthA, heightA, a->alphaType());
+    SkImageInfo info = SkImageInfo::MakeN32(widthA, heightA, a->alphaType());
     SkAutoPixmapStorage pmapA, pmapB;
     pmapA.alloc(info);
     pmapB.alloc(info);

tests/ReadPixelsTest.cpp

diff --git a/tests/ReadPixelsTest.cpp b/tests/ReadPixelsTest.cpp
index 71cd8f5..487152c 100644
--- a/tests/ReadPixelsTest.cpp
+++ b/tests/ReadPixelsTest.cpp
@@ -487,3 +487,40 @@
     }
 }
 #endif
+
+#if SK_SUPPORT_GPU
+DEF_GPUTEST_FOR_RENDERING_CONTEXTS(ReadPixelsColorSpaceVariants_Gpu, reporter, ctxInfo) {
+    // Create surfaces with and without an attached color space
+    sk_sp<SkColorSpace> srgbColorSpace = SkColorSpace::MakeNamed(SkColorSpace::kSRGB_Named);
+    SkImageInfo srgbInfo = SkImageInfo::MakeS32(DEV_W, DEV_H, kPremul_SkAlphaType);
+    SkImageInfo legacyInfo = srgbInfo.makeColorSpace(nullptr);
+
+    sk_sp<SkSurface> srgbSurface = SkSurface::MakeRenderTarget(ctxInfo.grContext(), SkBudgeted::kNo,
+                                                               srgbInfo);
+    sk_sp<SkSurface> legacySurface = SkSurface::MakeRenderTarget(ctxInfo.grContext(),
+                                                                 SkBudgeted::kNo, legacyInfo);
+    SkCanvas* srgbCanvas = srgbSurface->getCanvas();
+    SkCanvas* legacyCanvas = legacySurface->getCanvas();
+
+    struct {
+        SkCanvas* fCanvas;
+        const SkImageInfo& fBmpInfo;
+        bool fExpectSuccess;
+    } kTestConfigs[] ={
+        // Both kinds of surface should be able to read into a legacy destination
+        { srgbCanvas, legacyInfo, true },
+        { legacyCanvas, legacyInfo, true },
+        // Tagged surface should be able to read into tagged destination
+        { srgbCanvas, srgbInfo, true },
+        // Legacy surface shouldn't read into tagged destination
+        { legacyCanvas, srgbInfo, false },
+    };
+
+    for (auto testConfig : kTestConfigs) {
+        SkBitmap bmp;
+        bmp.setInfo(testConfig.fBmpInfo);
+        bool result = testConfig.fCanvas->readPixels(&bmp, 0, 0);
+        REPORTER_ASSERT(reporter, result == testConfig.fExpectSuccess);
+    }
+}
+#endif