diff options
author | Mike Klein <mtklein@chromium.org> | 2016-12-14 13:38:24 +0000 |
---|---|---|
committer | Skia Commit-Bot <skia-commit-bot@chromium.org> | 2016-12-14 13:38:41 +0000 |
commit | d37d5d96493604c12cfaa2d64bcbd32c41b01f3b (patch) | |
tree | 3de3ad9a4a08194c75314b07736170a7150fb5f2 | |
parent | 65115a1b1a5c72b47492dc447d1d282353ae3121 (diff) |
Revert "Revert "clamp to premul when reading premul sRGB""
This reverts commit 2e018f548d76b0688f9873c683cffc681fec40ec.
Reason for revert: doesn't appear to have been the roll problem.
Original change's description:
> Revert "clamp to premul when reading premul sRGB"
>
> This reverts commit 04e10da8362a0dcabd795a4ad53f617719ca0d20.
>
> Reason for revert: roll?
>
> Change-Id: Id0a8dcd62763bd6eddde120c513ca97e098a4268
> Reviewed-on: https://skia-review.googlesource.com/6022
> Commit-Queue: Mike Klein <mtklein@chromium.org>
> Reviewed-by: Mike Klein <mtklein@chromium.org>
>
TBR=mtklein@chromium.org,reviews@skia.org,brianosman@google.com
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
Change-Id: I399ca5e728ce6766c6707682c4c6b685681ffdeb
Reviewed-on: https://skia-review.googlesource.com/6025
Commit-Queue: Mike Klein <mtklein@chromium.org>
Reviewed-by: Mike Klein <mtklein@chromium.org>
-rw-r--r-- | bench/SkRasterPipelineBench.cpp | 4 | ||||
-rw-r--r-- | src/core/SkConfig8888.cpp | 2 | ||||
-rw-r--r-- | src/core/SkRasterPipeline.cpp | 34 | ||||
-rw-r--r-- | src/core/SkRasterPipeline.h | 8 | ||||
-rw-r--r-- | src/core/SkRasterPipelineBlitter.cpp | 2 | ||||
-rw-r--r-- | src/image/SkImageShader.cpp | 2 | ||||
-rw-r--r-- | src/opts/SkRasterPipeline_opts.h | 16 |
7 files changed, 57 insertions, 11 deletions
diff --git a/bench/SkRasterPipelineBench.cpp b/bench/SkRasterPipelineBench.cpp index 64e59c8b54..16dea8aa66 100644 --- a/bench/SkRasterPipelineBench.cpp +++ b/bench/SkRasterPipelineBench.cpp @@ -38,13 +38,13 @@ public: SkRasterPipeline p; p.append(SkRasterPipeline::load_8888, &src_ctx); - p.append(SkRasterPipeline::from_srgb); + p.append_from_srgb(kUnpremul_SkAlphaType); p.append(SkRasterPipeline::scale_u8, &mask_ctx); if (kF16) { p.append(SkRasterPipeline::load_f16_d, &dst_ctx); } else { p.append(SkRasterPipeline::load_8888_d, &dst_ctx); - p.append(SkRasterPipeline::from_srgb_d); + p.append_from_srgb_d(kPremul_SkAlphaType); } p.append(SkRasterPipeline::srcover); if (kF16) { diff --git a/src/core/SkConfig8888.cpp b/src/core/SkConfig8888.cpp index 3906a9a1ef..08635a2b84 100644 --- a/src/core/SkConfig8888.cpp +++ b/src/core/SkConfig8888.cpp @@ -47,7 +47,7 @@ static bool copy_pipeline_pixels(const SkImageInfo& dstInfo, void* dstRow, size_ case kBGRA_8888_SkColorType: pipeline.append(SkRasterPipeline::load_8888, &srcRow); if (src_srgb) { - pipeline.append(SkRasterPipeline::from_srgb); + pipeline.append_from_srgb(srcInfo.alphaType()); } if (swap_rb) { pipeline.append(SkRasterPipeline::swap_rb); diff --git a/src/core/SkRasterPipeline.cpp b/src/core/SkRasterPipeline.cpp index 4c1dbc762e..00ba81e6f3 100644 --- a/src/core/SkRasterPipeline.cpp +++ b/src/core/SkRasterPipeline.cpp @@ -11,6 +11,14 @@ SkRasterPipeline::SkRasterPipeline() {} void SkRasterPipeline::append(StockStage stage, void* ctx) { +#ifdef SK_DEBUG + switch (stage) { + case from_srgb: + case from_srgb_d: + SkDEBUGFAIL("Please use append_srgb[_d]() instead."); + default: break; + } +#endif fStages.push_back({stage, ctx}); } @@ -42,3 +50,29 @@ void SkRasterPipeline::dump() const { } SkDebugf("\n"); } + +// It's pretty easy to start with sound premultiplied linear floats, pack those +// to sRGB encoded bytes, then read them back to linear floats and find them not +// quite premultiplied, with a color channel just a smidge greater than the alpha +// channel. This can happen basically any time we have different transfer +// functions for alpha and colors... sRGB being the only one we draw into. + +// This is an annoying problem with no known good solution. So apply the clamp hammer. + +void SkRasterPipeline::append_from_srgb(SkAlphaType at) { + //this->append(from_srgb); + fStages.push_back({from_srgb, nullptr}); + + if (at == kPremul_SkAlphaType) { + this->append(SkRasterPipeline::clamp_a); + } +} + +void SkRasterPipeline::append_from_srgb_d(SkAlphaType at) { + //this->append(from_srgb_d); + fStages.push_back({from_srgb_d, nullptr}); + + if (at == kPremul_SkAlphaType) { + this->append(SkRasterPipeline::clamp_a_d); + } +} diff --git a/src/core/SkRasterPipeline.h b/src/core/SkRasterPipeline.h index b3f513d69e..0486bb3e34 100644 --- a/src/core/SkRasterPipeline.h +++ b/src/core/SkRasterPipeline.h @@ -8,6 +8,7 @@ #ifndef SkRasterPipeline_DEFINED #define SkRasterPipeline_DEFINED +#include "SkImageInfo.h" #include "SkNx.h" #include "SkTArray.h" #include "SkTypes.h" @@ -58,7 +59,7 @@ #define SK_RASTER_PIPELINE_STAGES(M) \ M(trace) M(registers) \ M(move_src_dst) M(move_dst_src) M(swap_rb) M(swap_rb_d) \ - M(clamp_0) M(clamp_a) M(clamp_1) \ + M(clamp_0) M(clamp_1) M(clamp_a) M(clamp_a_d) \ M(unpremul) M(premul) \ M(set_rgb) \ M(from_srgb) M(from_srgb_d) M(to_srgb) \ @@ -119,6 +120,11 @@ public: void* ctx; }; + // Conversion from sRGB can be subtly tricky when premultiplication is involved. + // Use these helpers to keep things sane. + void append_from_srgb (SkAlphaType); + void append_from_srgb_d(SkAlphaType); + private: std::vector<Stage> fStages; }; diff --git a/src/core/SkRasterPipelineBlitter.cpp b/src/core/SkRasterPipelineBlitter.cpp index d2a0b3d4c1..cbdb09faa4 100644 --- a/src/core/SkRasterPipelineBlitter.cpp +++ b/src/core/SkRasterPipelineBlitter.cpp @@ -197,7 +197,7 @@ void SkRasterPipelineBlitter::append_load_d(SkRasterPipeline* p) const { p->append(SkRasterPipeline::swap_rb_d); } if (fDst.info().gammaCloseToSRGB()) { - p->append(SkRasterPipeline::from_srgb_d); + p->append_from_srgb_d(fDst.info().alphaType()); } } diff --git a/src/image/SkImageShader.cpp b/src/image/SkImageShader.cpp index ce9cbadbbc..e009b4a849 100644 --- a/src/image/SkImageShader.cpp +++ b/src/image/SkImageShader.cpp @@ -350,7 +350,7 @@ bool SkImageShader::onAppendStages(SkRasterPipeline* p, SkColorSpace* dst, SkFal default: SkASSERT(false); } if (info.gammaCloseToSRGB() && dst != nullptr) { - p->append(SkRasterPipeline::from_srgb); + p->append_from_srgb(info.alphaType()); } }; diff --git a/src/opts/SkRasterPipeline_opts.h b/src/opts/SkRasterPipeline_opts.h index 3cc3f2f01a..135d927b85 100644 --- a/src/opts/SkRasterPipeline_opts.h +++ b/src/opts/SkRasterPipeline_opts.h @@ -301,17 +301,23 @@ STAGE(clamp_0) { g = SkNf::Max(g, 0.0f); b = SkNf::Max(b, 0.0f); } +STAGE(clamp_1) { + a = SkNf::Min(a, 1.0f); + r = SkNf::Min(r, 1.0f); + g = SkNf::Min(g, 1.0f); + b = SkNf::Min(b, 1.0f); +} STAGE(clamp_a) { a = SkNf::Min(a, 1.0f); r = SkNf::Min(r, a); g = SkNf::Min(g, a); b = SkNf::Min(b, a); } -STAGE(clamp_1) { - a = SkNf::Min(a, 1.0f); - r = SkNf::Min(r, 1.0f); - g = SkNf::Min(g, 1.0f); - b = SkNf::Min(b, 1.0f); +STAGE(clamp_a_d) { + da = SkNf::Min(da, 1.0f); + dr = SkNf::Min(dr, da); + dg = SkNf::Min(dg, da); + db = SkNf::Min(db, da); } STAGE(unpremul) { |