Switch SkBlurImageFilter over to new onFilterImage interface

src/effects/SkBlurImageFilter.cpp

Index: src/effects/SkBlurImageFilter.cpp
diff --git a/src/effects/SkBlurImageFilter.cpp b/src/effects/SkBlurImageFilter.cpp
index 249e69779deec6ff2490f2be5bbad3ed967f2bf6..421d8f1d2d16b8747a3b9c431b6e7cad0c0711c0 100644
--- a/src/effects/SkBlurImageFilter.cpp
+++ b/src/effects/SkBlurImageFilter.cpp
@@ -5,14 +5,16 @@
  * found in the LICENSE file.
  */
 
-#include "SkBitmap.h"
 #include "SkBlurImageFilter.h"
+
+#include "SkAutoPixmapStorage.h"
 #include "SkColorPriv.h"
-#include "SkDevice.h"
 #include "SkGpuBlurUtils.h"
 #include "SkOpts.h"
 #include "SkReadBuffer.h"
+#include "SkSpecialImage.h"
 #include "SkWriteBuffer.h"
+
 #if SK_SUPPORT_GPU
 #include "GrContext.h"
 #include "SkGr.h"
@@ -53,8 +55,8 @@ void SkBlurImageFilter::flatten(SkWriteBuffer& buffer) const {
     buffer.writeScalar(fSigma.fHeight);
 }
 
-static void getBox3Params(SkScalar s, int *kernelSize, int* kernelSize3, int *lowOffset,
-                          int *highOffset)
+static void get_box3_params(SkScalar s, int *kernelSize, int* kernelSize3, int *lowOffset,
+                            int *highOffset)
 {
     float pi = SkScalarToFloat(SK_ScalarPI);
     int d = static_cast<int>(floorf(SkScalarToFloat(s) * 3.0f * sqrtf(2.0f * pi) / 4.0f + 0.5f));
@@ -69,113 +71,156 @@ static void getBox3Params(SkScalar s, int *kernelSize, int* kernelSize3, int *lo
     }
 }
 
-bool SkBlurImageFilter::onFilterImageDeprecated(Proxy* proxy,
-                                                const SkBitmap& source, const Context& ctx,
-                                                SkBitmap* dst, SkIPoint* offset) const {
-    SkBitmap src = source;
-    SkIPoint srcOffset = SkIPoint::Make(0, 0);
-    if (!this->filterInputDeprecated(0, proxy, source, ctx, &src, &srcOffset)) {
-        return false;
-    }
+SkSpecialImage* SkBlurImageFilter::onFilterImage(SkSpecialImage* source, const Context& ctx,
+                                                 SkIPoint* offset) const {
+    SkIPoint inputOffset = SkIPoint::Make(0, 0);
 
-    if (src.colorType() != kN32_SkColorType) {
-        return false;
+    SkAutoTUnref<SkSpecialImage> input(this->filterInput(0, source, ctx, &inputOffset));
+    if (!input) {
+        return nullptr;
     }
 
-    SkIRect srcBounds = src.bounds();
-    srcBounds.offset(srcOffset);
+    SkIRect inputBounds = SkIRect::MakeXYWH(inputOffset.fX, inputOffset.fY,
+                                            input->width(), input->height());
+
     SkIRect dstBounds;
-    if (!this->applyCropRect(this->mapContext(ctx), srcBounds, &dstBounds)) {
-        return false;
+    if (!this->applyCropRect(this->mapContext(ctx), inputBounds, &dstBounds)) {
+        return nullptr;
     }
-    if (!srcBounds.intersect(dstBounds)) {
-        return false;
+    if (!inputBounds.intersect(dstBounds)) {
+        return nullptr;
     }
 
-    SkVector sigma = map_sigma(fSigma, ctx.ctm());
+    const SkVector sigma = map_sigma(fSigma, ctx.ctm());
+
+#if SK_SUPPORT_GPU
+    if (input->peekTexture()) {
+        if (0 == sigma.x() && 0 == sigma.y()) {

[Stephen White, 2016/03/21 18:11:28]

Side note: we should probably consider optimizing the zero-sigma-with-croprect
case into another filter in the Create() factory. Or at a minimum, combining the
raster & GPU versions of this early-out.

[robertphillips, 2016/03/21 21:32:40]

Acknowledged. I considered it for this CL but the raster path does a little bit
of extra work.

+            offset->fX = inputBounds.x();
+            offset->fY = inputBounds.y();
+            return input->makeSubset(inputBounds.makeOffset(-inputOffset.x(),
+                                                            -inputOffset.y())).release();
+        }
+
+        GrTexture* inputTexture = input->peekTexture();
+
+        offset->fX = dstBounds.fLeft;
+        offset->fY = dstBounds.fTop;
+        inputBounds.offset(-inputOffset);
+        dstBounds.offset(-inputOffset);
+        SkRect inputBoundsF(SkRect::Make(inputBounds));
+        SkAutoTUnref<GrTexture> tex(SkGpuBlurUtils::GaussianBlur(inputTexture->getContext(),
+                                                                 inputTexture,
+                                                                 false,
+                                                                 SkRect::Make(dstBounds),
+                                                                 &inputBoundsF,
+                                                                 sigma.x(),
+                                                                 sigma.y()));
+        if (!tex) {
+            return nullptr;
+        }
+
+        return SkSpecialImage::MakeFromGpu(source->internal_getProxy(),
+                                           SkIRect::MakeWH(dstBounds.width(), dstBounds.height()),
+                                           kNeedNewImageUniqueID_SpecialImage,
+                                           tex).release();

[Stephen White, 2016/03/21 18:11:28]

Q: out of curiosity, is this release() necessary for correctness?

[robertphillips, 2016/03/21 21:32:40]

I think so. MakeFromGpu now returns an sk_sp which will unref its owned pointer
in its destructor.

+    }
+#endif
 
     int kernelSizeX, kernelSizeX3, lowOffsetX, highOffsetX;
     int kernelSizeY, kernelSizeY3, lowOffsetY, highOffsetY;
-    getBox3Params(sigma.x(), &kernelSizeX, &kernelSizeX3, &lowOffsetX, &highOffsetX);
-    getBox3Params(sigma.y(), &kernelSizeY, &kernelSizeY3, &lowOffsetY, &highOffsetY);
+    get_box3_params(sigma.x(), &kernelSizeX, &kernelSizeX3, &lowOffsetX, &highOffsetX);
+    get_box3_params(sigma.y(), &kernelSizeY, &kernelSizeY3, &lowOffsetY, &highOffsetY);
 
     if (kernelSizeX < 0 || kernelSizeY < 0) {
-        return false;
+        return nullptr;
     }
 
     if (kernelSizeX == 0 && kernelSizeY == 0) {
-        src.extractSubset(dst, srcBounds.makeOffset(-srcOffset.x(), -srcOffset.y()));
-        offset->fX = srcBounds.x();
-        offset->fY = srcBounds.y();
-        return true;
+        offset->fX = inputBounds.x();
+        offset->fY = inputBounds.y();
+        return input->makeSubset(inputBounds.makeOffset(-inputOffset.x(),
+                                                        -inputOffset.y())).release();
     }
 
-    SkAutoLockPixels alp(src);
-    if (!src.getPixels()) {
-        return false;
-    }
+    SkPixmap inputPixmap;
 
-    SkAutoTUnref<SkBaseDevice> device(proxy->createDevice(dstBounds.width(), dstBounds.height()));
-    if (!device) {
-        return false;
+    if (!input->peekPixels(&inputPixmap)) {
+        return nullptr;
     }
-    *dst = device->accessBitmap(false);
-    SkAutoLockPixels alp_dst(*dst);
 
-    SkAutoTUnref<SkBaseDevice> tempDevice(proxy->createDevice(dst->width(), dst->height()));
-    if (!tempDevice) {
-        return false;
+    if (inputPixmap.colorType() != kN32_SkColorType) {
+        return nullptr;
     }
-    SkBitmap temp = tempDevice->accessBitmap(false);
-    SkAutoLockPixels alpTemp(temp);
+
+    SkImageInfo info = SkImageInfo::Make(dstBounds.width(), dstBounds.height(),
+                                         inputPixmap.colorType(), inputPixmap.alphaType());
+
+    SkAutoPixmapStorage dst, tmp;
+
+    dst.alloc(info);
+    tmp.alloc(info);
 
     offset->fX = dstBounds.fLeft;
     offset->fY = dstBounds.fTop;
-    SkPMColor* t = temp.getAddr32(0, 0);
-    SkPMColor* d = dst->getAddr32(0, 0);
+    SkPMColor* t = (SkPMColor*) tmp.addr32(0, 0);

[Stephen White, 2016/03/21 18:11:28]

Could we use writeable_addr() here to avoid a cast?

[robertphillips, 2016/03/21 21:32:40]

Done.

+    SkPMColor* d = (SkPMColor*) dst.addr32(0, 0);
     int w = dstBounds.width(), h = dstBounds.height();
-    const SkPMColor* s = src.getAddr32(srcBounds.x() - srcOffset.x(), srcBounds.y() - srcOffset.y());
-    srcBounds.offset(-dstBounds.x(), -dstBounds.y());
+    const SkPMColor* s = inputPixmap.addr32(inputBounds.x() - inputOffset.x(),
+                                            inputBounds.y() - inputOffset.y());
+    inputBounds.offset(-dstBounds.x(), -dstBounds.y());
     dstBounds.offset(-dstBounds.x(), -dstBounds.y());
-    SkIRect srcBoundsT = SkIRect::MakeLTRB(srcBounds.top(), srcBounds.left(), srcBounds.bottom(), srcBounds.right());
+    SkIRect inputBoundsT = SkIRect::MakeLTRB(inputBounds.top(), inputBounds.left(),
+                                             inputBounds.bottom(), inputBounds.right());
     SkIRect dstBoundsT = SkIRect::MakeWH(dstBounds.height(), dstBounds.width());
-    int sw = src.rowBytesAsPixels();
+    int sw = inputPixmap.rowBytes() >> 2;
 
     /**
-     *
-     * In order to make memory accesses cache-friendly, we reorder the passes to
-     * use contiguous memory reads wherever possible.
-     *
-     * For example, the 6 passes of the X-and-Y blur case are rewritten as
-     * follows. Instead of 3 passes in X and 3 passes in Y, we perform
-     * 2 passes in X, 1 pass in X transposed to Y on write, 2 passes in X,
-     * then 1 pass in X transposed to Y on write.
-     *
-     * +----+       +----+       +----+        +---+       +---+       +---+        +----+
-     * + AB + ----> | AB | ----> | AB | -----> | A | ----> | A | ----> | A | -----> | AB |
-     * +----+ blurX +----+ blurX +----+ blurXY | B | blurX | B | blurX | B | blurXY +----+
-     *                                         +---+       +---+       +---+
-     *
-     * In this way, two of the y-blurs become x-blurs applied to transposed
-     * images, and all memory reads are contiguous.
-     */
+        *
+        * In order to make memory accesses cache-friendly, we reorder the passes to
+        * use contiguous memory reads wherever possible.
+        *
+        * For example, the 6 passes of the X-and-Y blur case are rewritten as
+        * follows. Instead of 3 passes in X and 3 passes in Y, we perform
+        * 2 passes in X, 1 pass in X transposed to Y on write, 2 passes in X,
+        * then 1 pass in X transposed to Y on write.
+        *
+        * +----+       +----+       +----+        +---+       +---+       +---+        +----+
+        * + AB + ----> | AB | ----> | AB | -----> | A | ----> | A | ----> | A | -----> | AB |
+        * +----+ blurX +----+ blurX +----+ blurXY | B | blurX | B | blurX | B | blurXY +----+
+        *                                         +---+       +---+       +---+
+        *
+        * In this way, two of the y-blurs become x-blurs applied to transposed
+        * images, and all memory reads are contiguous.
+        */
     if (kernelSizeX > 0 && kernelSizeY > 0) {
-        SkOpts::box_blur_xx(s, sw,  srcBounds,  t, kernelSizeX,  lowOffsetX,  highOffsetX, w, h);
-        SkOpts::box_blur_xx(t,  w,  dstBounds,  d, kernelSizeX,  highOffsetX, lowOffsetX,  w, h);
-        SkOpts::box_blur_xy(d,  w,  dstBounds,  t, kernelSizeX3, highOffsetX, highOffsetX, w, h);
-        SkOpts::box_blur_xx(t,  h,  dstBoundsT, d, kernelSizeY,  lowOffsetY,  highOffsetY, h, w);
-        SkOpts::box_blur_xx(d,  h,  dstBoundsT, t, kernelSizeY,  highOffsetY, lowOffsetY,  h, w);
-        SkOpts::box_blur_xy(t,  h,  dstBoundsT, d, kernelSizeY3, highOffsetY, highOffsetY, h, w);
+        SkOpts::box_blur_xx(s, sw,  inputBounds,  t, kernelSizeX,  lowOffsetX,  highOffsetX, w, h);
+        SkOpts::box_blur_xx(t,  w,  dstBounds,    d, kernelSizeX,  highOffsetX, lowOffsetX,  w, h);
+        SkOpts::box_blur_xy(d,  w,  dstBounds,    t, kernelSizeX3, highOffsetX, highOffsetX, w, h);
+        SkOpts::box_blur_xx(t,  h,  dstBoundsT,   d, kernelSizeY,  lowOffsetY,  highOffsetY, h, w);
+        SkOpts::box_blur_xx(d,  h,  dstBoundsT,   t, kernelSizeY,  highOffsetY, lowOffsetY,  h, w);
+        SkOpts::box_blur_xy(t,  h,  dstBoundsT,   d, kernelSizeY3, highOffsetY, highOffsetY, h, w);
     } else if (kernelSizeX > 0) {
-        SkOpts::box_blur_xx(s, sw,  srcBounds,  d, kernelSizeX,  lowOffsetX,  highOffsetX, w, h);
-        SkOpts::box_blur_xx(d,  w,  dstBounds,  t, kernelSizeX,  highOffsetX, lowOffsetX,  w, h);
-        SkOpts::box_blur_xx(t,  w,  dstBounds,  d, kernelSizeX3, highOffsetX, highOffsetX, w, h);
+        SkOpts::box_blur_xx(s, sw,  inputBounds,  d, kernelSizeX,  lowOffsetX,  highOffsetX, w, h);
+        SkOpts::box_blur_xx(d,  w,  dstBounds,    t, kernelSizeX,  highOffsetX, lowOffsetX,  w, h);
+        SkOpts::box_blur_xx(t,  w,  dstBounds,    d, kernelSizeX3, highOffsetX, highOffsetX, w, h);
     } else if (kernelSizeY > 0) {
-        SkOpts::box_blur_yx(s, sw,  srcBoundsT, d, kernelSizeY,  lowOffsetY,  highOffsetY, h, w);
-        SkOpts::box_blur_xx(d,  h,  dstBoundsT, t, kernelSizeY,  highOffsetY, lowOffsetY,  h, w);
-        SkOpts::box_blur_xy(t,  h,  dstBoundsT, d, kernelSizeY3, highOffsetY, highOffsetY, h, w);
+        SkOpts::box_blur_yx(s, sw,  inputBoundsT, d, kernelSizeY,  lowOffsetY,  highOffsetY, h, w);
+        SkOpts::box_blur_xx(d,  h,  dstBoundsT,   t, kernelSizeY,  highOffsetY, lowOffsetY,  h, w);
+        SkOpts::box_blur_xy(t,  h,  dstBoundsT,   d, kernelSizeY3, highOffsetY, highOffsetY, h, w);
     }
-    return true;
+
+    sk_sp<SkSpecialImage> result(SkSpecialImage::MakeFromPixmap(
+                                                            source->internal_getProxy(), 
+                                                            SkIRect::MakeWH(dstBounds.width(),
+                                                                            dstBounds.height()),
+                                                            dst,
+                                                            [] (void* addr, void*) -> void {
+                                                                sk_free(addr);

[Stephen White, 2016/03/21 18:11:28]

This feels really error-prone (requiring an sk_free() lambda). Is there a better
way we could encapsulate the memory management here? Presumably we're going to
need it in all the filters with pixel loops.

[robertphillips, 2016/03/21 21:32:40]

The MakeFromPixmap entry point is patterned on SkImage::MakeFromRaster which has
the ReleaseProc & ReleaseContext pair. It appears that this is the approved way
to deal with raster-specific pixel APIs.

[Stephen White, 2016/03/21 21:49:46]

Could we perhaps create an SkSpecialImage::MakeFromPixmap() that takes an
SkAutoPixmapStorage? Then we could hide the sk_free() and the release() call in
there.

+                                                            },
+                                                            nullptr));
+    dst.release();  // 'result' now owns the pixels

[Stephen White, 2016/03/21 18:11:28]

This also feels error-prone.

[robertphillips, 2016/03/21 21:32:40]

The new SkPixmap-based APIs push the lifetime management onto the callers. We
could store the dst pixels in something other than an SkAutoPixmapStorage but we
would have to copy a bit of code. If it makes it any more palatable one can
think of the SkAutoPixmapStorage as owning a ref on the pixel data which it is
now releasing.

[Stephen White, 2016/03/21 21:49:46]

It's more about copying these bits of code (the sk_free() lambda and the
release()) into all raster image filters that I'm worried about.

+    return result.release();
 }
 
 
@@ -198,55 +243,6 @@ void SkBlurImageFilter::onFilterNodeBounds(const SkIRect& src, const SkMatrix& c
                 SkScalarCeilToInt(SkScalarMul(sigma.y(), SkIntToScalar(3))));
 }
 
-bool SkBlurImageFilter::filterImageGPUDeprecated(Proxy* proxy, const SkBitmap& src,
-                                                 const Context& ctx,
-                                                 SkBitmap* result, SkIPoint* offset) const {
-#if SK_SUPPORT_GPU
-    SkBitmap input = src;
-    SkIPoint srcOffset = SkIPoint::Make(0, 0);
-    if (!this->filterInputGPUDeprecated(0, proxy, src, ctx, &input, &srcOffset)) {
-        return false;
-    }
-    SkIRect srcBounds = input.bounds();
-    srcBounds.offset(srcOffset);
-    SkIRect dstBounds;
-    if (!this->applyCropRect(this->mapContext(ctx), srcBounds, &dstBounds)) {
-        return false;
-    }
-    if (!srcBounds.intersect(dstBounds)) {
-        return false;
-    }
-    SkVector sigma = map_sigma(fSigma, ctx.ctm());
-    if (sigma.x() == 0 && sigma.y() == 0) {
-        input.extractSubset(result, srcBounds.makeOffset(-srcOffset.x(), -srcOffset.y()));
-        offset->fX = srcBounds.x();
-        offset->fY = srcBounds.y();
-        return true;
-    }
-    offset->fX = dstBounds.fLeft;
-    offset->fY = dstBounds.fTop;
-    srcBounds.offset(-srcOffset);
-    dstBounds.offset(-srcOffset);
-    SkRect srcBoundsF(SkRect::Make(srcBounds));
-    GrTexture* inputTexture = input.getTexture();
-    SkAutoTUnref<GrTexture> tex(SkGpuBlurUtils::GaussianBlur(inputTexture->getContext(),
-                                                             inputTexture,
-                                                             false,
-                                                             SkRect::Make(dstBounds),
-                                                             &srcBoundsF,
-                                                             sigma.x(),
-                                                             sigma.y()));
-    if (!tex) {
-        return false;
-    }
-    GrWrapTextureInBitmap(tex, dstBounds.width(), dstBounds.height(), false, result);
-    return true;
-#else
-    SkDEBUGFAIL("Should not call in GPU-less build");
-    return false;
-#endif
-}
-
 #ifndef SK_IGNORE_TO_STRING
 void SkBlurImageFilter::toString(SkString* str) const {
     str->appendf("SkBlurImageFilter: (");