Encode images with DCTDecode (JPEG) in PDFs if it makes sense. Fallback to FlateDecode (zip) if it …

Encode images with DCTDecode (JPEG) in PDFs if it makes sense. Fallback to FlateDecode (zip) if it makes sense. Otherewise include uncompressed stream.
This change will reduce the size of PDFs to 50% (in the case of the existing SKPs, we reduce the total size of PDFs from 105MB to 50MB) 
Committed: https://code.google.com/p/skia/source/detail?r=8835

[reed1, 2013-01-09 15:08:49]

I love the idea of the feature... we need to be thoughtful as to how we hook-up
the compressor code (i.e. SkImageEncoder). Today that package is not strictly
part of skia-core.

Adding Leon, who has done some thinking about this for pictures (and lazy
pixelrefs). Do we want a global factory, or have the caller passing in a
functionptr? Other options?

[Steve VanDeBogart, 2013-01-09 16:46:49]

This is a pretty invasive change.  I think a lot of the modifications can be
avoided by making a new class SkPDFImageStream that inherits from SkPDFStream
and handles DCT as well as flate encoding.

Also this doesn't honor the current flag in SkPDFDocument, kNoCompression_Flags.
 That flag was added for performance reasons.  In Chrome, we regenerate the PDF
multiple times if the user changes the orientation, the page range selected,
etc.  For all of those PDF except the last one, we prefer speed over size, since
it's just going to the screen.

So if the no compression flag is set, we shouldn't do compression that isn't
already done, i.e. it's fine to look in the pixel ref and use the already
compressed data, but we shouldn't compress.

Furthermore, we should add more flags to let the application control jpeg
compression since it's lossy.  Sure, any image who's source is already
compressed should stay compressed, but if the source isn't already compressed,
we should only use jpeg compression if the user requests it.  Maybe the
following flags: kJpegCompressAllImages and kJpegCompressLargeImages.

[Leon, 2013-01-09 18:21:57]

> I love the idea of the feature... we need to be thoughtful as to how we
hook-up
> the compressor code (i.e. SkImageEncoder). Today that package is not strictly
> part of skia-core.

> Adding Leon, who has done some thinking about this for pictures (and lazy
> pixelrefs). Do we want a global factory, or have the caller passing in a
> functionptr? Other options?

I'm fine with either of those options.

https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFImage.cpp
File src/pdf/SkPDFImage.cpp (right):

https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFImage.cpp#newco...
src/pdf/SkPDFImage.cpp:254: sc_None,
The pattern Skia typically uses for enums follows the format:

None_StreamCompression,
...

https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFImage.cpp#newco...
src/pdf/SkPDFImage.cpp:279: SkASSERT(bitmap.extractSubset(&subset, srcRect));
This line will not get executed in release mode. A way to do this follows:

SkDEBUGCODE(bool success =) bitmap.extractSubset(&subset, srcRect);
SkASSERT(success);

https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFImage.cpp#newco...
src/pdf/SkPDFImage.cpp:320: if (zipCompressedWStream.getOffset() <
imageData->getLength()) {
It seems a shame that we just throw away the zipped version. Seems like another
good reason to wait until we actually request that it is compressed.

https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFImage.cpp#newco...
src/pdf/SkPDFImage.cpp:330: SkMemoryStream* jpegCompressedStream = new
SkMemoryStream();
SkNEW(SkMemoryStream);

https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFImage.cpp#newco...
src/pdf/SkPDFImage.cpp:331:
jpegCompressedStream->setData(jpegCompressedWStream.copyToData());
I think humper@ just added a constructor for SkMemoryStream that takes an
SkData, so you can combine these two lines.

https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFImage.cpp#newco...
src/pdf/SkPDFImage.cpp:332: delete imageData;
In the interest of consistency, should we instead say

imageData->unref()

?

https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFImage.cpp#newco...
src/pdf/SkPDFImage.cpp:334: jpegCompressedStream = NULL;
Seems like a lot of extra lines. What if we said

imageData->unref();
imageData = SkNEW_ARGS(SkMemoryStream, (jpegCompressedWStream.copyToData()));

https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFStream.h
File src/pdf/SkPDFStream.h (right):

https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFStream.h#newcode32
src/pdf/SkPDFStream.h:32: explicit SkPDFStream(SkData* data, bool
usingJpegCompression);
Could you add documentation?

Also, if there was a default value of false for usingJpegCompression (which
seems to be the normal case), you wouldn't have to update all the call sites.

https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFStream.h#newcode34
src/pdf/SkPDFStream.h:34: explicit SkPDFStream(SkStream* stream, bool
usingJpegCompression);
Do we need to update a deprecated constructor?

https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFStream.h#newcode60
src/pdf/SkPDFStream.h:60: kJpegCompressed_State, //!< The stream's already been
JPEG compressed.
Would it be possible to reuse kCompressed_State? I guess not because we want to
remove the Length for JpegCompressed, but not if it was compressed the normal
way? Or would it be possible to remove the Length ahead of time?

https://codereview.appspot.com/7068055/diff/6012/tests/PDFPrimitivesTest.cpp
File tests/PDFPrimitivesTest.cpp (right):

https://codereview.appspot.com/7068055/diff/6012/tests/PDFPrimitivesTest.cpp#...
tests/PDFPrimitivesTest.cpp:278: "/Width 1\n"
These should line up with the first parameter

[edisonn, 2013-01-22 21:24:55]

I am refactoring this, but as I am not aware of all use cases, adding flags
without understanding the use case is bad.

Please provide a minispec: {flag, what it should do. and I will add them}, but I
would rather not add flags now, but when someone in chrome really needs it and
has a use case.





On 2013/01/09 16:46:49, Steve VanDeBogart wrote:
> This is a pretty invasive change.  I think a lot of the modifications can be
> avoided by making a new class SkPDFImageStream that inherits from SkPDFStream
> and handles DCT as well as flate encoding.
> 
> Also this doesn't honor the current flag in SkPDFDocument,
kNoCompression_Flags.
>  That flag was added for performance reasons.  In Chrome, we regenerate the
PDF
> multiple times if the user changes the orientation, the page range selected,
> etc.  For all of those PDF except the last one, we prefer speed over size,
since
> it's just going to the screen.
> 
> So if the no compression flag is set, we shouldn't do compression that isn't
> already done, i.e. it's fine to look in the pixel ref and use the already
> compressed data, but we shouldn't compress.
> 
> Furthermore, we should add more flags to let the application control jpeg
> compression since it's lossy.  Sure, any image who's source is already
> compressed should stay compressed, but if the source isn't already compressed,
> we should only use jpeg compression if the user requests it.  Maybe the
> following flags: kJpegCompressAllImages and kJpegCompressLargeImages.

[edisonn, 2013-01-22 21:32:16]

On 2013/01/09 16:46:49, Steve VanDeBogart wrote:
> This is a pretty invasive change.  I think a lot of the modifications can be
> avoided by making a new class SkPDFImageStream that inherits from SkPDFStream
> and handles DCT as well as flate encoding.

Then SkPdfImage will handle no compression + Flate and SkPdfImageStream will
handle DCT + Flate + no compression ... we will have a lot of duplicated code.
Are we sure we want to go through the  trouble of maintaining more code? I do
not understand the use case of SkPdfImage vs SkPdfImageStream. It seems to me
SkPdfImage might just end up dead code.

> 
> Also this doesn't honor the current flag in SkPDFDocument,
kNoCompression_Flags.
>  That flag was added for performance reasons.  In Chrome, we regenerate the
PDF
> multiple times if the user changes the orientation, the page range selected,
> etc.  For all of those PDF except the last one, we prefer speed over size,
since
> it's just going to the screen.
> 
> So if the no compression flag is set, we shouldn't do compression that isn't
> already done, i.e. it's fine to look in the pixel ref and use the already
> compressed data, but we shouldn't compress.
> 
> Furthermore, we should add more flags to let the application control jpeg
> compression since it's lossy.  Sure, any image who's source is already
> compressed should stay compressed, but if the source isn't already compressed,
> we should only use jpeg compression if the user requests it.  Maybe the
> following flags: kJpegCompressAllImages and kJpegCompressLargeImages.

[Steve VanDeBogart, 2013-01-22 23:09:32]

On 2013/01/22 21:24:55, edisonn wrote:
> I am refactoring this, but as I am not aware of all use cases, adding flags
> without understanding the use case is bad.
> 
> Please provide a minispec: {flag, what it should do. and I will add them}, but
I
> would rather not add flags now, but when someone in chrome really needs it and
> has a use case.

kNoCompression_Flags is meant to minimize CPU time.  If this flag is specified
and the image is backed by DCT data that doesn't require and re-encoding use
that.  Otherwise use the current, deflate code path.

If kNoCompression_Flags is not specified and the image is backed by DCT data,
then we should generate a DCT encoded image, even if that requires subsetting
the image and re-encoding it. Though any re-encoding should be done at the same
quality settings as the original image.  If it is not DCT data, use the current
deflate code path.  This is kind of the "do no harm" option.

kJpegCompressAllImages_Flags would be used to make things as small as possible. 
Use DCT compression for all images.  I'm not sure where the quality setting
should come from.  Probably a compile constant is good enough for now.

kJpegCompressLargeImages_Flags would be a compromise between
kJpegCompressAllImages_Flags and not specifying anything.  Based on a compile
time constant, if the image is over a certain size (X pixels), DCT compress the
image, otherwise compress it only if it was already DCT compressed.

Chrome will use the no compression flag, and likely the compress large image
flag, but possibly just no flags.

Let me know if you have any questions/thoughts.

On 2013/01/22 21:32:16, edisonn wrote:
> On 2013/01/09 16:46:49, Steve VanDeBogart wrote:
> > This is a pretty invasive change.  I think a lot of the modifications can be
> > avoided by making a new class SkPDFImageStream that inherits from
SkPDFStream
> > and handles DCT as well as flate encoding.
> 
> Then SkPdfImage will handle no compression + Flate and SkPdfImageStream will
> handle DCT + Flate + no compression ... we will have a lot of duplicated code.
> Are we sure we want to go through the  trouble of maintaining more code? I do
> not understand the use case of SkPdfImage vs SkPdfImageStream. It seems to me
> SkPdfImage might just end up dead code.

I'm not sure I communicated what I meant effectively. Right now SkPDFImage
inherits from SkPDFStream.  I think we should make a new class, SkPDFImageStream
that is like SKPDFStream, buit also can handle DCT compression.  Then SkPDFImage
will just inherit from SkPDFImageStream.  The SkPDFStream constructor takes
SkData.  I think the SkPDFImageStream would take an SkBitmap and maybe some
other stuff (srcRect?).

[edisonn, 2013-03-27 14:14:15]

Steve, I have not implemented the flags suggested. (actually I did implememted
them, but then reverted) because it became redundant with adding a function
pointer for bitmap->jpeg_stream compression. 

if the caller does not want to compress using jpeg, then it can simply implement
this logic in the function pointer passed. It can also check there the width and
height of the bitmap for any heuristic 

There is no benefit from having extra flags, it would lead to confusion to have
2 mechanisms to enable/disable jpeg compression.

https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFImage.cpp
File src/pdf/SkPDFImage.cpp (right):

https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFImage.cpp#newco...
src/pdf/SkPDFImage.cpp:254: sc_None,
On 2013/01/09 18:21:57, Leon wrote:
> The pattern Skia typically uses for enums follows the format:
> 
> None_StreamCompression,
> ...

Done.

https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFImage.cpp#newco...
src/pdf/SkPDFImage.cpp:279: SkASSERT(bitmap.extractSubset(&subset, srcRect));
used SkAssertResult which does the same but it is more readable

On 2013/01/09 18:21:57, Leon wrote:
> This line will not get executed in release mode. A way to do this follows:
> 
> SkDEBUGCODE(bool success =) bitmap.extractSubset(&subset, srcRect);
> SkASSERT(success);

https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFImage.cpp#newco...
src/pdf/SkPDFImage.cpp:320: if (zipCompressedWStream.getOffset() <
imageData->getLength()) {
code is now in SkPDFImageStream
On 2013/01/09 18:21:57, Leon wrote:
> It seems a shame that we just throw away the zipped version. Seems like
another
> good reason to wait until we actually request that it is compressed.

https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFImage.cpp#newco...
src/pdf/SkPDFImage.cpp:330: SkMemoryStream* jpegCompressedStream = new
SkMemoryStream();
On 2013/01/09 18:21:57, Leon wrote:
> SkNEW(SkMemoryStream);

Done.

https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFImage.cpp#newco...
src/pdf/SkPDFImage.cpp:331:
jpegCompressedStream->setData(jpegCompressedWStream.copyToData());
On 2013/01/09 18:21:57, Leon wrote:
> I think humper@ just added a constructor for SkMemoryStream that takes an
> SkData, so you can combine these two lines.

Done.

https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFImage.cpp#newco...
src/pdf/SkPDFImage.cpp:332: delete imageData;
On 2013/01/09 18:21:57, Leon wrote:
> In the interest of consistency, should we instead say
> 
> imageData->unref()
> 
> ?

Done.

https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFImage.cpp#newco...
src/pdf/SkPDFImage.cpp:334: jpegCompressedStream = NULL;
On 2013/01/09 18:21:57, Leon wrote:
> Seems like a lot of extra lines. What if we said
> 
> imageData->unref();
> imageData = SkNEW_ARGS(SkMemoryStream, (jpegCompressedWStream.copyToData()));

Done.

https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFStream.h
File src/pdf/SkPDFStream.h (right):

https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFStream.h#newcode32
src/pdf/SkPDFStream.h:32: explicit SkPDFStream(SkData* data, bool
usingJpegCompression);
reverted
On 2013/01/09 18:21:57, Leon wrote:
> Could you add documentation?
> 
> Also, if there was a default value of false for usingJpegCompression (which
> seems to be the normal case), you wouldn't have to update all the call sites.

https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFStream.h#newcode34
src/pdf/SkPDFStream.h:34: explicit SkPDFStream(SkStream* stream, bool
usingJpegCompression);
reverted
On 2013/01/09 18:21:57, Leon wrote:
> Do we need to update a deprecated constructor?

https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFStream.h#newcode60
src/pdf/SkPDFStream.h:60: kJpegCompressed_State, //!< The stream's already been
JPEG compressed.
done
On 2013/01/09 18:21:57, Leon wrote:
> Would it be possible to reuse kCompressed_State? I guess not because we want
to
> remove the Length for JpegCompressed, but not if it was compressed the normal
> way? Or would it be possible to remove the Length ahead of time?

https://codereview.appspot.com/7068055/diff/6012/tests/PDFPrimitivesTest.cpp
File tests/PDFPrimitivesTest.cpp (right):

https://codereview.appspot.com/7068055/diff/6012/tests/PDFPrimitivesTest.cpp#...
tests/PDFPrimitivesTest.cpp:278: "/Width 1\n"
On 2013/01/09 18:21:57, Leon wrote:
> These should line up with the first parameter

Done.

[Steve VanDeBogart, 2013-03-27 20:34:54]

Looking a lot closer.

https://codereview.appspot.com/7068055/diff/43001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/7068055/diff/43001/include/pdf/SkPDFDevice.h#n...
include/pdf/SkPDFDevice.h:69: EncodeToDCTStream encoder = NULL);
Interesting idea and I think I like it.  However, I think it would be better to
make it an explicit method of SkPDFDdevice: void setDCTEncoder(EncodeToDCTStream
encoder);

The comment for the new function can be a bit more verbose.  It should suggest
that the implementation may want to not compress if the image is below a certain
size, etc.

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImage.cpp
File src/pdf/SkPDFImage.cpp (right):

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImage.cpp#newc...
src/pdf/SkPDFImage.cpp:272: // Don't even try to compress using JPEG the alpha
chanel.
nit: Don't try to use DCT compression for the alpha channel.
Also, why not?

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cpp
File src/pdf/SkPDFImageStream.cpp (right):

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:15: #define kNoColorTransform (0)
no ()s

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:101: if (fSrcRect != bitmapBounds) {
extractImageData in SkPDFImage already takes care of srcRect

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:114: fEncoder(&jpegCompressedWStream,
*bitmapToUse)) {
I wonder if we should honor the choices of fEncoder more strictly.  i.e. if we
get a DCT encoded result back from the consumer through this function, we should
just use it.

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:123: StreamCompression sc = None_StreamCompression;
You can do this block much more simply:

   StreamCompression sc = None_StreamCompression;
   size_t min_size = fData->getLength();

   if (hasZipCompressedWStream && zipCompressedWStream.getOffset() < min_size) {
       sc = Zip_StreamCompression;
       min_size = zipCompressedWStream.getOffset();
   }
   if (hasJpegCompressedWStream && jpegCompressedWStream.getOffset() < min_size)
{
       sc = Jpeg_StreamCompression;
  }

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:159: insertInt("Length", fData->getLength());
Don't move this - we need it in all cases.

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:160: }else {
nit: space

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:163: } else if (fState == kNoCompression_State &&
!skip_compression(catalog)) {
We still need to check that we have compressors here - if we have none, there's
no point in setting a substitute object.

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.h
File src/pdf/SkPDFImageStream.h (right):

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.h#...
src/pdf/SkPDFImageStream.h:27: specific compression. Currently we support JPEG
and ZIP (ZIP compression is
nit: DCT (jpeg) and flate (zip).

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.h#...
src/pdf/SkPDFImageStream.h:30: class SkPDFImageStream : public SkPDFDict {
Should this inherit from SkPDFStream?  It seems like it would reduce a lot of
duplicate code.

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.h#...
src/pdf/SkPDFImageStream.h:53: Zip_StreamCompression,
So this is a technicality, but it's better to be precise when it's not too
difficult.... PDF uses flate and DCT compression.  Flate compression is used in
the zip format, but zip defines a file format, not just the compression method. 
Similarly JPEG is a file format that uses DCT compression.

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.h#...
src/pdf/SkPDFImageStream.h:61: kCompressed_State,     //!< The stream's already
been compressed.
You could replace kCompressed with kFlateCompressed and kDCTCompressed and have
a macro isCompressed() that checks for either if that would be easier.

https://codereview.appspot.com/7068055/diff/43001/tests/PDFPrimitivesTest.cpp
File tests/PDFPrimitivesTest.cpp (right):

https://codereview.appspot.com/7068055/diff/43001/tests/PDFPrimitivesTest.cpp...
tests/PDFPrimitivesTest.cpp:286: TestImage(reporter, bitmap,
Looking for the whole image dictionary is fragile to changes... It would be a
bit more work, but you should be able to query the catalog for the item number
and then search for the start of the object and confirm that it has the right
(or no) filter option before the next ">> stream" ?  Up to you though.

https://codereview.appspot.com/7068055/diff/43001/tools/render_pdfs_main.cpp
File tools/render_pdfs_main.cpp (right):

https://codereview.appspot.com/7068055/diff/43001/tools/render_pdfs_main.cpp#...
tools/render_pdfs_main.cpp:81: int jpegQuality = 100;
name the global with a g prefix?

[Steve VanDeBogart, 2013-03-27 20:36:57]

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cpp
File src/pdf/SkPDFImageStream.cpp (right):

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:101: if (fSrcRect != bitmapBounds) {
On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> extractImageData in SkPDFImage already takes care of srcRect

But not for the DCT case... Maybe move this part down to where we try to use
encoder?

[edisonn, 2013-03-28 17:09:37]

https://codereview.appspot.com/7068055/diff/43001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/7068055/diff/43001/include/pdf/SkPDFDevice.h#n...
include/pdf/SkPDFDevice.h:69: EncodeToDCTStream encoder = NULL);
I don't think I want to see this encoder mutable. I am not sure how it would
play out, especially in the usage of chrome. For example, if we added a image to
a catalog, and the first encoder decides not to encode, but we add the image to
another catalog, and we change the encoder to do encoding, what should be the
result?. If we find a need, we can update the code, but at this point it would
create confusion and callers might have expectations that would not be meet.
This could still happen with a encoder that woud have a global state that
changes behavior, but I will add to the comments, that once we get the jpeg
stream once, we will cache it.

On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> Interesting idea and I think I like it.  However, I think it would be better
to
> make it an explicit method of SkPDFDdevice: void
setDCTEncoder(EncodeToDCTStream
> encoder);
> 
> The comment for the new function can be a bit more verbose.  It should suggest
> that the implementation may want to not compress if the image is below a
certain
> size, etc.

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImage.cpp
File src/pdf/SkPDFImage.cpp (right):

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImage.cpp#newc...
src/pdf/SkPDFImage.cpp:272: // Don't even try to compress using JPEG the alpha
chanel.
It is not worth it. Alpha masks are not like pictures, they have usually a low
degree of information, and zip is best. The second problem is that JPEG is
losing information, and it will amplify the errors of rendering.
On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> nit: Don't try to use DCT compression for the alpha channel.
> Also, why not?

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cpp
File src/pdf/SkPDFImageStream.cpp (right):

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:101: if (fSrcRect != bitmapBounds) {
On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> extractImageData in SkPDFImage already takes care of srcRect

Done.

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:101: if (fSrcRect != bitmapBounds) {
On 2013/03/27 20:36:57, Steve VanDeBogart wrote:
> On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> > extractImageData in SkPDFImage already takes care of srcRect
> 
> But not for the DCT case... Maybe move this part down to where we try to use
> encoder?

Done.

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:114: fEncoder(&jpegCompressedWStream,
*bitmapToUse)) {
Unless for testing purposes, I don't se why. DCT compression should be slower
anyway, so why would it use it if it does nto save space?
On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> I wonder if we should honor the choices of fEncoder more strictly.  i.e. if we
> get a DCT encoded result back from the consumer through this function, we
should
> just use it.

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:123: StreamCompression sc = None_StreamCompression;
On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> You can do this block much more simply:
> 
>    StreamCompression sc = None_StreamCompression;
>    size_t min_size = fData->getLength();
> 
>    if (hasZipCompressedWStream && zipCompressedWStream.getOffset() < min_size)
{
>        sc = Zip_StreamCompression;
>        min_size = zipCompressedWStream.getOffset();
>    }
>    if (hasJpegCompressedWStream && jpegCompressedWStream.getOffset() <
min_size)
> {
>        sc = Jpeg_StreamCompression;
>   }

Done.

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:159: insertInt("Length", fData->getLength());
On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> Don't move this - we need it in all cases.

Done.

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:160: }else {
On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> nit: space

Done.

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:163: } else if (fState == kNoCompression_State &&
!skip_compression(catalog)) {
done. added encoder along with the HasFlate()
On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> We still need to check that we have compressors here - if we have none,
there's
> no point in setting a substitute object.

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.h
File src/pdf/SkPDFImageStream.h (right):

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.h#...
src/pdf/SkPDFImageStream.h:27: specific compression. Currently we support JPEG
and ZIP (ZIP compression is
On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> nit: DCT (jpeg) and flate (zip).

Done.

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.h#...
src/pdf/SkPDFImageStream.h:27: specific compression. Currently we support JPEG
and ZIP (ZIP compression is
On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> nit: DCT (jpeg) and flate (zip).

Done.

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.h#...
src/pdf/SkPDFImageStream.h:30: class SkPDFImageStream : public SkPDFDict {
That depend on our view if "SkPDFImageStream" is a "SkPDFStream". I don't have a
strong opinion either. if you wish I will inherit SkPDFImageStream from
SkPDFStream

On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> Should this inherit from SkPDFStream?  It seems like it would reduce a lot of
> duplicate code.

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.h#...
src/pdf/SkPDFImageStream.h:53: Zip_StreamCompression,
On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> So this is a technicality, but it's better to be precise when it's not too
> difficult.... PDF uses flate and DCT compression.  Flate compression is used
in
> the zip format, but zip defines a file format, not just the compression
method. 
> Similarly JPEG is a file format that uses DCT compression.

Done.

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.h#...
src/pdf/SkPDFImageStream.h:61: kCompressed_State,     //!< The stream's already
been compressed.
Did that, and Leon suggested not doing that:
https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFStream.h#newcode60

And I prefer Leon suggestion, as there is no use case to check the compression
methon so far. I would rather not add dead code in the hope it will be used.
On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> You could replace kCompressed with kFlateCompressed and kDCTCompressed and
have
> a macro isCompressed() that checks for either if that would be easier.

https://codereview.appspot.com/7068055/diff/43001/tests/PDFPrimitivesTest.cpp
File tests/PDFPrimitivesTest.cpp (right):

https://codereview.appspot.com/7068055/diff/43001/tests/PDFPrimitivesTest.cpp...
tests/PDFPrimitivesTest.cpp:286: TestImage(reporter, bitmap,
I prefer this way, because it also test how it is serialized to the file also.

On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> Looking for the whole image dictionary is fragile to changes... It would be a
> bit more work, but you should be able to query the catalog for the item number
> and then search for the start of the object and confirm that it has the right
> (or no) filter option before the next ">> stream" ?  Up to you though.

https://codereview.appspot.com/7068055/diff/43001/tools/render_pdfs_main.cpp
File tools/render_pdfs_main.cpp (right):

https://codereview.appspot.com/7068055/diff/43001/tools/render_pdfs_main.cpp#...
tools/render_pdfs_main.cpp:81: int jpegQuality = 100;
On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> name the global with a g prefix?

Done.

[Steve VanDeBogart, 2013-03-28 19:27:42]

https://codereview.appspot.com/7068055/diff/43001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/7068055/diff/43001/include/pdf/SkPDFDevice.h#n...
include/pdf/SkPDFDevice.h:69: EncodeToDCTStream encoder = NULL);
Interesting... In general we have this issue because a image can be in multiple
documents.  But having a separate with the comment that it's only used with the
document is generated should solve this concern.  (It's only used in populate,
which IIRC, is only called through emitDocument.) 

So I still think a setter would be cleaner.

On 2013/03/28 17:09:37, edisonn wrote:
> I don't think I want to see this encoder mutable. I am not sure how it would
> play out, especially in the usage of chrome. For example, if we added a image
to
> a catalog, and the first encoder decides not to encode, but we add the image
to
> another catalog, and we change the encoder to do encoding, what should be the
> result?. If we find a need, we can update the code, but at this point it would
> create confusion and callers might have expectations that would not be meet.
> This could still happen with a encoder that woud have a global state that
> changes behavior, but I will add to the comments, that once we get the jpeg
> stream once, we will cache it.
> 
> On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> > Interesting idea and I think I like it.  However, I think it would be better
> to
> > make it an explicit method of SkPDFDdevice: void
> setDCTEncoder(EncodeToDCTStream
> > encoder);
> > 
> > The comment for the new function can be a bit more verbose.  It should
suggest
> > that the implementation may want to not compress if the image is below a
> certain
> > size, etc.
>

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImage.cpp
File src/pdf/SkPDFImage.cpp (right):

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImage.cpp#newc...
src/pdf/SkPDFImage.cpp:272: // Don't even try to compress using JPEG the alpha
chanel.
Say that?  "Don't try to use DCT compression with alpha because alpha is small
anyway and it could lead to artifacts." ?

On 2013/03/28 17:09:37, edisonn wrote:
> It is not worth it. Alpha masks are not like pictures, they have usually a low
> degree of information, and zip is best. The second problem is that JPEG is
> losing information, and it will amplify the errors of rendering.
> On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> > nit: Don't try to use DCT compression for the alpha channel.
> > Also, why not?
>

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cpp
File src/pdf/SkPDFImageStream.cpp (right):

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:15: #define kNoColorTransform (0)
On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> no ()s

Missed this one.

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:114: fEncoder(&jpegCompressedWStream,
*bitmapToUse)) {
What I meant is if fEncoder gives us a result, maybe we should skip trying to
use flate compression.

On 2013/03/28 17:09:37, edisonn wrote:
> Unless for testing purposes, I don't se why. DCT compression should be slower
> anyway, so why would it use it if it does nto save space?
> On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> > I wonder if we should honor the choices of fEncoder more strictly.  i.e. if
we
> > get a DCT encoded result back from the consumer through this function, we
> should
> > just use it.
>

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.h
File src/pdf/SkPDFImageStream.h (right):

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.h#...
src/pdf/SkPDFImageStream.h:30: class SkPDFImageStream : public SkPDFDict {
SkPDFImageStream is a SkPDFStream.  I think inheriting will simplify the code.

On 2013/03/28 17:09:37, edisonn wrote:
> That depend on our view if "SkPDFImageStream" is a "SkPDFStream". I don't have
a
> strong opinion either. if you wish I will inherit SkPDFImageStream from
> SkPDFStream
> 
> On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> > Should this inherit from SkPDFStream?  It seems like it would reduce a lot
of
> > duplicate code.
>

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.h#...
src/pdf/SkPDFImageStream.h:61: kCompressed_State,     //!< The stream's already
been compressed.
That's not exactly what I suggested.  I suggested removing kCompressed_State and
adding the two different kinds of compression to the enum.  Then you can use a
macro that tests for either kind of compression.

I think having just one enum will make the code easier to understand.  Please
give it a try.

On 2013/03/28 17:09:37, edisonn wrote:
> Did that, and Leon suggested not doing that:
>
https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFStream.h#newcode60
> 
> And I prefer Leon suggestion, as there is no use case to check the compression
> methon so far. I would rather not add dead code in the hope it will be used.
> On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> > You could replace kCompressed with kFlateCompressed and kDCTCompressed and
> have
> > a macro isCompressed() that checks for either if that would be easier.
>

https://codereview.appspot.com/7068055/diff/43001/tests/PDFPrimitivesTest.cpp
File tests/PDFPrimitivesTest.cpp (right):

https://codereview.appspot.com/7068055/diff/43001/tests/PDFPrimitivesTest.cpp...
tests/PDFPrimitivesTest.cpp:286: TestImage(reporter, bitmap,
But we don't want to test that specifically. It's ok if the precise formatting
changes (I may eventually get around to removing some of the unneeded spaces in
the output formatting).  The important part of the test is that the expected
kind of compression was used.

On 2013/03/28 17:09:37, edisonn wrote:
> I prefer this way, because it also test how it is serialized to the file also.
> 
> On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> > Looking for the whole image dictionary is fragile to changes... It would be
a
> > bit more work, but you should be able to query the catalog for the item
number
> > and then search for the start of the object and confirm that it has the
right
> > (or no) filter option before the next ">> stream" ?  Up to you though.
>

https://codereview.appspot.com/7068055/diff/59001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/7068055/diff/59001/include/pdf/SkPDFDevice.h#n...
include/pdf/SkPDFDevice.h:66: *         behaivour dinamically, and an image is
added to a second catalog,
behavior dynamically and...

[edisonn, 2013-03-28 21:31:54]

https://codereview.appspot.com/7068055/diff/43001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/7068055/diff/43001/include/pdf/SkPDFDevice.h#n...
include/pdf/SkPDFDevice.h:69: EncodeToDCTStream encoder = NULL);
On 2013/03/28 19:27:42, Steve VanDeBogart wrote:
> Interesting... In general we have this issue because a image can be in
multiple
> documents.  But having a separate with the comment that it's only used with
the
> document is generated should solve this concern.  (It's only used in populate,
> which IIRC, is only called through emitDocument.) 
> 
> So I still think a setter would be cleaner.
> 
> On 2013/03/28 17:09:37, edisonn wrote:
> > I don't think I want to see this encoder mutable. I am not sure how it would
> > play out, especially in the usage of chrome. For example, if we added a
image
> to
> > a catalog, and the first encoder decides not to encode, but we add the image
> to
> > another catalog, and we change the encoder to do encoding, what should be
the
> > result?. If we find a need, we can update the code, but at this point it
would
> > create confusion and callers might have expectations that would not be meet.
> > This could still happen with a encoder that woud have a global state that
> > changes behavior, but I will add to the comments, that once we get the jpeg
> > stream once, we will cache it.
> > 
> > On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> > > Interesting idea and I think I like it.  However, I think it would be
better
> > to
> > > make it an explicit method of SkPDFDdevice: void
> > setDCTEncoder(EncodeToDCTStream
> > > encoder);
> > > 
> > > The comment for the new function can be a bit more verbose.  It should
> suggest
> > > that the implementation may want to not compress if the image is below a
> > certain
> > > size, etc.
> > 
> 

Done.

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImage.cpp
File src/pdf/SkPDFImage.cpp (right):

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImage.cpp#newc...
src/pdf/SkPDFImage.cpp:272: // Don't even try to compress using JPEG the alpha
chanel.
On 2013/03/28 19:27:42, Steve VanDeBogart wrote:
> Say that?  "Don't try to use DCT compression with alpha because alpha is small
> anyway and it could lead to artifacts." ?
> 
> On 2013/03/28 17:09:37, edisonn wrote:
> > It is not worth it. Alpha masks are not like pictures, they have usually a
low
> > degree of information, and zip is best. The second problem is that JPEG is
> > losing information, and it will amplify the errors of rendering.
> > On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> > > nit: Don't try to use DCT compression for the alpha channel.
> > > Also, why not?
> > 
> 

Done.

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cpp
File src/pdf/SkPDFImageStream.cpp (right):

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:15: #define kNoColorTransform (0)
On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> no ()s

Done.

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:15: #define kNoColorTransform (0)
On 2013/03/28 19:27:42, Steve VanDeBogart wrote:
> On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> > no ()s
> 
> Missed this one.

Done.

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:114: fEncoder(&jpegCompressedWStream,
*bitmapToUse)) {
If we can we flate, which would have no loss, and it would be smaller, why use
DCT then?
On 2013/03/28 19:27:42, Steve VanDeBogart wrote:
> What I meant is if fEncoder gives us a result, maybe we should skip trying to
> use flate compression.
> 
> On 2013/03/28 17:09:37, edisonn wrote:
> > Unless for testing purposes, I don't se why. DCT compression should be
slower
> > anyway, so why would it use it if it does nto save space?
> > On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> > > I wonder if we should honor the choices of fEncoder more strictly.  i.e.
if
> we
> > > get a DCT encoded result back from the consumer through this function, we
> > should
> > > just use it.
> > 
>

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.h
File src/pdf/SkPDFImageStream.h (right):

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.h#...
src/pdf/SkPDFImageStream.h:30: class SkPDFImageStream : public SkPDFDict {
On 2013/03/28 19:27:42, Steve VanDeBogart wrote:
> SkPDFImageStream is a SkPDFStream.  I think inheriting will simplify the code.
> 
> On 2013/03/28 17:09:37, edisonn wrote:
> > That depend on our view if "SkPDFImageStream" is a "SkPDFStream". I don't
have
> a
> > strong opinion either. if you wish I will inherit SkPDFImageStream from
> > SkPDFStream
> > 
> > On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> > > Should this inherit from SkPDFStream?  It seems like it would reduce a lot
> of
> > > duplicate code.
> > 
> 

Done.

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.h#...
src/pdf/SkPDFImageStream.h:61: kCompressed_State,     //!< The stream's already
been compressed.
did that; but no one is checking the compression state like this anyway.
On 2013/03/28 19:27:42, Steve VanDeBogart wrote:
> That's not exactly what I suggested.  I suggested removing kCompressed_State
and
> adding the two different kinds of compression to the enum.  Then you can use a
> macro that tests for either kind of compression.
> 
> I think having just one enum will make the code easier to understand.  Please
> give it a try.
> 
> On 2013/03/28 17:09:37, edisonn wrote:
> > Did that, and Leon suggested not doing that:
> >
>
https://codereview.appspot.com/7068055/diff/6012/src/pdf/SkPDFStream.h#newcode60
> > 
> > And I prefer Leon suggestion, as there is no use case to check the
compression
> > methon so far. I would rather not add dead code in the hope it will be used.
> > On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> > > You could replace kCompressed with kFlateCompressed and kDCTCompressed and
> > have
> > > a macro isCompressed() that checks for either if that would be easier.
> > 
>

https://codereview.appspot.com/7068055/diff/43001/tests/PDFPrimitivesTest.cpp
File tests/PDFPrimitivesTest.cpp (right):

https://codereview.appspot.com/7068055/diff/43001/tests/PDFPrimitivesTest.cpp...
tests/PDFPrimitivesTest.cpp:286: TestImage(reporter, bitmap,
Agree, but since the test does test the expected compression, I prefer to keep
it like this.
On 2013/03/28 19:27:42, Steve VanDeBogart wrote:
> But we don't want to test that specifically. It's ok if the precise formatting
> changes (I may eventually get around to removing some of the unneeded spaces
in
> the output formatting).  The important part of the test is that the expected
> kind of compression was used.
> 
> On 2013/03/28 17:09:37, edisonn wrote:
> > I prefer this way, because it also test how it is serialized to the file
also.
> > 
> > On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> > > Looking for the whole image dictionary is fragile to changes... It would
be
> a
> > > bit more work, but you should be able to query the catalog for the item
> number
> > > and then search for the start of the object and confirm that it has the
> right
> > > (or no) filter option before the next ">> stream" ?  Up to you though.
> > 
>

https://codereview.appspot.com/7068055/diff/59001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/7068055/diff/59001/include/pdf/SkPDFDevice.h#n...
include/pdf/SkPDFDevice.h:66: *         behaivour dinamically, and an image is
added to a second catalog,
On 2013/03/28 19:27:42, Steve VanDeBogart wrote:
> behavior dynamically and...

Done.

[Steve VanDeBogart, 2013-03-29 17:51:23]

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cpp
File src/pdf/SkPDFImageStream.cpp (right):

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:114: fEncoder(&jpegCompressedWStream,
*bitmapToUse)) {
It will take more time to calculate the deflated result and I suspect that most
of the time it won't be smaller.

On 2013/03/28 21:31:54, edisonn wrote:
> If we can we flate, which would have no loss, and it would be smaller, why use
> DCT then?
> On 2013/03/28 19:27:42, Steve VanDeBogart wrote:
> > What I meant is if fEncoder gives us a result, maybe we should skip trying
to
> > use flate compression.
> > 
> > On 2013/03/28 17:09:37, edisonn wrote:
> > > Unless for testing purposes, I don't se why. DCT compression should be
> slower
> > > anyway, so why would it use it if it does nto save space?
> > > On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> > > > I wonder if we should honor the choices of fEncoder more strictly.  i.e.
> if
> > we
> > > > get a DCT encoded result back from the consumer through this function,
we
> > > should
> > > > just use it.
> > > 
> > 
>

https://codereview.appspot.com/7068055/diff/43001/tests/PDFPrimitivesTest.cpp
File tests/PDFPrimitivesTest.cpp (right):

https://codereview.appspot.com/7068055/diff/43001/tests/PDFPrimitivesTest.cpp...
tests/PDFPrimitivesTest.cpp:286: TestImage(reporter, bitmap,
I'm sorry, I don't understand what you mean.  If you agree that we don't
necessarily want to test the exact formatting, then my previous suggestion
should work: Look for a Filter option between the start of the object and ">>
stream"

On 2013/03/28 21:31:54, edisonn wrote:
> Agree, but since the test does test the expected compression, I prefer to keep
> it like this.
> On 2013/03/28 19:27:42, Steve VanDeBogart wrote:
> > But we don't want to test that specifically. It's ok if the precise
formatting
> > changes (I may eventually get around to removing some of the unneeded spaces
> in
> > the output formatting).  The important part of the test is that the expected
> > kind of compression was used.
> > 
> > On 2013/03/28 17:09:37, edisonn wrote:
> > > I prefer this way, because it also test how it is serialized to the file
> also.
> > > 
> > > On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> > > > Looking for the whole image dictionary is fragile to changes... It would
> be
> > a
> > > > bit more work, but you should be able to query the catalog for the item
> > number
> > > > and then search for the start of the object and confirm that it has the
> > right
> > > > (or no) filter option before the next ">> stream" ?  Up to you though.
> > > 
> > 
>

https://codereview.appspot.com/7068055/diff/86001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/7068055/diff/86001/include/pdf/SkPDFDevice.h#n...
include/pdf/SkPDFDevice.h:23: typedef bool (*EncodeToDCTStream)(SkWStream*,
const SkBitmap&);
Thinking about this interface a bit, I think you want to pass the srcRect down
to the DCT encoder function instead of automatically extracting the subset
image.  If the encoder function decides that it doesn't want to encode the image
(for what ever reason) then extracting the subset is wasted work.  It's easy
enough for the DCT encoder function to extract the subset if it does want to use
it.

https://codereview.appspot.com/7068055/diff/86001/include/pdf/SkPDFDevice.h#n...
include/pdf/SkPDFDevice.h:70: /** Sets the DCTEncoder for images.
nit: move this down to line 142; it's more like the methods defined there.

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImage.cpp
File src/pdf/SkPDFImage.cpp (right):

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImage.cpp#newc...
src/pdf/SkPDFImage.cpp:296: const SkIRect& srcRect, bool doingAlpha,
nit: put doingAlpha on it's own line.

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImageStream.cpp
File src/pdf/SkPDFImageStream.cpp (right):

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:26: : SkPDFStream(stream),
Hmm, SkPDFStream(stream) is the deprecated interface.  It hasn't been cleaned
up... so I guess you should just add a TODO to change the interface to use
SkData instead of SkStream

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:31: SkSafeRef(stream);
SkPDFStream refs stream, so this shouldn't.

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:37: fBitmap(pdfStream.fBitmap),
nit: extra tab character.

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:45: if (fState == kUnused_State) {
You may be able to to just do:
if (!Encoder)
  return INHERITED::populate(catalog);

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:52: if (fEncoder) {
I'm not sure if you wanted to wait until a follow up CL to add it, but I think
it would be helpful to just add in here now; the code that checks to see if the
Bitmap is already backed by a jpeg and just uses the existing encoding?

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:69: if (fEncoder(&dctCompressedWStream,
*bitmapToUse)) {
hasDct... = fEncoder(...);

Though, if this returns false, you might be able to again just return
INHERITED:populate(catalog);

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImageStream.h
File src/pdf/SkPDFImageStream.h (left):

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImageStream.h#...
src/pdf/SkPDFImageStream.h:52: void setData(SkStream* stream);
setData needs to be made virtual in SkPDFStream as well.

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImageStream.h#...
src/pdf/SkPDFImageStream.h:72: bool populate(SkPDFCatalog* catalog);
populate needs to be made virtual and moved to the protected section.

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImageStream.h
File src/pdf/SkPDFImageStream.h (right):

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImageStream.h#...
src/pdf/SkPDFImageStream.h:21: typedef bool (*EncodeToDCTStream)(SkWStream*,
const SkBitmap&);
This should only be defined in one place - you have it here and in SkPDFDevice.h

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImageStream.h#...
src/pdf/SkPDFImageStream.h:27: This class is similar with SkPDFStream, but it is
also able to use image
nit: similar to

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImageStream.h#...
src/pdf/SkPDFImageStream.h:35: explicit SkPDFImageStream(const SkPDFImageStream&
pdfStream);
Do we need the copy constructor?

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImageStream.h#...
src/pdf/SkPDFImageStream.h:39: /* Create a PDF stream with no data.  The setData
method must be called to
This comment is out of date.

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImageStream.h#...
src/pdf/SkPDFImageStream.h:58: kRawCompressed_State,    //!< No benefit from
stream's compression.
Sorry, this change in incompatible with inheriting from SkPDFStream.

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImageStream.h#...
src/pdf/SkPDFImageStream.h:63: State fState;
fState is defined in SkPDFStream.  If you need to change it, you'll need to add
a protected setState method.

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImageStream.h#...
src/pdf/SkPDFImageStream.h:69: typedef SkPDFDict INHERITED;
This is out of date.

[edisonn, 2013-03-29 18:05:23]

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cpp
File src/pdf/SkPDFImageStream.cpp (right):

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:114: fEncoder(&jpegCompressedWStream,
*bitmapToUse)) {
Convert all our SKPs to PDFs
- no DCT (today) -> 157MB
- best of Raw, DCT, Flate -> 67MB
- DCT or Raw even if Flate is smaller: 70MB

overall size is 4% larger 

I am worried about computer generated graphics that is not that great encoded by
DCT. But given that the size increase is only 4%, I will do as you said.

On 2013/03/29 17:51:23, Steve VanDeBogart wrote:
> It will take more time to calculate the deflated result and I suspect that
most
> of the time it won't be smaller.
> 
> On 2013/03/28 21:31:54, edisonn wrote:
> > If we can we flate, which would have no loss, and it would be smaller, why
use
> > DCT then?
> > On 2013/03/28 19:27:42, Steve VanDeBogart wrote:
> > > What I meant is if fEncoder gives us a result, maybe we should skip trying
> to
> > > use flate compression.
> > > 
> > > On 2013/03/28 17:09:37, edisonn wrote:
> > > > Unless for testing purposes, I don't se why. DCT compression should be
> > slower
> > > > anyway, so why would it use it if it does nto save space?
> > > > On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> > > > > I wonder if we should honor the choices of fEncoder more strictly. 
i.e.
> > if
> > > we
> > > > > get a DCT encoded result back from the consumer through this function,
> we
> > > > should
> > > > > just use it.
> > > > 
> > > 
> > 
>

[Steve VanDeBogart, 2013-03-29 18:08:10]

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cpp
File src/pdf/SkPDFImageStream.cpp (right):

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:114: fEncoder(&jpegCompressedWStream,
*bitmapToUse)) {
Data is great!  What's the difference in runtime for the best of case vs DCT or
Raw... ?

On 2013/03/29 18:05:23, edisonn wrote:
> Convert all our SKPs to PDFs
> - no DCT (today) -> 157MB
> - best of Raw, DCT, Flate -> 67MB
> - DCT or Raw even if Flate is smaller: 70MB
> 
> overall size is 4% larger 
> 
> I am worried about computer generated graphics that is not that great encoded
by
> DCT. But given that the size increase is only 4%, I will do as you said.
> 
> On 2013/03/29 17:51:23, Steve VanDeBogart wrote:
> > It will take more time to calculate the deflated result and I suspect that
> most
> > of the time it won't be smaller.
> > 
> > On 2013/03/28 21:31:54, edisonn wrote:
> > > If we can we flate, which would have no loss, and it would be smaller, why
> use
> > > DCT then?
> > > On 2013/03/28 19:27:42, Steve VanDeBogart wrote:
> > > > What I meant is if fEncoder gives us a result, maybe we should skip
trying
> > to
> > > > use flate compression.
> > > > 
> > > > On 2013/03/28 17:09:37, edisonn wrote:
> > > > > Unless for testing purposes, I don't se why. DCT compression should be
> > > slower
> > > > > anyway, so why would it use it if it does nto save space?
> > > > > On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> > > > > > I wonder if we should honor the choices of fEncoder more strictly. 
> i.e.
> > > if
> > > > we
> > > > > > get a DCT encoded result back from the consumer through this
function,
> > we
> > > > > should
> > > > > > just use it.
> > > > > 
> > > > 
> > > 
> > 
>

[edisonn, 2013-03-29 18:41:46]

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cpp
File src/pdf/SkPDFImageStream.cpp (right):

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:114: fEncoder(&jpegCompressedWStream,
*bitmapToUse)) {
51 vs 66 seconds

On 2013/03/29 18:08:10, Steve VanDeBogart wrote:
> Data is great!  What's the difference in runtime for the best of case vs DCT
or
> Raw... ?
> 
> On 2013/03/29 18:05:23, edisonn wrote:
> > Convert all our SKPs to PDFs
> > - no DCT (today) -> 157MB
> > - best of Raw, DCT, Flate -> 67MB
> > - DCT or Raw even if Flate is smaller: 70MB
> > 
> > overall size is 4% larger 
> > 
> > I am worried about computer generated graphics that is not that great
encoded
> by
> > DCT. But given that the size increase is only 4%, I will do as you said.
> > 
> > On 2013/03/29 17:51:23, Steve VanDeBogart wrote:
> > > It will take more time to calculate the deflated result and I suspect that
> > most
> > > of the time it won't be smaller.
> > > 
> > > On 2013/03/28 21:31:54, edisonn wrote:
> > > > If we can we flate, which would have no loss, and it would be smaller,
why
> > use
> > > > DCT then?
> > > > On 2013/03/28 19:27:42, Steve VanDeBogart wrote:
> > > > > What I meant is if fEncoder gives us a result, maybe we should skip
> trying
> > > to
> > > > > use flate compression.
> > > > > 
> > > > > On 2013/03/28 17:09:37, edisonn wrote:
> > > > > > Unless for testing purposes, I don't se why. DCT compression should
be
> > > > slower
> > > > > > anyway, so why would it use it if it does nto save space?
> > > > > > On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> > > > > > > I wonder if we should honor the choices of fEncoder more strictly.

> > i.e.
> > > > if
> > > > > we
> > > > > > > get a DCT encoded result back from the consumer through this
> function,
> > > we
> > > > > > should
> > > > > > > just use it.
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
>

[Steve VanDeBogart, 2013-03-29 18:45:24]

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cpp
File src/pdf/SkPDFImageStream.cpp (right):

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:114: fEncoder(&jpegCompressedWStream,
*bitmapToUse)) {
So 4% improvement on size cost nearly 30% more time?  I think it's the right
move to only deflate if no DCT...

On 2013/03/29 18:41:47, edisonn wrote:
> 51 vs 66 seconds
> 
> On 2013/03/29 18:08:10, Steve VanDeBogart wrote:
> > Data is great!  What's the difference in runtime for the best of case vs DCT
> or
> > Raw... ?
> > 
> > On 2013/03/29 18:05:23, edisonn wrote:
> > > Convert all our SKPs to PDFs
> > > - no DCT (today) -> 157MB
> > > - best of Raw, DCT, Flate -> 67MB
> > > - DCT or Raw even if Flate is smaller: 70MB
> > > 
> > > overall size is 4% larger 
> > > 
> > > I am worried about computer generated graphics that is not that great
> encoded
> > by
> > > DCT. But given that the size increase is only 4%, I will do as you said.
> > > 
> > > On 2013/03/29 17:51:23, Steve VanDeBogart wrote:
> > > > It will take more time to calculate the deflated result and I suspect
that
> > > most
> > > > of the time it won't be smaller.
> > > > 
> > > > On 2013/03/28 21:31:54, edisonn wrote:
> > > > > If we can we flate, which would have no loss, and it would be smaller,
> why
> > > use
> > > > > DCT then?
> > > > > On 2013/03/28 19:27:42, Steve VanDeBogart wrote:
> > > > > > What I meant is if fEncoder gives us a result, maybe we should skip
> > trying
> > > > to
> > > > > > use flate compression.
> > > > > > 
> > > > > > On 2013/03/28 17:09:37, edisonn wrote:
> > > > > > > Unless for testing purposes, I don't se why. DCT compression
should
> be
> > > > > slower
> > > > > > > anyway, so why would it use it if it does nto save space?
> > > > > > > On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> > > > > > > > I wonder if we should honor the choices of fEncoder more
strictly.
> 
> > > i.e.
> > > > > if
> > > > > > we
> > > > > > > > get a DCT encoded result back from the consumer through this
> > function,
> > > > we
> > > > > > > should
> > > > > > > > just use it.
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
>

[edisonn, 2013-04-02 20:28:16]

I hope I did not forgot to address any comments. Let me know if I missed
anything

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cpp
File src/pdf/SkPDFImageStream.cpp (right):

https://codereview.appspot.com/7068055/diff/43001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:114: fEncoder(&jpegCompressedWStream,
*bitmapToUse)) {
agree
On 2013/03/29 18:45:24, Steve VanDeBogart wrote:
> So 4% improvement on size cost nearly 30% more time?  I think it's the right
> move to only deflate if no DCT...
> 
> On 2013/03/29 18:41:47, edisonn wrote:
> > 51 vs 66 seconds
> > 
> > On 2013/03/29 18:08:10, Steve VanDeBogart wrote:
> > > Data is great!  What's the difference in runtime for the best of case vs
DCT
> > or
> > > Raw... ?
> > > 
> > > On 2013/03/29 18:05:23, edisonn wrote:
> > > > Convert all our SKPs to PDFs
> > > > - no DCT (today) -> 157MB
> > > > - best of Raw, DCT, Flate -> 67MB
> > > > - DCT or Raw even if Flate is smaller: 70MB
> > > > 
> > > > overall size is 4% larger 
> > > > 
> > > > I am worried about computer generated graphics that is not that great
> > encoded
> > > by
> > > > DCT. But given that the size increase is only 4%, I will do as you said.
> > > > 
> > > > On 2013/03/29 17:51:23, Steve VanDeBogart wrote:
> > > > > It will take more time to calculate the deflated result and I suspect
> that
> > > > most
> > > > > of the time it won't be smaller.
> > > > > 
> > > > > On 2013/03/28 21:31:54, edisonn wrote:
> > > > > > If we can we flate, which would have no loss, and it would be
smaller,
> > why
> > > > use
> > > > > > DCT then?
> > > > > > On 2013/03/28 19:27:42, Steve VanDeBogart wrote:
> > > > > > > What I meant is if fEncoder gives us a result, maybe we should
skip
> > > trying
> > > > > to
> > > > > > > use flate compression.
> > > > > > > 
> > > > > > > On 2013/03/28 17:09:37, edisonn wrote:
> > > > > > > > Unless for testing purposes, I don't se why. DCT compression
> should
> > be
> > > > > > slower
> > > > > > > > anyway, so why would it use it if it does nto save space?
> > > > > > > > On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> > > > > > > > > I wonder if we should honor the choices of fEncoder more
> strictly.
> > 
> > > > i.e.
> > > > > > if
> > > > > > > we
> > > > > > > > > get a DCT encoded result back from the consumer through this
> > > function,
> > > > > we
> > > > > > > > should
> > > > > > > > > just use it.
> > > > > > > > 
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
>

https://codereview.appspot.com/7068055/diff/43001/tests/PDFPrimitivesTest.cpp
File tests/PDFPrimitivesTest.cpp (right):

https://codereview.appspot.com/7068055/diff/43001/tests/PDFPrimitivesTest.cpp...
tests/PDFPrimitivesTest.cpp:286: TestImage(reporter, bitmap,
I want to keep the test as is. 
There are other places where we check for hardcoded order of fields (like
/Length 12\n/Attribute 42) and I feel more confortable with an end-to-end test.
I am not that worried about field reordering and spacing.

On 2013/03/29 17:51:23, Steve VanDeBogart wrote:
> I'm sorry, I don't understand what you mean.  If you agree that we don't
> necessarily want to test the exact formatting, then my previous suggestion
> should work: Look for a Filter option between the start of the object and ">>
> stream"
> 
> On 2013/03/28 21:31:54, edisonn wrote:
> > Agree, but since the test does test the expected compression, I prefer to
keep
> > it like this.
> > On 2013/03/28 19:27:42, Steve VanDeBogart wrote:
> > > But we don't want to test that specifically. It's ok if the precise
> formatting
> > > changes (I may eventually get around to removing some of the unneeded
spaces
> > in
> > > the output formatting).  The important part of the test is that the
expected
> > > kind of compression was used.
> > > 
> > > On 2013/03/28 17:09:37, edisonn wrote:
> > > > I prefer this way, because it also test how it is serialized to the file
> > also.
> > > > 
> > > > On 2013/03/27 20:34:55, Steve VanDeBogart wrote:
> > > > > Looking for the whole image dictionary is fragile to changes... It
would
> > be
> > > a
> > > > > bit more work, but you should be able to query the catalog for the
item
> > > number
> > > > > and then search for the start of the object and confirm that it has
the
> > > right
> > > > > (or no) filter option before the next ">> stream" ?  Up to you though.
> > > > 
> > > 
> > 
>

https://codereview.appspot.com/7068055/diff/86001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/7068055/diff/86001/include/pdf/SkPDFDevice.h#n...
include/pdf/SkPDFDevice.h:23: typedef bool (*EncodeToDCTStream)(SkWStream*,
const SkBitmap&);
On 2013/03/29 17:51:24, Steve VanDeBogart wrote:
> Thinking about this interface a bit, I think you want to pass the srcRect down
> to the DCT encoder function instead of automatically extracting the subset
> image.  If the encoder function decides that it doesn't want to encode the
image
> (for what ever reason) then extracting the subset is wasted work.  It's easy
> enough for the DCT encoder function to extract the subset if it does want to
use
> it.

Done.

https://codereview.appspot.com/7068055/diff/86001/include/pdf/SkPDFDevice.h#n...
include/pdf/SkPDFDevice.h:70: /** Sets the DCTEncoder for images.
On 2013/03/29 17:51:24, Steve VanDeBogart wrote:
> nit: move this down to line 142; it's more like the methods defined there.

Done.

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImage.cpp
File src/pdf/SkPDFImage.cpp (right):

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImage.cpp#newc...
src/pdf/SkPDFImage.cpp:296: const SkIRect& srcRect, bool doingAlpha,
On 2013/03/29 17:51:24, Steve VanDeBogart wrote:
> nit: put doingAlpha on it's own line.

Done.

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImageStream.cpp
File src/pdf/SkPDFImageStream.cpp (right):

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:26: : SkPDFStream(stream),
On 2013/03/29 17:51:24, Steve VanDeBogart wrote:
> Hmm, SkPDFStream(stream) is the deprecated interface.  It hasn't been cleaned
> up... so I guess you should just add a TODO to change the interface to use
> SkData instead of SkStream

Done.

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:31: SkSafeRef(stream);
On 2013/03/29 17:51:24, Steve VanDeBogart wrote:
> SkPDFStream refs stream, so this shouldn't.

Done.

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:37: fBitmap(pdfStream.fBitmap),
On 2013/03/29 17:51:24, Steve VanDeBogart wrote:
> nit: extra tab character.

Done.

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:45: if (fState == kUnused_State) {
no, because SkPDFStream and SkPDFImage stream each has its own private fState
On 2013/03/29 17:51:24, Steve VanDeBogart wrote:
> You may be able to to just do:
> if (!Encoder)
>   return INHERITED::populate(catalog);

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:52: if (fEncoder) {
this, will be up to the caller, which has the SkBitmap and do it. Especially
since I will move the srcRect in the callback

On 2013/03/29 17:51:24, Steve VanDeBogart wrote:
> I'm not sure if you wanted to wait until a follow up CL to add it, but I think
> it would be helpful to just add in here now; the code that checks to see if
the
> Bitmap is already backed by a jpeg and just uses the existing encoding?

https://codereview.appspot.com/7068055/diff/86001/src/pdf/SkPDFImageStream.cp...
src/pdf/SkPDFImageStream.cpp:69: if (fEncoder(&dctCompressedWStream,
*bitmapToUse)) {
no, because SkPDFStream and SkPDFImage stream each has its own private fState
On 2013/03/29 17:51:24, Steve VanDeBogart wrote:
> hasDct... = fEncoder(...);
> 
> Though, if this returns false, you might be able to again just return
> INHERITED:populate(catalog);

[Steve VanDeBogart, 2013-04-02 21:01:52]

It looks like you missed a bunch of comments.  Added the high level ones again,
but I didn't do a deep review.

https://codereview.appspot.com/7068055/diff/107001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/7068055/diff/107001/include/pdf/SkPDFDevice.h#...
include/pdf/SkPDFDevice.h:23: typedef bool (*EncodeToDCTStream)(SkWStream*,
const SkBitmap&, const SkIRect&);
In this case, I think you need to have parameter names.  It's not clear what the
different arguments are for.

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.cpp
File src/pdf/SkPDFImageStream.cpp (right):

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.c...
src/pdf/SkPDFImageStream.cpp:44: bool SkPDFImageStream::populate(SkPDFCatalog*
catalog) {
In a number of cases here, you can just call INHERITED::populate()

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.h
File src/pdf/SkPDFImageStream.h (right):

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.h...
src/pdf/SkPDFImageStream.h:21: typedef bool (*EncodeToDCTStream)(SkWStream*,
const SkBitmap&, const SkIRect&);
There should be only one definition for this typedef.  You have it here and in
SkPDFDevice.h

In either place, the arguments should have names to help explain what they do.

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.h...
src/pdf/SkPDFImageStream.h:27: This class is similar with SkPDFStream, but it is
also able to use image
nit: similar to

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.h...
src/pdf/SkPDFImageStream.h:35: explicit SkPDFImageStream(const SkPDFImageStream&
pdfStream);
Do we need the copy constructor?

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.h...
src/pdf/SkPDFImageStream.h:39: /* Create a PDF stream with no data.  The setData
method must be called to
This comment is out of date.

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.h...
src/pdf/SkPDFImageStream.h:45: virtual void setData(SkStream* stream);
You need to change SkPDFStream as well.

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.h...
src/pdf/SkPDFImageStream.h:48: enum StreamCompression {
You should add another member variable fCompressionType

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.h...
src/pdf/SkPDFImageStream.h:54: enum State {
Don't include this here as well (only in SkPDFStream)

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.h...
src/pdf/SkPDFImageStream.h:63: State fState;
There should only be on fState - in SkPDFStream.

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.h...
src/pdf/SkPDFImageStream.h:69: typedef SkPDFDict INHERITED;
INHERITED should now be SkPDFStream

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.h...
src/pdf/SkPDFImageStream.h:73: bool populate(SkPDFCatalog* catalog);
This needs to be virtual and in the protected section - here and in SkPDFStream

[edisonn, 2013-04-05 17:20:52]

https://codereview.appspot.com/7068055/diff/107001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/7068055/diff/107001/include/pdf/SkPDFDevice.h#...
include/pdf/SkPDFDevice.h:23: typedef bool (*EncodeToDCTStream)(SkWStream*,
const SkBitmap&, const SkIRect&);
On 2013/04/02 21:01:52, Steve VanDeBogart wrote:
> In this case, I think you need to have parameter names.  It's not clear what
the
> different arguments are for.

Done.

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.cpp
File src/pdf/SkPDFImageStream.cpp (right):

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.c...
src/pdf/SkPDFImageStream.cpp:44: bool SkPDFImageStream::populate(SkPDFCatalog*
catalog) {
I don't see how. can you elaborate?
On 2013/04/02 21:01:52, Steve VanDeBogart wrote:
> In a number of cases here, you can just call INHERITED::populate()

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.h
File src/pdf/SkPDFImageStream.h (right):

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.h...
src/pdf/SkPDFImageStream.h:21: typedef bool (*EncodeToDCTStream)(SkWStream*,
const SkBitmap&, const SkIRect&);
On 2013/04/02 21:01:52, Steve VanDeBogart wrote:
> There should be only one definition for this typedef.  You have it here and in
> SkPDFDevice.h
> 
> In either place, the arguments should have names to help explain what they do.

Done.

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.h...
src/pdf/SkPDFImageStream.h:27: This class is similar with SkPDFStream, but it is
also able to use image
On 2013/04/02 21:01:52, Steve VanDeBogart wrote:
> nit: similar to

Done.

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.h...
src/pdf/SkPDFImageStream.h:35: explicit SkPDFImageStream(const SkPDFImageStream&
pdfStream);
yes, used in substitute creation
On 2013/04/02 21:01:52, Steve VanDeBogart wrote:
> Do we need the copy constructor?

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.h...
src/pdf/SkPDFImageStream.h:39: /* Create a PDF stream with no data.  The setData
method must be called to
removed
On 2013/04/02 21:01:52, Steve VanDeBogart wrote:
> This comment is out of date.

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.h...
src/pdf/SkPDFImageStream.h:45: virtual void setData(SkStream* stream);
it was done, just that the file was not in cl
On 2013/04/02 21:01:52, Steve VanDeBogart wrote:
> You need to change SkPDFStream as well.

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.h...
src/pdf/SkPDFImageStream.h:48: enum StreamCompression {
On 2013/04/02 21:01:52, Steve VanDeBogart wrote:
> You should add another member variable fCompressionType

Done.

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.h...
src/pdf/SkPDFImageStream.h:54: enum State {
done

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.h...
src/pdf/SkPDFImageStream.h:63: State fState;
done
On 2013/04/02 21:01:52, Steve VanDeBogart wrote:
> There should only be on fState - in SkPDFStream.

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.h...
src/pdf/SkPDFImageStream.h:69: typedef SkPDFDict INHERITED;
On 2013/04/02 21:01:52, Steve VanDeBogart wrote:
> INHERITED should now be SkPDFStream

Done.

https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.h...
src/pdf/SkPDFImageStream.h:73: bool populate(SkPDFCatalog* catalog);
On 2013/04/02 21:01:52, Steve VanDeBogart wrote:
> This needs to be virtual and in the protected section - here and in
SkPDFStream

Done.

[Steve VanDeBogart, 2013-04-06 00:41:05]

https://codereview.appspot.com/7068055/diff/113002/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/7068055/diff/113002/include/pdf/SkPDFDevice.h#...
include/pdf/SkPDFDevice.h:23: typedef bool (*EncodeToDCTStream)(SkWStream*
stream, const SkBitmap& bitmap, const SkIRect& rect);
nit: should stream go at the end of the argument list since it's an output
parameter?

https://codereview.appspot.com/7068055/diff/113002/include/pdf/SkPDFDevice.h#...
include/pdf/SkPDFDevice.h:136: *         Callers might not want to encode small
images, as the time spent
nit: say that by returning false from the encoder function, the encoder result
is not used.

https://codereview.appspot.com/7068055/diff/113002/src/pdf/SkPDFImage.h
File src/pdf/SkPDFImage.h (right):

https://codereview.appspot.com/7068055/diff/113002/src/pdf/SkPDFImage.h#newco...
src/pdf/SkPDFImage.h:14: #include "SkPDFTypes.h"
Do you need to include SkPDFDevice.h here to define EncodeToDCTStream

https://codereview.appspot.com/7068055/diff/113002/src/pdf/SkPDFImageStream.cpp
File src/pdf/SkPDFImageStream.cpp (right):

https://codereview.appspot.com/7068055/diff/113002/src/pdf/SkPDFImageStream.c...
src/pdf/SkPDFImageStream.cpp:42: bool SkPDFImageStream::populate(SkPDFCatalog*
catalog) {
You asked how you could use INHERITED::populate to simply this.  Something like
this:

bool SkPDFImageStream::populate(SkPDFCatalog* catalog) {
    if (fState == kUnused_State) {
        fState = kNoCompression_State;
        if (!skip_compression(catalog)) {
            SkDynamicMemoryWStream dctCompressedWStream;
            if (!fEncoder || !fEncoder(&dctCompressedWStream, fBitmap,
fSrcRect)) {
                return INHERITED::populate(catalog);
            }

            if (dctCompressedWStream.getOffset() < fData->getLenth()) {
                setData(SkNEW_ARGS(SkMemoryStream,
(dctCompressedWStream.copyToData())));
                insertName("Filter", "DCTDecode");
                insertInt("ColorTransform", kNoColorTransform);
                fState = kCompressed_State;
            }
        }
        insertInt("Length", fData->getLength());
    } else if (fState == kNoCompression_State && !skip_compression(catalog) &&
               (SkFlate::HaveFlate() || fEncoder)) {
        // Compression has not been requested when the stream was first created.
        // But a new Catalog would want it compressed.
        if (!fSubstitute.get()) {
            fSubstitute.reset(new SkPDFImageStream(*this));
            catalog->setSubstitute(this, fSubstitute.get());
        }
        return false;
    }
    return true;
}

https://codereview.appspot.com/7068055/diff/113002/src/pdf/SkPDFImageStream.c...
src/pdf/SkPDFImageStream.cpp:102: void SkPDFImageStream::setData(SkStream*
stream) {
I don't think you want this.  Just leave the base class implementation.

https://codereview.appspot.com/7068055/diff/113002/src/pdf/SkPDFImageStream.h
File src/pdf/SkPDFImageStream.h (right):

https://codereview.appspot.com/7068055/diff/113002/src/pdf/SkPDFImageStream.h...
src/pdf/SkPDFImageStream.h:55: StreamCompression fCompressionType;
You only use this within populate, so I don't think you need it.  Just use a
local variable instead.

https://codereview.appspot.com/7068055/diff/113002/src/pdf/SkPDFStream.h
File src/pdf/SkPDFStream.h (right):

https://codereview.appspot.com/7068055/diff/113002/src/pdf/SkPDFStream.h#newc...
src/pdf/SkPDFStream.h:59: SkAutoTUnref<SkStream> fData;
Instead of making these data member (fData, fSubstitute, and fState) protected,
you should make them private and make accessors and setters (depending on your
needs) protected.

setData is already a setter for fData, that's why I say you probably don't want
to override it with an assert(false).

[edisonn, 2013-04-18 12:48:09]

https://codereview.appspot.com/7068055/diff/113002/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/7068055/diff/113002/include/pdf/SkPDFDevice.h#...
include/pdf/SkPDFDevice.h:23: typedef bool (*EncodeToDCTStream)(SkWStream*
stream, const SkBitmap& bitmap, const SkIRect& rect);
In skia, we seem not to use/repect this rule, and I kept the same order as
SkImageEncoder::EncodeStream

On 2013/04/06 00:41:05, Steve VanDeBogart wrote:
> nit: should stream go at the end of the argument list since it's an output
> parameter?

https://codereview.appspot.com/7068055/diff/113002/include/pdf/SkPDFDevice.h#...
include/pdf/SkPDFDevice.h:136: *         Callers might not want to encode small
images, as the time spent
On 2013/04/06 00:41:05, Steve VanDeBogart wrote:
> nit: say that by returning false from the encoder function, the encoder result
> is not used.

Done.

https://codereview.appspot.com/7068055/diff/113002/src/pdf/SkPDFImage.h
File src/pdf/SkPDFImage.h (right):

https://codereview.appspot.com/7068055/diff/113002/src/pdf/SkPDFImage.h#newco...
src/pdf/SkPDFImage.h:14: #include "SkPDFTypes.h"
done. it was already available from ImageStream, by explicit inclusion should be
better
On 2013/04/06 00:41:05, Steve VanDeBogart wrote:
> Do you need to include SkPDFDevice.h here to define EncodeToDCTStream

https://codereview.appspot.com/7068055/diff/113002/src/pdf/SkPDFImageStream.cpp
File src/pdf/SkPDFImageStream.cpp (right):

https://codereview.appspot.com/7068055/diff/113002/src/pdf/SkPDFImageStream.c...
src/pdf/SkPDFImageStream.cpp:42: bool SkPDFImageStream::populate(SkPDFCatalog*
catalog) {
On 2013/04/06 00:41:05, Steve VanDeBogart wrote:
> You asked how you could use INHERITED::populate to simply this.  Something
like
> this:
> 
> bool SkPDFImageStream::populate(SkPDFCatalog* catalog) {
>     if (fState == kUnused_State) {
>         fState = kNoCompression_State;
>         if (!skip_compression(catalog)) {
>             SkDynamicMemoryWStream dctCompressedWStream;
>             if (!fEncoder || !fEncoder(&dctCompressedWStream, fBitmap,
> fSrcRect)) {
>                 return INHERITED::populate(catalog);
>             }
> 
>             if (dctCompressedWStream.getOffset() < fData->getLenth()) {
>                 setData(SkNEW_ARGS(SkMemoryStream,
> (dctCompressedWStream.copyToData())));
>                 insertName("Filter", "DCTDecode");
>                 insertInt("ColorTransform", kNoColorTransform);
>                 fState = kCompressed_State;
>             }
>         }
>         insertInt("Length", fData->getLength());
>     } else if (fState == kNoCompression_State && !skip_compression(catalog) &&
>                (SkFlate::HaveFlate() || fEncoder)) {
>         // Compression has not been requested when the stream was first
created.
>         // But a new Catalog would want it compressed.
>         if (!fSubstitute.get()) {
>             fSubstitute.reset(new SkPDFImageStream(*this));
>             catalog->setSubstitute(this, fSubstitute.get());
>         }
>         return false;
>     }
>     return true;
> }

Done.

https://codereview.appspot.com/7068055/diff/113002/src/pdf/SkPDFImageStream.c...
src/pdf/SkPDFImageStream.cpp:102: void SkPDFImageStream::setData(SkStream*
stream) {
On 2013/04/06 00:41:05, Steve VanDeBogart wrote:
> I don't think you want this.  Just leave the base class implementation.

Done.

https://codereview.appspot.com/7068055/diff/113002/src/pdf/SkPDFImageStream.h
File src/pdf/SkPDFImageStream.h (right):

https://codereview.appspot.com/7068055/diff/113002/src/pdf/SkPDFImageStream.h...
src/pdf/SkPDFImageStream.h:55: StreamCompression fCompressionType;
done
https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.h...
You should add another member variable fCompressionType

On 2013/04/06 00:41:05, Steve VanDeBogart wrote:
> You only use this within populate, so I don't think you need it.  Just use a
> local variable instead.

https://codereview.appspot.com/7068055/diff/113002/src/pdf/SkPDFStream.h
File src/pdf/SkPDFStream.h (right):

https://codereview.appspot.com/7068055/diff/113002/src/pdf/SkPDFStream.h#newc...
src/pdf/SkPDFStream.h:59: SkAutoTUnref<SkStream> fData;
On 2013/04/06 00:41:05, Steve VanDeBogart wrote:
> Instead of making these data member (fData, fSubstitute, and fState)
protected,
> you should make them private and make accessors and setters (depending on your
> needs) protected.
> 
> setData is already a setter for fData, that's why I say you probably don't
want
> to override it with an assert(false).

Done.

[Steve VanDeBogart, 2013-04-18 18:54:26]

Mostly just nits and one memory question.

https://codereview.appspot.com/7068055/diff/113002/src/pdf/SkPDFImageStream.h
File src/pdf/SkPDFImageStream.h (right):

https://codereview.appspot.com/7068055/diff/113002/src/pdf/SkPDFImageStream.h...
src/pdf/SkPDFImageStream.h:55: StreamCompression fCompressionType;

On 2013/04/18 12:48:09, edisonn wrote:
> done
>
https://codereview.appspot.com/7068055/diff/107001/src/pdf/SkPDFImageStream.h...
> You should add another member variable fCompressionType

Right, if you needed to save that state, it should go in another member
variable, and not change the enum in the base class.  But it turns out, you
don't need to save the state.

> 
> On 2013/04/06 00:41:05, Steve VanDeBogart wrote:
> > You only use this within populate, so I don't think you need it.  Just use a
> > local variable instead.
>

https://codereview.appspot.com/7068055/diff/151001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.appspot.com/7068055/diff/151001/gm/gmmain.cpp#newcode177
gm/gmmain.cpp:177: bool encodeToDCTStream(SkWStream* stream, const SkBitmap&
bitmap, const SkIRect& rect);
Any reason to not just move the function definition up here?

https://codereview.appspot.com/7068055/diff/151001/gm/gmmain.cpp#newcode1256
gm/gmmain.cpp:1256: bool encodeToDCTStream(SkWStream* stream, const SkBitmap&
bitmap, const SkIRect& rect) {
nit: I think this should be static bool encode_to_dct_stream

https://codereview.appspot.com/7068055/diff/151001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/7068055/diff/151001/include/pdf/SkPDFDevice.h#...
include/pdf/SkPDFDevice.h:23: typedef bool (*EncodeToDCTStream)(SkWStream*
stream, const SkBitmap& bitmap, const SkIRect& rect);
nit: this should probably go down to line 41.

https://codereview.appspot.com/7068055/diff/151001/include/pdf/SkPDFDevice.h#...
include/pdf/SkPDFDevice.h:132: *  @param encoder The encoder to encode a bitmap
as JPEG (DCTDeflate).
nit: (DCT) not (DCTDeflate)

https://codereview.appspot.com/7068055/diff/151001/src/pdf/SkPDFImageStream.cpp
File src/pdf/SkPDFImageStream.cpp (right):

https://codereview.appspot.com/7068055/diff/151001/src/pdf/SkPDFImageStream.c...
src/pdf/SkPDFImageStream.cpp:54: // copyToData and new SkMemoryStream both call
ref(), supress one.
Do you mean the SkMemoryStream constructor on line 57?  If so, that hasn't
happened yet, so this data->unref() will free data.  You have to wait until
after line 57.

https://codereview.appspot.com/7068055/diff/151001/src/pdf/SkPDFImageStream.c...
src/pdf/SkPDFImageStream.cpp:80: 
nit: remove line

https://codereview.appspot.com/7068055/diff/151001/src/pdf/SkPDFImageStream.h
File src/pdf/SkPDFImageStream.h (right):

https://codereview.appspot.com/7068055/diff/151001/src/pdf/SkPDFImageStream.h...
src/pdf/SkPDFImageStream.h:46: enum StreamCompression {
nit: remove, not used.

https://codereview.appspot.com/7068055/diff/151001/src/pdf/SkPDFStream.h
File src/pdf/SkPDFStream.h (right):

https://codereview.appspot.com/7068055/diff/151001/src/pdf/SkPDFStream.h#newc...
src/pdf/SkPDFStream.h:56: enum State {
nit: move to line 47.

https://codereview.appspot.com/7068055/diff/151001/tests/PDFPrimitivesTest.cpp
File tests/PDFPrimitivesTest.cpp (right):

https://codereview.appspot.com/7068055/diff/151001/tests/PDFPrimitivesTest.cp...
tests/PDFPrimitivesTest.cpp:42: static bool encodeToDCTStream(SkWStream* stream,
const SkBitmap& bitmap, const SkIRect& rect) {
encode_to_dct_stream()

https://codereview.appspot.com/7068055/diff/151001/tools/render_pdfs_main.cpp
File tools/render_pdfs_main.cpp (right):

https://codereview.appspot.com/7068055/diff/151001/tools/render_pdfs_main.cpp...
tools/render_pdfs_main.cpp:82: static bool encodeToDCTStream(SkWStream* stream,
const SkBitmap& bitmap, const SkIRect& rect) {
encode_to_dct_stream

https://codereview.appspot.com/7068055/diff/151001/tools/render_pdfs_main.cpp...
tools/render_pdfs_main.cpp:82: static bool encodeToDCTStream(SkWStream* stream,
const SkBitmap& bitmap, const SkIRect& rect) {
This block of code is repeated a number of times.  Is there some utility file
you could put it into?

[edisonn, 2013-04-18 20:13:16]

https://codereview.appspot.com/7068055/diff/151001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.appspot.com/7068055/diff/151001/gm/gmmain.cpp#newcode177
gm/gmmain.cpp:177: bool encodeToDCTStream(SkWStream* stream, const SkBitmap&
bitmap, const SkIRect& rect);
On 2013/04/18 18:54:27, Steve VanDeBogart wrote:
> Any reason to not just move the function definition up here?
I wish the define flag to stay with the rest of the flags

https://codereview.appspot.com/7068055/diff/151001/gm/gmmain.cpp#newcode1256
gm/gmmain.cpp:1256: bool encodeToDCTStream(SkWStream* stream, const SkBitmap&
bitmap, const SkIRect& rect) {
On 2013/04/18 18:54:27, Steve VanDeBogart wrote:
> nit: I think this should be static bool encode_to_dct_stream

Done.

https://codereview.appspot.com/7068055/diff/151001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/7068055/diff/151001/include/pdf/SkPDFDevice.h#...
include/pdf/SkPDFDevice.h:23: typedef bool (*EncodeToDCTStream)(SkWStream*
stream, const SkBitmap& bitmap, const SkIRect& rect);
On 2013/04/18 18:54:27, Steve VanDeBogart wrote:
> nit: this should probably go down to line 41.

Done.

https://codereview.appspot.com/7068055/diff/151001/src/pdf/SkPDFImageStream.cpp
File src/pdf/SkPDFImageStream.cpp (right):

https://codereview.appspot.com/7068055/diff/151001/src/pdf/SkPDFImageStream.c...
src/pdf/SkPDFImageStream.cpp:54: // copyToData and new SkMemoryStream both call
ref(), supress one.
On 2013/04/18 18:54:27, Steve VanDeBogart wrote:
> Do you mean the SkMemoryStream constructor on line 57?  If so, that hasn't
> happened yet, so this data->unref() will free data.  You have to wait until
> after line 57.

Done.

https://codereview.appspot.com/7068055/diff/151001/src/pdf/SkPDFImageStream.c...
src/pdf/SkPDFImageStream.cpp:80: 
On 2013/04/18 18:54:27, Steve VanDeBogart wrote:
> nit: remove line

Done.

https://codereview.appspot.com/7068055/diff/151001/src/pdf/SkPDFImageStream.h
File src/pdf/SkPDFImageStream.h (right):

https://codereview.appspot.com/7068055/diff/151001/src/pdf/SkPDFImageStream.h...
src/pdf/SkPDFImageStream.h:46: enum StreamCompression {
On 2013/04/18 18:54:27, Steve VanDeBogart wrote:
> nit: remove, not used.

Done.

https://codereview.appspot.com/7068055/diff/151001/src/pdf/SkPDFStream.h
File src/pdf/SkPDFStream.h (right):

https://codereview.appspot.com/7068055/diff/151001/src/pdf/SkPDFStream.h#newc...
src/pdf/SkPDFStream.h:56: enum State {
On 2013/04/18 18:54:27, Steve VanDeBogart wrote:
> nit: move to line 47.

Done.

https://codereview.appspot.com/7068055/diff/151001/tests/PDFPrimitivesTest.cpp
File tests/PDFPrimitivesTest.cpp (right):

https://codereview.appspot.com/7068055/diff/151001/tests/PDFPrimitivesTest.cp...
tests/PDFPrimitivesTest.cpp:42: static bool encodeToDCTStream(SkWStream* stream,
const SkBitmap& bitmap, const SkIRect& rect) {
On 2013/04/18 18:54:27, Steve VanDeBogart wrote:
> encode_to_dct_stream()

Done.

https://codereview.appspot.com/7068055/diff/151001/tools/render_pdfs_main.cpp
File tools/render_pdfs_main.cpp (right):

https://codereview.appspot.com/7068055/diff/151001/tools/render_pdfs_main.cpp...
tools/render_pdfs_main.cpp:82: static bool encodeToDCTStream(SkWStream* stream,
const SkBitmap& bitmap, const SkIRect& rect) {
On 2013/04/18 18:54:27, Steve VanDeBogart wrote:
> encode_to_dct_stream

Done.

https://codereview.appspot.com/7068055/diff/151001/tools/render_pdfs_main.cpp...
tools/render_pdfs_main.cpp:82: static bool encodeToDCTStream(SkWStream* stream,
const SkBitmap& bitmap, const SkIRect& rect) {
Not now, but I can create a new util file only with this function.
But the code might diverge in future because it is used by different callers
(test can simply return an empty string, render pdf and gm might have other
quirks)
If you think it helps, I can do it, but I prefer the code duplicated


On 2013/04/18 18:54:27, Steve VanDeBogart wrote:
> This block of code is repeated a number of times.  Is there some utility file
> you could put it into?

[Steve VanDeBogart, 2013-04-18 21:49:41]

LGTM

https://codereview.appspot.com/7068055/diff/151001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/7068055/diff/151001/include/pdf/SkPDFDevice.h#...
include/pdf/SkPDFDevice.h:132: *  @param encoder The encoder to encode a bitmap
as JPEG (DCTDeflate).
On 2013/04/18 18:54:27, Steve VanDeBogart wrote:
> nit: (DCT) not (DCTDeflate)

Missed

https://codereview.appspot.com/7068055/diff/151001/tools/render_pdfs_main.cpp
File tools/render_pdfs_main.cpp (right):

https://codereview.appspot.com/7068055/diff/151001/tools/render_pdfs_main.cpp...
tools/render_pdfs_main.cpp:82: static bool encodeToDCTStream(SkWStream* stream,
const SkBitmap& bitmap, const SkIRect& rect) {
On 2013/04/18 20:13:16, edisonn wrote:
> Not now, but I can create a new util file only with this function.
> But the code might diverge in future because it is used by different callers
> (test can simply return an empty string, render pdf and gm might have other
> quirks)
> If you think it helps, I can do it, but I prefer the code duplicated
> 
> 
> On 2013/04/18 18:54:27, Steve VanDeBogart wrote:
> > This block of code is repeated a number of times.  Is there some utility
file
> > you could put it into?
> 

In general, code duplication makes things harder to maintain. If you fix a bug
or add a feature in one version, you have to remember to make that change in all
the different versions.  I would strongly suggest that you pull out the common
code.  Either now, or in a follow up CL.

[edisonn, 2013-04-24 13:01:09]

Committed patchset #39 manually as r8835 (presubmit successful).