Deserialize pictures with custom image-deserializer

Deserialize pictures with custom image-deserializer

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2187613002

Committed: https://skia.googlesource.com/skia/+/a9ca05ca5e604b9ee18e9cce19b059085ca0e22c

[reed1, 2016-07-26 18:34:18]

Description was changed from

==========
Deserialize pictures with custom image-deserializer

BUG=skia:
==========

to

==========
Deserialize pictures with custom image-deserializer

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2187613002
==========

[reed1, 2016-07-26 20:59:55]

reed@google.com changed reviewers:
+ fmalita@chromium.org, mtklein@google.com

[reed1, 2016-07-26 20:59:56]

Incomplete, but worth looking at

[reed1, 2016-07-26 21:00:13]

Description was changed from

==========
Deserialize pictures with custom image-deserializer

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2187613002
==========

to

==========
Deserialize pictures with custom image-deserializer

wants to rebase against https://codereview.chromium.org/2185563003/ when it
lands

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2187613002
==========

[f(malita), 2016-07-26 21:29:43]

https://codereview.chromium.org/2187613002/diff/20001/src/core/SkBitmapProcSh...
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.chromium.org/2187613002/diff/20001/src/core/SkBitmapProcSh...
src/core/SkBitmapProcShader.cpp:287: sk_sp<SkImage> image =
buffer.readBitmapAsImage();
Is nullptr an error condition, and if so should we bail immediately rather than
attempt to read?

https://codereview.chromium.org/2187613002/diff/20001/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.chromium.org/2187613002/diff/20001/src/core/SkPicture.cpp#...
src/core/SkPicture.cpp:41: return SkImage::MakeFromBitmap(bitmap);
bitmap.setImmutable first to avoid a copy?

[reed1, 2016-07-26 21:55:15]

Description was changed from

==========
Deserialize pictures with custom image-deserializer

wants to rebase against https://codereview.chromium.org/2185563003/ when it
lands

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2187613002
==========

to

==========
Deserialize pictures with custom image-deserializer

May be blocked by https://bugs.chromium.org/p/skia/issues/detail?id=5579

wants to rebase against https://codereview.chromium.org/2185563003/ when it
lands

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2187613002
==========

[reed1, 2016-07-26 21:55:35]

reed@google.com changed reviewers:
+ msarett@google.com

[reed1, 2016-07-29 13:12:04]

-- still think I can remove writeBitmap entirely ..

https://codereview.chromium.org/2187613002/diff/20001/src/core/SkBitmapProcSh...
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.chromium.org/2187613002/diff/20001/src/core/SkBitmapProcSh...
src/core/SkBitmapProcShader.cpp:287: sk_sp<SkImage> image =
buffer.readBitmapAsImage();
On 2016/07/26 21:29:43, f(malita) wrote:
> Is nullptr an error condition, and if so should we bail immediately rather
than
> attempt to read?

Done.

https://codereview.chromium.org/2187613002/diff/20001/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.chromium.org/2187613002/diff/20001/src/core/SkPicture.cpp#...
src/core/SkPicture.cpp:41: return SkImage::MakeFromBitmap(bitmap);
On 2016/07/26 21:29:43, f(malita) wrote:
> bitmap.setImmutable first to avoid a copy?

Done.

[reed1, 2016-07-29 13:12:10]

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

[commit-bot: I haz the power, 2016-07-29 13:12:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 13:41:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-29 13:41:50]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[reed1, 2016-08-10 15:59:10]

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

[commit-bot: I haz the power, 2016-08-10 15:59:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 17:26:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 17:26:47]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[reed1, 2016-08-10 19:05:25]

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

[commit-bot: I haz the power, 2016-08-10 19:05:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[reed1, 2016-08-10 19:05:40]

ptal

[commit-bot: I haz the power, 2016-08-10 19:10:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 19:10:22]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)

[reed1, 2016-08-10 19:20:04]

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

[commit-bot: I haz the power, 2016-08-10 19:20:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 20:22:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 20:22:14]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[mtklein, 2016-08-10 21:48:32]

https://codereview.chromium.org/2187613002/diff/140001/include/core/SkImageDe...
File include/core/SkImageDeserializer.h (right):

https://codereview.chromium.org/2187613002/diff/140001/include/core/SkImageDe...
include/core/SkImageDeserializer.h:26: virtual sk_sp<SkImage>
deserialize(sk_sp<SkData>, const SkIRect* subset);
const sk_sp<SkData>& ?

Passing by value (i.e. always reffing the data) seems like a detail specific to
SkImage::MakeFromEncoded().  We could pretty easily imagine an
SkImageDeserializer that doesn't need to ref the data.

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

https://codereview.chromium.org/2187613002/diff/140001/include/core/SkPicture...
include/core/SkPicture.h:80: static sk_sp<SkPicture> MakeFromStream(SkStream*
stream, std::nullptr_t) {
Confused.  Are you sure we need two MakeFromStream() methods, and do we care
which one MakeFromStream(stream, nullptr) goes to?

Do we want these two pieces of code to behave differently?
   SkPicture::MakeFromStream(stream, nullptr);
vs.
   SkImageDeserializer* deserializer = nullptr;
   SkPicture::MakeFromStream(stream, deserializer);

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

https://codereview.chromium.org/2187613002/diff/140001/src/core/SkReadBuffer....
src/core/SkReadBuffer.cpp:265: if (fImageDeserializer) {
Might be nice to default this to a singleton default deserializer that calls
SkImage::MakeFromEncoded() instead of null.  That way we never have to check
down here, just always call the deserializer.

[reed1, 2016-08-10 23:43:19]

https://codereview.chromium.org/2187613002/diff/140001/include/core/SkImageDe...
File include/core/SkImageDeserializer.h (right):

https://codereview.chromium.org/2187613002/diff/140001/include/core/SkImageDe...
include/core/SkImageDeserializer.h:26: virtual sk_sp<SkImage>
deserialize(sk_sp<SkData>, const SkIRect* subset);
On 2016/08/10 21:48:31, mtklein wrote:
> const sk_sp<SkData>& ?
> 
> Passing by value (i.e. always reffing the data) seems like a detail specific
to
> SkImage::MakeFromEncoded().  We could pretty easily imagine an
> SkImageDeserializer that doesn't need to ref the data.

Happy to go either way. Will switch to pointer.

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

https://codereview.chromium.org/2187613002/diff/140001/include/core/SkPicture...
include/core/SkPicture.h:80: static sk_sp<SkPicture> MakeFromStream(SkStream*
stream, std::nullptr_t) {
On 2016/08/10 21:48:31, mtklein wrote:
> Confused.  Are you sure we need two MakeFromStream() methods, and do we care
> which one MakeFromStream(stream, nullptr) goes to?
> 
> Do we want these two pieces of code to behave differently?
>    SkPicture::MakeFromStream(stream, nullptr);
> vs.
>    SkImageDeserializer* deserializer = nullptr;
>    SkPicture::MakeFromStream(stream, deserializer);

This is needed as chrome has callers today that pass nullptr and that pass
InstallPixelRef. When I've updated the caller after this lands (and removed the
InstallPixelRef variant) the other variants can also go away.

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

https://codereview.chromium.org/2187613002/diff/140001/src/core/SkReadBuffer....
src/core/SkReadBuffer.cpp:265: if (fImageDeserializer) {
On 2016/08/10 21:48:31, mtklein wrote:
> Might be nice to default this to a singleton default deserializer that calls
> SkImage::MakeFromEncoded() instead of null.  That way we never have to check
> down here, just always call the deserializer.

Agreed. I had meant to mark that as a TODO. WIll implement that now.

[reed1, 2016-08-11 00:32:52]

ptal

[reed1, 2016-08-11 00:32:58]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2016-08-11 00:33:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[reed1, 2016-08-11 00:33:26]

The CQ bit was unchecked by reed@google.com

[reed1, 2016-08-11 00:39:42]

Description was changed from

==========
Deserialize pictures with custom image-deserializer

May be blocked by https://bugs.chromium.org/p/skia/issues/detail?id=5579

wants to rebase against https://codereview.chromium.org/2185563003/ when it
lands

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2187613002
==========

to

==========
Deserialize pictures with custom image-deserializer

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2187613002
==========

[reed1, 2016-08-11 00:40:04]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2016-08-11 00:40:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mtklein, 2016-08-11 01:02:55]

lgtm

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

https://codereview.chromium.org/2187613002/diff/200001/src/core/SkReadBuffer....
src/core/SkReadBuffer.cpp:106: fImageDeserializer = deserializer ? deserializer
: &gDefaultImageDeserializer;
Don't you just wish || worked for types other than bool?  One of the things I
miss most from Lisp...

https://codereview.chromium.org/2187613002/diff/200001/src/core/SkReadBuffer....
src/core/SkReadBuffer.cpp:297: if (fImageDeserializer) {
always true?

[commit-bot: I haz the power, 2016-08-11 01:50:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 01:50:49]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[reed1, 2016-08-11 10:34:25]

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

https://codereview.chromium.org/2187613002/diff/200001/src/core/SkReadBuffer....
src/core/SkReadBuffer.cpp:297: if (fImageDeserializer) {
On 2016/08/11 01:02:55, mtklein wrote:
> always true?

Done.

[reed1, 2016-08-11 10:34:30]

The CQ bit was checked by reed@google.com

[reed1, 2016-08-11 10:34:31]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com
Link to the patchset: https://codereview.chromium.org/2187613002/#ps220001
(title: "fix legacy case, remove null-check")

[commit-bot: I haz the power, 2016-08-11 10:34:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 10:55:19]

Description was changed from

==========
Deserialize pictures with custom image-deserializer

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2187613002
==========

to

==========
Deserialize pictures with custom image-deserializer

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2187613002

Committed:
https://skia.googlesource.com/skia/+/a9ca05ca5e604b9ee18e9cce19b059085ca0e22c
==========

[commit-bot: I haz the power, 2016-08-11 10:55:21]

Committed patchset #12 (id:220001) as
https://skia.googlesource.com/skia/+/a9ca05ca5e604b9ee18e9cce19b059085ca0e22c