More general version of image filtering; reworked to be robust and easier to SSE

More general image filter interface; tested implementation of standalone
image scaler (not yet plumbed).  High quality downsampler.  Fast SSE
resampler.

BUG=
R=reed@google.com

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

[reed1, 2013-06-18 20:21:21]

https://codereview.chromium.org/17381008/diff/1/include/core/SkPaint.h
File include/core/SkPaint.h (right):

https://codereview.chromium.org/17381008/diff/1/include/core/SkPaint.h#newcod...
include/core/SkPaint.h:111: kHighQualityFilterBitmap_Flag = 0x4000, // temporary
flag
Lets skip this change, or at most make it an alias for the old flag name...
1. we have checked-in chrome code using the old flag
2. we expect that neither name is "correct", as we likely will use an enum or
some other API down the road

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapFilter.cpp
File src/core/SkBitmapFilter.cpp (right):

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapFilter.cpp#n...
src/core/SkBitmapFilter.cpp:18: dst[0] = fFilterTable[ SkTMin(int((1+t) *
fBitmapFilter->invWidth() * SKBITMAP_FILTER_TABLE_SIZE),
SKBITMAP_FILTER_TABLE_SIZE-1) ];
keep line-wrap to 80 or 100 cols

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapFilter.h
File src/core/SkBitmapFilter.h (right):

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapFilter.h#new...
src/core/SkBitmapFilter.h:23: virtual float Evaluate( float x ) const = 0;
non-static methods are lowerCamelCase

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapFilter.h#new...
src/core/SkBitmapFilter.h:35: virtual float Evaluate( float x ) const {
SK_OVERRIDE

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapFilter.h#new...
src/core/SkBitmapFilter.h:80: SkBoxFilter( float width=.5 )
need .5f to avoid warning

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapFilter.h#new...
src/core/SkBitmapFilter.h:98: x = fabsf(x/fWidth);
sk_float_abs

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapFilter.h#new...
src/core/SkBitmapFilter.h:98: x = fabsf(x/fWidth);
x * fInvWidth

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapFilter.h#new...
src/core/SkBitmapFilter.h:99: if (x < 1e-5) return 1.f;
1e-5f for warning I think

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapFilter.h#new...
src/core/SkBitmapFilter.h:102: float sinc = sinf(x) / x;
sk_float_sin

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapProcState.h
File src/core/SkBitmapProcState.h (right):

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapProcState.h#...
src/core/SkBitmapProcState.h:35: #define SKBITMAP_FILTER_TABLE_SIZE 128
// Where does this value come from?

https://codereview.chromium.org/17381008/diff/1/tests/PaintTest.cpp
File tests/PaintTest.cpp (right):

https://codereview.chromium.org/17381008/diff/1/tests/PaintTest.cpp#newcode17
tests/PaintTest.cpp:17: REPORTER_ASSERT(reporter, 0 == (p0.getFlags() &
SkPaint::kHighQualityFilterBitmap_Flag));
I suggest (in SkPaint.h) that this change is unnecessary.

[Stephen White, 2013-06-18 20:47:50]

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapFilter.cpp
File src/core/SkBitmapFilter.cpp (right):

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapFilter.cpp#n...
src/core/SkBitmapFilter.cpp:18: dst[0] = fFilterTable[ SkTMin(int((1+t) *
fBitmapFilter->invWidth() * SKBITMAP_FILTER_TABLE_SIZE),
SKBITMAP_FILTER_TABLE_SIZE-1) ];
Mostly just comments from the peanut gallery, but how does use of a table
compare in performance to the ALU interpolation impl?  Do you have a GPU
implementation in mind?

[humper, 2013-06-19 20:49:26]

https://codereview.chromium.org/17381008/diff/1/include/core/SkPaint.h
File include/core/SkPaint.h (right):

https://codereview.chromium.org/17381008/diff/1/include/core/SkPaint.h#newcod...
include/core/SkPaint.h:111: kHighQualityFilterBitmap_Flag = 0x4000, // temporary
flag
On 2013/06/18 20:21:21, reed1 wrote:
> Lets skip this change, or at most make it an alias for the old flag name...
> 1. we have checked-in chrome code using the old flag
> 2. we expect that neither name is "correct", as we likely will use an enum or
> some other API down the road

Done.

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapFilter.cpp
File src/core/SkBitmapFilter.cpp (right):

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapFilter.cpp#n...
src/core/SkBitmapFilter.cpp:18: dst[0] = fFilterTable[ SkTMin(int((1+t) *
fBitmapFilter->invWidth() * SKBITMAP_FILTER_TABLE_SIZE),
SKBITMAP_FILTER_TABLE_SIZE-1) ];
On 2013/06/18 20:21:21, reed1 wrote:
> keep line-wrap to 80 or 100 cols

deleted this unused method.

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapFilter.cpp#n...
src/core/SkBitmapFilter.cpp:18: dst[0] = fFilterTable[ SkTMin(int((1+t) *
fBitmapFilter->invWidth() * SKBITMAP_FILTER_TABLE_SIZE),
SKBITMAP_FILTER_TABLE_SIZE-1) ];
On 2013/06/18 20:47:50, Stephen White wrote:
> Mostly just comments from the peanut gallery, but how does use of a table
> compare in performance to the ALU interpolation impl?  Do you have a GPU
> implementation in mind?

I haven't measured explicitly, but this implementation lends itself nicely to
GPU, and supports more general filter kernels should we want to do a higher
quality filter and/or allow user-specified filters.  Should be easy to roll back
and benchmark compare after this is landed.

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapFilter.h
File src/core/SkBitmapFilter.h (right):

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapFilter.h#new...
src/core/SkBitmapFilter.h:23: virtual float Evaluate( float x ) const = 0;
On 2013/06/18 20:21:21, reed1 wrote:
> non-static methods are lowerCamelCase

Done.

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapFilter.h#new...
src/core/SkBitmapFilter.h:35: virtual float Evaluate( float x ) const {
On 2013/06/18 20:21:21, reed1 wrote:
> SK_OVERRIDE

Done.

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapFilter.h#new...
src/core/SkBitmapFilter.h:80: SkBoxFilter( float width=.5 )
On 2013/06/18 20:21:21, reed1 wrote:
> need .5f to avoid warning

Done.

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapFilter.h#new...
src/core/SkBitmapFilter.h:98: x = fabsf(x/fWidth);
On 2013/06/18 20:21:21, reed1 wrote:
> sk_float_abs

Done.

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapFilter.h#new...
src/core/SkBitmapFilter.h:98: x = fabsf(x/fWidth);
On 2013/06/18 20:21:21, reed1 wrote:
> x * fInvWidth

Done.

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapFilter.h#new...
src/core/SkBitmapFilter.h:98: x = fabsf(x/fWidth);
On 2013/06/18 20:21:21, reed1 wrote:
> sk_float_abs

Done.

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapFilter.h#new...
src/core/SkBitmapFilter.h:99: if (x < 1e-5) return 1.f;
On 2013/06/18 20:21:21, reed1 wrote:
> 1e-5f for warning I think

Done.

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapFilter.h#new...
src/core/SkBitmapFilter.h:102: float sinc = sinf(x) / x;
On 2013/06/18 20:21:21, reed1 wrote:
> sk_float_sin

Done.

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapProcState.h
File src/core/SkBitmapProcState.h (right):

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapProcState.h#...
src/core/SkBitmapProcState.h:35: #define SKBITMAP_FILTER_TABLE_SIZE 128
On 2013/06/18 20:21:21, reed1 wrote:
> // Where does this value come from?

Nowhere -- it's probably a little high, actually; I was using 32 before and just
upped it to make sure the results were independent of table size.

https://codereview.chromium.org/17381008/diff/1/tests/PaintTest.cpp
File tests/PaintTest.cpp (right):

https://codereview.chromium.org/17381008/diff/1/tests/PaintTest.cpp#newcode17
tests/PaintTest.cpp:17: REPORTER_ASSERT(reporter, 0 == (p0.getFlags() &
SkPaint::kHighQualityFilterBitmap_Flag));
On 2013/06/18 20:21:21, reed1 wrote:
> I suggest (in SkPaint.h) that this change is unnecessary.

I made one an alias of the other, so I will revert the changes to the test.

[humper, 2013-06-19 20:51:19]

I haven't specialized the SSE version for scale-only yet (or benchmarked it),
but it works.

[reed1, 2013-06-19 21:14:08]

looking forward to seeing what the speedup might be.

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapProcState.h
File src/core/SkBitmapProcState.h (right):

https://codereview.chromium.org/17381008/diff/1/src/core/SkBitmapProcState.h#...
src/core/SkBitmapProcState.h:35: #define SKBITMAP_FILTER_TABLE_SIZE 128
On 2013/06/19 20:49:26, humper wrote:
> On 2013/06/18 20:21:21, reed1 wrote:
> > // Where does this value come from?
> 
> Nowhere -- it's probably a little high, actually; I was using 32 before and
just
> upped it to make sure the results were independent of table size.

Sorry. I was asking you to document
1. what it is used for
2. why it has the value it has

[reed1, 2013-06-24 23:58:15]

works for me. deferring to others for any specifics around impl.

[ernstm, 2013-07-02 01:39:24]

For the SSE2 implementation: have you considered doing the math with 16-bit
integer operations? That way you could process two samples instead of one, and
you could save the int-float and float-int conversions.

[reed1, 2013-07-09 14:25:47]

https://codereview.chromium.org/17381008/diff/30001/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.chromium.org/17381008/diff/30001/include/core/SkBitmap.h#n...
include/core/SkBitmap.h:545: void scale(SkBitmap *dst) const;
1. Need to document what parts of dst need to be set, and which parts will be
overwritten
2. Do we need to make a public method for this. Seems like just a 4 line helper
for drawBitmap / matrix.

https://codereview.chromium.org/17381008/diff/30001/src/core/SkBitmapFilter.cpp
File src/core/SkBitmapFilter.cpp (right):

https://codereview.chromium.org/17381008/diff/30001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:17: void highQualityFilter(const SkBitmapProcState&
s, int x, int y,
is this guy exported? (I don't see static, nor some Sk scoping of the name)

https://codereview.chromium.org/17381008/diff/30001/src/core/SkBitmapProcState.h
File src/core/SkBitmapProcState.h (right):

https://codereview.chromium.org/17381008/diff/30001/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.h:127: SkBitmapFilter *fBitmapFilter;
who needs direct access to this object (other than this class)?

https://codereview.chromium.org/17381008/diff/30001/src/opts/SkBitmapFilter_o...
File src/opts/SkBitmapFilter_opts_SSE2.h (right):

https://codereview.chromium.org/17381008/diff/30001/src/opts/SkBitmapFilter_o...
src/opts/SkBitmapFilter_opts_SSE2.h:11: 
need some sort of #ifndef guard for multiple includes

[humper, 2013-07-09 14:47:26]

https://codereview.chromium.org/17381008/diff/30001/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.chromium.org/17381008/diff/30001/include/core/SkBitmap.h#n...
include/core/SkBitmap.h:545: void scale(SkBitmap *dst) const;
1) Yep, will do.

2) It's not a helper; this directly scales the bitmap with proper HQ resampling,
bypassing the entire draw pipeline.

On 2013/07/09 14:25:47, reed1 wrote:
> 1. Need to document what parts of dst need to be set, and which parts will be
> overwritten
> 2. Do we need to make a public method for this. Seems like just a 4 line
helper
> for drawBitmap / matrix.

https://codereview.chromium.org/17381008/diff/30001/src/core/SkBitmapFilter.cpp
File src/core/SkBitmapFilter.cpp (right):

https://codereview.chromium.org/17381008/diff/30001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:17: void highQualityFilter(const SkBitmapProcState&
s, int x, int y,
I suppose it's not; only a function pointer to it is returned.

On 2013/07/09 14:25:47, reed1 wrote:
> is this guy exported? (I don't see static, nor some Sk scoping of the name)

https://codereview.chromium.org/17381008/diff/30001/src/core/SkBitmapProcState.h
File src/core/SkBitmapProcState.h (right):

https://codereview.chromium.org/17381008/diff/30001/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.h:127: SkBitmapFilter *fBitmapFilter;
Nobody.  Will restrict.  

On 2013/07/09 14:25:47, reed1 wrote:
> who needs direct access to this object (other than this class)?

https://codereview.chromium.org/17381008/diff/30001/src/opts/SkBitmapFilter_o...
File src/opts/SkBitmapFilter_opts_SSE2.h (right):

https://codereview.chromium.org/17381008/diff/30001/src/opts/SkBitmapFilter_o...
src/opts/SkBitmapFilter_opts_SSE2.h:11: 
On 2013/07/09 14:25:47, reed1 wrote:
> need some sort of #ifndef guard for multiple includes

Done.

[reed1, 2013-07-09 15:01:01]

https://codereview.chromium.org/17381008/diff/30001/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.chromium.org/17381008/diff/30001/include/core/SkBitmap.h#n...
include/core/SkBitmap.h:545: void scale(SkBitmap *dst) const;
On 2013/07/09 14:47:26, humper wrote:
> 1) Yep, will do.
> 
> 2) It's not a helper; this directly scales the bitmap with proper HQ
resampling,
> bypassing the entire draw pipeline.

Does it generate diff results from just setting up a paint correctly and calling
draw? If not, then I call it a helper.

I'm not against it per-se, but any additional public API should be well
justified (especially one that diverts clients from our higher-level APIs like
canvas/surface/image, which are more future-proofed).

> 
> On 2013/07/09 14:25:47, reed1 wrote:
> > 1. Need to document what parts of dst need to be set, and which parts will
be
> > overwritten
> > 2. Do we need to make a public method for this. Seems like just a 4 line
> helper
> > for drawBitmap / matrix.
>

[humper, 2013-07-09 15:26:29]

https://codereview.chromium.org/17381008/diff/30001/src/core/SkBitmapFilter.cpp
File src/core/SkBitmapFilter.cpp (right):

https://codereview.chromium.org/17381008/diff/30001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:17: void highQualityFilter(const SkBitmapProcState&
s, int x, int y,
ah, actually it is; I look to see if the shader proc is equal to these when
deciding whether or not to set them to their SSE equivalents.

On 2013/07/09 14:47:26, humper wrote:
> I suppose it's not; only a function pointer to it is returned.
> 
> On 2013/07/09 14:25:47, reed1 wrote:
> > is this guy exported? (I don't see static, nor some Sk scoping of the name)
>

[humper, 2013-07-09 17:21:18]

https://codereview.chromium.org/17381008/diff/41001/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.chromium.org/17381008/diff/41001/include/core/SkBitmap.h#n...
include/core/SkBitmap.h:714: void scale(SkBitmap *dst) const;
Made private; disabled the GM and bench.

[reed1, 2013-07-09 17:26:29]

just nits

lgtm

https://codereview.chromium.org/17381008/diff/41001/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.chromium.org/17381008/diff/41001/include/core/SkBitmap.h#n...
include/core/SkBitmap.h:711: * move once it has been properly plumbed into the
bitmap shader infrastructure.
this is an 80-col file

https://codereview.chromium.org/17381008/diff/41001/include/core/SkPaint.h
File include/core/SkPaint.h (right):

https://codereview.chromium.org/17381008/diff/41001/include/core/SkPaint.h#ne...
include/core/SkPaint.h:46: #define kBicubicFilterBitmap_Flag
kHighQualityFilterBitmap_Flag
Lets update the callers in chrome to use your preferred name, so we can remove
this alias

https://codereview.chromium.org/17381008/diff/41001/src/core/SkBitmapProcState.h
File src/core/SkBitmapProcState.h (right):

https://codereview.chromium.org/17381008/diff/41001/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.h:118: SkBitmapFilter *getBitmapFilter() const {
return fBitmapFilter; }
nit: skia places the * on the type, not the other guy.

e.g.

SkBitmapFilter* getBitmapFilter() ...

[humper, 2013-07-09 17:46:13]

https://codereview.chromium.org/17381008/diff/41001/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.chromium.org/17381008/diff/41001/include/core/SkBitmap.h#n...
include/core/SkBitmap.h:711: * move once it has been properly plumbed into the
bitmap shader infrastructure.
On 2013/07/09 17:26:29, reed1 wrote:
> this is an 80-col file

Done.

https://codereview.chromium.org/17381008/diff/41001/include/core/SkPaint.h
File include/core/SkPaint.h (right):

https://codereview.chromium.org/17381008/diff/41001/include/core/SkPaint.h#ne...
include/core/SkPaint.h:46: #define kBicubicFilterBitmap_Flag
kHighQualityFilterBitmap_Flag
On 2013/07/09 17:26:29, reed1 wrote:
> Lets update the callers in chrome to use your preferred name, so we can remove
> this alias

Once we change the API properly we'll need to nuke these bits entirely, so
probably let's just defer fixing things until then.

[humper, 2013-07-09 17:48:23]

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