SkImageSource

SkImageSource

Blink is migrating away from SkBitmaps, so we need an SkImage-based
SkImageFilter source.  This is pretty much a 1-1 equivalent of
SkBitmapSource.

To avoid duplication, relocate the SkImage deserialization logic
from SkPictureData to SkReadBuffer.

R=reed@google.com,robertphillips@google.com,senorblanco@chromium.org

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

[f(malita), 2015-09-14 18:11:52]

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

[f(malita), 2015-09-14 18:12:49]

The CQ bit was checked by fmalita@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-14 18:13:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1343703005/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1343703005/1

[f(malita), 2015-09-14 18:16:57]

Planning to migrate Blink and all the internal clients to the new class.  At
that point I think we could remove SkBitmapSource completely.

https://codereview.chromium.org/1343703005/diff/1/src/effects/SkImageSource.cpp
File src/effects/SkImageSource.cpp (right):

https://codereview.chromium.org/1343703005/diff/1/src/effects/SkImageSource.c...
src/effects/SkImageSource.cpp:75: return fImage->asLegacyBitmap(result,
SkImage::kRW_LegacyBitmapMode);
Can the result be RO/immutable?

[commit-bot: I haz the power, 2015-09-14 18:18:41]

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

[commit-bot: I haz the power, 2015-09-14 18:18:42]

Dry run: This issue passed the CQ dry run.

[f(malita), 2015-09-14 18:20:33]

The CQ bit was checked by fmalita@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-14 18:20:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1343703005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1343703005/20001

[commit-bot: I haz the power, 2015-09-14 18:26:34]

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

[commit-bot: I haz the power, 2015-09-14 18:26:34]

Dry run: This issue passed the CQ dry run.

[Stephen White, 2015-09-14 18:39:28]

https://codereview.chromium.org/1343703005/diff/20001/include/effects/SkImage...
File include/effects/SkImageSource.h (right):

https://codereview.chromium.org/1343703005/diff/20001/include/effects/SkImage...
include/effects/SkImageSource.h:21: SkFilterQuality = kHigh_SkFilterQuality);
API Q for reed@: since we have the chance, should we get rid of the default
param here?

It will require explicitly choosing one in Blink where we don't currently, but
maybe that's a good thing.

https://codereview.chromium.org/1343703005/diff/20001/include/effects/SkImage...
include/effects/SkImageSource.h:36: explicit SkImageSource(const SkImage*,
Nit: explicit is unnecessary here?

https://codereview.chromium.org/1343703005/diff/20001/src/effects/SkImageSour...
File src/effects/SkImageSource.cpp (right):

https://codereview.chromium.org/1343703005/diff/20001/src/effects/SkImageSour...
src/effects/SkImageSource.cpp:29: : INHERITED(0, 0)
Nit: 2nd arg should probably be nullptr (since our C++ compilers are out of the
dark ages now).

https://codereview.chromium.org/1343703005/diff/20001/src/effects/SkImageSour...
src/effects/SkImageSource.cpp:53: if (image) {
Should this be

if (!image) {

?

(and add a test case? :) )

[f(malita), 2015-09-14 19:14:08]

The CQ bit was checked by fmalita@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-14 19:14:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1343703005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1343703005/40001

[f(malita), 2015-09-14 19:19:35]

https://codereview.chromium.org/1343703005/diff/20001/include/effects/SkImage...
File include/effects/SkImageSource.h (right):

https://codereview.chromium.org/1343703005/diff/20001/include/effects/SkImage...
include/effects/SkImageSource.h:21: SkFilterQuality = kHigh_SkFilterQuality);
On 2015/09/14 18:39:27, Stephen White wrote:
> API Q for reed@: since we have the chance, should we get rid of the default
> param here?
> 
> It will require explicitly choosing one in Blink where we don't currently, but
> maybe that's a good thing.

Up to reed@, but SGTM. Updated.

https://codereview.chromium.org/1343703005/diff/20001/include/effects/SkImage...
include/effects/SkImageSource.h:36: explicit SkImageSource(const SkImage*,
On 2015/09/14 18:39:27, Stephen White wrote:
> Nit: explicit is unnecessary here?

Done.

https://codereview.chromium.org/1343703005/diff/20001/src/effects/SkImageSour...
File src/effects/SkImageSource.cpp (right):

https://codereview.chromium.org/1343703005/diff/20001/src/effects/SkImageSour...
src/effects/SkImageSource.cpp:29: : INHERITED(0, 0)
On 2015/09/14 18:39:27, Stephen White wrote:
> Nit: 2nd arg should probably be nullptr (since our C++ compilers are out of
the
> dark ages now).

Done.

https://codereview.chromium.org/1343703005/diff/20001/src/effects/SkImageSour...
src/effects/SkImageSource.cpp:53: if (image) {
On 2015/09/14 18:39:27, Stephen White wrote:
> Should this be
> 
> if (!image) {
> 
> ?

Doh!  Updated.


> (and add a test case? :) )

Will do.

[commit-bot: I haz the power, 2015-09-14 19:20:10]

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

[commit-bot: I haz the power, 2015-09-14 19:20:11]

Dry run: This issue passed the CQ dry run.

[Stephen White, 2015-09-14 19:49:24]

LGTM w/test added

https://codereview.chromium.org/1343703005/diff/20001/src/effects/SkImageSour...
File src/effects/SkImageSource.cpp (right):

https://codereview.chromium.org/1343703005/diff/20001/src/effects/SkImageSour...
src/effects/SkImageSource.cpp:53: if (image) {
On 2015/09/14 19:19:35, f(malita) wrote:
> On 2015/09/14 18:39:27, Stephen White wrote:
> > (and add a test case? :) )
> 
> Will do.

Don't see a test case yet.. is that still coming?

https://codereview.chromium.org/1343703005/diff/40001/gm/imagesource.cpp
File gm/imagesource.cpp (right):

https://codereview.chromium.org/1343703005/diff/40001/gm/imagesource.cpp#newc...
gm/imagesource.cpp:47: SkAutoTUnref<SkImageFilter>
bitmapSource(SkImageSource::Create(fImage));
uNit: bitmapSource -> imageSource?

[f(malita), 2015-09-14 19:51:16]

https://codereview.chromium.org/1343703005/diff/20001/src/effects/SkImageSour...
File src/effects/SkImageSource.cpp (right):

https://codereview.chromium.org/1343703005/diff/20001/src/effects/SkImageSour...
src/effects/SkImageSource.cpp:53: if (image) {
On 2015/09/14 19:49:23, Stephen White wrote:
> On 2015/09/14 19:19:35, f(malita) wrote:
> > On 2015/09/14 18:39:27, Stephen White wrote:
> > > (and add a test case? :) )
> > 
> > Will do.
> 
> Don't see a test case yet.. is that still coming?

Yup, as soon as I figure how to make SkImageSource serialization actually work
:)

[f(malita), 2015-09-14 20:06:10]

The CQ bit was checked by fmalita@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-14 20:06:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1343703005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1343703005/60001

[f(malita), 2015-09-14 20:07:20]

On 2015/09/14 19:51:16, f(malita) wrote:
>
https://codereview.chromium.org/1343703005/diff/20001/src/effects/SkImageSour...
> File src/effects/SkImageSource.cpp (right):
> 
>
https://codereview.chromium.org/1343703005/diff/20001/src/effects/SkImageSour...
> src/effects/SkImageSource.cpp:53: if (image) {
> On 2015/09/14 19:49:23, Stephen White wrote:
> > On 2015/09/14 19:19:35, f(malita) wrote:
> > > On 2015/09/14 18:39:27, Stephen White wrote:
> > > > (and add a test case? :) )
> > > 
> > > Will do.
> > 
> > Don't see a test case yet.. is that still coming?
> 
> Yup, as soon as I figure how to make SkImageSource serialization actually work
> :)

Found it: was missing SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkImageSource).

Added test.

https://codereview.chromium.org/1343703005/diff/40001/gm/imagesource.cpp
File gm/imagesource.cpp (right):

https://codereview.chromium.org/1343703005/diff/40001/gm/imagesource.cpp#newc...
gm/imagesource.cpp:47: SkAutoTUnref<SkImageFilter>
bitmapSource(SkImageSource::Create(fImage));
On 2015/09/14 19:49:24, Stephen White wrote:
> uNit: bitmapSource -> imageSource?

Done.

[reed1, 2015-09-14 20:08:48]

lgtm

https://codereview.chromium.org/1343703005/diff/40001/include/effects/SkImage...
File include/effects/SkImageSource.h (right):

https://codereview.chromium.org/1343703005/diff/40001/include/effects/SkImage...
include/effects/SkImageSource.h:17: static SkImageSource* Create(const
SkImage*);
Can these just return SkImageFilter* ?

[f(malita), 2015-09-14 20:12:50]

https://codereview.chromium.org/1343703005/diff/40001/include/effects/SkImage...
File include/effects/SkImageSource.h (right):

https://codereview.chromium.org/1343703005/diff/40001/include/effects/SkImage...
include/effects/SkImageSource.h:17: static SkImageSource* Create(const
SkImage*);
On 2015/09/14 20:08:48, reed1 wrote:
> Can these just return SkImageFilter* ?

Done.

[f(malita), 2015-09-14 20:12:56]

The CQ bit was checked by fmalita@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-14 20:13:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1343703005/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1343703005/80001

[commit-bot: I haz the power, 2015-09-14 20:18:51]

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

[commit-bot: I haz the power, 2015-09-14 20:18:52]

Dry run: This issue passed the CQ dry run.

[f(malita), 2015-09-14 20:30:12]

The CQ bit was checked by fmalita@chromium.org

[f(malita), 2015-09-14 20:30:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
senorblanco@chromium.org, reed@google.com
Link to the patchset: https://codereview.chromium.org/1343703005/#ps80001
(title: "review comments")

[commit-bot: I haz the power, 2015-09-14 20:30:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1343703005/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1343703005/80001

[commit-bot: I haz the power, 2015-09-14 20:31:21]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/cd56f812e09fdd8f8322c5c28cbc4423a74b9a0a