From 6b622963a065fe004372b811d1cbdefcfbe1a7cb Mon Sep 17 00:00:00 2001 From: Brian Osman Date: Mon, 27 Aug 2018 19:16:02 +0000 Subject: [PATCH] Reland "Stop conflating F16 with linear gamma" This reverts commit 5f7b5e3624dcb055acc64fcf90c513408d1789ed. Reason for revert: Codec CL has re-landed. Original change's description: > Revert "Stop conflating F16 with linear gamma" > > This reverts commit d1589c7213d4a23c7c5c352f70d753eb7f07518d. > > Reason for revert: Depends on skcms CL that's been reverted. > > Original change's description: > > Stop conflating F16 with linear gamma > > > > Note to self: I debugged this, realized that the codecs > > need to handle A2B -> XYZ, then realized that I just need > > to wait for https://skia-review.googlesource.com/c/skia/+/136062 > > > > Bug: skia: > > Change-Id: I594c22076feb3700b8a40c471a541fef5ff4e13e > > Reviewed-on: https://skia-review.googlesource.com/137587 > > Commit-Queue: Brian Osman > > Reviewed-by: Mike Klein > > TBR=mtklein@google.com,brianosman@google.com > > Change-Id: I6dca583697c8efd2563d30cb7ab9ef505b6903ae > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: skia: > Reviewed-on: https://skia-review.googlesource.com/148860 > Reviewed-by: Brian Osman > Commit-Queue: Brian Osman TBR=mtklein@google.com,brianosman@google.com # Not skipping CQ checks because original CL landed > 1 day ago. Bug: skia: Change-Id: Iee66531049843758e7ed4130b99d8df6a553d805 Reviewed-on: https://skia-review.googlesource.com/149700 Reviewed-by: Brian Osman Commit-Queue: Brian Osman --- bench/MipMapBench.cpp | 7 +++---- dm/DMSrcSink.cpp | 30 +----------------------------- gm/encode-srgb.cpp | 17 ++++------------- gm/readpixels.cpp | 14 ++------------ tests/BitmapCopyTest.cpp | 2 +- tests/CodecRecommendedTypeTest.cpp | 2 +- tests/DeferredDisplayListTest.cpp | 2 -- tests/ReadPixelsTest.cpp | 8 -------- 8 files changed, 12 insertions(+), 70 deletions(-) diff --git a/bench/MipMapBench.cpp b/bench/MipMapBench.cpp index e41d39b928..aed2c59523 100644 --- a/bench/MipMapBench.cpp +++ b/bench/MipMapBench.cpp @@ -33,10 +33,9 @@ protected: const char* onGetName() override { return fName.c_str(); } void onDelayedSetup() override { - SkImageInfo info = fHalfFoat ? SkImageInfo::Make(fW, fH, kRGBA_F16_SkColorType, - kPremul_SkAlphaType, - SkColorSpace::MakeSRGBLinear()) - : SkImageInfo::MakeS32(fW, fH, kPremul_SkAlphaType); + SkColorType ct = fHalfFoat ? kRGBA_F16_SkColorType : kN32_SkColorType; + SkImageInfo info = SkImageInfo::Make(fW, fH, ct, kPremul_SkAlphaType, + SkColorSpace::MakeSRGB()); fBitmap.allocPixels(info); fBitmap.eraseColor(SK_ColorWHITE); // so we don't read uninitialized memory } diff --git a/dm/DMSrcSink.cpp b/dm/DMSrcSink.cpp index 57827e0bb5..a4d623e5a7 100644 --- a/dm/DMSrcSink.cpp +++ b/dm/DMSrcSink.cpp @@ -145,14 +145,6 @@ static inline void alpha8_to_gray8(SkBitmap* bitmap) { } Error BRDSrc::draw(SkCanvas* canvas) const { - if (canvas->imageInfo().colorSpace() && - kRGBA_F16_SkColorType != canvas->imageInfo().colorType()) { - // SkAndroidCodec uses legacy premultiplication and blending. Therefore, we only - // run these tests on legacy canvases. - // We allow an exception for F16, since Android uses F16. - return Error::Nonfatal("Skip testing to color correct canvas."); - } - SkColorType colorType = canvas->imageInfo().colorType(); if (kRGB_565_SkColorType == colorType && CodecSrc::kGetFromCanvas_DstColorType != fDstColorType) { @@ -401,11 +393,6 @@ static bool get_decode_info(SkImageInfo* decodeInfo, SkColorType canvasColorType return false; } - if (kRGBA_F16_SkColorType == canvasColorType) { - sk_sp linearSpace = decodeInfo->colorSpace()->makeLinearGamma(); - *decodeInfo = decodeInfo->makeColorSpace(std::move(linearSpace)); - } - *decodeInfo = decodeInfo->makeColorType(canvasColorType); break; } @@ -431,11 +418,7 @@ static void draw_to_canvas(SkCanvas* canvas, const SkImageInfo& info, void* pixe // "pretend" that the color space is standard sRGB to avoid triggering color conversion // at draw time. static void set_bitmap_color_space(SkImageInfo* info) { - if (kRGBA_F16_SkColorType == info->colorType()) { - *info = info->makeColorSpace(SkColorSpace::MakeSRGBLinear()); - } else { - *info = info->makeColorSpace(SkColorSpace::MakeSRGB()); - } + *info = info->makeColorSpace(SkColorSpace::MakeSRGB()); } Error CodecSrc::draw(SkCanvas* canvas) const { @@ -824,14 +807,6 @@ bool AndroidCodecSrc::veto(SinkFlags flags) const { } Error AndroidCodecSrc::draw(SkCanvas* canvas) const { - if (canvas->imageInfo().colorSpace() && - kRGBA_F16_SkColorType != canvas->imageInfo().colorType()) { - // SkAndroidCodec uses legacy premultiplication and blending. Therefore, we only - // run these tests on legacy canvases. - // We allow an exception for F16, since Android uses F16. - return Error::Nonfatal("Skip testing to color correct canvas."); - } - sk_sp encoded(SkData::MakeFromFileName(fPath.c_str())); if (!encoded) { return SkStringPrintf("Couldn't read %s.", fPath.c_str()); @@ -1090,9 +1065,6 @@ Error ColorCodecSrc::draw(SkCanvas* canvas) const { if (kUnpremul_SkAlphaType == decodeInfo.alphaType()) { decodeInfo = decodeInfo.makeAlphaType(kPremul_SkAlphaType); } - if (kRGBA_F16_SkColorType == fColorType) { - decodeInfo = decodeInfo.makeColorSpace(decodeInfo.colorSpace()->makeLinearGamma()); - } SkImageInfo bitmapInfo = decodeInfo; set_bitmap_color_space(&bitmapInfo); diff --git a/gm/encode-srgb.cpp b/gm/encode-srgb.cpp index 2538e6e722..bef029fbed 100644 --- a/gm/encode-srgb.cpp +++ b/gm/encode-srgb.cpp @@ -23,18 +23,6 @@ namespace skiagm { static const int imageWidth = 128; static const int imageHeight = 128; -sk_sp fix_for_colortype(sk_sp colorSpace, SkColorType colorType) { - if (kRGBA_F16_SkColorType == colorType) { - if (!colorSpace) { - return SkColorSpace::MakeSRGBLinear(); - } - - return colorSpace->makeLinearGamma(); - } - - return colorSpace; -} - static void make(SkBitmap* bitmap, SkColorType colorType, SkAlphaType alphaType, sk_sp colorSpace) { const char* resource; @@ -55,9 +43,12 @@ static void make(SkBitmap* bitmap, SkColorType colorType, SkAlphaType alphaType, sk_sp data = GetResourceAsData(resource); std::unique_ptr codec = SkCodec::MakeFromData(data); + if (kRGBA_F16_SkColorType == colorType && !colorSpace) { + colorSpace = SkColorSpace::MakeSRGB(); + } SkImageInfo dstInfo = codec->getInfo().makeColorType(colorType) .makeAlphaType(alphaType) - .makeColorSpace(fix_for_colortype(colorSpace, colorType)); + .makeColorSpace(colorSpace); bitmap->allocPixels(dstInfo); codec->getPixels(dstInfo, bitmap->getPixels(), bitmap->rowBytes()); } diff --git a/gm/readpixels.cpp b/gm/readpixels.cpp index aac2aa60ad..acb821c22b 100644 --- a/gm/readpixels.cpp +++ b/gm/readpixels.cpp @@ -40,14 +40,6 @@ static void clamp_if_necessary(const SkImageInfo& info, void* pixels) { } } -sk_sp fix_for_colortype(SkColorSpace* colorSpace, SkColorType colorType) { - if (kRGBA_F16_SkColorType == colorType) { - return colorSpace->makeLinearGamma(); - } - - return sk_ref_sp(colorSpace); -} - static const int kWidth = 64; static const int kHeight = 64; @@ -58,8 +50,7 @@ static sk_sp make_raster_image(SkColorType colorType) { SkBitmap bitmap; SkImageInfo info = codec->getInfo().makeWH(kWidth, kHeight) .makeColorType(colorType) - .makeAlphaType(kPremul_SkAlphaType) - .makeColorSpace(fix_for_colortype(codec->getInfo().colorSpace(), colorType)); + .makeAlphaType(kPremul_SkAlphaType); bitmap.allocPixels(info); codec->getPixels(info, bitmap.getPixels(), bitmap.rowBytes()); bitmap.setImmutable(); @@ -132,7 +123,6 @@ static void draw_image(SkCanvas* canvas, SkImage* image, SkColorType dstColorTyp SkImage::CachingHint hint) { size_t rowBytes = image->width() * SkColorTypeBytesPerPixel(dstColorType); sk_sp data = SkData::MakeUninitialized(rowBytes * image->height()); - dstColorSpace = fix_for_colortype(dstColorSpace.get(), dstColorType); SkImageInfo dstInfo = SkImageInfo::Make(image->width(), image->height(), dstColorType, dstAlphaType, dstColorSpace); if (!image->readPixels(dstInfo, data->writable_data(), rowBytes, 0, 0, hint)) { @@ -154,7 +144,7 @@ static void draw_image(SkCanvas* canvas, SkImage* image, SkColorType dstColorTyp clamp_if_necessary(dstInfo, data->writable_data()); // Now that we have called readPixels(), dump the raw pixels into an srgb image. - sk_sp srgb = fix_for_colortype(sk_srgb_singleton(), dstColorType); + sk_sp srgb = SkColorSpace::MakeSRGB(); sk_sp raw = SkImage::MakeRasterData(dstInfo.makeColorSpace(srgb), data, rowBytes); canvas->drawImage(raw.get(), 0.0f, 0.0f, nullptr); } diff --git a/tests/BitmapCopyTest.cpp b/tests/BitmapCopyTest.cpp index b1b079c531..c2a8f25613 100644 --- a/tests/BitmapCopyTest.cpp +++ b/tests/BitmapCopyTest.cpp @@ -71,7 +71,7 @@ static void setup_src_bitmaps(SkBitmap* srcOpaque, SkBitmap* srcPremul, SkColorType ct) { sk_sp colorSpace = nullptr; if (kRGBA_F16_SkColorType == ct) { - colorSpace = SkColorSpace::MakeSRGBLinear(); + colorSpace = SkColorSpace::MakeSRGB(); } srcOpaque->allocPixels(SkImageInfo::Make(W, H, ct, kOpaque_SkAlphaType, colorSpace)); diff --git a/tests/CodecRecommendedTypeTest.cpp b/tests/CodecRecommendedTypeTest.cpp index 6ba1157829..53b6427938 100644 --- a/tests/CodecRecommendedTypeTest.cpp +++ b/tests/CodecRecommendedTypeTest.cpp @@ -25,7 +25,7 @@ DEF_TEST(Codec_recommendedF16, r) { // such a PNG. SkBitmap bm; bm.allocPixels(SkImageInfo::Make(10, 10, kRGBA_F16_SkColorType, - kPremul_SkAlphaType, SkColorSpace::MakeSRGBLinear())); + kPremul_SkAlphaType, SkColorSpace::MakeSRGB())); // What is drawn is not important. bm.eraseColor(SK_ColorBLUE); diff --git a/tests/DeferredDisplayListTest.cpp b/tests/DeferredDisplayListTest.cpp index caeed3660a..66aec1c8a7 100644 --- a/tests/DeferredDisplayListTest.cpp +++ b/tests/DeferredDisplayListTest.cpp @@ -289,10 +289,8 @@ public: break; case 3: // The color type and config need to be changed together. - // The original SRGB color space no longer makes sense for F16 fColorType = kRGBA_F16_SkColorType; fConfig = kRGBA_half_GrPixelConfig; - fColorSpace = SkColorSpace::MakeSRGBLinear(); break; case 4: // This just needs to be a colorSpace different from that returned by MakeSRGB(). diff --git a/tests/ReadPixelsTest.cpp b/tests/ReadPixelsTest.cpp index 5777a2919f..4079a8c893 100644 --- a/tests/ReadPixelsTest.cpp +++ b/tests/ReadPixelsTest.cpp @@ -678,14 +678,6 @@ DEF_TEST(ReadPixels_ValidConversion, reporter) { for (SkColorType srcCT : kColorTypes) { for (SkAlphaType srcAT: kAlphaTypes) { for (sk_sp srcCS : kColorSpaces) { - if (kRGBA_F16_SkColorType == dstCT && dstCS) { - dstCS = dstCS->makeLinearGamma(); - } - - if (kRGBA_F16_SkColorType == srcCT && srcCS) { - srcCS = srcCS->makeLinearGamma(); - } - test_conversion(reporter, SkImageInfo::Make(kNumPixels, 1, dstCT, dstAT, dstCS), SkImageInfo::Make(kNumPixels, 1, srcCT, srcAT, srcCS));