Fix Morphology effects sourcing outside of the crop rect.

Fix Morphology effects sourcing outside of the crop rect.

BUG=skia:1766
Committed: https://skia.googlesource.com/skia/+/f6be925b5615f07039ce95c3433039694a8d1679

Committed: https://skia.googlesource.com/skia/+/80a61df691bd5756dfbcbc57441806f2594511d8

[cwallez, 2014-12-15 14:20:22]

cwallez@google.com changed reviewers:
+ senorblanco@google.com

[cwallez, 2014-12-16 16:09:14]

cwallez@google.com changed reviewers:
+ junov@google.com
- senorblanco@google.com

[cwallez, 2014-12-19 23:28:44]

+junov

[Justin Novosad, 2014-12-22 20:11:11]

junov@chromium.org changed reviewers:
+ junov@chromium.org

[Justin Novosad, 2014-12-22 20:11:11]

https://codereview.chromium.org/781153002/diff/1/gm/imagefilterscropped.cpp
File gm/imagefilterscropped.cpp (right):

https://codereview.chromium.org/781153002/diff/1/gm/imagefilterscropped.cpp#n...
gm/imagefilterscropped.cpp:117: canvas.clear(0x00000000);
0 -> SK_ColorTRANSPARENT

https://codereview.chromium.org/781153002/diff/1/src/effects/SkGpuBlurUtils.cpp
File src/effects/SkGpuBlurUtils.cpp (left):

https://codereview.chromium.org/781153002/diff/1/src/effects/SkGpuBlurUtils.c...
src/effects/SkGpuBlurUtils.cpp:59: paint.reset();
This seems unrelated. Should go in a separate CL.

https://codereview.chromium.org/781153002/diff/1/src/effects/SkMorphologyImag...
File src/effects/SkMorphologyImageFilter.cpp (right):

https://codereview.chromium.org/781153002/diff/1/src/effects/SkMorphologyImag...
src/effects/SkMorphologyImageFilter.cpp:289: static GrFragmentProcessor*
Create(GrTexture* tex, Direction dir, int radius, bool useBounds, float
bounds[2],
Instead of having useBounds, you should consider having to Create methods, one
of which takes bounds.

https://codereview.chromium.org/781153002/diff/1/src/effects/SkMorphologyImag...
src/effects/SkMorphologyImageFilter.cpp:298: const float* bounds() const {
return fBounds; }
"Bounds" are usually SkRect throughout skia. Perhaps you should call this
something else (range, limit, size, maxRadius, etc.)? Also, since this is an x-y
pair, you should use SkVector or SkPoint, and return "const SkVector&" here.

https://codereview.chromium.org/781153002/diff/1/src/effects/SkMorphologyImag...
src/effects/SkMorphologyImageFilter.cpp:631: /*SkIRect clearRect =
SkIRect::MakeXYWH(dstRect.fLeft, dstRect.fBottom,
Don't commit commented code

[cwallez, 2015-01-19 21:34:39]

Still need to make a new gm instead of hacking it in.

https://codereview.chromium.org/781153002/diff/1/gm/imagefilterscropped.cpp
File gm/imagefilterscropped.cpp (right):

https://codereview.chromium.org/781153002/diff/1/gm/imagefilterscropped.cpp#n...
gm/imagefilterscropped.cpp:117: canvas.clear(0x00000000);
On 2014/12/22 20:11:11, junov wrote:
> 0 -> SK_ColorTRANSPARENT

Done.

https://codereview.chromium.org/781153002/diff/1/src/effects/SkGpuBlurUtils.cpp
File src/effects/SkGpuBlurUtils.cpp (left):

https://codereview.chromium.org/781153002/diff/1/src/effects/SkGpuBlurUtils.c...
src/effects/SkGpuBlurUtils.cpp:59: paint.reset();
On 2014/12/22 20:11:11, junov wrote:
> This seems unrelated. Should go in a separate CL.

Done.

https://codereview.chromium.org/781153002/diff/1/src/effects/SkMorphologyImag...
File src/effects/SkMorphologyImageFilter.cpp (right):

https://codereview.chromium.org/781153002/diff/1/src/effects/SkMorphologyImag...
src/effects/SkMorphologyImageFilter.cpp:289: static GrFragmentProcessor*
Create(GrTexture* tex, Direction dir, int radius, bool useBounds, float
bounds[2],
On 2014/12/22 20:11:11, junov wrote:
> Instead of having useBounds, you should consider having to Create methods, one
> of which takes bounds.

Done.

https://codereview.chromium.org/781153002/diff/1/src/effects/SkMorphologyImag...
src/effects/SkMorphologyImageFilter.cpp:298: const float* bounds() const {
return fBounds; }
On 2014/12/22 20:11:11, junov wrote:
> "Bounds" are usually SkRect throughout skia. Perhaps you should call this
> something else (range, limit, size, maxRadius, etc.)? Also, since this is an
x-y
> pair, you should use SkVector or SkPoint, and return "const SkVector&" here.

This is not an actual x-y par but more like a min-max pair so I'll keep the [2]
I think.

https://codereview.chromium.org/781153002/diff/1/src/effects/SkMorphologyImag...
src/effects/SkMorphologyImageFilter.cpp:631: /*SkIRect clearRect =
SkIRect::MakeXYWH(dstRect.fLeft, dstRect.fBottom,
On 2014/12/22 20:11:11, junov wrote:
> Don't commit commented code

Done.

[Justin Novosad, 2015-01-19 21:54:17]

On 2015/01/19 21:34:39, cwallez wrote:
> On 2014/12/22 20:11:11, junov wrote:
> > "Bounds" are usually SkRect throughout skia. Perhaps you should call this
> > something else (range, limit, size, maxRadius, etc.)? Also, since this is an
> x-y
> > pair, you should use SkVector or SkPoint, and return "const SkVector&" here.
> 
> This is not an actual x-y par but more like a min-max pair so I'll keep the
[2]
> I think.
> 

Then perhaps you should make that a bit clearer by using a term that is less
ambiguous like "interval"

[Justin Novosad, 2015-01-19 21:54:44]

junov@chromium.org changed reviewers:
+ bsalomon@google.com

[Justin Novosad, 2015-01-19 21:54:44]

+bsalomon

[cwallez, 2015-01-19 21:55:44]

On 2015/01/19 21:54:17, junov wrote:
> On 2015/01/19 21:34:39, cwallez wrote:
> > On 2014/12/22 20:11:11, junov wrote:
> > > "Bounds" are usually SkRect throughout skia. Perhaps you should call this
> > > something else (range, limit, size, maxRadius, etc.)? Also, since this is
an
> > x-y
> > > pair, you should use SkVector or SkPoint, and return "const SkVector&"
here.
> > 
> > This is not an actual x-y par but more like a min-max pair so I'll keep the
> [2]
> > I think.
> > 
> 
> Then perhaps you should make that a bit clearer by using a term that is less
> ambiguous like "interval"

I renamed all of them to range.

[reed1, 2015-01-20 13:53:04]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2015-01-20 13:53:05]

https://codereview.chromium.org/781153002/diff/20001/src/effects/SkMorphology...
File src/effects/SkMorphologyImageFilter.cpp (right):

https://codereview.chromium.org/781153002/diff/20001/src/effects/SkMorphology...
src/effects/SkMorphologyImageFilter.cpp:305: static GrFragmentProcessor*
Create(GrTexture* tex, Direction dir, int radius, MorphologyType type,
nit: 100col max

[bsalomon, 2015-01-20 14:57:17]

seems ok to me, but curious what's motivating the addition. Is this part of the
svg filter spec?

[cwallez, 2015-01-20 16:38:14]

On 2015/01/20 14:57:17, bsalomon wrote:
> seems ok to me, but curious what's motivating the addition. Is this part of
the
> svg filter spec?

This fixes Morphology effects sourcing values outside of the clip rect: the SVG
spec says that the result of a filter should only depend on what is in the clip
rect.

15.5 Filter effects region
  The bounds of this rectangle act as a hard clipping region for each filter
primitive included with a given ‘filter’ element; thus, if the effect of a given
filter primitive would extend beyond the bounds of the rectangle (this sometimes
happens when using a ‘feGaussianBlur’ filter primitive with a very large
‘stdDeviation’), parts of the effect will get clipped.

[bsalomon, 2015-01-20 18:06:57]

On 2015/01/20 16:38:14, cwallez wrote:
> On 2015/01/20 14:57:17, bsalomon wrote:
> > seems ok to me, but curious what's motivating the addition. Is this part of
> the
> > svg filter spec?
> 
> This fixes Morphology effects sourcing values outside of the clip rect: the
SVG
> spec says that the result of a filter should only depend on what is in the
clip
> rect.
> 
> 15.5 Filter effects region
>   The bounds of this rectangle act as a hard clipping region for each filter
> primitive included with a given ‘filter’ element; thus, if the effect of a
given
> filter primitive would extend beyond the bounds of the rectangle (this
sometimes
> happens when using a ‘feGaussianBlur’ filter primitive with a very large
> ‘stdDeviation’), parts of the effect will get clipped.

Gotcha, thanks for the pointer.

[bsalomon, 2015-01-20 18:09:24]

lgtm

[cwallez, 2015-01-21 18:37:59]

Added the tests to the GM without removing others so it is no longer a WIP.

Also added the changed GM to the ignored list.

[Justin Novosad, 2015-01-22 20:09:23]

On 2015/01/21 18:37:59, cwallez wrote:
> Added the tests to the GM without removing others so it is no longer a WIP.
> 
> Also added the changed GM to the ignored list.

lgtm

[cwallez, 2015-01-22 20:38:11]

The CQ bit was checked by cwallez@google.com

[commit-bot: I haz the power, 2015-01-22 20:38:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/781153002/60001

[commit-bot: I haz the power, 2015-01-22 20:38:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-22 20:38:27]

Failed to apply patch for expectations/gm/ignored-tests.txt:
While running git apply --index -3 -p1;
  Created missing directory expectations/gm.
  error: expectations/gm/ignored-tests.txt: does not exist in index

Patch:       expectations/gm/ignored-tests.txt
Index: expectations/gm/ignored-tests.txt
diff --git a/expectations/gm/ignored-tests.txt
b/expectations/gm/ignored-tests.txt
index
b2c1007ec3e3248c27cc49e4b790d1c562923ea9..895d7eb7fc6ae9173394aa8f68dbd1dbdc4dcddc
100644
--- a/expectations/gm/ignored-tests.txt
+++ b/expectations/gm/ignored-tests.txt
@@ -109,3 +109,6 @@ stringart
 strokerects
 strokes3
 stroketext
+
+#cwallez - added checkerboard and more filters to this gm - skia:1766
+imagefilterscropped

[cwallez, 2015-01-22 20:38:57]

The CQ bit was unchecked by cwallez@google.com

[cwallez, 2015-01-23 19:12:20]

I removed the change to ignored-tests.txt as we should now be using skiagold.com

[cwallez, 2015-01-23 21:20:49]

The CQ bit was checked by cwallez@google.com

[commit-bot: I haz the power, 2015-01-23 21:20:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/781153002/80001

[commit-bot: I haz the power, 2015-01-23 21:20:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-23 21:21:00]

Failed to apply patch for gm/imagefilterscropped.cpp:
While running git apply --index -3 -p1;
  error: patch failed: gm/imagefilterscropped.cpp:115
  Falling back to three-way merge...
  Applied patch to 'gm/imagefilterscropped.cpp' with conflicts.
  U gm/imagefilterscropped.cpp

Patch:       gm/imagefilterscropped.cpp
Index: gm/imagefilterscropped.cpp
diff --git a/gm/imagefilterscropped.cpp b/gm/imagefilterscropped.cpp
index
ad8fd5cc0423eb3d97f933ff4732f3db3330e183..ebb7faf3c715aba27d8d6e82b891fdaec4f40e2e
100644
--- a/gm/imagefilterscropped.cpp
+++ b/gm/imagefilterscropped.cpp
@@ -12,7 +12,9 @@
 #include "SkShader.h"
 
 #include "SkBlurImageFilter.h"
+#include "SkMorphologyImageFilter.h"
 #include "SkColorFilterImageFilter.h"
+#include "SkBitmapSource.h"
 #include "SkMergeImageFilter.h"
 #include "SkOffsetImageFilter.h"
 #include "SkTestImageFilters.h"
@@ -98,7 +100,28 @@ protected:
         return SkString("imagefilterscropped");
     }
 
-    virtual SkISize onISize() { return SkISize::Make(400, 640); }
+    virtual SkISize onISize() { return SkISize::Make(400, 880); }
+
+    void make_checkerboard() {
+        fCheckerboard.allocN32Pixels(80, 80);
+        SkCanvas canvas(fCheckerboard);
+        canvas.clear(SK_ColorTRANSPARENT);
+        SkPaint darkPaint;
+        darkPaint.setColor(0xFF404040);
+        SkPaint lightPaint;
+        lightPaint.setColor(0xFFA0A0A0);
+        for (int y = 0; y < 80; y += 16) {
+            for (int x = 0; x < 80; x += 16) {
+                canvas.save();
+                canvas.translate(SkIntToScalar(x), SkIntToScalar(y));
+                canvas.drawRect(SkRect::MakeXYWH(0, 0, 8, 8), darkPaint);
+                canvas.drawRect(SkRect::MakeXYWH(8, 0, 8, 8), lightPaint);
+                canvas.drawRect(SkRect::MakeXYWH(0, 8, 8, 8), lightPaint);
+                canvas.drawRect(SkRect::MakeXYWH(8, 8, 8, 8), darkPaint);
+                canvas.restore();
+            }
+        }
+    }
 
     void draw_frame(SkCanvas* canvas, const SkRect& r) {
         SkPaint paint;
@@ -115,6 +138,10 @@ protected:
         return kSkipScaledReplay_Flag | kSkipTiled_Flag;
     }
 
+    virtual void onOnceBeforeDraw() SK_OVERRIDE{
+        make_checkerboard();
+    }
+
     virtual void onDraw(SkCanvas* canvas) {
         void (*drawProc[])(SkCanvas*, const SkRect&, SkImageFilter*) = {
             draw_sprite, draw_bitmap, draw_path, draw_paint, draw_text
@@ -129,7 +156,6 @@ protected:
             SkIntToScalar(-10), SkIntToScalar(-10)));
 
         SkAutoTUnref<SkImageFilter>
cfOffset(SkColorFilterImageFilter::Create(cf.get(), offset.get()));
-
         SkImageFilter* filters[] = {
             NULL,
             SkColorFilterImageFilter::Create(cf.get(), NULL, &cropRect),
@@ -137,6 +163,10 @@ protected:
             SkBlurImageFilter::Create(8.0f, 0.0f, NULL, &cropRect),
             SkBlurImageFilter::Create(0.0f, 8.0f, NULL, &cropRect),
             SkBlurImageFilter::Create(8.0f, 8.0f, NULL, &cropRect),
+            SkErodeImageFilter::Create(1, 1, NULL, &cropRect),
+            SkErodeImageFilter::Create(8, 0, SkErodeImageFilter::Create(0, 8,
NULL, &cropRect), &cropRect),
+            SkErodeImageFilter::Create(0, 8, SkErodeImageFilter::Create(8, 0,
NULL, &cropRect), &cropRect),
+            SkErodeImageFilter::Create(8, 8, NULL, &cropRect),
             SkMergeImageFilter::Create(NULL, cfOffset.get(),
SkXfermode::kSrcOver_Mode, &cropRect),
             SkBlurImageFilter::Create(8.0f, 8.0f, NULL, &bogusRect),
             SkColorFilterImageFilter::Create(cf.get(), NULL, &bogusRect),
@@ -151,6 +181,8 @@ protected:
         for (size_t j = 0; j < SK_ARRAY_COUNT(drawProc); ++j) {
             canvas->save();
             for (size_t i = 0; i < SK_ARRAY_COUNT(filters); ++i) {
+                SkPaint paint;
+                canvas->drawBitmap(fCheckerboard, 0, 0);
                 drawProc[j](canvas, r, filters[i]);
                 canvas->translate(0, DY);
             }
@@ -164,6 +196,7 @@ protected:
     }
 
 private:
+    SkBitmap fCheckerboard;
     typedef GM INHERITED;
 };

[cwallez, 2015-01-23 21:27:09]

The CQ bit was checked by cwallez@google.com

[commit-bot: I haz the power, 2015-01-23 21:28:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/781153002/100001

[commit-bot: I haz the power, 2015-01-23 21:35:14]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/f6be925b5615f07039ce95c3433039694a8d1679

[mtklein, 2015-01-24 00:08:51]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/868973005/ by mtklein@google.com.

The reason for reverting is: Looks like this is causing memory leaks:
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...

And causing crashes on Mac 10.6:
http://build.chromium.org/p/client.skia/builders/Test-Mac10.6-MacMini4.1-GeFo....

[reed1, 2015-01-26 14:45:55]

we have several pre-written checkerboard makers. See if you can use one of them.

[cwallez, 2015-01-26 16:00:35]

On 2015/01/26 14:45:55, reed1 wrote:
> we have several pre-written checkerboard makers. See if you can use one of
them.

grep -rni "make.*checkerboard" . finds a dozen of checkerboard implementation
but none in a header file. Which one would you like to have used?

[reed1, 2015-01-26 16:46:14]

you're right :(
I was thinking of sk_tool_utils.h, but it doesn't have any of those yet. Perhaps
we need to fix that.

[cwallez, 2015-01-26 20:14:17]

The CQ bit was checked by cwallez@google.com

[commit-bot: I haz the power, 2015-01-26 20:14:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/781153002/120001

[commit-bot: I haz the power, 2015-01-26 20:20:17]

Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/80a61df691bd5756dfbcbc57441806f2594511d8