SkPDF: Implement Paeth predictor for images.

SkPDF:  Implement Paeth predictor for images.
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1686313005

[hal.canary, 2016-02-11 22:07:34]

Description was changed from

==========
SkPDF:  Implement Paeth predictor for images.
==========

to

==========
SkPDF:  Implement Paeth predictor for images.
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[hal.canary, 2016-02-11 22:21:05]

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

[commit-bot: I haz the power, 2016-02-11 22:21:13]

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

[commit-bot: I haz the power, 2016-02-11 22:48:34]

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

[commit-bot: I haz the power, 2016-02-11 22:48:35]

Dry run: This issue passed the CQ dry run.

[hal.canary, 2016-02-16 20:55:00]

halcanary@google.com changed reviewers:
+ msarett@google.com, mtklein@google.com

[hal.canary, 2016-02-16 20:55:01]

PTAL.

[hal.canary, 2016-02-16 21:07:53]

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

[commit-bot: I haz the power, 2016-02-16 21:08:03]

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

[commit-bot: I haz the power, 2016-02-16 21:33:13]

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

[commit-bot: I haz the power, 2016-02-16 21:33:13]

Dry run: This issue passed the CQ dry run.

[mtklein, 2016-02-16 21:41:55]

let's put all our PNG filter stuff together.  the methods we wrote for libpng
are in src/codec/SkPngFilters.cpp

[hal.canary, 2016-02-17 16:08:39]

On 2016/02/16 at 21:41:55, mtklein wrote:
> let's put all our PNG filter stuff together.  the methods we wrote for libpng
are in src/codec/SkPngFilters.cpp

I'm not sure about that.  SkCodec needs to decode, while SkPDF needs to encode. 
Maybe code reuse is too hard to worry about.

[reed1, 2016-02-17 17:02:38]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2016-02-17 17:02:39]

In general we want to be able to encode as well as decode, not just in pdf

[msarett, 2016-02-17 17:07:24]

On 2016/02/16 21:41:55, mtklein wrote:
> let's put all our PNG filter stuff together.  the methods we wrote for libpng
> are in src/codec/SkPngFilters.cpp

Might be worth mentioning that libpng is reviewing a patch to upstream these
filters.  If/when that happens, src/codec/SkPngFilters.cpp will be deleted.
Let's not add another user of that code.

[hal.canary, 2016-02-19 20:42:19]

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

[commit-bot: I haz the power, 2016-02-19 20:42:24]

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

[commit-bot: I haz the power, 2016-02-19 21:16:53]

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

[commit-bot: I haz the power, 2016-02-19 21:16:54]

Dry run: This issue passed the CQ dry run.

[hal.canary, 2016-02-19 21:28:38]

On 2016/02/17 at 17:07:24, msarett wrote:
> On 2016/02/16 21:41:55, mtklein wrote:
> > let's put all our PNG filter stuff together.  the methods we wrote for
libpng
> > are in src/codec/SkPngFilters.cpp
> 
> Might be worth mentioning that libpng is reviewing a patch to upstream these
> filters.  If/when that happens, src/codec/SkPngFilters.cpp will be deleted.
> Let's not add another user of that code.

Okay.  I'm not using anything in src/codec.

I refactored the function so that it can either encode or decode, and it uses
50% less RAM-per-scanline.

[mtklein, 2016-02-19 21:53:08]

https://codereview.chromium.org/1686313005/diff/80001/src/pdf/SkPngPredictors...
File src/pdf/SkPngPredictors.cpp (right):

https://codereview.chromium.org/1686313005/diff/80001/src/pdf/SkPngPredictors...
src/pdf/SkPngPredictors.cpp:12: enum EncodeOrDecode {
Please remove the dead kDecode path and all related cruft, and please do not
send me code reviews with dead code.

https://codereview.chromium.org/1686313005/diff/80001/src/pdf/SkPngPredictors...
src/pdf/SkPngPredictors.cpp:21: template<int COMPONENT_COUNT, EncodeOrDecode
ENCODE_OR_DECODE>
ALL_UPPPER_CASE_IS_FOR_MACROS_DUDE

This is generally called bpp in libpng, for good reason... it's not the
component count.  You can have, e.g. bpp = 8 with 4 components.

https://codereview.chromium.org/1686313005/diff/80001/src/pdf/SkPngPredictors...
src/pdf/SkPngPredictors.cpp:26: uint8_t a[COMPONENT_COUNT]{},
c[COMPONENT_COUNT]{};
I think this would be easier to follow byte-by-byte instead of pixel-by-pixel. 
I'm finding it hard to track the state of a and c.

https://codereview.chromium.org/1686313005/diff/80001/src/pdf/SkPngPredictors...
src/pdf/SkPngPredictors.cpp:34: int smallest = SkTMin(pc, SkTMin(pa, pb));
For serial code, this is faster and simpler like this:

uint8_t pred = a;
if (pb < pa) { pred = b; pa = pb; }
if (pc < pa) { pred = c;          }

https://codereview.chromium.org/1686313005/diff/80001/src/pdf/SkPngPredictors...
src/pdf/SkPngPredictors.cpp:46: } else {
Let's split this else case off as 
  static void sub(...)
and call it from SkPaethEncode{1,3} instead of having to have paeth() think
about it.

https://codereview.chromium.org/1686313005/diff/80001/src/pdf/SkPngPredictors...
src/pdf/SkPngPredictors.cpp:51: *out++ = x - a[cmp];
This is why you should not land dead code.

https://codereview.chromium.org/1686313005/diff/80001/src/pdf/SkPngPredictors.h
File src/pdf/SkPngPredictors.h (right):

https://codereview.chromium.org/1686313005/diff/80001/src/pdf/SkPngPredictors...
src/pdf/SkPngPredictors.h:10: #include "stdint.h"
<stdint.h>, or "SkTypes.h"

https://codereview.chromium.org/1686313005/diff/80001/src/pdf/SkPngPredictors...
src/pdf/SkPngPredictors.h:12: void SkPaethEncode3(int width, const uint8_t*
prev, const uint8_t* scanline, uint8_t* out);
Note whether width is in bytes or pixels?

[hal.canary, 2016-02-25 20:01:51]

For testing purposes, this patch can be switched on or off easily:

Here are results (using the PDFImage benchmark):

    test                           microsec   summed .skp
                                   benchmark  PDF sizes.
    CONTROL                        2209.96    29179 Kb
    SK_PDF_USE_PAETH_PNG_PREDICTOR 2959.47    27136 Kb
    SK_PDF_USE_BEST_PNG_PREDICTOR  3344.24    26909 Kb