Make it so that GrSurfaceContext with a sRGB GrPixelConfig must have a

color space with a sRGB-like gamma.

Change-Id: I99b80a9846caacd6848b0f9f55ed0f7f23e69b90
Reviewed-on: https://skia-review.googlesource.com/106640
Commit-Queue: Brian Salomon <bsalomon@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>

15 files changed, 109 insertions, 50 deletions

diff --git a/src/core/SkSpecialImage.cpp b/src/core/SkSpecialImage.cpp
index f6e4224452..61acd3fc69 100644
--- a/src/core/SkSpecialImage.cpp
+++ b/src/core/SkSpecialImage.cpp
@@ -409,9 +409,12 @@ public:
         if (!rec) {
             return false;
         }
-
+        sk_sp<SkColorSpace> colorSpace;
+        if (GrPixelConfigIsSRGB(fTextureProxy->config())) {
+            colorSpace = SkColorSpace::MakeSRGB();
+        }
         sk_sp<GrSurfaceContext> sContext = fContext->contextPriv().makeWrappedSurfaceContext(
-                                                                                fTextureProxy);
+                fTextureProxy, std::move(colorSpace));
         if (!sContext) {
             return false;
         }
diff --git a/src/gpu/GrBackendTextureImageGenerator.cpp b/src/gpu/GrBackendTextureImageGenerator.cpp
index af28409cfe..b8d2474f7e 100644
--- a/src/gpu/GrBackendTextureImageGenerator.cpp
+++ b/src/gpu/GrBackendTextureImageGenerator.cpp
@@ -184,10 +184,14 @@ sk_sp<GrTextureProxy> GrBackendTextureImageGenerator::onGenerateTexture(
         // because Vulkan will want to do the copy as a draw. All other copies would require a
         // layout change in Vulkan and we do not change the layout of borrowed images.
         GrMipMapped mipMapped = willNeedMipMaps ? GrMipMapped::kYes : GrMipMapped::kNo;
+        sk_sp<SkColorSpace> colorSpace;
+        if (GrPixelConfigIsSRGB(desc.fConfig)) {
+            colorSpace = SkColorSpace::MakeSRGB();
+        }
 
         sk_sp<GrRenderTargetContext> rtContext(context->makeDeferredRenderTargetContext(
-                SkBackingFit::kExact, info.width(), info.height(), proxy->config(), nullptr, 1,
-                mipMapped, proxy->origin(), nullptr, SkBudgeted::kYes));
+                SkBackingFit::kExact, info.width(), info.height(), proxy->config(),
+                std::move(colorSpace), 1, mipMapped, proxy->origin(), nullptr, SkBudgeted::kYes));
 
         if (!rtContext) {
             return nullptr;
diff --git a/src/gpu/GrContext.cpp b/src/gpu/GrContext.cpp
index 50e466f8f1..1700c1be1d 100644
--- a/src/gpu/GrContext.cpp
+++ b/src/gpu/GrContext.cpp
@@ -707,16 +707,22 @@ bool GrContextPriv::readSurfacePixels(GrSurfaceContext* src,
         }
         // TODO: Need to decide the semantics of this function for color spaces. Do we support
         // conversion to a passed-in color space? For now, specifying nullptr means that this
-        // path will do no conversion, so it will match the behavior of the non-draw path.
-        sk_sp<GrRenderTargetContext> tempRTC = fContext->makeDeferredRenderTargetContext(
-                                                           tempDrawInfo.fTempSurfaceFit,
-                                                           tempDrawInfo.fTempSurfaceDesc.fWidth,
-                                                           tempDrawInfo.fTempSurfaceDesc.fHeight,
-                                                           tempDrawInfo.fTempSurfaceDesc.fConfig,
-                                                           nullptr,
-                                                           tempDrawInfo.fTempSurfaceDesc.fSampleCnt,
-                                                           GrMipMapped::kNo,
-                                                           tempDrawInfo.fTempSurfaceDesc.fOrigin);
+        // path will do no conversion, so it will match the behavior of the non-draw path. For
+        // now we simply infer an sRGB color space if the config is sRGB in order to avoid an
+        // illegal combination.
+        sk_sp<SkColorSpace> colorSpace;
+        if (GrPixelConfigIsSRGB(tempDrawInfo.fTempSurfaceDesc.fConfig)) {
+            colorSpace = SkColorSpace::MakeSRGB();
+        }
+        sk_sp<GrRenderTargetContext> tempRTC =
+                fContext->makeDeferredRenderTargetContext(tempDrawInfo.fTempSurfaceFit,
+                                                          tempDrawInfo.fTempSurfaceDesc.fWidth,
+                                                          tempDrawInfo.fTempSurfaceDesc.fHeight,
+                                                          tempDrawInfo.fTempSurfaceDesc.fConfig,
+                                                          std::move(colorSpace),
+                                                          tempDrawInfo.fTempSurfaceDesc.fSampleCnt,
+                                                          GrMipMapped::kNo,
+                                                          tempDrawInfo.fTempSurfaceDesc.fOrigin);
         if (tempRTC) {
             SkMatrix textureMatrix = SkMatrix::MakeTrans(SkIntToScalar(left), SkIntToScalar(top));
             sk_sp<GrTextureProxy> proxy = src->asTextureProxyRef();
@@ -821,6 +827,12 @@ sk_sp<GrSurfaceContext> GrContextPriv::makeWrappedSurfaceContext(sk_sp<GrSurface
                                                                  const SkSurfaceProps* props) {
     ASSERT_SINGLE_OWNER_PRIV
 
+    // sRGB pixel configs may only be used with near-sRGB gamma color spaces.
+    if (GrPixelConfigIsSRGB(proxy->config())) {
+        if (!colorSpace || !colorSpace->gammaCloseToSRGB()) {
+            return nullptr;
+        }
+    }
     if (proxy->asRenderTargetProxy()) {
         return this->drawingManager()->makeRenderTargetContext(std::move(proxy),
                                                                std::move(colorSpace), props);
@@ -834,8 +846,9 @@ sk_sp<GrSurfaceContext> GrContextPriv::makeWrappedSurfaceContext(sk_sp<GrSurface
 sk_sp<GrSurfaceContext> GrContextPriv::makeDeferredSurfaceContext(const GrSurfaceDesc& dstDesc,
                                                                   GrMipMapped mipMapped,
                                                                   SkBackingFit fit,
-                                                                  SkBudgeted isDstBudgeted) {
-
+                                                                  SkBudgeted isDstBudgeted,
+                                                                  sk_sp<SkColorSpace> colorSpace,
+                                                                  const SkSurfaceProps* props) {
     sk_sp<GrTextureProxy> proxy;
     if (GrMipMapped::kNo == mipMapped) {
         proxy = this->proxyProvider()->createProxy(dstDesc, fit, isDstBudgeted);
@@ -847,7 +860,7 @@ sk_sp<GrSurfaceContext> GrContextPriv::makeDeferredSurfaceContext(const GrSurfac
         return nullptr;
     }
 
-    return this->makeWrappedSurfaceContext(std::move(proxy));
+    return this->makeWrappedSurfaceContext(std::move(proxy), std::move(colorSpace), props);
 }
 
 sk_sp<GrTextureContext> GrContextPriv::makeBackendTextureContext(const GrBackendTexture& tex,
diff --git a/src/gpu/GrContextPriv.h b/src/gpu/GrContextPriv.h
index bd0931cc08..e0074252d8 100644
--- a/src/gpu/GrContextPriv.h
+++ b/src/gpu/GrContextPriv.h
@@ -38,7 +38,9 @@ public:
     sk_sp<GrSurfaceContext> makeDeferredSurfaceContext(const GrSurfaceDesc&,
                                                        GrMipMapped,
                                                        SkBackingFit,
-                                                       SkBudgeted);
+                                                       SkBudgeted,
+                                                       sk_sp<SkColorSpace> colorSpace = nullptr,
+                                                       const SkSurfaceProps* = nullptr);
 
     sk_sp<GrTextureContext> makeBackendTextureContext(const GrBackendTexture& tex,
                                                       GrSurfaceOrigin origin,
diff --git a/src/gpu/GrDrawingManager.cpp b/src/gpu/GrDrawingManager.cpp
index 52bad77d53..de6722f998 100644
--- a/src/gpu/GrDrawingManager.cpp
+++ b/src/gpu/GrDrawingManager.cpp
@@ -474,9 +474,8 @@ sk_sp<GrRenderTargetContext> GrDrawingManager::makeRenderTargetContext(
     }
 
     // SkSurface catches bad color space usage at creation. This check handles anything that slips
-    // by, including internal usage. We allow a null color space here, for read/write pixels and
-    // other special code paths. If a color space is provided, though, enforce all other rules.
-    if (colorSpace && !SkSurface_Gpu::Valid(fContext, sProxy->config(), colorSpace.get())) {
+    // by, including internal usage.
+    if (!SkSurface_Gpu::Valid(fContext, sProxy->config(), colorSpace.get())) {
         SkDEBUGFAIL("Invalid config and colorspace combination");
         return nullptr;
     }
@@ -497,9 +496,8 @@ sk_sp<GrTextureContext> GrDrawingManager::makeTextureContext(sk_sp<GrSurfaceProx
     }
 
     // SkSurface catches bad color space usage at creation. This check handles anything that slips
-    // by, including internal usage. We allow a null color space here, for read/write pixels and
-    // other special code paths. If a color space is provided, though, enforce all other rules.
-    if (colorSpace && !SkSurface_Gpu::Valid(fContext, sProxy->config(), colorSpace.get())) {
+    // by, including internal usage.
+    if (!SkSurface_Gpu::Valid(fContext, sProxy->config(), colorSpace.get())) {
         SkDEBUGFAIL("Invalid config and colorspace combination");
         return nullptr;
     }
diff --git a/src/gpu/GrResourceProvider.cpp b/src/gpu/GrResourceProvider.cpp
index 13c326a96a..df09e27d7e 100644
--- a/src/gpu/GrResourceProvider.cpp
+++ b/src/gpu/GrResourceProvider.cpp
@@ -135,8 +135,17 @@ sk_sp<GrTexture> GrResourceProvider::createTexture(const GrSurfaceDesc& desc,
                                                                              fit,
                                                                              budgeted);
         if (proxy) {
+            // We use an ephemeral surface context to do the write pixels. Here it isn't clear what
+            // color space to tag it with. That's ok because GrSurfaceContext::writePixels doesn't
+            // do any color space conversions. Though, that is likely to change. However, if the
+            // pixel config is sRGB then the passed color space here must have sRGB gamma or
+            // GrSurfaceContext creation fails.
+            sk_sp<SkColorSpace> colorSpace;
+            if (GrPixelConfigIsSRGB(desc.fConfig)) {
+                colorSpace = SkColorSpace::MakeSRGB();
+            }
             sk_sp<GrSurfaceContext> sContext = context->contextPriv().makeWrappedSurfaceContext(
-                                                                                std::move(proxy));
+                    std::move(proxy), std::move(colorSpace));
             if (sContext) {
                 if (sContext->writePixels(srcInfo, mipLevel.fPixels, mipLevel.fRowBytes, 0, 0)) {
                     return sk_ref_sp(sContext->asTextureProxy()->priv().peekTexture());
diff --git a/src/gpu/GrSurfaceContext.cpp b/src/gpu/GrSurfaceContext.cpp
index 342c77e5bc..8b6e281e47 100644
--- a/src/gpu/GrSurfaceContext.cpp
+++ b/src/gpu/GrSurfaceContext.cpp
@@ -36,6 +36,9 @@ GrSurfaceContext::GrSurfaceContext(GrContext* context,
         , fSingleOwner(singleOwner)
 #endif
 {
+    // We never should have a sRGB pixel config with a non-SRGB gamma color space.
+    SkASSERT(!GrPixelConfigIsSRGB(config) ||
+             (fColorSpaceInfo.colorSpace() && fColorSpaceInfo.colorSpace()->gammaCloseToSRGB()));
 }
 
 bool GrSurfaceContext::readPixels(const SkImageInfo& dstInfo, void* dstBuffer,
diff --git a/src/gpu/GrSurfaceProxy.cpp b/src/gpu/GrSurfaceProxy.cpp
index e311a820e1..6c81fc0bec 100644
--- a/src/gpu/GrSurfaceProxy.cpp
+++ b/src/gpu/GrSurfaceProxy.cpp
@@ -275,11 +275,17 @@ sk_sp<GrTextureProxy> GrSurfaceProxy::Copy(GrContext* context,
     dstDesc.fHeight = srcRect.height();
     dstDesc.fConfig = src->config();
 
+    // We use an ephemeral surface context to make the copy. Here it isn't clear what color space
+    // to tag it with. That's ok because GrSurfaceContext::copy doesn't do any color space
+    // conversions. However, if the pixel config is sRGB then the passed color space here must
+    // have sRGB gamma or GrSurfaceContext creation fails. See skbug.com/7611 about making this
+    // with the correct color space information and returning the context to the caller.
+    sk_sp<SkColorSpace> colorSpace;
+    if (GrPixelConfigIsSRGB(dstDesc.fConfig)) {
+        colorSpace = SkColorSpace::MakeSRGB();
+    }
     sk_sp<GrSurfaceContext> dstContext(context->contextPriv().makeDeferredSurfaceContext(
-                                                                            dstDesc,
-                                                                            mipMapped,
-                                                                            SkBackingFit::kExact,
-                                                                            budgeted));
+            dstDesc, mipMapped, SkBackingFit::kExact, budgeted, std::move(colorSpace)));
     if (!dstContext) {
         return nullptr;
     }
diff --git a/src/gpu/GrTextureProducer.cpp b/src/gpu/GrTextureProducer.cpp
index 9b344cb831..f1c8c8dbd0 100644
--- a/src/gpu/GrTextureProducer.cpp
+++ b/src/gpu/GrTextureProducer.cpp
@@ -24,9 +24,13 @@ sk_sp<GrTextureProxy> GrTextureProducer::CopyOnGpu(GrContext* context,
     const SkRect dstRect = SkRect::MakeIWH(copyParams.fWidth, copyParams.fHeight);
     GrMipMapped mipMapped = dstWillRequireMipMaps ? GrMipMapped::kYes : GrMipMapped::kNo;
 
+    sk_sp<SkColorSpace> colorSpace;
+    if (GrPixelConfigIsSRGB(inputProxy->config())) {
+        colorSpace = SkColorSpace::MakeSRGB();
+    }
     sk_sp<GrRenderTargetContext> copyRTC = context->makeDeferredRenderTargetContextWithFallback(
-            SkBackingFit::kExact, dstRect.width(), dstRect.height(), inputProxy->config(), nullptr,
-            1, mipMapped, inputProxy->origin());
+            SkBackingFit::kExact, dstRect.width(), dstRect.height(), inputProxy->config(),
+            std::move(colorSpace), 1, mipMapped, inputProxy->origin());
     if (!copyRTC) {
         return nullptr;
     }
diff --git a/src/gpu/GrYUVProvider.cpp b/src/gpu/GrYUVProvider.cpp
index 47d27c45a3..01d46f69e4 100644
--- a/src/gpu/GrYUVProvider.cpp
+++ b/src/gpu/GrYUVProvider.cpp
@@ -109,15 +109,16 @@ sk_sp<GrTextureProxy> GrYUVProvider::refAsTextureProxy(GrContext* ctx, const GrS
                                                                  1, SkBudgeted::kYes, fit);
     }
 
-    // We never want to perform color-space conversion during the decode
+    // We never want to perform color-space conversion during the decode. However, if the proxy
+    // config is sRGB then we must use a sRGB color space.
+    sk_sp<SkColorSpace> colorSpace;
+    if (GrPixelConfigIsSRGB(desc.fConfig)) {
+        colorSpace = SkColorSpace::MakeSRGB();
+    }
     // TODO: investigate preallocating mip maps here
     sk_sp<GrRenderTargetContext> renderTargetContext(ctx->makeDeferredRenderTargetContext(
-                                                                    SkBackingFit::kExact,
-                                                                    desc.fWidth, desc.fHeight,
-                                                                    desc.fConfig, nullptr,
-                                                                    desc.fSampleCnt,
-                                                                    GrMipMapped::kNo,
-                                                                    kTopLeft_GrSurfaceOrigin));
+            SkBackingFit::kExact, desc.fWidth, desc.fHeight, desc.fConfig, std::move(colorSpace),
+            desc.fSampleCnt, GrMipMapped::kNo, kTopLeft_GrSurfaceOrigin));
     if (!renderTargetContext) {
         return nullptr;
     }
diff --git a/src/gpu/SkGr.cpp b/src/gpu/SkGr.cpp
index 32907ae274..12e7670029 100644
--- a/src/gpu/SkGr.cpp
+++ b/src/gpu/SkGr.cpp
@@ -131,7 +131,12 @@ sk_sp<GrTextureProxy> GrCopyBaseMipMapToTextureProxy(GrContext* ctx, GrTexturePr
     }
 
     // Copy the base layer to our proxy
-    sk_sp<GrSurfaceContext> sContext = ctx->contextPriv().makeWrappedSurfaceContext(proxy);
+    sk_sp<SkColorSpace> colorSpace;
+    if (GrPixelConfigIsSRGB(proxy->config())) {
+        colorSpace = SkColorSpace::MakeSRGB();
+    }
+    sk_sp<GrSurfaceContext> sContext =
+            ctx->contextPriv().makeWrappedSurfaceContext(proxy, std::move(colorSpace));
     SkASSERT(sContext);
     SkAssertResult(sContext->copy(baseProxy));
 
diff --git a/src/image/SkImage_Gpu.cpp b/src/image/SkImage_Gpu.cpp
index ce3f6feb86..ad89e926f8 100644
--- a/src/image/SkImage_Gpu.cpp
+++ b/src/image/SkImage_Gpu.cpp
@@ -230,12 +230,18 @@ bool SkImage_Gpu::onReadPixels(const SkImageInfo& dstInfo, void* dstPixels, size
     // with arbitrary color spaces. Unfortunately, this is one spot where we go from image to
     // surface (rather than the opposite), and our lenient image rules break our (currently) more
     // strict surface rules.
-    // We treat null-dst color space as always equal to fColorSpace for this kind of read-back.
+    // GrSurfaceContext::readPixels does not make use of the context's color space. However, we
+    // don't allow creating a surface context for a sRGB GrPixelConfig unless the color space has
+    // sRGB gamma. So we choose null for non-SRGB GrPixelConfigs and sRGB for sRGB GrPixelConfigs.
     sk_sp<SkColorSpace> surfaceColorSpace = fColorSpace;
     if (!flags) {
         if (!dstInfo.colorSpace() ||
-                SkColorSpace::Equals(fColorSpace.get(), dstInfo.colorSpace())) {
-            surfaceColorSpace = nullptr;
+            SkColorSpace::Equals(fColorSpace.get(), dstInfo.colorSpace())) {
+            if (GrPixelConfigIsSRGB(fProxy->config())) {
+                surfaceColorSpace = SkColorSpace::MakeSRGB();
+            } else {
+                surfaceColorSpace = nullptr;
+            }
         }
     }
 
diff --git a/tests/GrSurfaceTest.cpp b/tests/GrSurfaceTest.cpp
index 1a8072f3b0..eb975852d1 100644
--- a/tests/GrSurfaceTest.cpp
+++ b/tests/GrSurfaceTest.cpp
@@ -154,6 +154,10 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(InitialTextureClear, reporter, context_info)
 
     for (int c = 0; c <= kLast_GrPixelConfig; ++c) {
         desc.fConfig = static_cast<GrPixelConfig>(c);
+        sk_sp<SkColorSpace> colorSpace;
+        if (GrPixelConfigIsSRGB(desc.fConfig)) {
+            colorSpace = SkColorSpace::MakeSRGB();
+        }
         if (!context_info.grContext()->caps()->isConfigTexturable(desc.fConfig)) {
             continue;
         }
@@ -175,9 +179,8 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(InitialTextureClear, reporter, context_info)
                         if (!proxy) {
                             continue;
                         }
-
                         auto texCtx = context->contextPriv().makeWrappedSurfaceContext(
-                                                                                std::move(proxy));
+                                std::move(proxy), colorSpace);
                         SkImageInfo info = SkImageInfo::Make(
                                 kSize, kSize, kRGBA_8888_SkColorType, kPremul_SkAlphaType);
                         memset(data.get(), 0xAB, kSize * kSize * sizeof(uint32_t));
@@ -202,7 +205,7 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(InitialTextureClear, reporter, context_info)
                     // Try creating the texture as a deferred proxy.
                     for (int i = 0; i < 2; ++i) {
                         auto surfCtx = context->contextPriv().makeDeferredSurfaceContext(
-                                desc, GrMipMapped::kNo, fit, SkBudgeted::kYes);
+                                desc, GrMipMapped::kNo, fit, SkBudgeted::kYes, colorSpace);
                         if (!surfCtx) {
                             continue;
                         }
diff --git a/tests/ReadWriteAlphaTest.cpp b/tests/ReadWriteAlphaTest.cpp
index e6c8c89e7d..68a3ee0983 100644
--- a/tests/ReadWriteAlphaTest.cpp
+++ b/tests/ReadWriteAlphaTest.cpp
@@ -178,8 +178,12 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(ReadWriteAlpha, reporter, ctxInfo) {
                 continue;
             }
 
+            sk_sp<SkColorSpace> colorSpace;
+            if (GrPixelConfigIsSRGB(proxy->config())) {
+                colorSpace = SkColorSpace::MakeSRGB();
+            }
             sk_sp<GrSurfaceContext> sContext = context->contextPriv().makeWrappedSurfaceContext(
-                                                                       std::move(proxy));
+                    std::move(proxy), std::move(colorSpace));
 
             for (auto rowBytes : kRowBytes) {
                 size_t nonZeroRowBytes = rowBytes ? rowBytes : X_SIZE;
diff --git a/tests/SRGBReadWritePixelsTest.cpp b/tests/SRGBReadWritePixelsTest.cpp
index bb7b77c5b9..a177f72ee5 100644
--- a/tests/SRGBReadWritePixelsTest.cpp
+++ b/tests/SRGBReadWritePixelsTest.cpp
@@ -174,11 +174,9 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(SRGBReadWritePixels, reporter, ctxInfo) {
     desc.fConfig = kSRGBA_8888_GrPixelConfig;
     if (context->caps()->isConfigRenderable(desc.fConfig) &&
         context->caps()->isConfigTexturable(desc.fConfig)) {
-
         sk_sp<GrSurfaceContext> sContext = context->contextPriv().makeDeferredSurfaceContext(
-                                                                             desc, GrMipMapped::kNo,
-                                                                             SkBackingFit::kExact,
-                                                                             SkBudgeted::kNo);
+                desc, GrMipMapped::kNo, SkBackingFit::kExact, SkBudgeted::kNo,
+                SkColorSpace::MakeSRGB());
         if (!sContext) {
             ERRORF(reporter, "Could not create SRGBA surface context.");
             return;