New API for encoding bitmaps during serialization.

New API for encoding bitmaps during serialization.

This change gives more flexibility to the implementation of
EncodeBitmap to prefer calling refEncodedData, doing its own
encode, or even doing both and making a decision about which
to use.

The new function signature also allows the implementation to
tell the ordered write buffer whether to store the pixel offset,
in the case where the encoded bitmap represents the larger
bitmap, or to ignore the pixel offset, in the case where the
implementation only encoded the subset that is used.

Requires changes to chromium to use the new function signature.
(https://codereview.chromium.org/15496006/)

SkPicture:
New API for EncodeBitmap. 

SkOrderedReadBuffer:
Ifdef'd out addition of reading the offset.

SkOrderedWriteBuffer:
Never call refEncodedData. Allow the user to call that from their
EncodeBitmap function, if desired.
This addresses https://code.google.com/p/skia/issues/detail?id=1239
Add in ifdef'd out code to record the offset.

PictureTest and PictureRenderer:
Implement the new definition of EncodeBitmap. Also update the name
of the function to meet coding style guidelines.

BUG=https://code.google.com/p/skia/issues/detail?id=1239
R=reed@google.com

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

[scroggo, 2013-05-20 19:50:52]

https://codereview.chromium.org/15489004/diff/2001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/15489004/diff/2001/include/core/SkPicture.h#n...
include/core/SkPicture.h:193: typedef SkData*
(*EncodeBitmap)(RecordPixelRefOffset*, const SkBitmap&);
Mike, you mentioned perhaps wanting to use the same signature in other places.
In that case, I think RecordPixelRefOffset should be changed to something more
general. Maybe ContainsLargerImage, or something that makes it clear that the
SkData represents a superset of the pixels used by the SkBitmap.

[reed1, 2013-05-20 20:08:56]

I like the direction. Is it as clear or clearer to return the "new" offset
instead of an enum?

[scroggo, 2013-05-20 22:47:44]

On 2013/05/20 20:08:56, reed1 wrote:
> I like the direction. Is it as clear or clearer to return the "new" offset
> instead of an enum?

Definitely clearer. I do think it is still a little bit confusing, since the
pixelRefOffset is an offset into the bitmap being encoded: the offset in the
deserialized bitmap may be different, if the decoder decoded differently (e.g.
used a different rowbytes/config etc). Let me know if the comment is clear
enough.

[djsollen, 2013-05-21 14:10:43]

https://codereview.chromium.org/15489004/diff/12001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/15489004/diff/12001/include/core/SkPicture.h#...
include/core/SkPicture.h:178: *  @param pixelRefOffset Output parameter, telling
the deserializer what
what happens if this is null? Also we usually put output params after input
params.

https://codereview.chromium.org/15489004/diff/12001/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (right):

https://codereview.chromium.org/15489004/diff/12001/src/core/SkBitmap.cpp#new...
src/core/SkBitmap.cpp:907: static bool getUpperLeftFromOffset(const SkBitmap&
bm, int32_t* x, int32_t* y) {
according to our style guide static methods should be lower case with
underscores separating words.

[scroggo, 2013-05-21 16:14:15]

https://codereview.chromium.org/15489004/diff/12001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/15489004/diff/12001/include/core/SkPicture.h#...
include/core/SkPicture.h:178: *  @param pixelRefOffset Output parameter, telling
the deserializer what
On 2013/05/21 14:10:44, djsollen wrote:
> what happens if this is null?

This is a function signature, so it will depend on the caller and
implementation. In the functions that take EncodeBitmap as a parameter, I have
added comments that they will never call the supplied function pointer with a
NULL pixelRefOffset. My implementations do not check for NULL, since the place
they are used will never have a NULL value passed in.

> Also we usually put output params after input params.

We seem to be pretty inconsistent about this one, and it is not in the style
guide.

https://codereview.chromium.org/15489004/diff/12001/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (right):

https://codereview.chromium.org/15489004/diff/12001/src/core/SkBitmap.cpp#new...
src/core/SkBitmap.cpp:907: static bool getUpperLeftFromOffset(const SkBitmap&
bm, int32_t* x, int32_t* y) {
On 2013/05/21 14:10:44, djsollen wrote:
> according to our style guide static methods should be lower case with
> underscores separating words.

Done.

[scroggo, 2013-05-21 18:15:59]

On 2013/05/21 16:14:15, scroggo wrote:
> https://codereview.chromium.org/15489004/diff/12001/include/core/SkPicture.h
> File include/core/SkPicture.h (right):
> 
>
https://codereview.chromium.org/15489004/diff/12001/include/core/SkPicture.h#...
> include/core/SkPicture.h:178: *  @param pixelRefOffset Output parameter,
telling
> the deserializer what
> On 2013/05/21 14:10:44, djsollen wrote:
> > what happens if this is null?
> 
> This is a function signature, so it will depend on the caller and
> implementation. In the functions that take EncodeBitmap as a parameter, I have
> added comments that they will never call the supplied function pointer with a
> NULL pixelRefOffset. My implementations do not check for NULL, since the place
> they are used will never have a NULL value passed in.
> 
> > Also we usually put output params after input params.
> 
> We seem to be pretty inconsistent about this one, and it is not in the style
> guide.
> 
> https://codereview.chromium.org/15489004/diff/12001/src/core/SkBitmap.cpp
> File src/core/SkBitmap.cpp (right):
> 
>
https://codereview.chromium.org/15489004/diff/12001/src/core/SkBitmap.cpp#new...
> src/core/SkBitmap.cpp:907: static bool getUpperLeftFromOffset(const SkBitmap&
> bm, int32_t* x, int32_t* y) {
> On 2013/05/21 14:10:44, djsollen wrote:
> > according to our style guide static methods should be lower case with
> > underscores separating words.
> 
> Done.

New patch available.

[reed1, 2013-05-21 18:30:47]

lgtm

https://codereview.chromium.org/15489004/diff/24002/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/15489004/diff/24002/include/core/SkPicture.h#...
include/core/SkPicture.h:183: typedef SkData* (*EncodeBitmap)(size_t*
pixelRefOffset, const SkBitmap& bm);
mike nit: I like the output-param to be last, but just a nit

[scroggo, 2013-05-21 20:31:32]

Committed patchset #5 manually as r9226 (presubmit successful).