From 0ec2c7a2dc05e47d049e3eaec7a0097d8941c7e2 Mon Sep 17 00:00:00 2001 From: Mike Klein Date: Tue, 31 Dec 2019 01:29:26 +0000 Subject: [PATCH] Revert "refresh image shader cs/at logic" This reverts commit 4311f19158f0386aaefa16cc91a48606e4515d45. Reason for revert: layout test failures seemingly not having to do with bicubic+clamping+sRGB Original change's description: > refresh image shader cs/at logic > > Was working on SkVM versions of these when I noticed the existing > SkRasterPipeline code wasn't terribly tidy. The main gist here is that > we can let SkColorSpaceXformSteps::apply() handle almost all the > final transformation on the way out of the shader. > > I remain a little puzzled why I got a few significant diffs when I tried > leaving the A8 color unpremul, letting the steps handle matching the > alpha types instead of me manually with SkRasterPipeline::premul. For > now I've left that as-is. > > Similarly I think we could transform that A8 color ahead of time rather > than doing that over and over at runtime. This is something I've left > as a TODO, largely because I don't care enough about coloring A8 to > investigate right now. > > I've inlined all the logic of explaining src-is-normalized into this > shader (which is its only user). That makes it clearer that we can > always set that bit when bicubic sampling, since we're clamping anyway. > This causes some invisible diffs when switching to the optimized sRGB > transfer function stages, which we may or may not be able to get away > with without guarding... > > Change-Id: Ie6670c6ca5c69958f41aac88324341a10eb3bee1 > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/261763 > Reviewed-by: Brian Osman > Commit-Queue: Mike Klein TBR=mtklein@google.com,brianosman@google.com,reed@google.com Change-Id: I9c414cb751d9e51219b18dc3d4f54c92d06664ce No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://skia-review.googlesource.com/c/skia/+/261815 Reviewed-by: Mike Klein Commit-Queue: Mike Klein --- src/core/SkColorSpaceXformSteps.h | 4 ++++ src/shaders/SkImageShader.cpp | 39 +++++++++++++++---------------- 2 files changed, 23 insertions(+), 20 deletions(-) diff --git a/src/core/SkColorSpaceXformSteps.h b/src/core/SkColorSpaceXformSteps.h index 5551927c62..e470885cc6 100644 --- a/src/core/SkColorSpaceXformSteps.h +++ b/src/core/SkColorSpaceXformSteps.h @@ -43,6 +43,10 @@ struct SkColorSpaceXformSteps { void apply(float rgba[4]) const; void apply(SkRasterPipeline*, bool src_is_normalized) const; + void apply(SkRasterPipeline* p, SkColorType srcCT) const { + return this->apply(p, SkColorTypeIsNormalized(srcCT)); + } + Flags flags; bool srcTF_is_sRGB, diff --git a/src/shaders/SkImageShader.cpp b/src/shaders/SkImageShader.cpp index 98415e927b..1794cf03fd 100755 --- a/src/shaders/SkImageShader.cpp +++ b/src/shaders/SkImageShader.cpp @@ -479,37 +479,36 @@ bool SkImageShader::doStages(const SkStageRec& rec, SkImageStageUpdater* updater }; auto append_misc = [&] { - SkColorSpace* cs = info.colorSpace(); - SkAlphaType at = info.alphaType(); - - // Color for A8 images comes from the paint. TODO: all alpha formats? maybe none? + // TODO: if ref.fDstCS isn't null, we'll premul here then immediately unpremul + // to do the color space transformation. Might be possible to streamline. if (info.colorType() == kAlpha_8_SkColorType) { + // The color for A8 images comes from the (sRGB) paint color. p->append_set_rgb(alloc, rec.fPaint.getColor4f()); p->append(SkRasterPipeline::premul); - cs = sk_srgb_singleton(); // TODO: transform to dstCS here instead of at runtime? - at = kPremul_SkAlphaType; + } else if (info.alphaType() == kUnpremul_SkAlphaType) { + // Convert unpremul images to premul before we carry on with the rest of the pipeline. + p->append(SkRasterPipeline::premul); } - // This is an unessential optimization... it's logically safe to set this to false. - // But if... - // - we know the image is definitely normalized, and - // - we're doing some color space conversion, and - // - sRGB curves are involved, - // then we can use slightly faster math that doesn't work well outside [0,1]. - bool src_is_normalized = SkColorTypeIsNormalized(info.colorType()); - - // Bicubic filtering naturally produces out of range values on both sides of [0,1]. if (quality > kLow_SkFilterQuality) { + // Bicubic filtering naturally produces out of range values on both sides. p->append(SkRasterPipeline::clamp_0); p->append(fClampAsIfUnpremul ? SkRasterPipeline::clamp_1 : SkRasterPipeline::clamp_a); - src_is_normalized = true; } - // Transform color space and alpha type to match shader convention (dst CS, premul alpha). - alloc->make(cs, at, - rec.fDstCS, kPremul_SkAlphaType) - ->apply(p, src_is_normalized); + if (rec.fDstCS) { + // If color managed, convert from premul source all the way to premul dst color space. + auto srcCS = info.colorSpace(); + if (!srcCS || info.colorType() == kAlpha_8_SkColorType) { + // We treat untagged images as sRGB. + // A8 images get their r,g,b from the paint color, so they're also sRGB. + srcCS = sk_srgb_singleton(); + } + alloc->make(srcCS , kPremul_SkAlphaType, + rec.fDstCS, kPremul_SkAlphaType) + ->apply(p, info.colorType()); + } return true; };