Working plumb of image scaling:

Working plumb of image scaling:

1) always generate mipmaps if we detect that we are downsampling.
2) pre-scale the image if we detect that we are upsampling
   (currently valid for scale+translate only)
3) A few miscellaneous bug fixes related to image scaling.

Still need SSE/Neon versions of these image scalers.

BUG=
R=bsalomon@google.com, robertphillips@google.com

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

[reed1, 2013-07-11 14:06:36]

I think if we add a new bool to SkBitmapProcState.h (next to fDoFilter) or
change fDoFilter to an enum { none, bilerp, hq }, then we can keep the test for
hqfilter inside its choose method (as we did before this CL) as you'll have
access to all the state you need to decide if you can handle it)

fPurgeBitmap could become a real bitmap instead of a pointer (simpler to track),
and renamed to fScaledBitmap or something.

[humper, 2013-07-11 14:22:51]

On 2013/07/11 14:06:36, reed1 wrote:
> I think if we add a new bool to SkBitmapProcState.h (next to fDoFilter) or
> change fDoFilter to an enum { none, bilerp, hq }, then we can keep the test
for
> hqfilter inside its choose method (as we did before this CL) as you'll have
> access to all the state you need to decide if you can handle it)
> 
> fPurgeBitmap could become a real bitmap instead of a pointer (simpler to
track),
> and renamed to fScaledBitmap or something.

will do, ty

[reed1, 2013-07-11 15:43:42]

https://codereview.chromium.org/18978014/diff/6001/src/core/SkBitmapProcState...
File src/core/SkBitmapProcState.cpp (right):

https://codereview.chromium.org/18978014/diff/6001/src/core/SkBitmapProcState...
src/core/SkBitmapProcState.cpp:130:
fScaledBitmap.setConfig(SkBitmap::kARGB_8888_Config, fOrigBitmap.width() /
inv.getScaleX(), fOrigBitmap.height() / inv.getScaleY());
nit: line-wrap

https://codereview.chromium.org/18978014/diff/6001/src/core/SkBitmapProcState...
src/core/SkBitmapProcState.cpp:145: fUnitInvMatrix.setTranslate(
1/m->getScaleX() * m->getTranslateX(), 1/m->getScaleY() * m->getTranslateY() );
nit: line-wrap

https://codereview.chromium.org/18978014/diff/6001/src/core/SkBitmapProcState...
src/core/SkBitmapProcState.cpp:243: if (fInvType > SkMatrix::kTranslate_Mask &&
Shouldn't we perform this (not scaled enough to justify any filtering) test
*before* we create a HQ proc?

https://codereview.chromium.org/18978014/diff/6001/src/core/SkBitmapProcState...
src/core/SkBitmapProcState.cpp:468: SkASSERT(s.fFilterQuality ==
kNone_BitmapFilter);
nit: skia likes to place the literal on the left (but not so important when the
var is const I guess)

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

https://codereview.chromium.org/18978014/diff/6001/src/core/SkBitmapProcState...
src/core/SkBitmapProcState.h:36: 
Why this WS change?

https://codereview.chromium.org/18978014/diff/6001/src/core/SkBitmapProcState...
src/core/SkBitmapProcState.h:64: SkBitmap*           fBitmap;            //
chooseProcs - orig or mip
why is this non-const now?

https://codereview.chromium.org/18978014/diff/6001/src/core/SkBitmapProcState...
src/core/SkBitmapProcState.h:153: SkBitmap            fScaledBitmap;      //
chooseProcs
Do we ever need both fScaledBitmap and fMipMitmap? I wonder if we can drop
fMipBitmap, and always use the former when we need a "scaled" version of the
input?

[reed1, 2013-07-11 15:49:08]

https://codereview.chromium.org/18978014/diff/6001/src/core/SkBitmapProcState...
File src/core/SkBitmapProcState.cpp (right):

https://codereview.chromium.org/18978014/diff/6001/src/core/SkBitmapProcState...
src/core/SkBitmapProcState.cpp:130:
fScaledBitmap.setConfig(SkBitmap::kARGB_8888_Config, fOrigBitmap.width() /
inv.getScaleX(), fOrigBitmap.height() / inv.getScaleY());
grrrr, this is tricky, since we have one predicate to decide if we should create
fScaledBitmap, and a (potentially) different one to decide if we should actually
create/use a HQ filter...

https://codereview.chromium.org/18978014/diff/6001/src/core/SkBitmapProcState...
src/core/SkBitmapProcState.cpp:243: if (fInvType > SkMatrix::kTranslate_Mask &&
On 2013/07/11 15:43:42, reed1 wrote:
> Shouldn't we perform this (not scaled enough to justify any filtering) test
> *before* we create a HQ proc?

Ah, I see that you are *re*calling this logic after you've (potentially) changed
fBitmap to point to a scaled version. Good. Perhaps we could refactor to make
this clearer (and also perhaps we should do this 2nd-pass logic in the miplevel
case as well.

[reed1, 2013-07-11 15:49:43]

Seems like we should *not* create a mipmap unless the caller is asking for HQ
filtering. Is that how it is in this CL?

[humper, 2013-07-11 16:03:03]

https://codereview.chromium.org/18978014/diff/6001/src/core/SkBitmapProcState...
File src/core/SkBitmapProcState.cpp (right):

https://codereview.chromium.org/18978014/diff/6001/src/core/SkBitmapProcState...
src/core/SkBitmapProcState.cpp:130:
fScaledBitmap.setConfig(SkBitmap::kARGB_8888_Config, fOrigBitmap.width() /
inv.getScaleX(), fOrigBitmap.height() / inv.getScaleY());
On 2013/07/11 15:43:42, reed1 wrote:
> nit: line-wrap

Done.

https://codereview.chromium.org/18978014/diff/6001/src/core/SkBitmapProcState...
src/core/SkBitmapProcState.cpp:145: fUnitInvMatrix.setTranslate(
1/m->getScaleX() * m->getTranslateX(), 1/m->getScaleY() * m->getTranslateY() );
On 2013/07/11 15:43:42, reed1 wrote:
> nit: line-wrap

Done.

https://codereview.chromium.org/18978014/diff/6001/src/core/SkBitmapProcState...
src/core/SkBitmapProcState.cpp:243: if (fInvType > SkMatrix::kTranslate_Mask &&
On 2013/07/11 15:43:42, reed1 wrote:
> Shouldn't we perform this (not scaled enough to justify any filtering) test
> *before* we create a HQ proc?

the valid_for_filtering function actually checks to make sure the image isn't
too huge, I think :)

However, you're right, we shouldn't install the HQ proc if the matrix is
translate only or simpler.  the chooseBitmapFilterProc() function in
SkBitmapFilter will actually do that by making sure that the matrix is either
affine or scale, so the right thing is actually happening.

[humper, 2013-07-11 16:04:09]

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

https://codereview.chromium.org/18978014/diff/6001/src/core/SkBitmapProcState...
src/core/SkBitmapProcState.h:64: SkBitmap*           fBitmap;            //
chooseProcs - orig or mip
On 2013/07/11 15:43:42, reed1 wrote:
> why is this non-const now?

constness reinstated due to some code-reorg

https://codereview.chromium.org/18978014/diff/6001/src/core/SkBitmapProcState...
src/core/SkBitmapProcState.h:153: SkBitmap            fScaledBitmap;      //
chooseProcs
On 2013/07/11 15:43:42, reed1 wrote:
> Do we ever need both fScaledBitmap and fMipMitmap? I wonder if we can drop
> fMipBitmap, and always use the former when we need a "scaled" version of the
> input?

Good point; I nuked the mip one.

[reed1, 2013-07-11 20:26:18]

https://codereview.chromium.org/18978014/diff/5629499534213120/src/core/SkBit...
File src/core/SkBitmapProcState.cpp (right):

https://codereview.chromium.org/18978014/diff/5629499534213120/src/core/SkBit...
src/core/SkBitmapProcState.cpp:111: fOrigBitmap.width() /
fUnitInvMatrix.getScaleX(),
This / produces a float (SkScalar). We should use some explicit round/cast etc.
since setConfig wants an int.

https://codereview.chromium.org/18978014/diff/5629499534213120/src/core/SkBit...
src/core/SkBitmapProcState.cpp:124: fFilterQuality = kNone_BitmapFilter;
Perhaps we should return now, since everything below (even after the else
completes) is specific to mipmaps. That would eliminate the need to test fBitmap
== &fOrigBitmap as well.

https://codereview.chromium.org/18978014/diff/5629499534213120/src/core/SkBit...
src/core/SkBitmapProcState.cpp:248: if (fFilterQuality == kHQ_BitmapFilter) {
nit: reverse the sides of the ==

https://codereview.chromium.org/18978014/diff/5629499534213120/src/core/SkBit...
src/core/SkBitmapProcState.cpp:254: fShaderProc32 =
this->chooseBitmapFilterProc();
A little tricky (in the future) here, as we will want to give the
"platform/neon/sse" code a shot at this proc *before we down-grade to Bilerp.
May just a comment to that effect here for now.

https://codereview.chromium.org/18978014/diff/5629499534213120/src/core/SkBit...
src/core/SkBitmapProcState.cpp:424: SkASSERT(s.fFilterQuality ==
SkBitmapProcState::kNone_BitmapFilter);
nit: reverse the sides of the ==

https://codereview.chromium.org/18978014/diff/5629499534213120/src/core/SkBit...
src/core/SkBitmapProcState.cpp:498: SkASSERT(s.fFilterQuality ==
SkBitmapProcState::kNone_BitmapFilter);
nit: reverse

https://codereview.chromium.org/18978014/diff/5629499534213120/src/core/SkBit...
File src/core/SkBitmapProcState_matrixProcs.cpp (right):

https://codereview.chromium.org/18978014/diff/5629499534213120/src/core/SkBit...
src/core/SkBitmapProcState_matrixProcs.cpp:475: SkASSERT(fFilterQuality ==
kNone_BitmapFilter);
nit: reverse

[humper, 2013-07-11 22:01:41]

On 2013/07/11 20:26:18, reed1 wrote:
>
https://codereview.chromium.org/18978014/diff/5629499534213120/src/core/SkBit...
> File src/core/SkBitmapProcState.cpp (right):
> 
>
https://codereview.chromium.org/18978014/diff/5629499534213120/src/core/SkBit...
> src/core/SkBitmapProcState.cpp:111: fOrigBitmap.width() /
> fUnitInvMatrix.getScaleX(),
> This / produces a float (SkScalar). We should use some explicit round/cast
etc.
> since setConfig wants an int.
> 
>
https://codereview.chromium.org/18978014/diff/5629499534213120/src/core/SkBit...
> src/core/SkBitmapProcState.cpp:124: fFilterQuality = kNone_BitmapFilter;
> Perhaps we should return now, since everything below (even after the else
> completes) is specific to mipmaps. That would eliminate the need to test
fBitmap
> == &fOrigBitmap as well.
> 
>
https://codereview.chromium.org/18978014/diff/5629499534213120/src/core/SkBit...
> src/core/SkBitmapProcState.cpp:248: if (fFilterQuality == kHQ_BitmapFilter) {
> nit: reverse the sides of the ==
> 
>
https://codereview.chromium.org/18978014/diff/5629499534213120/src/core/SkBit...
> src/core/SkBitmapProcState.cpp:254: fShaderProc32 =
> this->chooseBitmapFilterProc();
> A little tricky (in the future) here, as we will want to give the
> "platform/neon/sse" code a shot at this proc *before we down-grade to Bilerp.
> May just a comment to that effect here for now.
> 
>
https://codereview.chromium.org/18978014/diff/5629499534213120/src/core/SkBit...
> src/core/SkBitmapProcState.cpp:424: SkASSERT(s.fFilterQuality ==
> SkBitmapProcState::kNone_BitmapFilter);
> nit: reverse the sides of the ==
> 
>
https://codereview.chromium.org/18978014/diff/5629499534213120/src/core/SkBit...
> src/core/SkBitmapProcState.cpp:498: SkASSERT(s.fFilterQuality ==
> SkBitmapProcState::kNone_BitmapFilter);
> nit: reverse
> 
>
https://codereview.chromium.org/18978014/diff/5629499534213120/src/core/SkBit...
> File src/core/SkBitmapProcState_matrixProcs.cpp (right):
> 
>
https://codereview.chromium.org/18978014/diff/5629499534213120/src/core/SkBit...
> src/core/SkBitmapProcState_matrixProcs.cpp:475: SkASSERT(fFilterQuality ==
> kNone_BitmapFilter);
> nit: reverse

all done

[reed1, 2013-07-12 14:01:05]

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

https://codereview.chromium.org/18978014/diff/35002/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:55: fr = SkScalarDiv(fr, weight);
All of the SkScalarDiv() calls are correct, but we have now committed to a world
where SkScalar must be a floaty type thing (float or double). Thus it is now
perfectly fine to just use / or * instead of the associated SkScalarFoo macro

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

https://codereview.chromium.org/18978014/diff/35002/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:93: void SkBitmapProcState::possiblyScaleImage()
{
I think we may need to pass in the clip-bounds, so we can allocate/scale a
minimal subset of the src bitmap in the case where it is clipped. Naturally,
this will complicate (yet more) any caching scheme, but we know we see very
large images sometimes that are only used partially.

https://codereview.chromium.org/18978014/diff/35002/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:184: bool trivial_matrix = (inv.getType() &
~SkMatrix::kTranslate_Mask) == 0;
This is probably too soon to be computing "trivialness" of the matrix, since we
may change the matrix later on... See the users of trivial_matrix below.

https://codereview.chromium.org/18978014/diff/35002/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:188: fUnitInvMatrix = inv;
I see now that your change meas we *always* use fUnitInvMatrix, instead of only
as needed. That is probably ok (a tiny bit slower?) but if that is the case,
lets rename it to fInverMatrix or something more general.

https://codereview.chromium.org/18978014/diff/35002/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:214: possiblyScaleImage();
nit: this->possiblyScaleImage();

https://codereview.chromium.org/18978014/diff/35002/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:221: if
(matrix_only_scale_translate(fUnitInvMatrix)) {
unrelated to this CL, but this guy should probably only execute if the matrix
has *some* scale. If it is already purely translate, then I think there is not
need to invert and do the trickery. Maybe you can add a comment to that effect.

https://codereview.chromium.org/18978014/diff/35002/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:238: fInvProc        =
fInvMatrix->getMapXYProc();
Maybe we no longer need fInvMatrix as a pointer anymore, if we are *always*
using a known storage field.

Thus, perhaps we kill this pointer, and rename fUnitInvMatrix to fInvMatrix (and
then use a . instead of a ->)

If that is too hairy for now, lets just add a comment in State.h to that effect
near these two fields.

https://codereview.chromium.org/18978014/diff/35002/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:252: if (kHQ_BitmapFilter == fFilterQuality) {
*maybe* we can add an ASSERT here that claims that the matrix is more
complicated than translate, just to catch any intervening bugs/slips.

[humper, 2013-07-12 17:18:01]

getting closer!

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

https://codereview.chromium.org/18978014/diff/35002/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:93: void SkBitmapProcState::possiblyScaleImage()
{
On 2013/07/12 14:01:06, reed1 wrote:
> I think we may need to pass in the clip-bounds, so we can allocate/scale a
> minimal subset of the src bitmap in the case where it is clipped. Naturally,
> this will complicate (yet more) any caching scheme, but we know we see very
> large images sometimes that are only used partially.

Is it OK to leave this as a TODO comment for now and file a bug against
ourselves?

https://codereview.chromium.org/18978014/diff/35002/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:184: bool trivial_matrix = (inv.getType() &
~SkMatrix::kTranslate_Mask) == 0;
On 2013/07/12 14:01:06, reed1 wrote:
> This is probably too soon to be computing "trivialness" of the matrix, since
we
> may change the matrix later on... See the users of trivial_matrix below.

trivial_matrix is used immediately (so we need it now), to do a div by the width
and height of the image, and also later in choosing the matrix proc.  Should we
recompute it before its later use?

https://codereview.chromium.org/18978014/diff/35002/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:188: fUnitInvMatrix = inv;
On 2013/07/12 14:01:06, reed1 wrote:
> I see now that your change meas we *always* use fUnitInvMatrix, instead of
only
> as needed. That is probably ok (a tiny bit slower?) but if that is the case,
> lets rename it to fInverMatrix or something more general.

Done.

https://codereview.chromium.org/18978014/diff/35002/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:214: possiblyScaleImage();
On 2013/07/12 14:01:06, reed1 wrote:
> nit: this->possiblyScaleImage();

Done.

https://codereview.chromium.org/18978014/diff/35002/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:221: if
(matrix_only_scale_translate(fUnitInvMatrix)) {
On 2013/07/12 14:01:06, reed1 wrote:
> unrelated to this CL, but this guy should probably only execute if the matrix
> has *some* scale. If it is already purely translate, then I think there is not
> need to invert and do the trickery. Maybe you can add a comment to that
effect.

Done.

https://codereview.chromium.org/18978014/diff/35002/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:238: fInvProc        =
fInvMatrix->getMapXYProc();
On 2013/07/12 14:01:06, reed1 wrote:
> Maybe we no longer need fInvMatrix as a pointer anymore, if we are *always*
> using a known storage field.
> 
> Thus, perhaps we kill this pointer, and rename fUnitInvMatrix to fInvMatrix
(and
> then use a . instead of a ->)
> 
> If that is too hairy for now, lets just add a comment in State.h to that
effect
> near these two fields.

Done.

https://codereview.chromium.org/18978014/diff/35002/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:252: if (kHQ_BitmapFilter == fFilterQuality) {
On 2013/07/12 14:01:06, reed1 wrote:
> *maybe* we can add an ASSERT here that claims that the matrix is more
> complicated than translate, just to catch any intervening bugs/slips.

Done.

[reed1, 2013-07-12 18:33:24]

I think pre-clipping the src bitmap is non-trivial, so it may be fine to do that
in a later CL.

That said, I don't know for sure how to make the call whether we should use the
new code-path w/o it.
pro: HQ scale-only is definitely faster (for mostly unclipped images)
con: somewhat unbounded temp allocation size for very large images, and possibly
slowing heavily-clipped image draws down

What say the other reviewers about this particular question pair:
1. land this CL w/o any preclipping
2. land this and enable or disable using it until we have a grip on the
preclipping impact

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

https://codereview.chromium.org/18978014/diff/35002/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:184: bool trivial_matrix = (inv.getType() &
~SkMatrix::kTranslate_Mask) == 0;
On 2013/07/12 17:18:01, humper wrote:
> On 2013/07/12 14:01:06, reed1 wrote:
> > This is probably too soon to be computing "trivialness" of the matrix, since
> we
> > may change the matrix later on... See the users of trivial_matrix below.
> 
> trivial_matrix is used immediately (so we need it now), to do a div by the
width
> and height of the image, and also later in choosing the matrix proc.  Should
we
> recompute it before its later use?

Yes, lets recompute it, either when we know we've changed fInvMatrix, or right
before we need it again (since it is so easy to compute). Probably a 1-line
helper function to encapsulate it like trans_or_identity()

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

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.h:64: const SkBitmap*     fBitmap;            //
chooseProcs - orig or mip
orig or scaled

[humper, 2013-07-12 18:34:14]

As of patch set 14, here are the bitmap_scale benchmark numbers for top-of-tree
vs. this unlanded CL:

top of tree: 
running bench [256 256]   bitmap_scale_filter_64_256  8888: cmsecs = 360.81
running bench [64 64]   bitmap_scale_filter_256_64  8888: cmsecs =  23.32
running bench [10 10]    bitmap_scale_filter_90_10  8888: cmsecs =   0.90
running bench [30 30]    bitmap_scale_filter_90_30  8888: cmsecs =   6.69
running bench [80 80]    bitmap_scale_filter_90_80  8888: cmsecs =  36.06
running bench [90 90]    bitmap_scale_filter_90_90  8888: cmsecs =   0.07
running bench [90 90]    bitmap_scale_filter_80_90  8888: cmsecs =  44.89
running bench [90 90]    bitmap_scale_filter_30_90  8888: cmsecs =  48.07
running bench [90 90]    bitmap_scale_filter_10_90  8888: cmsecs =  42.67
with CL:
running bench [256 256]   bitmap_scale_filter_64_256  8888: cmsecs = 111.89
running bench [64 64]   bitmap_scale_filter_256_64  8888: cmsecs =   4.96
running bench [10 10]    bitmap_scale_filter_90_10  8888: cmsecs =   0.66
running bench [30 30]    bitmap_scale_filter_90_30  8888: cmsecs =   0.79
running bench [80 80]    bitmap_scale_filter_90_80  8888: cmsecs =   1.09
running bench [90 90]    bitmap_scale_filter_90_90  8888: cmsecs =   0.07
running bench [90 90]    bitmap_scale_filter_80_90  8888: cmsecs =  21.23
running bench [90 90]    bitmap_scale_filter_30_90  8888: cmsecs =  17.32
running bench [90 90]    bitmap_scale_filter_10_90  8888: cmsecs =  12.74

[humper, 2013-07-12 19:02:47]

matrix triviality recomputed

[bsalomon, 2013-07-12 19:13:51]

I mostly just looked at the new code flow in src/core/SkBitmapProcShader.cpp and
it lgtm

[robertphillips, 2013-07-12 19:42:30]

lgtm + nits & some questions

https://codereview.chromium.org/18978014/diff/50001/gm/downsamplebitmap.cpp
File gm/downsamplebitmap.cpp (right):

https://codereview.chromium.org/18978014/diff/50001/gm/downsamplebitmap.cpp#n...
gm/downsamplebitmap.cpp:29: this->fBitmapMade = false;
no this-> on member variable access

https://codereview.chromium.org/18978014/diff/50001/gm/downsamplebitmap.cpp#n...
gm/downsamplebitmap.cpp:98: void make_bitmap() SK_OVERRIDE {
add virtual

https://codereview.chromium.org/18978014/diff/50001/gm/downsamplebitmap.cpp#n...
gm/downsamplebitmap.cpp:124: DownsampleBitmapCheckerboardGM(int size, int
num_checks)
numChecks

https://codereview.chromium.org/18978014/diff/50001/gm/downsamplebitmap.cpp#n...
gm/downsamplebitmap.cpp:136: void make_bitmap() SK_OVERRIDE {
virtual

https://codereview.chromium.org/18978014/diff/50001/gm/downsamplebitmap.cpp#n...
gm/downsamplebitmap.cpp:139: SkAutoLockPixels lock(fBM);
++x && ++y

https://codereview.chromium.org/18978014/diff/50001/gm/downsamplebitmap.cpp#n...
gm/downsamplebitmap.cpp:171: void make_bitmap() SK_OVERRIDE {
virtual

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

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:37: 
srcY

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:38: for (int src_y = y0; src_y <= y1; src_y++) {
xWeight, yWeight?

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:40: 
srcX

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:43: 
combinedWeight

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:95: 
srcY

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:96: for (int src_y = y0; src_y <= y1; src_y++) {
xWeight, yWeight?

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:101: 
combinedWeight

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:206: 
totalWeight

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:209: 
srcX

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:301: 
horizTemp

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:308: 
horizScale

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:316: 
vertScale

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

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.h...
src/core/SkBitmapFilter.h:25: precomputed = false;
this->invWidth()

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.h...
src/core/SkBitmapFilter.h:57: mutable bool precomputed;
fPrecomputed

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.h...
src/core/SkBitmapFilter.h:63: SkFixed *ftp = fFilterTable;
ftpScalar

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

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:113: // All the criteria are met; let's make a
new bitmap.
How does all this work out when the scaled width & height are fractional?

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:123: fInvMatrix.setTranslate(
1/fInvMatrix.getScaleX() * fInvMatrix.getTranslateX(),
align with above

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:187: 
trivialMatrix

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:188: bool trivial_matrix = (inv.getType() &
~SkMatrix::kTranslate_Mask) == 0;
clampClamp

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

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.h:37: SkBitmapProcState(): fBitmapFilter(NULL) {}
We don't need:

~SkBitmapProcState() {
    this->endContext()
}

?

https://codereview.chromium.org/18978014/diff/50001/src/opts/SkBitmapProcStat...
File src/opts/SkBitmapProcState_matrix_clamp_neon.h (right):

https://codereview.chromium.org/18978014/diff/50001/src/opts/SkBitmapProcStat...
src/opts/SkBitmapProcState_matrix_clamp_neon.h:70: s.fInvProc(s.fInvMatrix,
SkIntToScalar(x) + SK_ScalarHalf,
align this line

https://codereview.chromium.org/18978014/diff/50001/src/opts/SkBitmapProcStat...
src/opts/SkBitmapProcState_matrix_clamp_neon.h:495: s.fInvProc(s.fInvMatrix,
SkIntToScalar(x) + SK_ScalarHalf,
this line too

https://codereview.chromium.org/18978014/diff/50001/src/opts/SkBitmapProcStat...
File src/opts/SkBitmapProcState_matrix_repeat_neon.h (right):

https://codereview.chromium.org/18978014/diff/50001/src/opts/SkBitmapProcStat...
src/opts/SkBitmapProcState_matrix_repeat_neon.h:68: s.fInvProc(s.fInvMatrix,
SkIntToScalar(x) + SK_ScalarHalf,
this line too

https://codereview.chromium.org/18978014/diff/50001/src/opts/SkBitmapProcStat...
src/opts/SkBitmapProcState_matrix_repeat_neon.h:425: s.fInvProc(s.fInvMatrix,
SkIntToScalar(x) + SK_ScalarHalf,
align this line

https://codereview.chromium.org/18978014/diff/50001/src/opts/SkBitmapProcStat...
File src/opts/SkBitmapProcState_opts_SSE2.cpp (right):

https://codereview.chromium.org/18978014/diff/50001/src/opts/SkBitmapProcStat...
src/opts/SkBitmapProcState_opts_SSE2.cpp:258: s.fInvProc(s.fInvMatrix,
SkIntToScalar(x) + SK_ScalarHalf,
align

https://codereview.chromium.org/18978014/diff/50001/src/opts/SkBitmapProcStat...
src/opts/SkBitmapProcState_opts_SSE2.cpp:379: s.fInvProc(s.fInvMatrix,
SkIntToScalar(x) + SK_ScalarHalf,
align

[humper, 2013-07-12 21:12:03]

https://codereview.chromium.org/18978014/diff/50001/gm/downsamplebitmap.cpp
File gm/downsamplebitmap.cpp (right):

https://codereview.chromium.org/18978014/diff/50001/gm/downsamplebitmap.cpp#n...
gm/downsamplebitmap.cpp:29: this->fBitmapMade = false;
On 2013/07/12 19:42:31, robertphillips wrote:
> no this-> on member variable access

Done.

https://codereview.chromium.org/18978014/diff/50001/gm/downsamplebitmap.cpp#n...
gm/downsamplebitmap.cpp:98: void make_bitmap() SK_OVERRIDE {
On 2013/07/12 19:42:31, robertphillips wrote:
> add virtual

Done.

https://codereview.chromium.org/18978014/diff/50001/gm/downsamplebitmap.cpp#n...
gm/downsamplebitmap.cpp:124: DownsampleBitmapCheckerboardGM(int size, int
num_checks)
On 2013/07/12 19:42:31, robertphillips wrote:
> numChecks

Done.

https://codereview.chromium.org/18978014/diff/50001/gm/downsamplebitmap.cpp#n...
gm/downsamplebitmap.cpp:136: void make_bitmap() SK_OVERRIDE {
On 2013/07/12 19:42:31, robertphillips wrote:
> virtual

Done.

https://codereview.chromium.org/18978014/diff/50001/gm/downsamplebitmap.cpp#n...
gm/downsamplebitmap.cpp:139: SkAutoLockPixels lock(fBM);
On 2013/07/12 19:42:31, robertphillips wrote:
> ++x && ++y

Done.

https://codereview.chromium.org/18978014/diff/50001/gm/downsamplebitmap.cpp#n...
gm/downsamplebitmap.cpp:171: void make_bitmap() SK_OVERRIDE {
On 2013/07/12 19:42:31, robertphillips wrote:
> virtual

Done.

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

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:37: 
On 2013/07/12 19:42:31, robertphillips wrote:
> srcY

Done.

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:38: for (int src_y = y0; src_y <= y1; src_y++) {
On 2013/07/12 19:42:31, robertphillips wrote:
> xWeight, yWeight?

Done.

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:40: 
On 2013/07/12 19:42:31, robertphillips wrote:
> srcX

Done.

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:43: 
On 2013/07/12 19:42:31, robertphillips wrote:
> combinedWeight

Done.

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:95: 
On 2013/07/12 19:42:31, robertphillips wrote:
> srcY

Done.

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:96: for (int src_y = y0; src_y <= y1; src_y++) {
On 2013/07/12 19:42:31, robertphillips wrote:
> xWeight, yWeight?

Done.

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:101: 
On 2013/07/12 19:42:31, robertphillips wrote:
> combinedWeight

Done.

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:206: 
On 2013/07/12 19:42:31, robertphillips wrote:
> totalWeight

Done.

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:209: 
On 2013/07/12 19:42:31, robertphillips wrote:
> srcX

Done.

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:301: 
On 2013/07/12 19:42:31, robertphillips wrote:
> horizTemp

Done.

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:308: 
On 2013/07/12 19:42:31, robertphillips wrote:
> horizScale

Done.

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.c...
src/core/SkBitmapFilter.cpp:316: 
On 2013/07/12 19:42:31, robertphillips wrote:
> vertScale

Done.

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

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.h...
src/core/SkBitmapFilter.h:25: precomputed = false;
On 2013/07/12 19:42:31, robertphillips wrote:
> this->invWidth()

Done.

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.h...
src/core/SkBitmapFilter.h:57: mutable bool precomputed;
On 2013/07/12 19:42:31, robertphillips wrote:
> fPrecomputed

Done.

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapFilter.h...
src/core/SkBitmapFilter.h:63: SkFixed *ftp = fFilterTable;
On 2013/07/12 19:42:31, robertphillips wrote:
> ftpScalar

Done.

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

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:113: // All the criteria are met; let's make a
new bitmap.
On 2013/07/12 19:42:31, robertphillips wrote:
> How does all this work out when the scaled width & height are fractional?

Fractional widths are (currently) getting rounded down to the nearest integer. 
Perhaps this should be filed as a bug for future discussion.

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:123: fInvMatrix.setTranslate(
1/fInvMatrix.getScaleX() * fInvMatrix.getTranslateX(),
On 2013/07/12 19:42:31, robertphillips wrote:
> align with above

Done.

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:187: 
On 2013/07/12 19:42:31, robertphillips wrote:
> trivialMatrix

Done.

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:188: bool trivial_matrix = (inv.getType() &
~SkMatrix::kTranslate_Mask) == 0;
On 2013/07/12 19:42:31, robertphillips wrote:
> clampClamp

Done.

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

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.h:37: SkBitmapProcState(): fBitmapFilter(NULL) {}
On 2013/07/12 19:42:31, robertphillips wrote:
> We don't need:
> 
> ~SkBitmapProcState() {
>     this->endContext()
> }
> 
> ?

Cleaned up.  It's not right to call endcontext, but we should make sure that we
don't leak if, for some reason, endContext wasn't called.

https://codereview.chromium.org/18978014/diff/50001/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.h:64: const SkBitmap*     fBitmap;            //
chooseProcs - orig or mip
On 2013/07/12 18:33:24, reed1 wrote:
> orig or scaled

Done.

[humper, 2013-07-12 21:14:39]

Committed patchset #19 manually as r10056 (presubmit successful).

[robertphillips, 2013-07-13 15:39:55]

I tried to turn the bots green again by disabling this by defining
SK_IGNORE_IMAGE_PRESCALE but GM was still hitting some asserts even with this.

I reverted this CL in r10060.