PDF: Add (low-memory) SkPDFBitmap class

PDF: Add (low-memory) SkPDFBitmap class

Also: Add SkDeflateWStream and associated unit tests.

SkPDFBitmap is a replacement for SkPDFImage.  As of now, it only
supports 8888 bitmaps (the most common case).

SkPDFBitmap takes very little extra memory (aside from refing the
bitmap's pixels), and its emitObject() does not cache any data.

The SkPDFBitmap::Create function will check the canon for duplicates.
This can reduce the size of the output PDF.

Motivation:  this gives another ~40% decrease in PDF memory overhead

TODO: Support other ColorTypes and scrap SkPDFImage.

BUG=skia:3030
Committed: https://skia.googlesource.com/skia/+/1b5c604d9d344537941b11b136348edfc39f236c

[hal.canary, 2015-02-11 23:27:55]

Patchset #1 (id:1) has been deleted

[hal.canary, 2015-02-11 23:49:38]

Patchset #1 (id:20001) has been deleted

[hal.canary, 2015-02-11 23:50:07]

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

[hal.canary, 2015-02-11 23:50:08]

ptal

[mtklein, 2015-02-11 23:51:01]

> Motivation:  this gives another ~40% decrease in PDF memory overhead
Holy cow.  Way to go.

[hal.canary, 2015-02-11 23:59:30]

On 2015/02/11 23:51:01, mtklein wrote:
> > Motivation:  this gives another ~40% decrease in PDF memory overhead
> Holy cow.  Way to go.

I doubt I can do it again.

[mtklein, 2015-02-12 00:21:47]

I have only nits.  Totally on board with the general plan.  I don't even want to
know what SkPDFImage does if it doesn't do exactly this...

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkDeflateWStream...
File src/pdf/SkDeflateWStream.cpp (right):

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkDeflateWStream...
src/pdf/SkDeflateWStream.cpp:17: 
Don't we need to 
  struct SkDeflateWStream::Impl{};

You might just guard the whole header by SK_HAS_ZLIB if it's not going to work
without it.

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkDeflateWStream...
src/pdf/SkDeflateWStream.cpp:27: #define SKDEFLATEWSTREAM_OUTPUT_BUFFER_SIZE
4024  // 4096 + 128, usually big
Um, 4024 != 4096 + 128.

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkDeflateWStream...
src/pdf/SkDeflateWStream.cpp:92: // if the buffer isn't filled, don't call intp
zlib yet.
into

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkDeflateWStream.h
File src/pdf/SkDeflateWStream.h (right):

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkDeflateWStream...
src/pdf/SkDeflateWStream.h:12: 
Might be nice to explain what it does here.  At least, // See
http://en.wikipedia.org/wiki/DEFLATE

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkDeflateWStream...
src/pdf/SkDeflateWStream.h:23: Impl* fImpl;
Ought to be able to SkAutoTDelete<Impl> fImpl if your destructor is defined in
the .cpp ?

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFBitmap.cpp
File src/pdf/SkPDFBitmap.cpp (right):

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFBitmap.cpp#...
src/pdf/SkPDFBitmap.cpp:39: static void pmcolor_scanline_to_rgb24(const
SkPMColor* src,
This is just a suggestion.  I've grown to like (dst, src, count) argument order
for these types of functions, to stay consistent with memcpy, memset, etc.

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFBitmap.cpp#...
src/pdf/SkPDFBitmap.cpp:58: if (!fIsAlpha) {
It'd be nice to explain fIsAlpha and why we track it.  Reading linearly up to
here I just slammed into confusion wondering what fIsAlpha is and why it
matters.

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFBitmap.cpp#...
src/pdf/SkPDFBitmap.cpp:89: // deflateWStream is deleted.
Is this salient here but not in the rgb case?

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFBitmap.cpp#...
src/pdf/SkPDFBitmap.cpp:105: true,
Is this /* using DEFLATE */ and false /* not compressed */ ?

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFBitmap.cpp#...
src/pdf/SkPDFBitmap.cpp:112: size_t scanlineLength = fIsAlpha ? fBitmap.width()
Seems like an awful lot of code duplicated between the flate and non flate
paths.  Think you can factor it out?

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFBitmap.cpp#...
src/pdf/SkPDFBitmap.cpp:158: return NULL;
Is this really an error?

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFBitmap.cpp#...
src/pdf/SkPDFBitmap.cpp:168: smask = SkNEW_ARGS(SkPDFBitmap, (bm, true, NULL));
Are the smasks always alpha-only and the main SkPDFBitmaps always not?  If so,
you might want to cleave them apart into two different classes.

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFBitmap.h
File src/pdf/SkPDFBitmap.h (right):

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFBitmap.h#ne...
src/pdf/SkPDFBitmap.h:14: * This is a replacement for SkPDFImage.  As of now, it
only supports
Let's write this comment first as if SkPDFImage didn't exist, then follow up
with "This is a more memory-efficient replacement for SkPDFImage when we're
working with 8888 bitmaps, the most common format."  Reading this comment as it
stands doesn't help me when I don't know what SkPDFImage is.

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFBitmap.h#ne...
src/pdf/SkPDFBitmap.h:15: * 8888 bitmaps (the most common case).  This onbect
takes very little
object

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFBitmap.h#ne...
src/pdf/SkPDFBitmap.h:27: const SkIRect& subset);
Does passing a subset here have any different effect from subsetting the bitmap
before you pass it in?  If so, explain the difference?  If not, remove the
feature?

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFBitmap.h#ne...
src/pdf/SkPDFBitmap.h:35: fBitmap.dimensions() == other.dimensions() &&
!fIsAlpha;
What's the deal with && !fIsAlpha?  That deserves a comment about why it's
weird.

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFImage.cpp
File src/pdf/SkPDFImage.cpp (right):

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFImage.cpp#n...
src/pdf/SkPDFImage.cpp:728: if (pdfBitmap) {
Even

if (SkPDFObject* pdfBitmap = SkPDFBitmap::Create(bitmap, subset)) {
    return pdfBitmap;
}

https://codereview.chromium.org/918813002/diff/40001/tests/DeflateWStream.cpp
File tests/DeflateWStream.cpp (right):

https://codereview.chromium.org/918813002/diff/40001/tests/DeflateWStream.cpp...
tests/DeflateWStream.cpp:14: if (!SkFlate::HaveFlate()) {
Why do we pretend SkFlate::HaveFlate() is a runtime check?  It's always the same
as SK_HAS_ZLIB.  This might all be clearer if they were all preprocessor checks.
 Then you wouldn't have to define a phony SkDeflateWStream just to get things to
compile.

[hal.canary, 2015-02-12 21:28:04]

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkDeflateWStream...
File src/pdf/SkDeflateWStream.cpp (right):

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkDeflateWStream...
src/pdf/SkDeflateWStream.cpp:17: 
On 2015/02/12 00:21:45, mtklein wrote:
> Don't we need to 
>   struct SkDeflateWStream::Impl{};
> 
> You might just guard the whole header by SK_HAS_ZLIB if it's not going to work
> without it.

Done.

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkDeflateWStream...
src/pdf/SkDeflateWStream.cpp:27: #define SKDEFLATEWSTREAM_OUTPUT_BUFFER_SIZE
4024  // 4096 + 128, usually big
On 2015/02/12 00:21:45, mtklein wrote:
> Um, 4024 != 4096 + 128.

Done.

I was testing out big and small values for this constant (to make sure the code
worked regardless) and made a type when returning it to 4224.)

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkDeflateWStream...
src/pdf/SkDeflateWStream.cpp:92: // if the buffer isn't filled, don't call intp
zlib yet.
On 2015/02/12 00:21:45, mtklein wrote:
> into

Done.

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkDeflateWStream.h
File src/pdf/SkDeflateWStream.h (right):

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkDeflateWStream...
src/pdf/SkDeflateWStream.h:12: 
On 2015/02/12 00:21:46, mtklein wrote:
> Might be nice to explain what it does here.  At least, // See
> http://en.wikipedia.org/wiki/DEFLATE

Done.

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkDeflateWStream...
src/pdf/SkDeflateWStream.h:23: Impl* fImpl;
On 2015/02/12 00:21:46, mtklein wrote:
> Ought to be able to SkAutoTDelete<Impl> fImpl if your destructor is defined in
> the .cpp ?

Done.

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFBitmap.cpp
File src/pdf/SkPDFBitmap.cpp (right):

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFBitmap.cpp#...
src/pdf/SkPDFBitmap.cpp:39: static void pmcolor_scanline_to_rgb24(const
SkPMColor* src,
On 2015/02/12 00:21:46, mtklein wrote:
> This is just a suggestion.  I've grown to like (dst, src, count) argument
order
> for these types of functions, to stay consistent with memcpy, memset, etc.

Refactored out.

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFBitmap.cpp#...
src/pdf/SkPDFBitmap.cpp:58: if (!fIsAlpha) {
On 2015/02/12 00:21:46, mtklein wrote:
> It'd be nice to explain fIsAlpha and why we track it.  Reading linearly up to
> here I just slammed into confusion wondering what fIsAlpha is and why it
> matters.

Done.

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFBitmap.cpp#...
src/pdf/SkPDFBitmap.cpp:89: // deflateWStream is deleted.
On 2015/02/12 00:21:46, mtklein wrote:
> Is this salient here but not in the rgb case?

refactored out

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFBitmap.cpp#...
src/pdf/SkPDFBitmap.cpp:158: return NULL;
On 2015/02/12 00:21:46, mtklein wrote:
> Is this really an error?

Just a shortcut.

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFBitmap.cpp#...
src/pdf/SkPDFBitmap.cpp:168: smask = SkNEW_ARGS(SkPDFBitmap, (bm, true, NULL));
On 2015/02/12 00:21:46, mtklein wrote:
> Are the smasks always alpha-only and the main SkPDFBitmaps always not?  If so,
> you might want to cleave them apart into two different classes.

Okay, done.  Seems like more code this way.

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFBitmap.h
File src/pdf/SkPDFBitmap.h (right):

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFBitmap.h#ne...
src/pdf/SkPDFBitmap.h:14: * This is a replacement for SkPDFImage.  As of now, it
only supports
On 2015/02/12 00:21:46, mtklein wrote:
> Let's write this comment first as if SkPDFImage didn't exist, then follow up
> with "This is a more memory-efficient replacement for SkPDFImage when we're
> working with 8888 bitmaps, the most common format."  Reading this comment as
it
> stands doesn't help me when I don't know what SkPDFImage is.

Done.

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFBitmap.h#ne...
src/pdf/SkPDFBitmap.h:27: const SkIRect& subset);
On 2015/02/12 00:21:46, mtklein wrote:
> Does passing a subset here have any different effect from subsetting the
bitmap
> before you pass it in?  If so, explain the difference?  If not, remove the
> feature?

It's  only here to match the calling site.

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFBitmap.h#ne...
src/pdf/SkPDFBitmap.h:35: fBitmap.dimensions() == other.dimensions() &&
!fIsAlpha;
On 2015/02/12 00:21:46, mtklein wrote:
> What's the deal with && !fIsAlpha?  That deserves a comment about why it's
> weird.

When IsAlpha is true, the SkPDFBitmap represents that bitmap's alpha layer. 
Otherwise, the SkPDFBitmap represents the entire bitmap.

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFImage.cpp
File src/pdf/SkPDFImage.cpp (right):

https://codereview.chromium.org/918813002/diff/40001/src/pdf/SkPDFImage.cpp#n...
src/pdf/SkPDFImage.cpp:728: if (pdfBitmap) {
On 2015/02/12 00:21:46, mtklein wrote:
> Even
> 
> if (SkPDFObject* pdfBitmap = SkPDFBitmap::Create(bitmap, subset)) {
>     return pdfBitmap;
> }

Done.

https://codereview.chromium.org/918813002/diff/40001/tests/DeflateWStream.cpp
File tests/DeflateWStream.cpp (right):

https://codereview.chromium.org/918813002/diff/40001/tests/DeflateWStream.cpp...
tests/DeflateWStream.cpp:14: if (!SkFlate::HaveFlate()) {
On 2015/02/12 00:21:46, mtklein wrote:
> Why do we pretend SkFlate::HaveFlate() is a runtime check?  It's always the
same
> as SK_HAS_ZLIB.  This might all be clearer if they were all preprocessor
checks.
>  Then you wouldn't have to define a phony SkDeflateWStream just to get things
to
> compile.

I agree.  I'll change this at some time.  I think the author of SkFlate wanted
to someday have a non-zlib-backed deflate.

[mtklein, 2015-02-12 21:46:01]

lgtm

Please consider testing whether it's really worth the added code complexity to
do the size checks.  Seems simpler to just always deflate, provided that doesn't
hurt size / speed on our test set.

https://codereview.chromium.org/918813002/diff/60001/src/pdf/SkPDFBitmap.h
File src/pdf/SkPDFBitmap.h (right):

https://codereview.chromium.org/918813002/diff/60001/src/pdf/SkPDFBitmap.h#ne...
src/pdf/SkPDFBitmap.h:42: void emitFast(SkWStream*, SkPDFCatalog*) const;
emitUncompressed?

[hal.canary, 2015-02-12 22:09:39]

New patchsets have been uploaded after l-g-t-m from mtklein@google.com

[hal.canary, 2015-02-12 22:30:27]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-02-12 22:31:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/918813002/80001

[commit-bot: I haz the power, 2015-02-12 22:33:16]

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

[commit-bot: I haz the power, 2015-02-12 22:33:18]

Try jobs failed on following builders:
  Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Android-Trybot on client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu13.10-GC...)

[hal.canary, 2015-02-12 23:22:17]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-02-12 23:23:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/918813002/100001

[commit-bot: I haz the power, 2015-02-12 23:25:15]

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

[commit-bot: I haz the power, 2015-02-12 23:25:16]

Try jobs failed on following builders:
  Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Android-Trybot on client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu13.10-GC...)

[hal.canary, 2015-02-13 18:23:03]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-02-13 18:23:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/918813002/120001

[commit-bot: I haz the power, 2015-02-13 18:25:05]

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

[commit-bot: I haz the power, 2015-02-13 18:25:06]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...)

[hal.canary, 2015-02-13 18:36:11]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-02-13 18:36:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/918813002/140001

[commit-bot: I haz the power, 2015-02-13 18:56:55]

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

[commit-bot: I haz the power, 2015-02-13 18:56:56]

Try jobs failed on following builders:
  Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)

[hal.canary, 2015-02-13 19:23:16]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-02-13 19:23:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/918813002/160001

[commit-bot: I haz the power, 2015-02-13 19:48:36]

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

[commit-bot: I haz the power, 2015-02-13 19:48:37]

Try jobs failed on following builders:
  Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)

[hal.canary, 2015-02-13 21:59:06]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-02-13 21:59:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/918813002/180001

[commit-bot: I haz the power, 2015-02-13 22:18:19]

This issue passed the CQ. To commit it, remove "COMMIT=false" from
the description and try again.

[hal.canary, 2015-02-17 23:15:13]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-02-17 23:16:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/918813002/200001

[hal.canary, 2015-02-17 23:28:18]

The CQ bit was unchecked by halcanary@google.com

[hal.canary, 2015-02-18 04:24:21]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-02-18 04:25:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/918813002/240001

[hal.canary, 2015-02-18 04:25:44]

The CQ bit was unchecked by halcanary@google.com

[hal.canary, 2015-02-18 11:27:27]

Patchset #10 (id:220001) has been deleted

[hal.canary, 2015-02-18 11:37:55]

The CQ bit was checked by halcanary@google.com

[hal.canary, 2015-02-18 11:38:06]

Patchset #10 (id:240001) has been deleted

[commit-bot: I haz the power, 2015-02-18 11:38:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/918813002/260001

[hal.canary, 2015-02-18 11:40:20]

The CQ bit was unchecked by halcanary@google.com

[hal.canary, 2015-02-18 11:57:22]

Patchset #11 (id:280001) has been deleted

[hal.canary, 2015-02-18 11:58:07]

Patchset #10 (id:260001) has been deleted

[hal.canary, 2015-02-18 12:18:27]

Patchset #10 (id:300001) has been deleted

[hal.canary, 2015-02-18 12:18:57]

Patchset #10 (id:320001) has been deleted

[hal.canary, 2015-02-18 12:21:56]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-02-18 12:22:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/918813002/340001

[hal.canary, 2015-02-18 15:37:34]

The CQ bit was unchecked by halcanary@google.com

[hal.canary, 2015-02-18 19:03:53]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-02-18 19:04:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/918813002/360001

[commit-bot: I haz the power, 2015-02-18 19:30:02]

Committed patchset #11 (id:360001) as
https://skia.googlesource.com/skia/+/1b5c604d9d344537941b11b136348edfc39f236c