Allow PNG encoding/decoding during SkPicture serialization.

Add the ability to provide function pointers to SkPicture serialization
and deserialization for encoding and decoding bitmaps.

Remove kForceFlattenBitmapPixels_Flag, which is no longer used.

When an SkOrderedReadBuffer needs to read a bitmap, if it does not
have an image decoder, use a dummy bitmap.

In GM, add a tolerance option for color differences, used when
testing picture serialization, so it can assume two images are the
same even though PNG encoding/decoding may have resulted in small
differences.

Create dummy implementations for SkImageDecoder and SkImageEncoder
functions in SkImageDecoder_empty so that a project that does not
want to include the images project it can still build.

Allow ports to build without images project.

In Mac's image encoder, copy 4444 to 8888 before encoding.

Add SkWriter32::reservePad, to provide a pointer to write non 4 byte
aligned data, padded with zeroes.

In bench_ and render_ pictures, pass decode function to SkPicture
creation from a stream.

BUG=https://code.google.com/p/skia/issues/detail?id=842

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

[Leon, 2012-09-24 20:23:50]

https://codereview.appspot.com/6551071/diff/1/include/images/SkImages.h
File include/images/SkImages.h (left):

https://codereview.appspot.com/6551071/diff/1/include/images/SkImages.h#oldco...
include/images/SkImages.h:11: const GrGLInterface*
SkDebugGLContext::createGLContext() {
I am not sure why Rietveld is showing this. Looking at
https://code.google.com/p/skia/source/browse I do not see such a file (and it
was not my intention to modify one).

[reed1, 2012-09-24 20:52:18]

Will comment separately on how we can make the Codecs available to our
core-code...

https://codereview.appspot.com/6551071/diff/1001/src/core/SkOrderedWriteBuffe...
File src/core/SkOrderedWriteBuffer.cpp (right):

https://codereview.appspot.com/6551071/diff/1001/src/core/SkOrderedWriteBuffe...
src/core/SkOrderedWriteBuffer.cpp:139: bool encoded = false;
Would be nice if A8 just failed inside the encoder, so that explicit knowledge
didn't need to be here.

https://codereview.appspot.com/6551071/diff/1001/src/core/SkOrderedWriteBuffe...
src/core/SkOrderedWriteBuffer.cpp:160: // Indicates that the bitmap was encoded
successfully.
Perhaps we can always write a length (+ width , height), and if length == 0,
then we assume no compression? (instead of also having a bool)

https://codereview.appspot.com/6551071/diff/1001/src/core/SkOrderedWriteBuffe...
src/core/SkOrderedWriteBuffer.cpp:166: size_t length = pngStream.getOffset();
Can you use pngStream.copyTo(this->reserve(length)) to avoid the extra copy in
memStream?

[DerekS, 2012-09-24 20:53:18]

https://codereview.appspot.com/6551071/diff/1001/src/core/SkOrderedReadBuffer...
File src/core/SkOrderedReadBuffer.cpp (right):

https://codereview.appspot.com/6551071/diff/1001/src/core/SkOrderedReadBuffer...
src/core/SkOrderedReadBuffer.cpp:168: if (this->readBool()) {
instead of a bool should we write an enum/int so that if we want to add more
formats later like webp we don't have to break compatibility? what are your
thoughts on this?

https://codereview.appspot.com/6551071/diff/1001/src/core/SkOrderedReadBuffer...
src/core/SkOrderedReadBuffer.cpp:179: bitmap->eraseColor(SK_ColorRED);
I think we may want to print a debug message here.

[reed1, 2012-09-24 21:06:53]

Unorganized thoughts on Codecs and how they hook in:

1. Writer could stick a function-ptr in the FlattenableBuffer directly...
    - removes direct linkage with SkImageEncoder
    - allows for non-PNG formats

2. Reader could stick a factory-ptr in the FlattenableBuffer directly...
    - removes direct linkage with SkImageDecoder

or

1. What you have is fine, since
    - PNG is pretty darn good
    - The caller can always link against our SkImageDecoder_none.cpp

[Leon, 2012-09-25 16:20:35]

> Unorganized thoughts on Codecs and how they hook in:
> 
> 1. Writer could stick a function-ptr in the FlattenableBuffer directly...
>     - removes direct linkage with SkImageEncoder
>     - allows for non-PNG formats
> 
> 2. Reader could stick a factory-ptr in the FlattenableBuffer directly...
>     - removes direct linkage with SkImageDecoder
> 
> or
> 
> 1. What you have is fine, since
>     - PNG is pretty darn good
>     - The caller can always link against our SkImageDecoder_none.cpp

I began implementing it that way, but the downside to that approach is
that the reader is required to pass the decoding function (in addition to
building with the images project) in order to get the proper result if it
was encoded on recording. If we still think that's better (since it removes
the dependence on headers in the images project and the requirement
of linking against SkImageDecoder_empty in order to build) I can do it
that way instead.

https://codereview.appspot.com/6551071/diff/1001/src/core/SkOrderedReadBuffer...
File src/core/SkOrderedReadBuffer.cpp (right):

https://codereview.appspot.com/6551071/diff/1001/src/core/SkOrderedReadBuffer...
src/core/SkOrderedReadBuffer.cpp:168: if (this->readBool()) {
On 2012/09/24 20:53:18, DerekS wrote:
> instead of a bool should we write an enum/int so that if we want to add more
> formats later like webp we don't have to break compatibility? what are your
> thoughts on this?

I don't think that's necessary since the reader does not need to know the type
of encoding. We just call DecodeStream, which figures out the proper encoding.

https://codereview.appspot.com/6551071/diff/1001/src/core/SkOrderedReadBuffer...
src/core/SkOrderedReadBuffer.cpp:179: bitmap->eraseColor(SK_ColorRED);
On 2012/09/24 20:53:18, DerekS wrote:
> I think we may want to print a debug message here.

Done.

https://codereview.appspot.com/6551071/diff/1001/src/core/SkOrderedWriteBuffe...
File src/core/SkOrderedWriteBuffer.cpp (right):

https://codereview.appspot.com/6551071/diff/1001/src/core/SkOrderedWriteBuffe...
src/core/SkOrderedWriteBuffer.cpp:139: bool encoded = false;
On 2012/09/24 20:52:18, reed1 wrote:
> Would be nice if A8 just failed inside the encoder, so that explicit knowledge
> didn't need to be here.

I just realized that the reason A8 fails is not due to the encoder, but because
I attempt to copy it to an ARGB_8888 bitmap first, which is necessary for
ARGB_4444, but breaks A8. I have moved the copy (only for 4444) into the
encoder.

https://codereview.appspot.com/6551071/diff/1001/src/core/SkOrderedWriteBuffe...
src/core/SkOrderedWriteBuffer.cpp:160: // Indicates that the bitmap was encoded
successfully.
On 2012/09/24 20:52:18, reed1 wrote:
> Perhaps we can always write a length (+ width , height), and if length == 0,
> then we assume no compression? (instead of also having a bool)

Done.

https://codereview.appspot.com/6551071/diff/1001/src/core/SkOrderedWriteBuffe...
src/core/SkOrderedWriteBuffer.cpp:166: size_t length = pngStream.getOffset();
On 2012/09/24 20:52:18, reed1 wrote:
> Can you use pngStream.copyTo(this->reserve(length)) to avoid the extra copy in
> memStream?

Done.

[DerekS, 2012-09-25 17:27:03]

I'm ok with the current way of converting the bitmap to PNG and if we don't like
the results we can always change to a different format going forward.  In any
case, we can introduce new formats later and still be backward compatible as the
reader doesn't need to explicitly know the encoding format.

[Leon, 2012-09-27 18:25:47]

Latest patch allows the reader and writer to provide function pointers for
encoding and decoding.

[reed1, 2012-09-27 18:28:52]

I like, at least as something we can try out. Some questions for dox

https://codereview.appspot.com/6551071/diff/9004/include/core/SkSerialization...
File include/core/SkSerializationHelpers.h (right):

https://codereview.appspot.com/6551071/diff/9004/include/core/SkSerialization...
include/core/SkSerializationHelpers.h:16: namespace SkSerializationHelpers {
// What does the bool mean? For decode, I presume it means success/failure, and
if failure, we just skip that image (or perhaps try some other "default"
decoder. For encode, does it mean (1) skip this image, (2) try a default encoder
(if any), (3) store raw, (4) other?

[Leon, 2012-09-27 19:17:53]

https://codereview.appspot.com/6551071/diff/9004/include/core/SkSerialization...
File include/core/SkSerializationHelpers.h (right):

https://codereview.appspot.com/6551071/diff/9004/include/core/SkSerialization...
include/core/SkSerializationHelpers.h:16: namespace SkSerializationHelpers {
On 2012/09/27 18:28:52, reed1 wrote:
> // What does the bool mean? For decode, I presume it means success/failure,
and
> if failure, we just skip that image (or perhaps try some other "default"
> decoder. For encode, does it mean (1) skip this image, (2) try a default
encoder
> (if any), (3) store raw, (4) other?

Added a comment.

[reed1, 2012-09-27 20:26:55]

thanks for the dox.

Thinking more, we may want to change it later to allow some state for the
caller, so a base-class of some kind will be more useful than just a
function-ptr, but I think this is a fine start.

lgtm