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>

1 files changed, 27 insertions, 32 deletions

diff --git a/src/core/SkRasterPipelineBlitter.cpp b/src/core/SkRasterPipelineBlitter.cpp
index 576ba4ba19..d18decd1cd 100644
--- a/src/core/SkRasterPipelineBlitter.cpp
+++ b/src/core/SkRasterPipelineBlitter.cpp
@@ -60,10 +60,11 @@ private:
 
     // 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 @@ SkBlitter* SkRasterPipelineBlitter::Create(const SkPixmap& dst,
         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 @@ SkBlitter* SkRasterPipelineBlitter::Create(const SkPixmap& dst,
                              &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 @@ SkBlitter* SkRasterPipelineBlitter::Create(const SkPixmap& dst,
         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 @@ void SkRasterPipelineBlitter::append_store(SkRasterPipeline* p) const {
     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;