stop using bitmap-filter flags outside of paint itself, as a step towards really changing them into…

stop using bitmap-filter flags outside of paint itself, as a step towards really changing them into an enum

BUG=

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

[reed1, 2013-07-19 17:50:26]

1 or 2 places still to really fix (instead of comment-out), but reviewers should
look to see 

1. they like the feel of the newly introduced enum FilterLevel
2. the transcription is correct at the call-sites

[robertphillips, 2013-07-19 18:02:52]

1) I like it
2) they look good to me (but I defer to Greg)

[humper, 2013-07-19 18:20:26]

https://codereview.chromium.org/19825002/diff/1/gm/downsamplebitmap.cpp
File gm/downsamplebitmap.cpp (right):

https://codereview.chromium.org/19825002/diff/1/gm/downsamplebitmap.cpp#newco...
gm/downsamplebitmap.cpp:67: paint.setFilterLevel(SkPaint::kHigh_FilterLevel);
We should probably have a set of GM tests that demonstrate downsampling at all
four qualities.  I'll take care of expanding this GM once this change lands.

https://codereview.chromium.org/19825002/diff/1/gm/filterbitmap.cpp
File gm/filterbitmap.cpp (right):

https://codereview.chromium.org/19825002/diff/1/gm/filterbitmap.cpp#newcode38
gm/filterbitmap.cpp:38: paint.setFilterLevel(SkPaint::kHigh_FilterLevel);
Probably for completeness this GM should be changed to draw four columns and
explicitly set none, low, medium, and high.  The output bitmap will have to be
widened a little to make room for the fourth column.

Today, we expect medium and high quality up-scaling to be identical, but that
might not always be true and it would be good to already have a test in place to
at least verify this.

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

https://codereview.chromium.org/19825002/diff/1/include/core/SkPaint.h#newcode61
include/core/SkPaint.h:61: kHighQualityDownsampleBitmap_Flag = 0x8000, //
temporary flag
I don't think anyone uses kHighQualityDownsampleBitmap_Flag; you can probably
just delete it from the temporary deprecation structure.

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

https://codereview.chromium.org/19825002/diff/1/src/core/SkBitmapFilter.cpp#n...
src/core/SkBitmapFilter.cpp:152: if (fFilterLevel != SkPaint::kHigh_FilterLevel)
{
This should be a check for "less than medium" rather than "not equal to high".

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

https://codereview.chromium.org/19825002/diff/1/src/core/SkBitmapProcState.cp...
src/core/SkBitmapProcState.cpp:110: if (SkPaint::kHigh_FilterLevel ==
fFilterLevel &&
This logic needs to be more sophisticated, because medium/high upscaling are
both a direct HQ upscale, but medium/high downscaling are different (mipmap /
direct scale respectively).

https://codereview.chromium.org/19825002/diff/1/src/core/SkBitmapProcState.cp...
src/core/SkBitmapProcState.cpp:135: // If we get here, they are asking for a
mipmap (if we're scaling down)
Sort of -- the HQ downscaler isn't a mipmap.  The CL that I'm working on removes
the getScaleX() test from the big if-block above, which will do a direct
downscale always.  See my comment above regarding the four levels of downscale
quality.

https://codereview.chromium.org/19825002/diff/1/src/core/SkBitmapProcState.cp...
src/core/SkBitmapProcState.cpp:260: if (SkPaint::kHigh_FilterLevel ==
fFilterLevel) {
What about poor medium level

[reed1, 2013-07-19 18:29:16]

Greg raises a point that this CL implicitly is defining a mapping between the
new enum and our current impl.

CL's mapping:

none --> nearest neighbor
low --> bilerp
med --> down: mipmap+bilerp, up:bilerp
high --> cubic

There are other possible interpretations. How do we feel about this mapping?

https://codereview.chromium.org/19825002/diff/1/gm/downsamplebitmap.cpp
File gm/downsamplebitmap.cpp (right):

https://codereview.chromium.org/19825002/diff/1/gm/downsamplebitmap.cpp#newco...
gm/downsamplebitmap.cpp:67: paint.setFilterLevel(SkPaint::kHigh_FilterLevel);
On 2013/07/19 18:20:26, humper wrote:
> We should probably have a set of GM tests that demonstrate downsampling at all
> four qualities.  I'll take care of expanding this GM once this change lands.

agreed

https://codereview.chromium.org/19825002/diff/1/gm/filterbitmap.cpp
File gm/filterbitmap.cpp (right):

https://codereview.chromium.org/19825002/diff/1/gm/filterbitmap.cpp#newcode38
gm/filterbitmap.cpp:38: paint.setFilterLevel(SkPaint::kHigh_FilterLevel);
On 2013/07/19 18:20:26, humper wrote:
> Probably for completeness this GM should be changed to draw four columns and
> explicitly set none, low, medium, and high.  The output bitmap will have to be
> widened a little to make room for the fourth column.
> 
> Today, we expect medium and high quality up-scaling to be identical, but that
> might not always be true and it would be good to already have a test in place
to
> at least verify this.

Agreed. This CL was just meant to achieve parity along with the API change, but
agreed we will want expanded gms.

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

https://codereview.chromium.org/19825002/diff/1/include/core/SkPaint.h#newcode61
include/core/SkPaint.h:61: kHighQualityDownsampleBitmap_Flag = 0x8000, //
temporary flag
On 2013/07/19 18:20:26, humper wrote:
> I don't think anyone uses kHighQualityDownsampleBitmap_Flag; you can probably
> just delete it from the temporary deprecation structure.

After this lands, I plan to remove all of these, and replace with an explicit
enum (bit-field)

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

https://codereview.chromium.org/19825002/diff/1/src/core/SkBitmapFilter.cpp#n...
src/core/SkBitmapFilter.cpp:152: if (fFilterLevel != SkPaint::kHigh_FilterLevel)
{
On 2013/07/19 18:20:26, humper wrote:
> This should be a check for "less than medium" rather than "not equal to high".

see: mapping debate

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

https://codereview.chromium.org/19825002/diff/1/src/core/SkBitmapProcState.cp...
src/core/SkBitmapProcState.cpp:110: if (SkPaint::kHigh_FilterLevel ==
fFilterLevel &&
On 2013/07/19 18:20:26, humper wrote:
> This logic needs to be more sophisticated, because medium/high upscaling are
> both a direct HQ upscale, but medium/high downscaling are different (mipmap /
> direct scale respectively).

Not sure I agree. It may be more logical to have Low/Med both map to bilerp, and
only High map to cubic. That would more closely parallel what happens in
down-scaling.

https://codereview.chromium.org/19825002/diff/1/src/core/SkBitmapProcState.cp...
src/core/SkBitmapProcState.cpp:260: if (SkPaint::kHigh_FilterLevel ==
fFilterLevel) {
On 2013/07/19 18:20:26, humper wrote:
> What about poor medium level

again: depends on our (new) definition of what Med maps to...

[humper, 2013-07-19 18:34:35]

On 2013/07/19 18:29:16, reed1 wrote:
> Greg raises a point that this CL implicitly is defining a mapping between the
> new enum and our current impl.
> 
> CL's mapping:
> 
> none --> nearest neighbor
> low --> bilerp
> med --> down: mipmap+bilerp, up:bilerp
> high --> cubic
> 
> There are other possible interpretations. How do we feel about this mapping?
> 
> https://codereview.chromium.org/19825002/diff/1/gm/downsamplebitmap.cpp
> File gm/downsamplebitmap.cpp (right):
> 
>
https://codereview.chromium.org/19825002/diff/1/gm/downsamplebitmap.cpp#newco...
> gm/downsamplebitmap.cpp:67: paint.setFilterLevel(SkPaint::kHigh_FilterLevel);
> On 2013/07/19 18:20:26, humper wrote:
> > We should probably have a set of GM tests that demonstrate downsampling at
all
> > four qualities.  I'll take care of expanding this GM once this change lands.
> 
> agreed
> 
> https://codereview.chromium.org/19825002/diff/1/gm/filterbitmap.cpp
> File gm/filterbitmap.cpp (right):
> 
> https://codereview.chromium.org/19825002/diff/1/gm/filterbitmap.cpp#newcode38
> gm/filterbitmap.cpp:38: paint.setFilterLevel(SkPaint::kHigh_FilterLevel);
> On 2013/07/19 18:20:26, humper wrote:
> > Probably for completeness this GM should be changed to draw four columns and
> > explicitly set none, low, medium, and high.  The output bitmap will have to
be
> > widened a little to make room for the fourth column.
> > 
> > Today, we expect medium and high quality up-scaling to be identical, but
that
> > might not always be true and it would be good to already have a test in
place
> to
> > at least verify this.
> 
> Agreed. This CL was just meant to achieve parity along with the API change,
but
> agreed we will want expanded gms.
> 
> https://codereview.chromium.org/19825002/diff/1/include/core/SkPaint.h
> File include/core/SkPaint.h (right):
> 
>
https://codereview.chromium.org/19825002/diff/1/include/core/SkPaint.h#newcode61
> include/core/SkPaint.h:61: kHighQualityDownsampleBitmap_Flag = 0x8000, //
> temporary flag
> On 2013/07/19 18:20:26, humper wrote:
> > I don't think anyone uses kHighQualityDownsampleBitmap_Flag; you can
probably
> > just delete it from the temporary deprecation structure.
> 
> After this lands, I plan to remove all of these, and replace with an explicit
> enum (bit-field)
> 
> https://codereview.chromium.org/19825002/diff/1/src/core/SkBitmapFilter.cpp
> File src/core/SkBitmapFilter.cpp (right):
> 
>
https://codereview.chromium.org/19825002/diff/1/src/core/SkBitmapFilter.cpp#n...
> src/core/SkBitmapFilter.cpp:152: if (fFilterLevel !=
SkPaint::kHigh_FilterLevel)
> {
> On 2013/07/19 18:20:26, humper wrote:
> > This should be a check for "less than medium" rather than "not equal to
high".
> 
> see: mapping debate
> 
> https://codereview.chromium.org/19825002/diff/1/src/core/SkBitmapProcState.cpp
> File src/core/SkBitmapProcState.cpp (right):
> 
>
https://codereview.chromium.org/19825002/diff/1/src/core/SkBitmapProcState.cp...
> src/core/SkBitmapProcState.cpp:110: if (SkPaint::kHigh_FilterLevel ==
> fFilterLevel &&
> On 2013/07/19 18:20:26, humper wrote:
> > This logic needs to be more sophisticated, because medium/high upscaling are
> > both a direct HQ upscale, but medium/high downscaling are different (mipmap
/
> > direct scale respectively).
> 
> Not sure I agree. It may be more logical to have Low/Med both map to bilerp,
and
> only High map to cubic. That would more closely parallel what happens in
> down-scaling.
> 
>
https://codereview.chromium.org/19825002/diff/1/src/core/SkBitmapProcState.cp...
> src/core/SkBitmapProcState.cpp:260: if (SkPaint::kHigh_FilterLevel ==
> fFilterLevel) {
> On 2013/07/19 18:20:26, humper wrote:
> > What about poor medium level
> 
> again: depends on our (new) definition of what Med maps to...

You're correct; if we map medium upscales to bilerp and ONLY do the direct
scaling when high is requested, then I think it's been done correctly.

[bsalomon, 2013-07-19 19:26:41]

On 2013/07/19 18:34:35, humper wrote:
> On 2013/07/19 18:29:16, reed1 wrote:
> > Greg raises a point that this CL implicitly is defining a mapping between
the
> > new enum and our current impl.
> > 
> > CL's mapping:
> > 
> > none --> nearest neighbor
> > low --> bilerp
> > med --> down: mipmap+bilerp, up:bilerp
> > high --> cubic
> > 
> > There are other possible interpretations. How do we feel about this mapping?
> > 
> > https://codereview.chromium.org/19825002/diff/1/gm/downsamplebitmap.cpp
> > File gm/downsamplebitmap.cpp (right):
> > 
> >
>
https://codereview.chromium.org/19825002/diff/1/gm/downsamplebitmap.cpp#newco...
> > gm/downsamplebitmap.cpp:67:
paint.setFilterLevel(SkPaint::kHigh_FilterLevel);
> > On 2013/07/19 18:20:26, humper wrote:
> > > We should probably have a set of GM tests that demonstrate downsampling at
> all
> > > four qualities.  I'll take care of expanding this GM once this change
lands.
> > 
> > agreed
> > 
> > https://codereview.chromium.org/19825002/diff/1/gm/filterbitmap.cpp
> > File gm/filterbitmap.cpp (right):
> > 
> >
https://codereview.chromium.org/19825002/diff/1/gm/filterbitmap.cpp#newcode38
> > gm/filterbitmap.cpp:38: paint.setFilterLevel(SkPaint::kHigh_FilterLevel);
> > On 2013/07/19 18:20:26, humper wrote:
> > > Probably for completeness this GM should be changed to draw four columns
and
> > > explicitly set none, low, medium, and high.  The output bitmap will have
to
> be
> > > widened a little to make room for the fourth column.
> > > 
> > > Today, we expect medium and high quality up-scaling to be identical, but
> that
> > > might not always be true and it would be good to already have a test in
> place
> > to
> > > at least verify this.
> > 
> > Agreed. This CL was just meant to achieve parity along with the API change,
> but
> > agreed we will want expanded gms.
> > 
> > https://codereview.chromium.org/19825002/diff/1/include/core/SkPaint.h
> > File include/core/SkPaint.h (right):
> > 
> >
>
https://codereview.chromium.org/19825002/diff/1/include/core/SkPaint.h#newcode61
> > include/core/SkPaint.h:61: kHighQualityDownsampleBitmap_Flag = 0x8000, //
> > temporary flag
> > On 2013/07/19 18:20:26, humper wrote:
> > > I don't think anyone uses kHighQualityDownsampleBitmap_Flag; you can
> probably
> > > just delete it from the temporary deprecation structure.
> > 
> > After this lands, I plan to remove all of these, and replace with an
explicit
> > enum (bit-field)
> > 
> > https://codereview.chromium.org/19825002/diff/1/src/core/SkBitmapFilter.cpp
> > File src/core/SkBitmapFilter.cpp (right):
> > 
> >
>
https://codereview.chromium.org/19825002/diff/1/src/core/SkBitmapFilter.cpp#n...
> > src/core/SkBitmapFilter.cpp:152: if (fFilterLevel !=
> SkPaint::kHigh_FilterLevel)
> > {
> > On 2013/07/19 18:20:26, humper wrote:
> > > This should be a check for "less than medium" rather than "not equal to
> high".
> > 
> > see: mapping debate
> > 
> >
https://codereview.chromium.org/19825002/diff/1/src/core/SkBitmapProcState.cpp
> > File src/core/SkBitmapProcState.cpp (right):
> > 
> >
>
https://codereview.chromium.org/19825002/diff/1/src/core/SkBitmapProcState.cp...
> > src/core/SkBitmapProcState.cpp:110: if (SkPaint::kHigh_FilterLevel ==
> > fFilterLevel &&
> > On 2013/07/19 18:20:26, humper wrote:
> > > This logic needs to be more sophisticated, because medium/high upscaling
are
> > > both a direct HQ upscale, but medium/high downscaling are different
(mipmap
> /
> > > direct scale respectively).
> > 
> > Not sure I agree. It may be more logical to have Low/Med both map to bilerp,
> and
> > only High map to cubic. That would more closely parallel what happens in
> > down-scaling.
> > 
> >
>
https://codereview.chromium.org/19825002/diff/1/src/core/SkBitmapProcState.cp...
> > src/core/SkBitmapProcState.cpp:260: if (SkPaint::kHigh_FilterLevel ==
> > fFilterLevel) {
> > On 2013/07/19 18:20:26, humper wrote:
> > > What about poor medium level
> > 
> > again: depends on our (new) definition of what Med maps to...
> 
> You're correct; if we map medium upscales to bilerp and ONLY do the direct
> scaling when high is requested, then I think it's been done correctly.

What he said. It seems right that the HQ scaling code turns on at the same level
for both up/down.

[reed1, 2013-07-19 19:45:45]

Now just waiting on chrome CL to we can actually hide those bits.

[reed1, 2013-07-22 17:18:25]

Committed patchset #2 manually as r10240 (presubmit successful).