stop calling SkBitmap::flatten

stop calling SkBitmap::flatten

BUG=skia:

Committed: http://code.google.com/p/skia/source/detail?r=14867

[scroggo, 2014-05-20 13:27:50]

Can we go ahead and remove SkPixelRef::flatten/::unflatten? (Or are you leaving
that to its own CL?) We should note in SkPicture::serialize()'s comment that
bitmaps' pixels will be thrown away if the passed in EncodeBitmap is NULL and
the bitmaps don't contain encoded data (are we sure all callers are okay with
that behavior?).

Have you tested GM? stream_to_new_picture() does not pass an encoder, and
appears to compare the deserialized picture to the original. If there were any
bitmaps, this test would now fail.

Should SkPicture::willPlayBackBitmaps() return false if all its bitmaps have no
pixels?

https://codereview.chromium.org/295793002/diff/1/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.chromium.org/295793002/diff/1/include/core/SkBitmap.h#newc...
include/core/SkBitmap.h:703: #ifdef SK_SUPPORT_LEGACY_BITMAPFLATTEN
Could you move these before the comment, so the comment goes with the functions
it describes?

https://codereview.chromium.org/295793002/diff/1/include/core/SkBitmap.h#newc...
include/core/SkBitmap.h:708: void flatten(SkWriteBuffer&) const;
Can't we remove this entirely when SK_SUPPORT_LEGACY_BITMAPFLATTEN is not
defined?

https://codereview.chromium.org/295793002/diff/1/include/core/SkReadBuffer.h
File include/core/SkReadBuffer.h (right):

https://codereview.chromium.org/295793002/diff/1/include/core/SkReadBuffer.h#...
include/core/SkReadBuffer.h:141: virtual bool readBitmap(SkBitmap* bitmap);
If SkValidatingBuffer no longer overrides this, does it need to be virtual?

https://codereview.chromium.org/295793002/diff/1/src/core/SkReadBuffer.cpp
File src/core/SkReadBuffer.cpp (right):

https://codereview.chromium.org/295793002/diff/1/src/core/SkReadBuffer.cpp#ne...
src/core/SkReadBuffer.cpp:267: if (this->pictureVersion() > 0 &&
this->pictureVersion() < 27) {
Is there a valid reason for pictureVersion to be <= 0? Would it not break in
other ways?

https://codereview.chromium.org/295793002/diff/1/src/core/SkValidatingReadBuf...
File src/core/SkValidatingReadBuffer.cpp (left):

https://codereview.chromium.org/295793002/diff/1/src/core/SkValidatingReadBuf...
src/core/SkValidatingReadBuffer.cpp:219: if (!this->validate(!useBitmapHeap &&
(0 == length))) {
I don't understand what this code was doing before. If we were using the bitmap
heap, we go on to call unflatten?

Why is it okay to remove this function entirely? Do we no longer need to
validate bitmaps? They can still be written, correct? Just not using flatten.

[reed1, 2014-05-20 17:49:58]

https://codereview.chromium.org/295793002/diff/1/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.chromium.org/295793002/diff/1/include/core/SkBitmap.h#newc...
include/core/SkBitmap.h:703: #ifdef SK_SUPPORT_LEGACY_BITMAPFLATTEN
On 2014/05/20 13:27:50, scroggo wrote:
> Could you move these before the comment, so the comment goes with the
functions
> it describes?

Done.

https://codereview.chromium.org/295793002/diff/1/include/core/SkBitmap.h#newc...
include/core/SkBitmap.h:708: void flatten(SkWriteBuffer&) const;
On 2014/05/20 13:27:50, scroggo wrote:
> Can't we remove this entirely when SK_SUPPORT_LEGACY_BITMAPFLATTEN is not
> defined?

Done.

https://codereview.chromium.org/295793002/diff/1/include/core/SkReadBuffer.h
File include/core/SkReadBuffer.h (right):

https://codereview.chromium.org/295793002/diff/1/include/core/SkReadBuffer.h#...
include/core/SkReadBuffer.h:141: virtual bool readBitmap(SkBitmap* bitmap);
On 2014/05/20 13:27:50, scroggo wrote:
> If SkValidatingBuffer no longer overrides this, does it need to be virtual?

Done.

https://codereview.chromium.org/295793002/diff/1/src/core/SkReadBuffer.cpp
File src/core/SkReadBuffer.cpp (right):

https://codereview.chromium.org/295793002/diff/1/src/core/SkReadBuffer.cpp#ne...
src/core/SkReadBuffer.cpp:267: if (this->pictureVersion() > 0 &&
this->pictureVersion() < 27) {
On 2014/05/20 13:27:50, scroggo wrote:
> Is there a valid reason for pictureVersion to be <= 0? Would it not break in
> other ways?

When version is 0, we are an un-versioned buffer (i.e. in ram, not read back
from a disk). This oddness is hidden in the rebased version where we always call
isVersionLT()

https://codereview.chromium.org/295793002/diff/1/src/core/SkValidatingReadBuf...
File src/core/SkValidatingReadBuffer.cpp (left):

https://codereview.chromium.org/295793002/diff/1/src/core/SkValidatingReadBuf...
src/core/SkValidatingReadBuffer.cpp:219: if (!this->validate(!useBitmapHeap &&
(0 == length))) {
On 2014/05/20 13:27:50, scroggo wrote:
> I don't understand what this code was doing before. If we were using the
bitmap
> heap, we go on to call unflatten?
> 
> Why is it okay to remove this function entirely? Do we no longer need to
> validate bitmaps? They can still be written, correct? Just not using flatten.

1. I agree that it seems broken, and probably wasn't getting used for the
limited cases where we test validatingread
2. I don't think this did much "validating" anyway. If we decide later to test
and beef-up in this area, we can rewrite a correct version that does more sanity
checks.

[scroggo, 2014-05-20 18:49:45]

On 2014/05/20 17:49:58, reed1 wrote:
> https://codereview.chromium.org/295793002/diff/1/include/core/SkBitmap.h
> File include/core/SkBitmap.h (right):
> 
>
https://codereview.chromium.org/295793002/diff/1/include/core/SkBitmap.h#newc...
> include/core/SkBitmap.h:703: #ifdef SK_SUPPORT_LEGACY_BITMAPFLATTEN
> On 2014/05/20 13:27:50, scroggo wrote:
> > Could you move these before the comment, so the comment goes with the
> functions
> > it describes?
> 
> Done.
> 
>
https://codereview.chromium.org/295793002/diff/1/include/core/SkBitmap.h#newc...
> include/core/SkBitmap.h:708: void flatten(SkWriteBuffer&) const;
> On 2014/05/20 13:27:50, scroggo wrote:
> > Can't we remove this entirely when SK_SUPPORT_LEGACY_BITMAPFLATTEN is not
> > defined?
> 
> Done.
> 
> https://codereview.chromium.org/295793002/diff/1/include/core/SkReadBuffer.h
> File include/core/SkReadBuffer.h (right):
> 
>
https://codereview.chromium.org/295793002/diff/1/include/core/SkReadBuffer.h#...
> include/core/SkReadBuffer.h:141: virtual bool readBitmap(SkBitmap* bitmap);
> On 2014/05/20 13:27:50, scroggo wrote:
> > If SkValidatingBuffer no longer overrides this, does it need to be virtual?
> 
> Done.
> 
> https://codereview.chromium.org/295793002/diff/1/src/core/SkReadBuffer.cpp
> File src/core/SkReadBuffer.cpp (right):
> 
>
https://codereview.chromium.org/295793002/diff/1/src/core/SkReadBuffer.cpp#ne...
> src/core/SkReadBuffer.cpp:267: if (this->pictureVersion() > 0 &&
> this->pictureVersion() < 27) {
> On 2014/05/20 13:27:50, scroggo wrote:
> > Is there a valid reason for pictureVersion to be <= 0? Would it not break in
> > other ways?
> 
> When version is 0, we are an un-versioned buffer (i.e. in ram, not read back
> from a disk). This oddness is hidden in the rebased version where we always
call
> isVersionLT()
> 
>
https://codereview.chromium.org/295793002/diff/1/src/core/SkValidatingReadBuf...
> File src/core/SkValidatingReadBuffer.cpp (left):
> 
>
https://codereview.chromium.org/295793002/diff/1/src/core/SkValidatingReadBuf...
> src/core/SkValidatingReadBuffer.cpp:219: if (!this->validate(!useBitmapHeap &&
> (0 == length))) {
> On 2014/05/20 13:27:50, scroggo wrote:
> > I don't understand what this code was doing before. If we were using the
> bitmap
> > heap, we go on to call unflatten?
> > 
> > Why is it okay to remove this function entirely? Do we no longer need to
> > validate bitmaps? They can still be written, correct? Just not using
flatten.
> 
> 1. I agree that it seems broken, and probably wasn't getting used for the
> limited cases where we test validatingread
> 2. I don't think this did much "validating" anyway. If we decide later to test
> and beef-up in this area, we can rewrite a correct version that does more
sanity
> checks.

Did you mean to upload patch set 2?

[reed1, 2014-05-20 20:04:13]

new patchset uploaded

[scroggo, 2014-05-20 20:58:22]

On 2014/05/20 20:04:13, reed1 wrote:
> new patchset uploaded

1gtm, once you update stream_to_new_picture() to pass an encode function (or
otherwise fix those failures).

Any thoughts on SkPicture::willPlayBackBitmaps()?

[reed1, 2014-05-20 21:57:41]

ptal

[scroggo, 2014-05-21 14:47:49]

On 2014/05/20 21:57:41, reed1 wrote:
> ptal

lgtm.

Worth writing tests? Or do you think gm's serialization will be enough?

[scroggo, 2014-05-21 14:48:01]

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

https://codereview.chromium.org/295793002/diff/80001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1278: char* srcBytes = (char*)storage.get();
srcBytes is confusing here. How about dstBytes?

https://codereview.chromium.org/295793002/diff/80001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1310: char* srcBytes = (char*)sk_malloc_throw(ramSize);
Again, I find srcBytes confusing here. dstBytes? ramBytes?

[hal.canary, 2014-05-21 15:18:11]

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

https://codereview.chromium.org/295793002/diff/80001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1295: SkPixelRef* SkBitmap::ReadRawPixels(SkReadBuffer*
buffer) {
void SkBitmap::WriteRawPixels(SkWriteBuffer* buffer, const SkBitmap& bitmap);
SkPixelRef* SkBitmap::ReadRawPixels(SkReadBuffer* buffer);

Why are these asymmetric?  Why not modify a destination bitmap?

[hal.canary, 2014-05-21 15:20:29]

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

https://codereview.chromium.org/295793002/diff/80001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1304: const size_t ramRB = info.minRowBytes();
When is ramRB != snugRB?

[reed1, 2014-05-21 20:33:03]

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

https://codereview.chromium.org/295793002/diff/80001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1278: char* srcBytes = (char*)storage.get();
On 2014/05/21 14:48:01, scroggo wrote:
> srcBytes is confusing here. How about dstBytes?

Done.

https://codereview.chromium.org/295793002/diff/80001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1295: SkPixelRef* SkBitmap::ReadRawPixels(SkReadBuffer*
buffer) {
On 2014/05/21 15:18:11, Hal Canary wrote:
> void SkBitmap::WriteRawPixels(SkWriteBuffer* buffer, const SkBitmap& bitmap);
> SkPixelRef* SkBitmap::ReadRawPixels(SkReadBuffer* buffer);
> 
> Why are these asymmetric?  Why not modify a destination bitmap?

Done.

https://codereview.chromium.org/295793002/diff/80001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1304: const size_t ramRB = info.minRowBytes();
On 2014/05/21 15:20:29, Hal Canary wrote:
> When is ramRB != snugRB?

snugRB is the smallest that they *could* be (used for storage). ramRB could
including padding, which is perfectly allowed in RAM.

https://codereview.chromium.org/295793002/diff/80001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1310: char* srcBytes = (char*)sk_malloc_throw(ramSize);
On 2014/05/21 14:48:01, scroggo wrote:
> Again, I find srcBytes confusing here. dstBytes? ramBytes?

Done.

[hal.canary, 2014-05-21 20:39:49]

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

https://codereview.chromium.org/295793002/diff/80001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1304: const size_t ramRB = info.minRowBytes();
On 2014/05/21 20:33:04, reed1 wrote:
> On 2014/05/21 15:20:29, Hal Canary wrote:
> > When is ramRB != snugRB?
> 
> snugRB is the smallest that they *could* be (used for storage). ramRB could
> including padding, which is perfectly allowed in RAM.

But you set `ramRB = info.minRowBytes()`, which is width * bytesPerPixel, which
is what you selected for snugRB.  I still don't see how they could be unequal.

[scroggo, 2014-05-21 20:42:32]

On 2014/05/21 20:33:03, reed1 wrote:
> https://codereview.chromium.org/295793002/diff/80001/src/core/SkBitmap.cpp
> File src/core/SkBitmap.cpp (right):
> 
>
https://codereview.chromium.org/295793002/diff/80001/src/core/SkBitmap.cpp#ne...
> src/core/SkBitmap.cpp:1278: char* srcBytes = (char*)storage.get();
> On 2014/05/21 14:48:01, scroggo wrote:
> > srcBytes is confusing here. How about dstBytes?
> 
> Done.
> 
>
https://codereview.chromium.org/295793002/diff/80001/src/core/SkBitmap.cpp#ne...
> src/core/SkBitmap.cpp:1295: SkPixelRef* SkBitmap::ReadRawPixels(SkReadBuffer*
> buffer) {
> On 2014/05/21 15:18:11, Hal Canary wrote:
> > void SkBitmap::WriteRawPixels(SkWriteBuffer* buffer, const SkBitmap&
bitmap);
> > SkPixelRef* SkBitmap::ReadRawPixels(SkReadBuffer* buffer);
> > 
> > Why are these asymmetric?  Why not modify a destination bitmap?
> 
> Done.
> 
>
https://codereview.chromium.org/295793002/diff/80001/src/core/SkBitmap.cpp#ne...
> src/core/SkBitmap.cpp:1304: const size_t ramRB = info.minRowBytes();
> On 2014/05/21 15:20:29, Hal Canary wrote:
> > When is ramRB != snugRB?
> 
> snugRB is the smallest that they *could* be (used for storage). ramRB could
> including padding, which is perfectly allowed in RAM.
> 
>
https://codereview.chromium.org/295793002/diff/80001/src/core/SkBitmap.cpp#ne...
> src/core/SkBitmap.cpp:1310: char* srcBytes = (char*)sk_malloc_throw(ramSize);
> On 2014/05/21 14:48:01, scroggo wrote:
> > Again, I find srcBytes confusing here. dstBytes? ramBytes?
> 
> Done.

lgtm

[reed1, 2014-05-23 13:16:45]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-05-23 13:16:55]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/reed@google.com/295793002/130001

[commit-bot: I haz the power, 2014-05-23 13:21:59]

Change committed as 14867