Production quality fast image up/downsampler

The image resampling code has been transplanted from Chrome; it's incredibly fast.

We've tested this CL plumbed into Chrome and done benchmarking with excellent results.

This CL can land independent of any Chrome changes; it's completely internal to skia.

BUG=
R=reed@google.com

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

[humper, 2013-07-17 05:11:16]

On 2013/07/16 05:26:02, humper wrote:

Don't pay too much attention to this CL, this is just a place where manfred and
I are collaborating

[reed1, 2013-07-17 19:25:31]

SkBitmap.cpp is already pretty big. Maybe we should split out a helper .cpp file
to hold some of the mip-building stuff...

[ernstm, 2013-07-18 00:33:47]

I verified that it works in Chrome on Linux. Image quality and resampling
performance are excellent.

[humper, 2013-07-18 00:35:42]

On 2013/07/17 19:25:31, reed1 wrote:
> SkBitmap.cpp is already pretty big. Maybe we should split out a helper .cpp
file
> to hold some of the mip-building stuff...

Code has gone through a considerable cleanup and should now be ready for review.
 It's been tested inside Chrome and with the GM / benchmarks that I have for it.

[reed1, 2013-07-18 13:42:12]

Are there image differences w/ this change? (gm or layout-tests)

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

https://codereview.chromium.org/19335002/diff/20002/include/core/SkBitmap.h#n...
include/core/SkBitmap.h:708: static void scale( const SkBitmap *src, SkBitmap
*dst );
Static Methods Are Capitalized

Should this guy either take a quality level, or have its name indicated what
level its going to do?

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

https://codereview.chromium.org/19335002/diff/20002/src/core/SkBitmapFilter.h...
src/core/SkBitmapFilter.h:150: return ((sin(xpi) / xpi) *  // sinc(x)
this is the double version of sin. Do you want sk_float_sin to avoid the up /
down scaling between double and float?

https://codereview.chromium.org/19335002/diff/20002/src/core/SkBitmapFilter.h...
src/core/SkBitmapFilter.h:170: return (sin(xpi) / xpi) *  // sinc(x)
similar question about sin -vs- sk_float_sin

https://codereview.chromium.org/19335002/diff/20002/src/core/SkBitmapProcStat...
File src/core/SkBitmapProcState.cpp (right):

https://codereview.chromium.org/19335002/diff/20002/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:131: int dest_width =    
(int)(fOrigBitmap.width() / fInvMatrix.getScaleX());
round/ceil/floor?

https://codereview.chromium.org/19335002/diff/20002/src/core/SkConvolver.cpp
File src/core/SkConvolver.cpp (right):

https://codereview.chromium.org/19335002/diff/20002/src/core/SkConvolver.cpp#...
src/core/SkConvolver.cpp:5: #include <algorithm>
required?

https://codereview.chromium.org/19335002/diff/20002/src/core/SkConvolver.h
File src/core/SkConvolver.h (right):

https://codereview.chromium.org/19335002/diff/20002/src/core/SkConvolver.h#ne...
src/core/SkConvolver.h:8: #include <cmath>
must we bring in these (would be a first for skia)

[humper, 2013-07-18 17:11:04]

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

https://codereview.chromium.org/19335002/diff/20002/include/core/SkBitmap.h#n...
include/core/SkBitmap.h:708: static void scale( const SkBitmap *src, SkBitmap
*dst );
On 2013/07/18 13:42:12, reed1 wrote:
> Static Methods Are Capitalized
> 
> Should this guy either take a quality level, or have its name indicated what
> level its going to do?

Sorry, this function should have been deleted from the header.  It no longer
exists.  Nice catch.

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

https://codereview.chromium.org/19335002/diff/20002/src/core/SkBitmapFilter.h...
src/core/SkBitmapFilter.h:150: return ((sin(xpi) / xpi) *  // sinc(x)
On 2013/07/18 13:42:12, reed1 wrote:
> this is the double version of sin. Do you want sk_float_sin to avoid the up /
> down scaling between double and float?

Done.

https://codereview.chromium.org/19335002/diff/20002/src/core/SkBitmapFilter.h...
src/core/SkBitmapFilter.h:170: return (sin(xpi) / xpi) *  // sinc(x)
On 2013/07/18 13:42:12, reed1 wrote:
> similar question about sin -vs- sk_float_sin

Done.

https://codereview.chromium.org/19335002/diff/20002/src/core/SkBitmapProcStat...
File src/core/SkBitmapProcState.cpp (right):

https://codereview.chromium.org/19335002/diff/20002/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:131: int dest_width =    
(int)(fOrigBitmap.width() / fInvMatrix.getScaleX());
On 2013/07/18 13:42:12, reed1 wrote:
> round/ceil/floor?

Done.

https://codereview.chromium.org/19335002/diff/20002/src/core/SkConvolver.cpp
File src/core/SkConvolver.cpp (right):

https://codereview.chromium.org/19335002/diff/20002/src/core/SkConvolver.cpp#...
src/core/SkConvolver.cpp:5: #include <algorithm>
On 2013/07/18 13:42:12, reed1 wrote:
> required?

No, I can rewrite the code that uses it.

https://codereview.chromium.org/19335002/diff/20002/src/core/SkConvolver.h
File src/core/SkConvolver.h (right):

https://codereview.chromium.org/19335002/diff/20002/src/core/SkConvolver.h#ne...
src/core/SkConvolver.h:8: #include <cmath>
On 2013/07/18 13:42:12, reed1 wrote:
> must we bring in these (would be a first for skia)

cmath can probably be replaced by math.h; the other C++ libraries will take some
rewriting.

[Stephen White, 2013-07-18 17:39:45]

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

https://codereview.chromium.org/19335002/diff/31001/include/core/SkBitmap.h#n...
include/core/SkBitmap.h:26: class SkConvolutionProcs;
Are these necessary? They don't seem to be used in this file.

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

https://codereview.chromium.org/19335002/diff/31001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:9: #include <math.h>
What needs this that didn't before? I think we use constants from SkTypes.h or
SkScalar.h, but reed@ would know better than me.

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

https://codereview.chromium.org/19335002/diff/31001/src/core/SkBitmapFilter.h...
src/core/SkBitmapFilter.h:145: x < FLT_EPSILON)
OCDNit: either put these both on the same line, or split up the x <= -fWidth ||
x >= fWidth comparsions above to match.

https://codereview.chromium.org/19335002/diff/31001/src/core/SkBitmapFilter.h...
src/core/SkBitmapFilter.h:165: x < FLT_EPSILON) {
Same here.

https://codereview.chromium.org/19335002/diff/31001/src/opts/SkBitmapFilter_o...
File src/opts/SkBitmapFilter_opts_SSE2.cpp (right):

https://codereview.chromium.org/19335002/diff/31001/src/opts/SkBitmapFilter_o...
src/opts/SkBitmapFilter_opts_SSE2.cpp:185: static void
divideByWeights_SSE2(SkScalar *sums, SkScalar *weights, SkBitmap *dst) {
Doesn't look like SSE2 code; maybe it should just be divideByWeights(), since
it's file-static? Or is this going to be SSE2 eventually?

https://codereview.chromium.org/19335002/diff/31001/src/opts/SkBitmapFilter_o...
src/opts/SkBitmapFilter_opts_SSE2.cpp:205: static void
upScaleHorizTranspose_SSE2(const SkBitmap *src, SkBitmap *dst, float scale,
SkBitmapFilter *filter) {
Same here.

[humper, 2013-07-18 18:15:45]

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

https://codereview.chromium.org/19335002/diff/31001/include/core/SkBitmap.h#n...
include/core/SkBitmap.h:26: class SkConvolutionProcs;
On 2013/07/18 17:39:45, Stephen White wrote:
> Are these necessary? They don't seem to be used in this file.

Whoops, testing leakage.  Deleting.

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

https://codereview.chromium.org/19335002/diff/31001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:9: #include <math.h>
On 2013/07/18 17:39:45, Stephen White wrote:
> What needs this that didn't before? I think we use constants from SkTypes.h or
> SkScalar.h, but reed@ would know better than me.

Something that's since been deleted :)

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

https://codereview.chromium.org/19335002/diff/31001/src/core/SkBitmapFilter.h...
src/core/SkBitmapFilter.h:145: x < FLT_EPSILON)
On 2013/07/18 17:39:45, Stephen White wrote:
> OCDNit: either put these both on the same line, or split up the x <= -fWidth
||
> x >= fWidth comparsions above to match.

Yeah -- used to be std::long_line_of_stuff.  Thanks.

https://codereview.chromium.org/19335002/diff/31001/src/opts/SkBitmapFilter_o...
File src/opts/SkBitmapFilter_opts_SSE2.cpp (right):

https://codereview.chromium.org/19335002/diff/31001/src/opts/SkBitmapFilter_o...
src/opts/SkBitmapFilter_opts_SSE2.cpp:185: static void
divideByWeights_SSE2(SkScalar *sums, SkScalar *weights, SkBitmap *dst) {
On 2013/07/18 17:39:45, Stephen White wrote:
> Doesn't look like SSE2 code; maybe it should just be divideByWeights(), since
> it's file-static? Or is this going to be SSE2 eventually?

Yikes, these functions are dead anyway; deleting.

[reed1, 2013-07-19 17:47:23]

Lots of new (to skia) code. Is this easily disabled in case we find a bug after
a DEPS roll?

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

https://codereview.chromium.org/19335002/diff/40001/include/core/SkBitmap.h#n...
include/core/SkBitmap.h:25: class SkConvolutionProcs;
Why is this declared in this (public) header?

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

https://codereview.chromium.org/19335002/diff/40001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:123: fBitmapFilter = SkBitmapFilter::allocate();
Static Methods Should Be Capitalized

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

https://codereview.chromium.org/19335002/diff/40001/src/core/SkBitmapFilter.h...
src/core/SkBitmapFilter.h:52: static SkBitmapFilter *allocate();
* placement nit

https://codereview.chromium.org/19335002/diff/40001/src/core/SkBitmapFilter.h...
src/core/SkBitmapFilter.h:141: virtual float evaluate(float x) const {
SK_OVERRIDE

https://codereview.chromium.org/19335002/diff/40001/src/core/SkBitmapFilter.h...
src/core/SkBitmapFilter.h:142: if (x <= -fWidth || x >= fWidth)
{ } for all if-blocks

https://codereview.chromium.org/19335002/diff/40001/src/core/SkBitmapScaler.cpp
File src/core/SkBitmapScaler.cpp (right):

https://codereview.chromium.org/19335002/diff/40001/src/core/SkBitmapScaler.c...
src/core/SkBitmapScaler.cpp:18: SkConvolutionProcs *convolveProcs);
double nit:

1. if you're passing 1 value, Skia likes the * on the type, not the value
2. if you're logically passing an array, then Skia likes SkConvolutionProcs
procs[]

https://codereview.chromium.org/19335002/diff/40001/src/core/SkConvolver.cpp
File src/core/SkConvolver.cpp (right):

https://codereview.chromium.org/19335002/diff/40001/src/core/SkConvolver.cpp#...
src/core/SkConvolver.cpp:27: if (takeAbsolute)
nit: if-blocks should always have {}

https://codereview.chromium.org/19335002/diff/40001/src/core/SkConvolver.h
File src/core/SkConvolver.h (right):

https://codereview.chromium.org/19335002/diff/40001/src/core/SkConvolver.h#ne...
src/core/SkConvolver.h:28: typedef short Fixed;
nit: possibly confusing to use that name for 2.14, when skia already has Fixed
as 16.16

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

https://codereview.chromium.org/19335002/diff/40001/src/opts/SkBitmapFilter_o...
src/opts/SkBitmapFilter_opts_SSE2.h:35: void
applySIMDPadding_SSE2(SkConvolutionFilter1D *filter);
broken-record-nit: * on the type, not the value

[humper, 2013-07-19 18:08:00]

On 2013/07/19 17:47:23, reed1 wrote:
> Lots of new (to skia) code. Is this easily disabled in case we find a bug
after
> a DEPS roll?
> 
> https://codereview.chromium.org/19335002/diff/40001/include/core/SkBitmap.h
> File include/core/SkBitmap.h (right):
> 
>
https://codereview.chromium.org/19335002/diff/40001/include/core/SkBitmap.h#n...
> include/core/SkBitmap.h:25: class SkConvolutionProcs;
> Why is this declared in this (public) header?
> 
>
https://codereview.chromium.org/19335002/diff/40001/src/core/SkBitmapFilter.cpp
> File src/core/SkBitmapFilter.cpp (right):
> 
>
https://codereview.chromium.org/19335002/diff/40001/src/core/SkBitmapFilter.c...
> src/core/SkBitmapFilter.cpp:123: fBitmapFilter = SkBitmapFilter::allocate();
> Static Methods Should Be Capitalized
> 
> https://codereview.chromium.org/19335002/diff/40001/src/core/SkBitmapFilter.h
> File src/core/SkBitmapFilter.h (right):
> 
>
https://codereview.chromium.org/19335002/diff/40001/src/core/SkBitmapFilter.h...
> src/core/SkBitmapFilter.h:52: static SkBitmapFilter *allocate();
> * placement nit
> 
>
https://codereview.chromium.org/19335002/diff/40001/src/core/SkBitmapFilter.h...
> src/core/SkBitmapFilter.h:141: virtual float evaluate(float x) const {
> SK_OVERRIDE
> 
>
https://codereview.chromium.org/19335002/diff/40001/src/core/SkBitmapFilter.h...
> src/core/SkBitmapFilter.h:142: if (x <= -fWidth || x >= fWidth)
> { } for all if-blocks
> 
>
https://codereview.chromium.org/19335002/diff/40001/src/core/SkBitmapScaler.cpp
> File src/core/SkBitmapScaler.cpp (right):
> 
>
https://codereview.chromium.org/19335002/diff/40001/src/core/SkBitmapScaler.c...
> src/core/SkBitmapScaler.cpp:18: SkConvolutionProcs *convolveProcs);
> double nit:
> 
> 1. if you're passing 1 value, Skia likes the * on the type, not the value
> 2. if you're logically passing an array, then Skia likes SkConvolutionProcs
> procs[]
> 
> https://codereview.chromium.org/19335002/diff/40001/src/core/SkConvolver.cpp
> File src/core/SkConvolver.cpp (right):
> 
>
https://codereview.chromium.org/19335002/diff/40001/src/core/SkConvolver.cpp#...
> src/core/SkConvolver.cpp:27: if (takeAbsolute)
> nit: if-blocks should always have {}
> 
> https://codereview.chromium.org/19335002/diff/40001/src/core/SkConvolver.h
> File src/core/SkConvolver.h (right):
> 
>
https://codereview.chromium.org/19335002/diff/40001/src/core/SkConvolver.h#ne...
> src/core/SkConvolver.h:28: typedef short Fixed;
> nit: possibly confusing to use that name for 2.14, when skia already has Fixed
> as 16.16
> 
>
https://codereview.chromium.org/19335002/diff/40001/src/opts/SkBitmapFilter_o...
> File src/opts/SkBitmapFilter_opts_SSE2.h (right):
> 
>
https://codereview.chromium.org/19335002/diff/40001/src/opts/SkBitmapFilter_o...
> src/opts/SkBitmapFilter_opts_SSE2.h:35: void
> applySIMDPadding_SSE2(SkConvolutionFilter1D *filter);
> broken-record-nit: * on the type, not the value

Thanks, all addressed.

[humper, 2013-07-19 18:08:06]

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

https://codereview.chromium.org/19335002/diff/40001/include/core/SkBitmap.h#n...
include/core/SkBitmap.h:25: class SkConvolutionProcs;
On 2013/07/19 17:47:23, reed1 wrote:
> Why is this declared in this (public) header?

The bitmap proc state stores a pointer to the structure-of-pointers needed for
doing SIMD; it's a pointer rather than a structure so I could forward-declare
the class and avoid some circular header include dependencies.

[reed1, 2013-07-19 18:14:42]

On 2013/07/19 18:08:06, humper wrote:
> https://codereview.chromium.org/19335002/diff/40001/include/core/SkBitmap.h
> File include/core/SkBitmap.h (right):
> 
>
https://codereview.chromium.org/19335002/diff/40001/include/core/SkBitmap.h#n...
> include/core/SkBitmap.h:25: class SkConvolutionProcs;
> On 2013/07/19 17:47:23, reed1 wrote:
> > Why is this declared in this (public) header?
> 
> The bitmap proc state stores a pointer to the structure-of-pointers needed for
> doing SIMD; it's a pointer rather than a structure so I could forward-declare
> the class and avoid some circular header include dependencies.

Lets find a non-public header place for that sort of forward declaration

[humper, 2013-07-19 18:30:28]

On 2013/07/19 18:14:42, reed1 wrote:
> On 2013/07/19 18:08:06, humper wrote:
> > https://codereview.chromium.org/19335002/diff/40001/include/core/SkBitmap.h
> > File include/core/SkBitmap.h (right):
> > 
> >
>
https://codereview.chromium.org/19335002/diff/40001/include/core/SkBitmap.h#n...
> > include/core/SkBitmap.h:25: class SkConvolutionProcs;
> > On 2013/07/19 17:47:23, reed1 wrote:
> > > Why is this declared in this (public) header?
> > 
> > The bitmap proc state stores a pointer to the structure-of-pointers needed
for
> > doing SIMD; it's a pointer rather than a structure so I could
forward-declare
> > the class and avoid some circular header include dependencies.
> 
> Lets find a non-public header place for that sort of forward declaration

Done; moved to SkBitmapProcState.h

[reed1, 2013-07-19 18:39:05]

Assuming we have some idea of the GM-rebaseline impact, lgtm, with a request for
a follow-on that brings all the new code inline with Skia conventions

https://codereview.chromium.org/19335002/diff/63001/src/core/SkConvolver.cpp
File src/core/SkConvolver.cpp (right):

https://codereview.chromium.org/19335002/diff/63001/src/core/SkConvolver.cpp#...
src/core/SkConvolver.cpp:27: if (takeAbsolute) {
Thanks for this fix. Just about every "if" in this file (and likely the other
files that were copied from chrome) has the same problem. If we're not going to
address these now, it should be a high-priority to do as a next CL.

[humper, 2013-07-19 18:49:15]

On 2013/07/19 18:39:05, reed1 wrote:
> Assuming we have some idea of the GM-rebaseline impact, lgtm, with a request
for
> a follow-on that brings all the new code inline with Skia conventions
> 
> https://codereview.chromium.org/19335002/diff/63001/src/core/SkConvolver.cpp
> File src/core/SkConvolver.cpp (right):
> 
>
https://codereview.chromium.org/19335002/diff/63001/src/core/SkConvolver.cpp#...
> src/core/SkConvolver.cpp:27: if (takeAbsolute) {
> Thanks for this fix. Just about every "if" in this file (and likely the other
> files that were copied from chrome) has the same problem. If we're not going
to
> address these now, it should be a high-priority to do as a next CL.

I'll make another pass over them now.  It was actually a pretty herculean effort
to do
camelCase and things like that; I'm sure I missed a bunch.

[reed1, 2013-07-19 19:08:01]

On 2013/07/19 18:49:15, humper wrote:
> On 2013/07/19 18:39:05, reed1 wrote:
> > Assuming we have some idea of the GM-rebaseline impact, lgtm, with a request
> for
> > a follow-on that brings all the new code inline with Skia conventions
> > 
> > https://codereview.chromium.org/19335002/diff/63001/src/core/SkConvolver.cpp
> > File src/core/SkConvolver.cpp (right):
> > 
> >
>
https://codereview.chromium.org/19335002/diff/63001/src/core/SkConvolver.cpp#...
> > src/core/SkConvolver.cpp:27: if (takeAbsolute) {
> > Thanks for this fix. Just about every "if" in this file (and likely the
other
> > files that were copied from chrome) has the same problem. If we're not going
> to
> > address these now, it should be a high-priority to do as a next CL.
> 
> I'll make another pass over them now.  It was actually a pretty herculean
effort
> to do
> camelCase and things like that; I'm sure I missed a bunch.

Groovy

Specifics that come to mind: if-blocks-with-{}, Type* value

[humper, 2013-07-19 20:20:15]

Committed patchset #17 manually as r10206.