Some tests around surface creation and snapshotting with color space
Verify the rules that we're converging on for surfaces:
- For 8888, we only support sRGB-like gamma, or no color space at all.
- For F16, we require a color space, with linear gamma.
- For all other formats, we do not support color spaces.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2270823002
Review-Url: https://codereview.chromium.org/2270823002
diff --git a/bench/nanobench.cpp b/bench/nanobench.cpp
index 4ded061..a2f931e 100644
--- a/bench/nanobench.cpp
+++ b/bench/nanobench.cpp
@@ -471,7 +471,7 @@
CPU_CONFIG(srgb, kRaster_Backend,
kN32_SkColorType, kPremul_SkAlphaType, srgbColorSpace)
CPU_CONFIG(f16, kRaster_Backend,
- kRGBA_F16_SkColorType, kPremul_SkAlphaType, srgbColorSpace)
+ kRGBA_F16_SkColorType, kPremul_SkAlphaType, srgbColorSpace->makeLinearGamma())
}
#undef CPU_CONFIG
diff --git a/dm/DM.cpp b/dm/DM.cpp
index 5dd8a6f..a815827 100644
--- a/dm/DM.cpp
+++ b/dm/DM.cpp
@@ -865,7 +865,7 @@
SINK("565", RasterSink, kRGB_565_SkColorType);
SINK("8888", RasterSink, kN32_SkColorType);
SINK("srgb", RasterSink, kN32_SkColorType, srgbColorSpace);
- SINK("f16", RasterSink, kRGBA_F16_SkColorType, srgbColorSpace);
+ SINK("f16", RasterSink, kRGBA_F16_SkColorType, srgbColorSpace->makeLinearGamma());
SINK("pdf", PDFSink);
SINK("skp", SKPSink);
SINK("svg", SVGSink);
diff --git a/gm/surface.cpp b/gm/surface.cpp
index 7c0b0c9..6323c70 100644
--- a/gm/surface.cpp
+++ b/gm/surface.cpp
@@ -65,8 +65,7 @@
GrContext* ctx = canvas->getGrContext();
// must be opaque to have a hope of testing LCD text
- const SkImageInfo info = SkImageInfo::MakeN32(W, H, kOpaque_SkAlphaType,
- sk_ref_sp(canvas->imageInfo().colorSpace()));
+ const SkImageInfo info = SkImageInfo::MakeN32(W, H, kOpaque_SkAlphaType);
const struct {
SkPixelGeometry fGeo;
diff --git a/gm/textblobgeometrychange.cpp b/gm/textblobgeometrychange.cpp
index 1b7bff3..5e3f0d1 100644
--- a/gm/textblobgeometrychange.cpp
+++ b/gm/textblobgeometrychange.cpp
@@ -42,8 +42,7 @@
SkAutoTUnref<const SkTextBlob> blob(builder.build());
- SkImageInfo info = SkImageInfo::MakeN32(200, 200, kPremul_SkAlphaType,
- sk_ref_sp(canvas->imageInfo().colorSpace()));
+ SkImageInfo info = SkImageInfo::MakeN32Premul(200, 200);
SkSurfaceProps props(0, kUnknown_SkPixelGeometry);
auto surface = canvas->makeSurface(info, &props);
if (!surface) {
diff --git a/samplecode/SampleApp.cpp b/samplecode/SampleApp.cpp
index 9e7e651..b6b9cde 100644
--- a/samplecode/SampleApp.cpp
+++ b/samplecode/SampleApp.cpp
@@ -1556,9 +1556,12 @@
return true;
}
if (SkOSMenu::FindListIndex(evt, "ColorType", &selected)) {
- auto srgbColorSpace = SkColorSpace::NewNamed(SkColorSpace::kSRGB_Named);
+ auto colorSpace = SkColorSpace::NewNamed(SkColorSpace::kSRGB_Named);
+ if (kRGBA_F16_SkColorType == gConfig[selected].fColorType) {
+ colorSpace = colorSpace->makeLinearGamma();
+ }
this->setDeviceColorType(gConfig[selected].fColorType,
- gConfig[selected].fSRGB ? srgbColorSpace : nullptr);
+ gConfig[selected].fSRGB ? colorSpace : nullptr);
return true;
}
if (SkOSMenu::FindSwitchState(evt, "Slide Show", nullptr)) {
diff --git a/src/gpu/GrDrawingManager.cpp b/src/gpu/GrDrawingManager.cpp
index f4d00dc..df6bffd 100644
--- a/src/gpu/GrDrawingManager.cpp
+++ b/src/gpu/GrDrawingManager.cpp
@@ -11,6 +11,7 @@
#include "GrPathRenderingDrawContext.h"
#include "GrResourceProvider.h"
#include "GrSoftwarePathRenderer.h"
+#include "SkSurface_Gpu.h"
#include "SkTTopoSort.h"
#include "instanced/InstancedRendering.h"
@@ -191,6 +192,12 @@
return nullptr;
}
+ // 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, rt->config(), colorSpace.get())) {
+ return nullptr;
+ }
bool useDIF = false;
if (surfaceProps) {
diff --git a/src/image/SkSurface_Gpu.cpp b/src/image/SkSurface_Gpu.cpp
index 1d71570..ebe103a 100644
--- a/src/image/SkSurface_Gpu.cpp
+++ b/src/image/SkSurface_Gpu.cpp
@@ -133,9 +133,44 @@
///////////////////////////////////////////////////////////////////////////////
+bool SkSurface_Gpu::Valid(const SkImageInfo& info) {
+ switch (info.colorType()) {
+ case kRGBA_F16_SkColorType:
+ return info.colorSpace() &&
+ SkColorSpace::kLinear_GammaNamed == info.colorSpace()->gammaNamed();
+ case kRGBA_8888_SkColorType:
+ case kBGRA_8888_SkColorType:
+ return !info.colorSpace() || info.colorSpace()->gammaCloseToSRGB();
+ default:
+ return !info.colorSpace();
+ }
+}
+
+bool SkSurface_Gpu::Valid(GrContext* context, GrPixelConfig config, SkColorSpace* colorSpace) {
+ switch (config) {
+ case kRGBA_half_GrPixelConfig:
+ return colorSpace && SkColorSpace::kLinear_GammaNamed == colorSpace->gammaNamed();
+ case kSRGBA_8888_GrPixelConfig:
+ case kSBGRA_8888_GrPixelConfig:
+ return context->caps()->srgbSupport() && colorSpace && colorSpace->gammaCloseToSRGB();
+ case kRGBA_8888_GrPixelConfig:
+ case kBGRA_8888_GrPixelConfig:
+ // If we don't have sRGB support, we may get here with a color space. It still needs
+ // to be sRGB-like (so that the application will work correctly on sRGB devices.)
+ return !colorSpace ||
+ (!context->caps()->srgbSupport() && colorSpace->gammaCloseToSRGB());
+ default:
+ return !colorSpace;
+ }
+}
+
sk_sp<SkSurface> SkSurface::MakeRenderTarget(GrContext* ctx, SkBudgeted budgeted,
const SkImageInfo& info, int sampleCount,
GrSurfaceOrigin origin, const SkSurfaceProps* props) {
+ if (!SkSurface_Gpu::Valid(info)) {
+ return nullptr;
+ }
+
sk_sp<SkGpuDevice> device(SkGpuDevice::Make(
ctx, budgeted, info, sampleCount, origin, props, SkGpuDevice::kClear_InitContents));
if (!device) {
@@ -154,6 +189,9 @@
if (!SkToBool(desc.fFlags & kRenderTarget_GrBackendTextureFlag)) {
return nullptr;
}
+ if (!SkSurface_Gpu::Valid(context, desc.fConfig, colorSpace.get())) {
+ return nullptr;
+ }
sk_sp<GrDrawContext> dc(context->contextPriv().makeBackendTextureDrawContext(
desc,
@@ -179,6 +217,9 @@
if (!context) {
return nullptr;
}
+ if (!SkSurface_Gpu::Valid(context, desc.fConfig, colorSpace.get())) {
+ return nullptr;
+ }
sk_sp<GrDrawContext> dc(context->contextPriv().makeBackendRenderTargetDrawContext(
desc,
@@ -204,6 +245,9 @@
if (!context) {
return nullptr;
}
+ if (!SkSurface_Gpu::Valid(context, desc.fConfig, colorSpace.get())) {
+ return nullptr;
+ }
sk_sp<GrDrawContext> dc(context->contextPriv().makeBackendTextureAsRenderTargetDrawContext(
desc,
diff --git a/src/image/SkSurface_Gpu.h b/src/image/SkSurface_Gpu.h
index 8432d9f..b7088ea 100644
--- a/src/image/SkSurface_Gpu.h
+++ b/src/image/SkSurface_Gpu.h
@@ -30,6 +30,9 @@
SkGpuDevice* getDevice() { return fDevice.get(); }
+ static bool Valid(const SkImageInfo&);
+ static bool Valid(GrContext*, GrPixelConfig, SkColorSpace*);
+
private:
sk_sp<SkGpuDevice> fDevice;
diff --git a/src/image/SkSurface_Raster.cpp b/src/image/SkSurface_Raster.cpp
index 90f26be..9309487 100644
--- a/src/image/SkSurface_Raster.cpp
+++ b/src/image/SkSurface_Raster.cpp
@@ -49,15 +49,28 @@
int shift = 0;
switch (info.colorType()) {
case kAlpha_8_SkColorType:
+ if (info.colorSpace()) {
+ return false;
+ }
shift = 0;
break;
case kRGB_565_SkColorType:
+ if (info.colorSpace()) {
+ return false;
+ }
shift = 1;
break;
case kN32_SkColorType:
+ if (info.colorSpace() && !info.colorSpace()->gammaCloseToSRGB()) {
+ return false;
+ }
shift = 2;
break;
case kRGBA_F16_SkColorType:
+ if (!info.colorSpace() ||
+ SkColorSpace::kLinear_GammaNamed != info.colorSpace()->gammaNamed()) {
+ return false;
+ }
shift = 3;
break;
default:
diff --git a/tests/SurfaceTest.cpp b/tests/SurfaceTest.cpp
index 2703b54..27b1524 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"
@@ -21,6 +22,7 @@
#include "GrDrawContext.h"
#include "GrGpu.h"
#include "GrResourceProvider.h"
+#include <vector>
#endif
#include <initializer_list>
@@ -910,3 +912,105 @@
}
}
#endif
+
+static void test_surface_creation_and_snapshot_with_color_space(
+ skiatest::Reporter* reporter,
+ const char* prefix,
+ bool f16Support,
+ std::function<sk_sp<SkSurface>(const SkImageInfo&)> surfaceMaker) {
+
+ 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 = srgbColorSpace->makeLinearGamma();
+
+ const struct {
+ SkColorType fColorType;
+ sk_sp<SkColorSpace> fColorSpace;
+ bool fShouldWork;
+ const char* fDescription;
+ } testConfigs[] = {
+ { kN32_SkColorType, nullptr, true, "N32-nullptr" },
+ { kN32_SkColorType, linearColorSpace, false, "N32-linear" },
+ { kN32_SkColorType, srgbColorSpace, true, "N32-srgb" },
+ { kN32_SkColorType, adobeColorSpace, true, "N32-adobe" },
+ { kN32_SkColorType, oddColorSpace, false, "N32-odd" },
+ { kRGBA_F16_SkColorType, nullptr, false, "F16-nullptr" },
+ { kRGBA_F16_SkColorType, linearColorSpace, true, "F16-linear" },
+ { kRGBA_F16_SkColorType, srgbColorSpace, false, "F16-srgb" },
+ { kRGBA_F16_SkColorType, adobeColorSpace, false, "F16-adobe" },
+ { kRGBA_F16_SkColorType, oddColorSpace, false, "F16-odd" },
+ { kRGB_565_SkColorType, srgbColorSpace, false, "565-srgb" },
+ { kAlpha_8_SkColorType, srgbColorSpace, false, "A8-srgb" },
+ };
+
+ for (auto& testConfig : testConfigs) {
+ SkString fullTestName = SkStringPrintf("%s-%s", prefix, testConfig.fDescription);
+ SkImageInfo info = SkImageInfo::Make(10, 10, testConfig.fColorType, kPremul_SkAlphaType,
+ testConfig.fColorSpace);
+
+ // For some GPU contexts (eg ANGLE), we don't have f16 support, so we should fail to create
+ // any surface of that type:
+ bool shouldWork = testConfig.fShouldWork &&
+ (f16Support || kRGBA_F16_SkColorType != testConfig.fColorType);
+
+ auto surface(surfaceMaker(info));
+ REPORTER_ASSERT_MESSAGE(reporter, SkToBool(surface) == shouldWork, fullTestName.c_str());
+
+ if (shouldWork && 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(),
+ fullTestName.c_str());
+ }
+ }
+}
+
+DEF_TEST(SurfaceCreationWithColorSpace, reporter) {
+ auto surfaceMaker = [](const SkImageInfo& info) {
+ return SkSurface::MakeRaster(info);
+ };
+
+ test_surface_creation_and_snapshot_with_color_space(reporter, "raster", true, surfaceMaker);
+}
+
+#if SK_SUPPORT_GPU
+DEF_GPUTEST_FOR_RENDERING_CONTEXTS(SurfaceCreationWithColorSpace_Gpu, reporter, ctxInfo) {
+ GrContext* context = ctxInfo.grContext();
+ bool f16Support = context->caps()->isConfigRenderable(kRGBA_half_GrPixelConfig, false);
+ auto surfaceMaker = [context](const SkImageInfo& info) {
+ return SkSurface::MakeRenderTarget(context, SkBudgeted::kNo, info);
+ };
+
+ test_surface_creation_and_snapshot_with_color_space(reporter, "gpu", f16Support, surfaceMaker);
+
+ std::vector<GrBackendObject> textureHandles;
+ auto wrappedSurfaceMaker = [context,&textureHandles](const SkImageInfo& info) {
+ GrBackendTextureDesc desc;
+ desc.fConfig = SkImageInfo2GrPixelConfig(info, *context->caps());
+ desc.fWidth = 10;
+ desc.fHeight = 10;
+ desc.fFlags = kRenderTarget_GrBackendTextureFlag;
+ desc.fTextureHandle = context->getGpu()->createTestingOnlyBackendTexture(
+ nullptr, desc.fWidth, desc.fHeight, desc.fConfig, true);
+
+ if (!desc.fTextureHandle) {
+ return sk_sp<SkSurface>(nullptr);
+ }
+ textureHandles.push_back(desc.fTextureHandle);
+
+ return SkSurface::MakeFromBackendTexture(context, desc, sk_ref_sp(info.colorSpace()),
+ nullptr);
+ };
+
+ test_surface_creation_and_snapshot_with_color_space(reporter, "wrapped", f16Support,
+ wrappedSurfaceMaker);
+
+ for (auto textureHandle : textureHandles) {
+ context->getGpu()->deleteTestingOnlyBackendTexture(textureHandle);
+ }
+}
+#endif
diff --git a/tests/TestConfigParsing.cpp b/tests/TestConfigParsing.cpp
index 6f5c64a..1a7e5b9 100644
--- a/tests/TestConfigParsing.cpp
+++ b/tests/TestConfigParsing.cpp
@@ -123,7 +123,11 @@
REPORTER_ASSERT(reporter, !configs[19]->asConfigGpu());
REPORTER_ASSERT(reporter, !configs[24]->asConfigGpu());
REPORTER_ASSERT(reporter, configs[25]->asConfigGpu()->getColorType() == kRGBA_F16_SkColorType);
- REPORTER_ASSERT(reporter, configs[25]->asConfigGpu()->getColorSpace() == srgbColorSpace.get());
+ REPORTER_ASSERT(reporter, configs[25]->asConfigGpu()->getColorSpace());
+ REPORTER_ASSERT(reporter, configs[25]->asConfigGpu()->getColorSpace()->gammaNamed() ==
+ SkColorSpace::kLinear_GammaNamed);
+ REPORTER_ASSERT(reporter, configs[25]->asConfigGpu()->getColorSpace()->xyz() ==
+ srgbColorSpace->xyz());
REPORTER_ASSERT(reporter, configs[26]->asConfigGpu()->getColorType() == kN32_SkColorType);
REPORTER_ASSERT(reporter, configs[26]->asConfigGpu()->getColorSpace() == srgbColorSpace.get());
REPORTER_ASSERT(reporter, configs[33]->asConfigGpu()->getContextType() ==
diff --git a/tools/flags/SkCommonFlagsConfig.cpp b/tools/flags/SkCommonFlagsConfig.cpp
index e25ccd3..447c56d 100644
--- a/tools/flags/SkCommonFlagsConfig.cpp
+++ b/tools/flags/SkCommonFlagsConfig.cpp
@@ -298,7 +298,7 @@
}
if (value.equals("f16")) {
*outColorType = kRGBA_F16_SkColorType;
- *outColorSpace = SkColorSpace::NewNamed(SkColorSpace::kSRGB_Named);
+ *outColorSpace = SkColorSpace::NewNamed(SkColorSpace::kSRGB_Named)->makeLinearGamma();
return true;
}
if (value.equals("srgb")) {
diff --git a/tools/skiaserve/Request.cpp b/tools/skiaserve/Request.cpp
index e3a0caa..22fa211 100644
--- a/tools/skiaserve/Request.cpp
+++ b/tools/skiaserve/Request.cpp
@@ -174,9 +174,12 @@
SkSurface* Request::createCPUSurface() {
SkIRect bounds = this->getBounds();
ColorAndProfile cap = ColorModes[fColorMode];
- auto srgbColorSpace = SkColorSpace::NewNamed(SkColorSpace::kSRGB_Named);
+ auto colorSpace = SkColorSpace::NewNamed(SkColorSpace::kSRGB_Named);
+ if (kRGBA_F16_SkColorType == cap.fColorType) {
+ colorSpace = colorSpace->makeLinearGamma();
+ }
SkImageInfo info = SkImageInfo::Make(bounds.width(), bounds.height(), cap.fColorType,
- kPremul_SkAlphaType, cap.fSRGB ? srgbColorSpace : nullptr);
+ kPremul_SkAlphaType, cap.fSRGB ? colorSpace : nullptr);
return SkSurface::MakeRaster(info).release();
}
@@ -184,9 +187,12 @@
GrContext* context = this->getContext();
SkIRect bounds = this->getBounds();
ColorAndProfile cap = ColorModes[fColorMode];
- auto srgbColorSpace = SkColorSpace::NewNamed(SkColorSpace::kSRGB_Named);
+ auto colorSpace = SkColorSpace::NewNamed(SkColorSpace::kSRGB_Named);
+ if (kRGBA_F16_SkColorType == cap.fColorType) {
+ colorSpace = colorSpace->makeLinearGamma();
+ }
SkImageInfo info = SkImageInfo::Make(bounds.width(), bounds.height(), cap.fColorType,
- kPremul_SkAlphaType, cap.fSRGB ? srgbColorSpace : nullptr);
+ kPremul_SkAlphaType, cap.fSRGB ? colorSpace: nullptr);
SkSurface* surface = SkSurface::MakeRenderTarget(context, SkBudgeted::kNo, info).release();
return surface;
}