move dither after the transfer function
There's no single dither rate that we can use in linear space if we're
using a non-linear transfer function... if it's too high (like today)
we'll dither too much around zero (e.g. 0 -> 5), and if it's too low we
won't dither near one.
We were thinking it'd be a good idea to move the dither later in the
pipeline anyway. This has to be the right spot!
Now that we're moving dither from operating in linear space to operating
in bit-space (after transfer function, aware of bit-depth of
destination) we have to start clamping [0,1] instead of [0,a]...
But, I think I've rewritten things to make sure we don't need to clamp
at all. The main idea is to make sure 0-dither and 1+dither round to 0
and 1 respectively. We can do this by making the dither span exclusive,
switching from [-0.5,+0.5] to (-0.5,+0.5). In practice I'm doing that
as [-0.4921875,+0.4921875], a maximum dither of 63/128 of a bit.
Similarly, I don't think it makes sense to fold in the multiply by alpha
anymore if we're after the transfer function.
Change-Id: I55857bca80377c639fcdd29acc9b362931dd9d12
Reviewed-on: https://skia-review.googlesource.com/15254
Reviewed-by: Florin Malita <fmalita@chromium.org>
Commit-Queue: Mike Klein <mtklein@chromium.org>
diff --git a/src/core/SkRasterPipelineBlitter.cpp b/src/core/SkRasterPipelineBlitter.cpp
index 576ba4b..d18decd 100644
--- a/src/core/SkRasterPipelineBlitter.cpp
+++ b/src/core/SkRasterPipelineBlitter.cpp
@@ -60,10 +60,11 @@
// These values are pointed to by the blit pipelines above,
// which allows us to adjust them from call to call.
- void* fDstPtr = nullptr;
- const void* fMaskPtr = nullptr;
- float fCurrentCoverage = 0.0f;
- int fCurrentY = 0;
+ void* fDstPtr = nullptr;
+ const void* fMaskPtr = nullptr;
+ float fCurrentCoverage = 0.0f;
+ int fCurrentY = 0;
+ SkJumper_DitherCtx fDitherCtx = { &fCurrentY, 0.0f };
typedef SkBlitter INHERITED;
};
@@ -107,8 +108,21 @@
return nullptr;
}
+ // TODO: Think more about under what conditions we dither:
+ // - if we're drawing anything into 565 and the user has asked us to dither, or
+ // - if we're drawing a gradient into 565 or 8888.
+ if ((paint.isDither() && dst.info().colorType() == kRGB_565_SkColorType) ||
+ (shader && shader->asAGradient(nullptr) >= SkShader::kLinear_GradientType)) {
+ switch (dst.info().colorType()) {
+ default: blitter->fDitherCtx.rate = 0.0f; break;
+ case kRGB_565_SkColorType: blitter->fDitherCtx.rate = 1/63.0f; break;
+ case kRGBA_8888_SkColorType:
+ case kBGRA_8888_SkColorType: blitter->fDitherCtx.rate = 1/255.0f; break;
+ }
+ }
+
bool is_opaque = paintColor->a() == 1.0f,
- is_constant = true;
+ is_constant = blitter->fDitherCtx.rate == 0.0f;
if (shader) {
pipeline->append(SkRasterPipeline::seed_shader, &blitter->fCurrentY);
if (!shader->appendStages(pipeline, dst.colorSpace(), alloc, ctm, paint)) {
@@ -119,8 +133,8 @@
&paintColor->fVec[SkPM4f::A]);
}
- is_opaque = is_opaque && shader->isOpaque();
- is_constant = shader->isConstant();
+ is_opaque = is_opaque && shader->isOpaque();
+ is_constant = is_constant && shader->isConstant();
} else {
pipeline->append(SkRasterPipeline::constant_color, paintColor);
}
@@ -132,30 +146,6 @@
is_opaque = is_opaque && (colorFilter->getFlags() & SkColorFilter::kAlphaUnchanged_Flag);
}
- // TODO: Think more about under what conditions we dither:
- // - if we're drawing anything into 565 and the user has asked us to dither, or
- // - if we're drawing a gradient into 565 or 8888.
- // TODO: move this later in the pipeline, perhaps the first thing we do in append_store()?
- if ((paint.isDither() && dst.info().colorType() == kRGB_565_SkColorType) ||
- (shader && shader->asAGradient(nullptr) >= SkShader::kLinear_GradientType)) {
- float rate;
- switch (dst.info().colorType()) {
- case kRGB_565_SkColorType: rate = 1/63.0f; break;
- case kBGRA_8888_SkColorType:
- case kRGBA_8888_SkColorType: rate = 1/255.0f; break;
- default: rate = 0.0f; break;
- }
- if (rate) {
- auto ctx = alloc->make<SkJumper_DitherCtx>();
- ctx->y = &blitter->fCurrentY;
- ctx->rate = rate;
- pipeline->append(SkRasterPipeline::dither, ctx);
- pipeline->append(SkRasterPipeline::clamp_0);
- pipeline->append(SkRasterPipeline::clamp_a);
- is_constant = false;
- }
- }
-
if (is_constant) {
pipeline->append(SkRasterPipeline::store_f32, &paintColor);
pipeline->run(0,1);
@@ -208,10 +198,15 @@
if (fDst.info().gammaCloseToSRGB()) {
p->append(SkRasterPipeline::to_srgb);
}
+ if (fDitherCtx.rate > 0.0f) {
+ // We dither after any sRGB transfer function to make sure our 1/255.0f is sensible
+ // over the whole range. If we did it before, 1/255.0f is too big a rate near zero.
+ p->append(SkRasterPipeline::dither, &fDitherCtx);
+ }
+
if (fDst.info().colorType() == kBGRA_8888_SkColorType) {
p->append(SkRasterPipeline::swap_rb);
}
-
SkASSERT(supported(fDst.info()));
switch (fDst.info().colorType()) {
case kAlpha_8_SkColorType: p->append(SkRasterPipeline::store_a8, &fDstPtr); break;