Centralize decision about whether to do bicubic filtering, and fallbacks to mip, bilerp, or nearest

Centralize decision about whether to do bicubic filtering, and fallbacks to mip, bilerp, or nearest

Committed: http://code.google.com/p/skia/source/detail?r=14809

[bsalomon, 2014-05-20 15:11:12]

This fixes a performance regression after Chromium started turning on HQ
filtering more often. We see a lot of draws that  draw bitmaps 1:1 with device
pixels with HQ filtering enabled. The sw path already disables filtering in this
case.

[bsalomon, 2014-05-20 15:13:28]

Depends on landing https://codereview.chromium.org/298473002/

[robertphillips, 2014-05-20 15:34:50]

https://codereview.chromium.org/282293004/diff/60001/src/core/SkBitmapProcSha...
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.chromium.org/282293004/diff/60001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.cpp:430: break;
Indent on this '}'?

https://codereview.chromium.org/282293004/diff/60001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.cpp:436: if (GrTextureParams::kNone_FilterMode ==
textureFilterMode) {
Why not textureFilterMode ?

https://codereview.chromium.org/282293004/diff/60001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/282293004/diff/60001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:1191: // Minification can look bad with the bicubic
effect.
Do we need to initialize textureFilterMode to be bicubic before calling
ShouldUseBicubic?

https://codereview.chromium.org/282293004/diff/60001/src/gpu/effects/GrBicubi...
File src/gpu/effects/GrBicubicEffect.cpp (right):

https://codereview.chromium.org/282293004/diff/60001/src/gpu/effects/GrBicubi...
src/gpu/effects/GrBicubicEffect.cpp:191: if (!matrix.getScaleFactors(scales) ||
scales[0] < SK_Scalar1) {
// b.c. the min scale factor is causing we will use mip maps to yield a better
minimization result ?

https://codereview.chromium.org/282293004/diff/60001/src/gpu/effects/GrBicubi...
src/gpu/effects/GrBicubicEffect.cpp:194: }
scales ?

https://codereview.chromium.org/282293004/diff/60001/src/gpu/effects/GrBicubi...
src/gpu/effects/GrBicubicEffect.cpp:201: *filterMode =
GrTextureParams::kBilerp_FilterMode;
// need bilerp in this case to handle the fractional translation ?

https://codereview.chromium.org/282293004/diff/60001/src/gpu/effects/GrBicubi...
File src/gpu/effects/GrBicubicEffect.h (right):

https://codereview.chromium.org/282293004/diff/60001/src/gpu/effects/GrBicubi...
src/gpu/effects/GrBicubicEffect.h:81: /**
The return value discussion doesn't seem right.
Maybe:
// Returns false if the filterMode has been changed; true otherwise. ?

[bsalomon, 2014-05-20 16:13:06]

https://codereview.chromium.org/282293004/diff/60001/src/core/SkBitmapProcSha...
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.chromium.org/282293004/diff/60001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.cpp:430: break;
On 2014/05/20 15:34:50, robertphillips wrote:
> Indent on this '}'?

Done.

https://codereview.chromium.org/282293004/diff/60001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.cpp:436: if (GrTextureParams::kNone_FilterMode ==
textureFilterMode) {
On 2014/05/20 15:34:50, robertphillips wrote:
> Why not textureFilterMode ?

It is now always set by ShouldUseBicubic.

https://codereview.chromium.org/282293004/diff/60001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/282293004/diff/60001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:1191: // Minification can look bad with the bicubic
effect.
On 2014/05/20 15:34:50, robertphillips wrote:
> Do we need to initialize textureFilterMode to be bicubic before calling
> ShouldUseBicubic?

not now :)

https://codereview.chromium.org/282293004/diff/60001/src/gpu/effects/GrBicubi...
File src/gpu/effects/GrBicubicEffect.cpp (right):

https://codereview.chromium.org/282293004/diff/60001/src/gpu/effects/GrBicubi...
src/gpu/effects/GrBicubicEffect.cpp:191: if (!matrix.getScaleFactors(scales) ||
scales[0] < SK_Scalar1) {
On 2014/05/20 15:34:50, robertphillips wrote:
> // b.c. the min scale factor is causing we will use mip maps to yield a better
> minimization result ?

Done.

https://codereview.chromium.org/282293004/diff/60001/src/gpu/effects/GrBicubi...
src/gpu/effects/GrBicubicEffect.cpp:194: }
On 2014/05/20 15:34:50, robertphillips wrote:
> scales ?

Done.

https://codereview.chromium.org/282293004/diff/60001/src/gpu/effects/GrBicubi...
src/gpu/effects/GrBicubicEffect.cpp:201: *filterMode =
GrTextureParams::kBilerp_FilterMode;
On 2014/05/20 15:34:50, robertphillips wrote:
> // need bilerp in this case to handle the fractional translation ?

Done.

https://codereview.chromium.org/282293004/diff/60001/src/gpu/effects/GrBicubi...
File src/gpu/effects/GrBicubicEffect.h (right):

https://codereview.chromium.org/282293004/diff/60001/src/gpu/effects/GrBicubi...
src/gpu/effects/GrBicubicEffect.h:81: /**
On 2014/05/20 15:34:50, robertphillips wrote:
> The return value discussion doesn't seem right.
> Maybe:
> // Returns false if the filterMode has been changed; true otherwise. ?

Done.

[humper, 2014-05-20 17:06:19]

On 2014/05/20 16:13:06, bsalomon wrote:
>
https://codereview.chromium.org/282293004/diff/60001/src/core/SkBitmapProcSha...
> File src/core/SkBitmapProcShader.cpp (right):
> 
>
https://codereview.chromium.org/282293004/diff/60001/src/core/SkBitmapProcSha...
> src/core/SkBitmapProcShader.cpp:430: break;
> On 2014/05/20 15:34:50, robertphillips wrote:
> > Indent on this '}'?
> 
> Done.
> 
>
https://codereview.chromium.org/282293004/diff/60001/src/core/SkBitmapProcSha...
> src/core/SkBitmapProcShader.cpp:436: if (GrTextureParams::kNone_FilterMode ==
> textureFilterMode) {
> On 2014/05/20 15:34:50, robertphillips wrote:
> > Why not textureFilterMode ?
> 
> It is now always set by ShouldUseBicubic.
> 
> https://codereview.chromium.org/282293004/diff/60001/src/gpu/SkGpuDevice.cpp
> File src/gpu/SkGpuDevice.cpp (right):
> 
>
https://codereview.chromium.org/282293004/diff/60001/src/gpu/SkGpuDevice.cpp#...
> src/gpu/SkGpuDevice.cpp:1191: // Minification can look bad with the bicubic
> effect.
> On 2014/05/20 15:34:50, robertphillips wrote:
> > Do we need to initialize textureFilterMode to be bicubic before calling
> > ShouldUseBicubic?
> 
> not now :)
> 
>
https://codereview.chromium.org/282293004/diff/60001/src/gpu/effects/GrBicubi...
> File src/gpu/effects/GrBicubicEffect.cpp (right):
> 
>
https://codereview.chromium.org/282293004/diff/60001/src/gpu/effects/GrBicubi...
> src/gpu/effects/GrBicubicEffect.cpp:191: if (!matrix.getScaleFactors(scales)
||
> scales[0] < SK_Scalar1) {
> On 2014/05/20 15:34:50, robertphillips wrote:
> > // b.c. the min scale factor is causing we will use mip maps to yield a
better
> > minimization result ?
> 
> Done.
> 
>
https://codereview.chromium.org/282293004/diff/60001/src/gpu/effects/GrBicubi...
> src/gpu/effects/GrBicubicEffect.cpp:194: }
> On 2014/05/20 15:34:50, robertphillips wrote:
> > scales ?
> 
> Done.
> 
>
https://codereview.chromium.org/282293004/diff/60001/src/gpu/effects/GrBicubi...
> src/gpu/effects/GrBicubicEffect.cpp:201: *filterMode =
> GrTextureParams::kBilerp_FilterMode;
> On 2014/05/20 15:34:50, robertphillips wrote:
> > // need bilerp in this case to handle the fractional translation ?
> 
> Done.
> 
>
https://codereview.chromium.org/282293004/diff/60001/src/gpu/effects/GrBicubi...
> File src/gpu/effects/GrBicubicEffect.h (right):
> 
>
https://codereview.chromium.org/282293004/diff/60001/src/gpu/effects/GrBicubi...
> src/gpu/effects/GrBicubicEffect.h:81: /**
> On 2014/05/20 15:34:50, robertphillips wrote:
> > The return value discussion doesn't seem right.
> > Maybe:
> > // Returns false if the filterMode has been changed; true otherwise. ?
> 
> Done.

LGTM, and the other patch that Robert mentioned above has landed.

[bsalomon, 2014-05-20 17:39:49]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2014-05-20 17:40:17]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/bsalomon@google.com/282293004/90001

[commit-bot: I haz the power, 2014-05-20 17:51:16]

Change committed as 14809