From 0e22eb8e6efc7d7ab7a601ba555947916d139906 Mon Sep 17 00:00:00 2001 From: brianosman Date: Tue, 30 Aug 2016 07:07:59 -0700 Subject: [PATCH] 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 --- bench/nanobench.cpp | 2 +- dm/DM.cpp | 2 +- gm/surface.cpp | 3 +- gm/textblobgeometrychange.cpp | 3 +- samplecode/SampleApp.cpp | 7 ++- src/gpu/GrDrawingManager.cpp | 7 +++ src/image/SkSurface_Gpu.cpp | 44 +++++++++++++++ src/image/SkSurface_Gpu.h | 3 ++ src/image/SkSurface_Raster.cpp | 13 +++++ tests/SurfaceTest.cpp | 104 ++++++++++++++++++++++++++++++++++++ tests/TestConfigParsing.cpp | 6 ++- tools/flags/SkCommonFlagsConfig.cpp | 2 +- tools/skiaserve/Request.cpp | 14 +++-- 13 files changed, 196 insertions(+), 14 deletions(-) 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 @@ static void create_config(const SkCommandLineConfig* config, SkTArray* c 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 @@ static Sink* create_sink(const SkCommandLineConfig* config) { 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 @@ protected: 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 @@ protected: SkAutoTUnref 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 @@ bool SampleWindow::onEvent(const SkEvent& evt) { 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 @@ sk_sp GrDrawingManager::makeDrawContext(sk_sp rt, 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 @@ void SkSurface_Gpu::onPrepareForExternalIO() { /////////////////////////////////////////////////////////////////////////////// +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::MakeRenderTarget(GrContext* ctx, SkBudgeted budgeted, const SkImageInfo& info, int sampleCount, GrSurfaceOrigin origin, const SkSurfaceProps* props) { + if (!SkSurface_Gpu::Valid(info)) { + return nullptr; + } + sk_sp device(SkGpuDevice::Make( ctx, budgeted, info, sampleCount, origin, props, SkGpuDevice::kClear_InitContents)); if (!device) { @@ -154,6 +189,9 @@ sk_sp SkSurface::MakeFromBackendTexture(GrContext* context, if (!SkToBool(desc.fFlags & kRenderTarget_GrBackendTextureFlag)) { return nullptr; } + if (!SkSurface_Gpu::Valid(context, desc.fConfig, colorSpace.get())) { + return nullptr; + } sk_sp dc(context->contextPriv().makeBackendTextureDrawContext( desc, @@ -179,6 +217,9 @@ sk_sp SkSurface::MakeFromBackendRenderTarget(GrContext* context, if (!context) { return nullptr; } + if (!SkSurface_Gpu::Valid(context, desc.fConfig, colorSpace.get())) { + return nullptr; + } sk_sp dc(context->contextPriv().makeBackendRenderTargetDrawContext( desc, @@ -204,6 +245,9 @@ sk_sp SkSurface::MakeFromBackendTextureAsRenderTarget(GrContext* cont if (!context) { return nullptr; } + if (!SkSurface_Gpu::Valid(context, desc.fConfig, colorSpace.get())) { + return nullptr; + } sk_sp 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 @@ public: SkGpuDevice* getDevice() { return fDevice.get(); } + static bool Valid(const SkImageInfo&); + static bool Valid(GrContext*, GrPixelConfig, SkColorSpace*); + private: sk_sp 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 @@ bool SkSurface_Raster::Valid(const SkImageInfo& info, size_t rowBytes) { 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 #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 #endif #include @@ -910,3 +912,105 @@ DEF_GPUTEST_FOR_GL_RENDERING_CONTEXTS(SurfaceAttachStencil_Gpu, reporter, ctxInf } } #endif + +static void test_surface_creation_and_snapshot_with_color_space( + skiatest::Reporter* reporter, + const char* prefix, + bool f16Support, + std::function(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 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 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 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(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 @@ DEF_TEST(ParseConfigs_DefaultConfigs, reporter) { 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 @@ static bool parse_option_gpu_color(const SkString& value, } 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 @@ ColorAndProfile ColorModes[] = { 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 @@ SkSurface* Request::createGPUSurface() { 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; } -- 2.7.4