change old picture serialization to really handle images

change old picture serialization to really handle images

BUG=skia:3965
Committed: https://skia.googlesource.com/skia/+/871872f3f247f6b699617f6d9ef50ef5da6fbe74

[reed1, 2015-06-19 16:09:07]

reed@google.com changed reviewers:
+ bsalomon@google.com, mtklein@google.com, robertphillips@google.com,
scroggo@google.com

[robertphillips, 2015-06-19 17:24:14]

lgtm but I would prefer others to weigh in

https://codereview.chromium.org/1199473002/diff/20001/gm/factory.cpp
File gm/factory.cpp (right):

https://codereview.chromium.org/1199473002/diff/20001/gm/factory.cpp#newcode63
gm/factory.cpp:63: 
DEF_GM while you're in here ?

[reed1, 2015-06-19 20:20:40]

The CQ bit was checked by reed@google.com to run a CQ dry run

[reed1, 2015-06-19 20:20:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1199473002/#ps70001
(title: "add test for subset (and fix bugs)")

[commit-bot: I haz the power, 2015-06-19 20:20:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1199473002/70001

[reed1, 2015-06-19 20:22:00]

The CQ bit was unchecked by reed@google.com

[reed1, 2015-06-19 20:25:46]

The CQ bit was checked by reed@google.com to run a CQ dry run

[reed1, 2015-06-19 20:25:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1199473002/#ps110001
(title: " ")

[commit-bot: I haz the power, 2015-06-19 20:26:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1199473002/110001

[reed1, 2015-06-19 20:26:35]

The CQ bit was unchecked by reed@google.com

[reed1, 2015-06-19 20:52:41]

The CQ bit was checked by reed@google.com to run a CQ dry run

[reed1, 2015-06-19 20:52:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1199473002/#ps130001
(title: "disable opaque check, and filed skbug.com/3965")

[commit-bot: I haz the power, 2015-06-19 20:52:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1199473002/130001

[reed1, 2015-06-19 20:57:55]

The CQ bit was unchecked by reed@google.com

[scroggo, 2015-06-22 17:40:01]

lgtm

https://codereview.chromium.org/1199473002/diff/150001/include/core/SkImage.h
File include/core/SkImage.h (right):

https://codereview.chromium.org/1199473002/diff/150001/include/core/SkImage.h...
include/core/SkImage.h:167: *  the true, and if pixmap is not NULL, set it to
point into the image.
nit: "the" at the beginning of the line is not needed.

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

https://codereview.chromium.org/1199473002/diff/150001/src/core/SkBitmap.cpp#...
src/core/SkBitmap.cpp:1273: SkPixelRef::LockRequest req = {
pr->info().dimensions(), kNone_SkFilterQuality };
Why this change?

https://codereview.chromium.org/1199473002/diff/150001/src/core/SkPictureData...
File src/core/SkPictureData.cpp (right):

https://codereview.chromium.org/1199473002/diff/150001/src/core/SkPictureData...
src/core/SkPictureData.cpp:432: if (width < 0 || height < 0) {
I thought images had to have non-zero dimensions?

https://codereview.chromium.org/1199473002/diff/150001/src/core/SkPictureData...
src/core/SkPictureData.cpp:440: if (!encoded || originX < 0 || originY < 0) {
It looks like readByteArrayAsData() always returns non-NULL. Should we instead
check if it is empty?

https://codereview.chromium.org/1199473002/diff/150001/src/core/SkPictureData...
src/core/SkPictureData.cpp:449: static const SkPicture*
create_picture_from_buffer(SkReadBuffer& buffer) {
Can we not just use SkPicture::CreateFromBuffer directly (like we do for
SkTextBlob)?

https://codereview.chromium.org/1199473002/diff/150001/src/core/SkPictureData...
src/core/SkPictureData.cpp:454: bool new_array_from_buffer(SkReadBuffer& buffer,
uint32_t size,
I found this signature confusing until I found the place where it's used. Can we
change the names to distinguish between size and count? Maybe size should be
encodedSize or reportedSize or something?

https://codereview.chromium.org/1199473002/diff/150001/src/core/SkPictureData...
src/core/SkPictureData.cpp:455: const T**& array, int& count, const T*
(*factory)(SkReadBuffer&)) {
nit: Don't we typically use pointers for variables we're going to modify, rather
than non-const references?

https://codereview.chromium.org/1199473002/diff/150001/src/core/SkPictureData...
src/core/SkPictureData.cpp:460: array = SkNEW_ARRAY(const T*, count);
If size is 0, we dutifully create a 0 size array. Should we early exit in that
case?

https://codereview.chromium.org/1199473002/diff/150001/src/core/SkPictureData...
src/core/SkPictureData.cpp:479: return false;
Should we validate(false) the buffer? Or do we assume that factory did that when
it returned NULL?

https://codereview.chromium.org/1199473002/diff/150001/src/lazy/SkDiscardable...
File src/lazy/SkDiscardablePixelRef.cpp (right):

https://codereview.chromium.org/1199473002/diff/150001/src/lazy/SkDiscardable...
src/lazy/SkDiscardablePixelRef.cpp:133: // must compute our desired rowBytes
w.r.t. the pixelRef's dimensions, now ours, which may be
not* ours

[reed1, 2015-06-22 18:41:51]

https://codereview.chromium.org/1199473002/diff/150001/include/core/SkImage.h
File include/core/SkImage.h (right):

https://codereview.chromium.org/1199473002/diff/150001/include/core/SkImage.h...
include/core/SkImage.h:167: *  the true, and if pixmap is not NULL, set it to
point into the image.
On 2015/06/22 17:40:00, scroggo wrote:
> nit: "the" at the beginning of the line is not needed.

Done.

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

https://codereview.chromium.org/1199473002/diff/150001/src/core/SkBitmap.cpp#...
src/core/SkBitmap.cpp:1273: SkPixelRef::LockRequest req = {
pr->info().dimensions(), kNone_SkFilterQuality };
On 2015/06/22 17:40:01, scroggo wrote:
> Why this change?

I'll add a comment -- the pixelref cannot do subset-locks (nor does the API
allow the offset to be specified). The older code was a bug waiting to happen if
in fact the bitmap *is* a subset of the pixelref.

https://codereview.chromium.org/1199473002/diff/150001/src/core/SkPictureData...
File src/core/SkPictureData.cpp (right):

https://codereview.chromium.org/1199473002/diff/150001/src/core/SkPictureData...
src/core/SkPictureData.cpp:432: if (width < 0 || height < 0) {
On 2015/06/22 17:40:01, scroggo wrote:
> I thought images had to have non-zero dimensions?

Done.

https://codereview.chromium.org/1199473002/diff/150001/src/core/SkPictureData...
src/core/SkPictureData.cpp:440: if (!encoded || originX < 0 || originY < 0) {
On 2015/06/22 17:40:01, scroggo wrote:
> It looks like readByteArrayAsData() always returns non-NULL. Should we instead
> check if it is empty?

Done.

https://codereview.chromium.org/1199473002/diff/150001/src/core/SkPictureData...
src/core/SkPictureData.cpp:449: static const SkPicture*
create_picture_from_buffer(SkReadBuffer& buffer) {
On 2015/06/22 17:40:01, scroggo wrote:
> Can we not just use SkPicture::CreateFromBuffer directly (like we do for
> SkTextBlob)?

That is defined to return SkPicture*, but the other methods return const T*, so
I needed a const-wrapper. Will add dox to that effect.

https://codereview.chromium.org/1199473002/diff/150001/src/core/SkPictureData...
src/core/SkPictureData.cpp:454: bool new_array_from_buffer(SkReadBuffer& buffer,
uint32_t size,
On 2015/06/22 17:40:01, scroggo wrote:
> I found this signature confusing until I found the place where it's used. Can
we
> change the names to distinguish between size and count? Maybe size should be
> encodedSize or reportedSize or something?

Done.

https://codereview.chromium.org/1199473002/diff/150001/src/core/SkPictureData...
src/core/SkPictureData.cpp:455: const T**& array, int& count, const T*
(*factory)(SkReadBuffer&)) {
On 2015/06/22 17:40:01, scroggo wrote:
> nit: Don't we typically use pointers for variables we're going to modify,
rather
> than non-const references?

Done.

https://codereview.chromium.org/1199473002/diff/150001/src/core/SkPictureData...
src/core/SkPictureData.cpp:460: array = SkNEW_ARRAY(const T*, count);
On 2015/06/22 17:40:01, scroggo wrote:
> If size is 0, we dutifully create a 0 size array. Should we early exit in that
> case?

Done.

https://codereview.chromium.org/1199473002/diff/150001/src/core/SkPictureData...
src/core/SkPictureData.cpp:479: return false;
On 2015/06/22 17:40:01, scroggo wrote:
> Should we validate(false) the buffer? Or do we assume that factory did that
when
> it returned NULL?

I think we assume the factories did it, if something was invalid. We could be
"valid" but still fail to instantiate a subclass for some other reason. Anyway,
the old logic did it this way.

https://codereview.chromium.org/1199473002/diff/150001/src/lazy/SkDiscardable...
File src/lazy/SkDiscardablePixelRef.cpp (right):

https://codereview.chromium.org/1199473002/diff/150001/src/lazy/SkDiscardable...
src/lazy/SkDiscardablePixelRef.cpp:133: // must compute our desired rowBytes
w.r.t. the pixelRef's dimensions, now ours, which may be
On 2015/06/22 17:40:01, scroggo wrote:
> not* ours

Done.

[reed1, 2015-06-22 18:42:00]

The CQ bit was checked by reed@google.com to run a CQ dry run

[reed1, 2015-06-22 18:42:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com, scroggo@google.com
Link to the patchset: https://codereview.chromium.org/1199473002/#ps170001
(title: "address scroggo comments")

[commit-bot: I haz the power, 2015-06-22 18:42:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1199473002/170001

[reed1, 2015-06-22 18:43:24]

The CQ bit was unchecked by reed@google.com

[scroggo, 2015-06-22 18:48:05]

https://codereview.chromium.org/1199473002/diff/170001/src/core/SkPictureData...
File src/core/SkPictureData.cpp (right):

https://codereview.chromium.org/1199473002/diff/170001/src/core/SkPictureData...
src/core/SkPictureData.cpp:458: if (!buffer.validate((0 == inCount) && (NULL ==
array))) {
This part I found confusing - you actually do want to check that outCount is 0
(at least that's what the old code did).

[reed1, 2015-06-22 19:19:35]

https://codereview.chromium.org/1199473002/diff/170001/src/core/SkPictureData...
File src/core/SkPictureData.cpp (right):

https://codereview.chromium.org/1199473002/diff/170001/src/core/SkPictureData...
src/core/SkPictureData.cpp:458: if (!buffer.validate((0 == inCount) && (NULL ==
array))) {
On 2015/06/22 18:48:05, scroggo wrote:
> This part I found confusing - you actually do want to check that outCount is 0
> (at least that's what the old code did).

Yea, that's odd. I'll change it to do that.

[reed1, 2015-06-22 19:33:26]

The CQ bit was checked by reed@google.com to run a CQ dry run

[reed1, 2015-06-22 19:33:26]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com,
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1199473002/#ps190001
(title: "fix check in new_array function")

[commit-bot: I haz the power, 2015-06-22 19:33:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1199473002/190001

[reed1, 2015-06-22 19:39:28]

The CQ bit was unchecked by reed@google.com

[reed1, 2015-06-22 19:46:11]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2015-06-22 19:46:31]

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

[commit-bot: I haz the power, 2015-06-22 19:48:30]

Committed patchset #11 (id:190001) as
https://skia.googlesource.com/skia/+/871872f3f247f6b699617f6d9ef50ef5da6fbe74

[mtklein, 2015-06-22 21:12:33]

Hey, this is weird.  If you have the SKP sp_desk_carsvg.skp laying around, try
running it through dm or nanobench?  Herb and I are seeing it crash after this
CL but not before it.

The problem appears to be failing to deserialize an image filter at the right
byte size:

frame #1: 0x0000000100606ef1
dm`SkReadBuffer::readFlattenable(this=0x00000001033919a0,
ft=kSkImageFilter_Type) + 481 at SkReadBuffer.cpp:331
   328 	        size_t sizeRead = fReader.offset() - offset;
   329 	        if (sizeRecorded != sizeRead) {
   330 	            // we could try to fix up the offset...
-> 331 	            sk_throw();
   332 	        }
   333 	    } else {
   334 	        // we must skip the remaining data

It was off by 4 when I looked:
(lldb) p sizeRecorded
(uint32_t) $0 = 948
(lldb) p sizeRead
(size_t) $1 = 944

Now, I stupidly deleted my copy of sp_desk_carsvg.skp and refetched the current
SKPs, which don't have any problems.  This is why the bots are not failing. 
Given that the bots are fine, I'm not going to revert this CL right now, but I
think there's a latent bug in here somewhere.

[mtklein, 2015-06-22 21:17:36]

Note to self / others: all SKPs are on google storage somewhere.  I'll find the
failing sp-car-svg tomorrow.

[mtklein, 2015-06-22 21:20:54]

herb sez maybe SKP_VERSION = 382