diff options
author | Mike Klein <mtklein@chromium.org> | 2016-12-13 15:44:23 -0500 |
---|---|---|
committer | Skia Commit-Bot <skia-commit-bot@chromium.org> | 2016-12-13 22:09:57 +0000 |
commit | 04e10da8362a0dcabd795a4ad53f617719ca0d20 (patch) | |
tree | 203575b49fb941990f0a19b288b6c6cc1eef9388 | |
parent | 21f783829619186442041de6008f7f58f4f6250d (diff) |
clamp to premul when reading premul sRGB
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.
These new calls on SkRasterPipeline should make it impossible to get wrong.
CQ_INCLUDE_TRYBOTS=skia.primary:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD
Change-Id: I4c974f4a7b151f3f684946f1e83d06b1b288fd01
Reviewed-on: https://skia-review.googlesource.com/5945
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: 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) { |