Implement SSE2-based implementations of the morphology filters (dilate &

Implement SSE2-based implementations of the morphology filters (dilate & erode). This gives a 3-5X speedup over the naive implementation, and also mitigates a timing-based security attack in Chrome (https://code.google.com/p/chromium/issues/detail?id=251711).

NOTE: this will require a corresponding GYP change on the Skia roll into Chrome: https://codereview.chromium.org/52453004/

R=mtklein@google.com, reed@google.com

Committed: https://code.google.com/p/skia/source/detail?r=12038

[Stephen White, 2013-10-30 15:08:43]

reed@: PTAL. Thanks!

[reed1, 2013-10-30 15:15:04]

do we already have tests and benches for these?

https://codereview.chromium.org/52603004/diff/30001/src/effects/SkMorphologyI...
File src/effects/SkMorphologyImageFilter.cpp (right):

https://codereview.chromium.org/52603004/diff/30001/src/effects/SkMorphologyI...
src/effects/SkMorphologyImageFilter.cpp:78: if (SkMorphologyProc erodeXProc =
SkErodeXGetPlatformProc()) {
can we change either the platformproc or your erode function to have the same
signature? That would make the call sites easier to read/confirm that we're
passing the right parameters...

e.g.

xproc = SkErodeXGetPlatformProc();
if (NULL == xproc) {
    xproc = erode_x;
}
xproc(...);

https://codereview.chromium.org/52603004/diff/30001/src/opts/SkMorphology_opts.h
File src/opts/SkMorphology_opts.h (right):

https://codereview.chromium.org/52603004/diff/30001/src/opts/SkMorphology_opt...
src/opts/SkMorphology_opts.h:11: int width, int height, int srcStride, int
dstStride);
are these logically size_t? we use size_t for byte-offsets in general (e.g.
rowBytes)

https://codereview.chromium.org/52603004/diff/30001/src/opts/SkMorphology_opt...
src/opts/SkMorphology_opts.h:13: SkMorphologyProc SkDilateXGetPlatformProc();
"if" this list wants to grow in the future, I wonder if an enum would be easier
to remember...

enum SkMorphologyType {
    kDilateX_SkMorphologyType,
    ...
};

SkMorphologyProc SkGetMorphologyPlatformProc(SkMorphologyType);

[mtklein, 2013-10-30 15:44:47]

https://codereview.chromium.org/52603004/diff/30001/src/opts/SkMorphology_opts.h
File src/opts/SkMorphology_opts.h (right):

https://codereview.chromium.org/52603004/diff/30001/src/opts/SkMorphology_opt...
src/opts/SkMorphology_opts.h:11: int width, int height, int srcStride, int
dstStride);
On 2013/10/30 15:15:04, reed1 wrote:
> are these logically size_t? we use size_t for byte-offsets in general (e.g.
> rowBytes)

These strides are pixel counts, so I think it makes sense to leave as ints.

https://codereview.chromium.org/52603004/diff/30001/src/opts/SkMorphology_opt...
File src/opts/SkMorphology_opts_SSE2.cpp (right):

https://codereview.chromium.org/52603004/diff/30001/src/opts/SkMorphology_opt...
src/opts/SkMorphology_opts_SSE2.cpp:17: void SkDilateX_SSE2(const SkPMColor*
src, SkPMColor* dst, int radius,
Do you see any value in sharing the structure of an X pass or a Y pass between
erode and dilate by parameterizing on the combiner (__mm_max_epu8,
__mm_min_epu8) and initial value (0 or 0xFFFFFFFF)?

I found myself doing a lot of paranoid double checking against the serial code
and across these dilate and erode here.

[reed1, 2013-10-30 15:49:52]

https://codereview.chromium.org/52603004/diff/30001/src/opts/SkMorphology_opts.h
File src/opts/SkMorphology_opts.h (right):

https://codereview.chromium.org/52603004/diff/30001/src/opts/SkMorphology_opt...
src/opts/SkMorphology_opts.h:11: int width, int height, int srcStride, int
dstStride);
On 2013/10/30 15:44:47, mtklein wrote:
> On 2013/10/30 15:15:04, reed1 wrote:
> > are these logically size_t? we use size_t for byte-offsets in general (e.g.
> > rowBytes)
> 
> These strides are pixel counts, so I think it makes sense to leave as ints.

Ah, then lets rename/document them to make that clear.

[Stephen White, 2013-10-30 19:47:17]

New patch up.

https://codereview.chromium.org/52603004/diff/30001/src/effects/SkMorphologyI...
File src/effects/SkMorphologyImageFilter.cpp (right):

https://codereview.chromium.org/52603004/diff/30001/src/effects/SkMorphologyI...
src/effects/SkMorphologyImageFilter.cpp:78: if (SkMorphologyProc erodeXProc =
SkErodeXGetPlatformProc()) {
On 2013/10/30 15:15:04, reed1 wrote:
> can we change either the platformproc or your erode function to have the same
> signature? That would make the call sites easier to read/confirm that we're
> passing the right parameters...
> 
> e.g.
> 
> xproc = SkErodeXGetPlatformProc();
> if (NULL == xproc) {
>     xproc = erode_x;
> }
> xproc(...);

Done.

https://codereview.chromium.org/52603004/diff/30001/src/opts/SkMorphology_opts.h
File src/opts/SkMorphology_opts.h (right):

https://codereview.chromium.org/52603004/diff/30001/src/opts/SkMorphology_opt...
src/opts/SkMorphology_opts.h:11: int width, int height, int srcStride, int
dstStride);
On 2013/10/30 15:49:52, reed1 wrote:
> On 2013/10/30 15:44:47, mtklein wrote:
> > On 2013/10/30 15:15:04, reed1 wrote:
> > > are these logically size_t? we use size_t for byte-offsets in general
(e.g.
> > > rowBytes)
> > 
> > These strides are pixel counts, so I think it makes sense to leave as ints.
> 
> Ah, then lets rename/document them to make that clear.

Done.

https://codereview.chromium.org/52603004/diff/30001/src/opts/SkMorphology_opt...
src/opts/SkMorphology_opts.h:13: SkMorphologyProc SkDilateXGetPlatformProc();
On 2013/10/30 15:15:04, reed1 wrote:
> "if" this list wants to grow in the future, I wonder if an enum would be
easier
> to remember...
> 
> enum SkMorphologyType {
>     kDilateX_SkMorphologyType,
>     ...
> };
> 
> SkMorphologyProc SkGetMorphologyPlatformProc(SkMorphologyType);

I don't forsee the list growing, but I've turned it into an enum anyway.

https://codereview.chromium.org/52603004/diff/30001/src/opts/SkMorphology_opt...
File src/opts/SkMorphology_opts_SSE2.cpp (right):

https://codereview.chromium.org/52603004/diff/30001/src/opts/SkMorphology_opt...
src/opts/SkMorphology_opts_SSE2.cpp:17: void SkDilateX_SSE2(const SkPMColor*
src, SkPMColor* dst, int radius,
On 2013/10/30 15:44:47, mtklein wrote:
> Do you see any value in sharing the structure of an X pass or a Y pass between
> erode and dilate by parameterizing on the combiner (__mm_max_epu8,
> __mm_min_epu8) and initial value (0 or 0xFFFFFFFF)?
> 
> I found myself doing a lot of paranoid double checking against the serial code
> and across these dilate and erode here.

OK, I've templated across type and direction. No effect on performance (which is
good). Let me know what you think.

[mtklein, 2013-10-30 20:02:20]

On 2013/10/30 19:47:17, Stephen White wrote:
> New patch up.
> 
>
https://codereview.chromium.org/52603004/diff/30001/src/effects/SkMorphologyI...
> File src/effects/SkMorphologyImageFilter.cpp (right):
> 
>
https://codereview.chromium.org/52603004/diff/30001/src/effects/SkMorphologyI...
> src/effects/SkMorphologyImageFilter.cpp:78: if (SkMorphologyProc erodeXProc =
> SkErodeXGetPlatformProc()) {
> On 2013/10/30 15:15:04, reed1 wrote:
> > can we change either the platformproc or your erode function to have the
same
> > signature? That would make the call sites easier to read/confirm that we're
> > passing the right parameters...
> > 
> > e.g.
> > 
> > xproc = SkErodeXGetPlatformProc();
> > if (NULL == xproc) {
> >     xproc = erode_x;
> > }
> > xproc(...);
> 
> Done.
> 
>
https://codereview.chromium.org/52603004/diff/30001/src/opts/SkMorphology_opts.h
> File src/opts/SkMorphology_opts.h (right):
> 
>
https://codereview.chromium.org/52603004/diff/30001/src/opts/SkMorphology_opt...
> src/opts/SkMorphology_opts.h:11: int width, int height, int srcStride, int
> dstStride);
> On 2013/10/30 15:49:52, reed1 wrote:
> > On 2013/10/30 15:44:47, mtklein wrote:
> > > On 2013/10/30 15:15:04, reed1 wrote:
> > > > are these logically size_t? we use size_t for byte-offsets in general
> (e.g.
> > > > rowBytes)
> > > 
> > > These strides are pixel counts, so I think it makes sense to leave as
ints.
> > 
> > Ah, then lets rename/document them to make that clear.
> 
> Done.
> 
>
https://codereview.chromium.org/52603004/diff/30001/src/opts/SkMorphology_opt...
> src/opts/SkMorphology_opts.h:13: SkMorphologyProc SkDilateXGetPlatformProc();
> On 2013/10/30 15:15:04, reed1 wrote:
> > "if" this list wants to grow in the future, I wonder if an enum would be
> easier
> > to remember...
> > 
> > enum SkMorphologyType {
> >     kDilateX_SkMorphologyType,
> >     ...
> > };
> > 
> > SkMorphologyProc SkGetMorphologyPlatformProc(SkMorphologyType);
> 
> I don't forsee the list growing, but I've turned it into an enum anyway.
> 
>
https://codereview.chromium.org/52603004/diff/30001/src/opts/SkMorphology_opt...
> File src/opts/SkMorphology_opts_SSE2.cpp (right):
> 
>
https://codereview.chromium.org/52603004/diff/30001/src/opts/SkMorphology_opt...
> src/opts/SkMorphology_opts_SSE2.cpp:17: void SkDilateX_SSE2(const SkPMColor*
> src, SkPMColor* dst, int radius,
> On 2013/10/30 15:44:47, mtklein wrote:
> > Do you see any value in sharing the structure of an X pass or a Y pass
between
> > erode and dilate by parameterizing on the combiner (__mm_max_epu8,
> > __mm_min_epu8) and initial value (0 or 0xFFFFFFFF)?
> > 
> > I found myself doing a lot of paranoid double checking against the serial
code
> > and across these dilate and erode here.
> 
> OK, I've templated across type and direction. No effect on performance (which
is
> good). Let me know what you think.

PS 3 and PS 4 both lgtm

[reed1, 2013-10-30 20:14:28]

lgtm w/ skia naming nits

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

https://codereview.chromium.org/52603004/diff/270001/src/effects/SkMorphology...
src/effects/SkMorphologyImageFilter.cpp:42: template<bool inX>
very nice code sharing!

bike shed : seeing erode<true> and erode<false> is not self-documenting. A
little enum like

enum MorphDirection {
  kX, kY
};

would be clearer, but perhaps that can be for another day.

https://codereview.chromium.org/52603004/diff/270001/src/opts/SkMorphology_op...
File src/opts/SkMorphology_opts.h (right):

https://codereview.chromium.org/52603004/diff/270001/src/opts/SkMorphology_op...
src/opts/SkMorphology_opts.h:21: SkMorphologyDilateX_Type,
bike shed : skia likes to have everything after the _ be the enum type name
(modulo the s if the name is plural)

e.g.

enum SkMorphologyProcType {
    kDilateX_SkMorphologyProcType,
    ...
};

This way the _ suffix can act as the visual grouper, and the leading part is the
distinctive part.

See SkAlpha.h -- SkAlphaType for an example

[Stephen White, 2013-10-30 20:48:06]

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

https://codereview.chromium.org/52603004/diff/270001/src/effects/SkMorphology...
src/effects/SkMorphologyImageFilter.cpp:42: template<bool inX>
On 2013/10/30 20:14:28, reed1 wrote:
> very nice code sharing!
> 
> bike shed : seeing erode<true> and erode<false> is not self-documenting. A
> little enum like
> 
> enum MorphDirection {
>   kX, kY
> };
> 
> would be clearer, but perhaps that can be for another day.

Done.

https://codereview.chromium.org/52603004/diff/270001/src/opts/SkMorphology_op...
File src/opts/SkMorphology_opts.h (right):

https://codereview.chromium.org/52603004/diff/270001/src/opts/SkMorphology_op...
src/opts/SkMorphology_opts.h:21: SkMorphologyDilateX_Type,
On 2013/10/30 20:14:28, reed1 wrote:
> bike shed : skia likes to have everything after the _ be the enum type name
> (modulo the s if the name is plural)
> 
> e.g.
> 
> enum SkMorphologyProcType {
>     kDilateX_SkMorphologyProcType,
>     ...
> };
> 
> This way the _ suffix can act as the visual grouper, and the leading part is
the
> distinctive part.
> 
> See SkAlpha.h -- SkAlphaType for an example

Done.

[Stephen White, 2013-10-30 21:57:11]

Committed patchset #7 manually as r12038 (presubmit successful).

[Stephen White, 2013-10-31 18:28:48]

On 2013/10/30 15:15:04, reed1 wrote:
> do we already have tests and benches for these?

Forgot to answer this: yes we do. :) GM:morphology, plus:

http://0.skia-gwt-dash.chrome-skia.ic.borg.google.com:25912/Skp.html#b-416-41...