Some tests around surface creation and snapshotting with color space

tests/SurfaceTest.cpp

Index: tests/SurfaceTest.cpp
diff --git a/tests/SurfaceTest.cpp b/tests/SurfaceTest.cpp
index 2703b541fc59854b2ac27989bfcea59adad2db0c..e75e5fdc5d12f83c774aa7f36fee679535a45f07 100644
--- a/tests/SurfaceTest.cpp
+++ b/tests/SurfaceTest.cpp
@@ -7,6 +7,7 @@
 
 #include <functional>
 #include "SkCanvas.h"
+#include "SkColorSpace_Base.h"
 #include "SkData.h"
 #include "SkDevice.h"
 #include "SkImage_Base.h"
@@ -910,3 +911,48 @@ DEF_GPUTEST_FOR_GL_RENDERING_CONTEXTS(SurfaceAttachStencil_Gpu, reporter, ctxInf
     }
 }
 #endif
+
+DEF_TEST(SurfaceCreationWithColorSpace, reporter) {
+    auto srgbColorSpace = SkColorSpace::NewNamed(SkColorSpace::kSRGB_Named);
+    auto adobeColorSpace = SkColorSpace::NewNamed(SkColorSpace::kAdobeRGB_Named);
+    SkMatrix44 srgbMatrix = srgbColorSpace->xyz();
+    const float oddGamma[] = { 2.4f, 2.4f, 2.4f };
+    auto oddColorSpace = SkColorSpace_Base::NewRGB(oddGamma, srgbMatrix);
+    auto linearColorSpace = SkColorSpace::NewRGB(SkColorSpace::kLinear_GammaNamed, srgbMatrix);
+
+    const struct {
+        SkColorType         fColorType;
+        sk_sp<SkColorSpace> fColorSpace;
+        bool                fShouldWork;
+        const char*         fDescription;
+    } testConfigs[] = {
+        { kN32_SkColorType,       nullptr,          true,  "N32-nullptr" },
+        { kN32_SkColorType,       linearColorSpace, true,  "N32-linear"  }, // TODO: Disallow this?

[Brian Osman, 2016/08/23 13:25:56]

Starting from the statement: "If there is a color space attached, then rendering
is gamma-correct. Sources are gamma-decoded, etc..."

This can work on GPU, but I'm not sure we have a raster pipeline for this. Do we
have a way to render to linear 8-bit, with gamma-decoding of sRGB sources?

[msarett, 2016/08/23 13:46:11]

FWIW, image decoders support linear 8-bit.  But it just falls into the slow
path, it's not exactly an intentional feature.

[mtklein, 2016/08/23 14:24:51]

Yes, ban it.  If you're well informed about color spaces and still find yourself
wanting 32-bit buffers, you'll pick N32-srgb.  If not, you'll keep using
N32-nullptr. Even if N32-linear turns out to be easy to support, there's always
a better choice.

[Brian Osman, 2016/08/23 14:58:33]

Acknowledged.

+        { kN32_SkColorType,       srgbColorSpace,   true,  "N32-srgb"    },
+        { kN32_SkColorType,       adobeColorSpace,  true,  "N32-adobe"   },
+        { kN32_SkColorType,       oddColorSpace,    false, "N32-odd"     },
+        { kRGBA_F16_SkColorType,  nullptr,          true,  "F16-nullptr" }, // TODO: Disallow this?

[Brian Osman, 2016/08/23 13:25:56]

We've mentioned requiring a color space with F16. Again, we can easily support
this on GPU, and it could be useful for auxiliary uses of F16 (shadow maps, high
precision masks, etc...). Does it cause any issues for Raster?

[msarett, 2016/08/23 13:46:11]

Image decoders will fail (on purpose) if you ask for F16 and the color space is
nullptr.

[mtklein, 2016/08/23 14:24:50]

Yes, let's ban this.  There are no legacy users of F16.  Anyone who wants to
play with future tech needs to become color space aware.

[Brian Osman, 2016/08/23 14:58:33]

Acknowledged.

+        { kRGBA_F16_SkColorType,  linearColorSpace, true,  "F16-linear"  },
+        { kRGBA_F16_SkColorType,  srgbColorSpace,   true,  "F16-srgb"    },
+        { kRGBA_F16_SkColorType,  adobeColorSpace,  true,  "F16-adobe"   },
+        { kRGBA_F16_SkColorType,  oddColorSpace,    true,  "F16-odd"     }, // TODO: Disallow this?

[Brian Osman, 2016/08/23 13:25:56]

Should we allow this? The gamma is ignored, so in some sense it works just fine,
and I think it could be a useful part of a pipeline for eventual down-sampling
to 8 or 10 bit with a different gamma curve. (Where we do a one-time adjustment
in a shader based on the gamma from the attached color space).

[msarett, 2016/08/23 13:46:11]

Supporting this doesn't bother the image decoders.  I think to answer this, we
need to decide how we want to handle dsts with strange gammas...  AFAICT,
they're rare, but they exist.

[mtklein, 2016/08/23 14:24:50]

Remind me why we'd support non-linear gamma for F16?

If we're dropping down to 8 or 10 bit, won't that format know its own gamma?
Why is it the F16 buffer's responsibility to carry around a notional gamma?

[Brian Osman, 2016/08/23 14:58:33]

I have several arguments each way:

1) We get an image off the internet, and it's got a gamma we can't directly
support. So we ask the codec to give it to us as F16. The easiest thing for the
codec to do is decode the stored gamma (so we get F16 1.0), but still attach the
original color space object. The alternative is to explicitly make a new version
of the color space with gamma 1.0. Not much work for this case, and this doesn't
really influence the surface creation path, but given that we'll have the same
checks on sources, I think the argument is the same.

2) Re: 8/10-bit, yes, probably. I'm just trying to decide what the world looks
like if someone queries their monitor and learns that it's got a 1.8 gamma. Then
they want to render a surface with that gamma*. We can't do that directly, so
they're going to need to render to something else with a different gamma
(probably F16 1.0). Then they're going to want to push that to the monitor - I
guess you're right that we could have deferred sticking the monitor gamma on
something until this step (we've talked about having a presentation-layer only
surface type that supports arbitrary gamma for this situation). So yeah - may
not matter for this case.

*: I'm still trying to decide if this is actually a thing that we're supposed to
do. It's not obvious to me that color-managed applications are (or aren't)
supposed to compensate for monitor gamma. I feel like one possible world is that
we're supposed to do that. The other world is where the OS contract is that they
accept things in either sRGB or linear gamma, then adapt output to the monitor's
gamma, because that's a function of viewing environment, and they don't (want
to) care about encoding of incoming layers for compositing.

+        { kRGB_565_SkColorType,   srgbColorSpace,   false, "565-srgb"    },
+        { kAlpha_8_SkColorType,   srgbColorSpace,   false, "A8-srgb"     }, // TODO: Allow this?

[Brian Osman, 2016/08/23 13:25:56]

Pretty sure we don't want to support this - obviously the gamma doesn't affect
what's happening in the alpha channel. But if you render to an A8 surface with a
paint that has behavior influenced by gamma-correction (shader / color filter,
etc...), should we respect that? Again, this probably adds a lot of complexity
to raster for an extreme edge case, so my gut says no.

[msarett, 2016/08/23 13:46:11]

Agree with your gut.  It's a nice simplification to say "alpha is always
linear".

[mtklein, 2016/08/23 14:24:50]

Right, let's ban non-linear alpha entirely.
Grey8-nullptr and Grey8-srgb can probably fill in the gaps, right?

[Brian Osman, 2016/08/23 14:58:33]

... actually, we don't support Grey8 as a surface format, at all. Raster doesn't
allow it currently, and GPU actually has bigger problems. We map Grey8 and
Alpha8 to the same backend format, and when I tried to support it properly,
things kind of spiraled out of control. (Grey8-srgb doesn't really exist in
modern GL, sadly. It does exist in *older* GL with some extensions, or in
Vulkan.)

+    };
+
+    for (auto& testConfig : testConfigs) {
+        SkImageInfo info = SkImageInfo::Make(10, 10, testConfig.fColorType, kPremul_SkAlphaType,
+                                                testConfig.fColorSpace);
+        auto surface(SkSurface::MakeRaster(info));
+        REPORTER_ASSERT_MESSAGE(reporter, SkToBool(surface) == testConfig.fShouldWork,
+                                testConfig.fDescription);
+
+        if (testConfig.fShouldWork && surface) {
+            sk_sp<SkImage> image(surface->makeImageSnapshot());
+            REPORTER_ASSERT_MESSAGE(reporter, image, testConfig.fDescription);
+            SkColorSpace* imageColorSpace = as_IB(image)->onImageInfo().colorSpace();
+            REPORTER_ASSERT_MESSAGE(reporter, imageColorSpace == testConfig.fColorSpace.get(),
+                                    testConfig.fDescription);
+        }
+    }
+}