Add bilerp filtering.

Add bilerp filtering.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1705203002

Committed: https://skia.googlesource.com/skia/+/c5eddd7d8d67a6e931973a729c5868c155cb751f

[herb_g, 2016-02-17 19:38:55]

Description was changed from

==========
Add bilerp filtering.

BUG=skia:
==========

to

==========
Add bilerp filtering.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[herb_g, 2016-02-17 19:44:04]

Description was changed from

==========
Add bilerp filtering.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Add bilerp filtering.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[herb_g, 2016-02-17 19:44:05]

herb@google.com changed reviewers:
+ mtklein@google.com, reed@google.com

[herb_g, 2016-02-17 19:47:33]

Here is the next stage of the CL pipeline

Roadmap of coming cls.
1) simple point list no filtering << checked in
2) add bilerp filtering << this cl
3) add nearest neighbor sampling spans using fall back to point list
4) add span specialized implementations
5) add bilerp spans using fall back to bilerp
6) add bilerp span specializations

[mtklein, 2016-02-17 20:25:10]

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
File src/core/SkLinearBitmapPipeline.cpp (right):

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
src/core/SkLinearBitmapPipeline.cpp:59: class BilerpProcessor : public
BilerpProcessorInterface {
final (and PointProcessor too)?

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
src/core/SkLinearBitmapPipeline.cpp:67: Sk4f newXs = xs;
This is so sad.  :(

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
src/core/SkLinearBitmapPipeline.cpp:99: virtual void bilerpList(Sk4fArg xs,
Sk4fArg ys) override {
I think override's plenty.

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
src/core/SkLinearBitmapPipeline.cpp:193: SkASSERT(n > 0 && n <= 4);
This seems awkward.  It's a bug if we call pointListFew(4, ...) externally, but
we've made it valid so that self calls can work?  I think I'd prefer:

void pointListFew(...) {
  SkASSERT(n > 0 && n < 4);
  for (int i = 0; i < n; i++) {
      ...
  }
}

void pointList4(...) {
    for (int i = 0; i < 4; i++) {
        ...
    }
}

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
src/core/SkLinearBitmapPipeline.cpp:201: pointListFew(4, xs, ys);
self calls usually have a this->

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
src/core/SkLinearBitmapPipeline.cpp:202: }
Is this why we distinguish PointProcessor and BilerpProcessor, because there's
no sensible implementation of bilerpList() here?

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
src/core/SkLinearBitmapPipeline.cpp:211: const Sk4f
ExpandBilerp<Next>::kXOffsets = {0.0f, 1.0f, 0.0f, 1.0f};
This is all sort of awkward and boilerplate-y.

Let's just write

const Sk4f xOffsets = { 0,1,0,1 },
           yOffsets = { 0,0,1,1 };

where needed?  The compiler will happily sort of the rest.

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
src/core/SkLinearBitmapPipeline.cpp:219: if (filterQuailty >=
SkFilterQuality::kLow_SkFilterQuality) {
Might wanna write as:
   none -> skipped
   else -> bilerp
?

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
src/core/SkLinearBitmapPipeline.cpp:388: inline Sk4f bilerp4(SkPoint pt, Sk4f
pixel00, Sk4f pixel01,
It's a good habit to get into writing static on all these.

Do you not need to use Sk4fArg?

Might be helpful to inline up the point arguments like they're laid out in
space, scooting over the second line a bit.

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
src/core/SkLinearBitmapPipeline.cpp:390: Sk4f Xs{X(pt) - std::floor(X(pt))};
If we're using std:: stuff, might want to consider 
   Sk4f Xs { std::fmod(X(pt), 1) };
etc.

Might be nice for these names to reflect that they're only the fractional part
of X and Y.

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
src/core/SkLinearBitmapPipeline.cpp:396: sum = sum + pixel00 * (Sk4f{1.0f} - Xs
- Ys + XYs);
This would be a really great place for an ASCII art diagram and comments
explaining the math.

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
src/core/SkLinearBitmapPipeline.cpp:424: SkPoint referencePoint{xs[0], ys[0]};
Do these all have the same fractional parts?  I'm curious if we can avoid having
to pick a reference point at all.

[herb_g, 2016-02-17 23:38:13]

Redo bilerp.

[herb_g, 2016-02-17 23:40:44]

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
File src/core/SkLinearBitmapPipeline.cpp (right):

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
src/core/SkLinearBitmapPipeline.cpp:59: class BilerpProcessor : public
BilerpProcessorInterface {
On 2016/02/17 20:25:09, mtklein wrote:
> final (and PointProcessor too)?

Done.

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
src/core/SkLinearBitmapPipeline.cpp:67: Sk4f newXs = xs;
On 2016/02/17 20:25:09, mtklein wrote:
> This is so sad.  :(

Acknowledged.

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
src/core/SkLinearBitmapPipeline.cpp:99: virtual void bilerpList(Sk4fArg xs,
Sk4fArg ys) override {
On 2016/02/17 20:25:09, mtklein wrote:
> I think override's plenty.

Done.

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
src/core/SkLinearBitmapPipeline.cpp:193: SkASSERT(n > 0 && n <= 4);
On 2016/02/17 20:25:09, mtklein wrote:
> This seems awkward.  It's a bug if we call pointListFew(4, ...) externally,
but
> we've made it valid so that self calls can work?  I think I'd prefer:
> 
> void pointListFew(...) {
>   SkASSERT(n > 0 && n < 4);
>   for (int i = 0; i < n; i++) {
>       ...
>   }
> }
> 
> void pointList4(...) {
>     for (int i = 0; i < 4; i++) {
>         ...
>     }
> }

Done.

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
src/core/SkLinearBitmapPipeline.cpp:201: pointListFew(4, xs, ys);
On 2016/02/17 20:25:09, mtklein wrote:
> self calls usually have a this->

Removed

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
src/core/SkLinearBitmapPipeline.cpp:202: }
On 2016/02/17 20:25:09, mtklein wrote:
> Is this why we distinguish PointProcessor and BilerpProcessor, because there's
> no sensible implementation of bilerpList() here?

There is no sensible bilerp here or at the matrix level.

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
src/core/SkLinearBitmapPipeline.cpp:211: const Sk4f
ExpandBilerp<Next>::kXOffsets = {0.0f, 1.0f, 0.0f, 1.0f};
On 2016/02/17 20:25:09, mtklein wrote:
> This is all sort of awkward and boilerplate-y.
> 
> Let's just write
> 
> const Sk4f xOffsets = { 0,1,0,1 },
>            yOffsets = { 0,0,1,1 };
> 
> where needed?  The compiler will happily sort of the rest.

Done.

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
src/core/SkLinearBitmapPipeline.cpp:219: if (filterQuailty >=
SkFilterQuality::kLow_SkFilterQuality) {
On 2016/02/17 20:25:09, mtklein wrote:
> Might wanna write as:
>    none -> skipped
>    else -> bilerp
> ?

Done.

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
src/core/SkLinearBitmapPipeline.cpp:388: inline Sk4f bilerp4(SkPoint pt, Sk4f
pixel00, Sk4f pixel01,
On 2016/02/17 20:25:09, mtklein wrote:
> It's a good habit to get into writing static on all these.
> 
> Do you not need to use Sk4fArg?
> 
> Might be helpful to inline up the point arguments like they're laid out in
> space, scooting over the second line a bit.

Done.

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
src/core/SkLinearBitmapPipeline.cpp:390: Sk4f Xs{X(pt) - std::floor(X(pt))};
On 2016/02/17 20:25:09, mtklein wrote:
> If we're using std:: stuff, might want to consider 
>    Sk4f Xs { std::fmod(X(pt), 1) };
> etc.
> 
> Might be nice for these names to reflect that they're only the fractional part
> of X and Y.

Redid whole function.

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
src/core/SkLinearBitmapPipeline.cpp:396: sum = sum + pixel00 * (Sk4f{1.0f} - Xs
- Ys + XYs);
On 2016/02/17 20:25:09, mtklein wrote:
> This would be a really great place for an ASCII art diagram and comments
> explaining the math.

Done.

https://codereview.chromium.org/1705203002/diff/1/src/core/SkLinearBitmapPipe...
src/core/SkLinearBitmapPipeline.cpp:424: SkPoint referencePoint{xs[0], ys[0]};
On 2016/02/17 20:25:09, mtklein wrote:
> Do these all have the same fractional parts?  I'm curious if we can avoid
having
> to pick a reference point at all.

Done.

[herb_g, 2016-02-17 23:40:52]

The CQ bit was checked by herb@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-17 23:40:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1705203002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1705203002/20001

[mtklein, 2016-02-17 23:46:56]

lgtm!

https://codereview.chromium.org/1705203002/diff/20001/src/core/SkLinearBitmap...
File src/core/SkLinearBitmapPipeline.cpp (right):

https://codereview.chromium.org/1705203002/diff/20001/src/core/SkLinearBitmap...
src/core/SkLinearBitmapPipeline.cpp:407: // So x * y is calculated first and
then used to calculate all the other factors.
This is beautiful.  Nominated for favorite code snippet of the week.

[commit-bot: I haz the power, 2016-02-18 00:03:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-18 00:03:18]

Dry run: This issue passed the CQ dry run.

[herb_g, 2016-02-18 03:49:06]

https://codereview.chromium.org/1705203002/diff/20001/src/core/SkLinearBitmap...
File src/core/SkLinearBitmapPipeline.cpp (right):

https://codereview.chromium.org/1705203002/diff/20001/src/core/SkLinearBitmap...
src/core/SkLinearBitmapPipeline.cpp:407: // So x * y is calculated first and
then used to calculate all the other factors.
On 2016/02/17 23:46:55, mtklein wrote:
> This is beautiful.  Nominated for favorite code snippet of the week.

Acknowledged.

[herb_g, 2016-02-18 03:49:16]

The CQ bit was checked by herb@google.com

[commit-bot: I haz the power, 2016-02-18 03:49:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1705203002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1705203002/20001

[commit-bot: I haz the power, 2016-02-18 03:50:08]

Description was changed from

==========
Add bilerp filtering.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Add bilerp filtering.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/c5eddd7d8d67a6e931973a729c5868c155cb751f
==========

[commit-bot: I haz the power, 2016-02-18 03:50:09]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/c5eddd7d8d67a6e931973a729c5868c155cb751f