add Make variations to return SkImage by sk_sp

add Make variations to return SkImage by sk_sp

some internal call-sites update, but not. Will follow-up in future to complete that.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1778393002

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

[reed1, 2016-03-10 16:06:44]

Description was changed from

==========
add Make variations to return SkImage by sk_sp

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

to

==========
add Make variations to return SkImage by sk_sp

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[reed1, 2016-03-10 16:10:21]

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

[commit-bot: I haz the power, 2016-03-10 16:10:26]

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

[reed1, 2016-03-10 16:10:52]

Description was changed from

==========
add Make variations to return SkImage by sk_sp

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
add Make variations to return SkImage by sk_sp

some internal call-sites update, but not. Will follow-up in future to complete
that.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[commit-bot: I haz the power, 2016-03-10 16:16:16]

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

[commit-bot: I haz the power, 2016-03-10 16:16:17]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[reed1, 2016-03-10 16:18:06]

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

[commit-bot: I haz the power, 2016-03-10 16:18:16]

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

[reed1, 2016-03-10 16:20:48]

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

[commit-bot: I haz the power, 2016-03-10 16:23:26]

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

[commit-bot: I haz the power, 2016-03-10 16:23:27]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[reed1, 2016-03-10 16:48:32]

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

[commit-bot: I haz the power, 2016-03-10 16:48:39]

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

[commit-bot: I haz the power, 2016-03-10 17:04:07]

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

[commit-bot: I haz the power, 2016-03-10 17:04:11]

Dry run: This issue passed the CQ dry run.

[f(malita), 2016-03-10 18:12:15]

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

https://codereview.chromium.org/1778393002/diff/60001/include/core/SkImage.h#...
include/core/SkImage.h:76: static sk_sp<SkImage>
MakeFromGenerator(SkImageGenerator*, const SkIRect* subset = NULL);
I think we want sk_sp<SkImageGenerator> here, otherwise we should keep the
ownership comment.

https://codereview.chromium.org/1778393002/diff/60001/include/core/SkShader.h
File include/core/SkShader.h (left):

https://codereview.chromium.org/1778393002/diff/60001/include/core/SkShader.h...
include/core/SkShader.h:407: static sk_sp<SkShader>
MakePictureShader(sk_sp<const SkPicture> src, TileMode tmx, TileMode tmy,
Curious whether we dropped const for a particular reason.

https://codereview.chromium.org/1778393002/diff/60001/src/image/SkImage.cpp
File src/image/SkImage.cpp (right):

https://codereview.chromium.org/1778393002/diff/60001/src/image/SkImage.cpp#n...
src/image/SkImage.cpp:159: return generator ?
SkImage::MakeFromGenerator(generator, subset) : nullptr;
sk_sp<SkImageGenerator> generator =...
SkImage::MakeFromGenerator(std::move(generator)...)
?

Maybe also move the null check to SkImage::MakeFromGenerator()?

https://codereview.chromium.org/1778393002/diff/60001/src/image/SkImage_Raste...
File src/image/SkImage_Raster.cpp (right):

https://codereview.chromium.org/1778393002/diff/60001/src/image/SkImage_Raste...
src/image/SkImage_Raster.cpp:200: return sk_make_sp<SkImage_Raster>(pmap.info(),
data, pmap.rowBytes(), pmap.ctable());
move(data)?

https://codereview.chromium.org/1778393002/diff/60001/src/image/SkImage_Raste...
src/image/SkImage_Raster.cpp:217: return sk_make_sp<SkImage_Raster>(info, data,
rowBytes, ctable);
move(data)?

https://codereview.chromium.org/1778393002/diff/60001/src/image/SkImage_Raste...
src/image/SkImage_Raster.cpp:228: return sk_make_sp<SkImage_Raster>(pmap.info(),
data, pmap.rowBytes(), pmap.ctable());
move(data)?

https://codereview.chromium.org/1778393002/diff/60001/src/utils/SkBitmapSourc...
File src/utils/SkBitmapSourceDeserializer.cpp (right):

https://codereview.chromium.org/1778393002/diff/60001/src/utils/SkBitmapSourc...
src/utils/SkBitmapSourceDeserializer.cpp:32: return
SkImageSource::Create(SkImage::MakeFromBitmap(bitmap).release(),
hmm, is this leaking?

based on prev usage, SkImageSource::Create doesn't transfer the ref but grabs a
new one => we still need to unref.

[reed1, 2016-03-10 19:12:57]

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

https://codereview.chromium.org/1778393002/diff/60001/include/core/SkImage.h#...
include/core/SkImage.h:76: static sk_sp<SkImage>
MakeFromGenerator(SkImageGenerator*, const SkIRect* subset = NULL);
On 2016/03/10 18:12:14, f(malita) wrote:
> I think we want sk_sp<SkImageGenerator> here, otherwise we should keep the
> ownership comment.

Agreed. I was removed when I tried switching to unique_ptr, that that change
proved too long taily for this CL

https://codereview.chromium.org/1778393002/diff/60001/include/core/SkShader.h
File include/core/SkShader.h (left):

https://codereview.chromium.org/1778393002/diff/60001/include/core/SkShader.h...
include/core/SkShader.h:407: static sk_sp<SkShader>
MakePictureShader(sk_sp<const SkPicture> src, TileMode tmx, TileMode tmy,
On 2016/03/10 18:12:15, f(malita) wrote:
> Curious whether we dropped const for a particular reason.

Tangential for this CL, but I have found it odd (and slightly bad) that we call
picture context, but all other similar objects are not (e.g. all effects,
images, data). I can revert const-ness if you object.

https://codereview.chromium.org/1778393002/diff/60001/src/image/SkImage.cpp
File src/image/SkImage.cpp (right):

https://codereview.chromium.org/1778393002/diff/60001/src/image/SkImage.cpp#n...
src/image/SkImage.cpp:159: return generator ?
SkImage::MakeFromGenerator(generator, subset) : nullptr;
On 2016/03/10 18:12:15, f(malita) wrote:
> sk_sp<SkImageGenerator> generator =...
> SkImage::MakeFromGenerator(std::move(generator)...)
> ?
> 
> Maybe also move the null check to SkImage::MakeFromGenerator()?

Generators are not reference counted.

null-check moved.

https://codereview.chromium.org/1778393002/diff/60001/src/image/SkImage_Raste...
File src/image/SkImage_Raster.cpp (right):

https://codereview.chromium.org/1778393002/diff/60001/src/image/SkImage_Raste...
src/image/SkImage_Raster.cpp:200: return sk_make_sp<SkImage_Raster>(pmap.info(),
data, pmap.rowBytes(), pmap.ctable());
On 2016/03/10 18:12:15, f(malita) wrote:
> move(data)?

Done.

https://codereview.chromium.org/1778393002/diff/60001/src/image/SkImage_Raste...
src/image/SkImage_Raster.cpp:217: return sk_make_sp<SkImage_Raster>(info, data,
rowBytes, ctable);
On 2016/03/10 18:12:15, f(malita) wrote:
> move(data)?

Done.

https://codereview.chromium.org/1778393002/diff/60001/src/image/SkImage_Raste...
src/image/SkImage_Raster.cpp:228: return sk_make_sp<SkImage_Raster>(pmap.info(),
data, pmap.rowBytes(), pmap.ctable());
On 2016/03/10 18:12:15, f(malita) wrote:
> move(data)?

Done.

https://codereview.chromium.org/1778393002/diff/60001/src/utils/SkBitmapSourc...
File src/utils/SkBitmapSourceDeserializer.cpp (right):

https://codereview.chromium.org/1778393002/diff/60001/src/utils/SkBitmapSourc...
src/utils/SkBitmapSourceDeserializer.cpp:32: return
SkImageSource::Create(SkImage::MakeFromBitmap(bitmap).release(),
On 2016/03/10 18:12:15, f(malita) wrote:
> hmm, is this leaking?
> 
> based on prev usage, SkImageSource::Create doesn't transfer the ref but grabs
a
> new one => we still need to unref.

GOOD CATCH!

[reed1, 2016-03-10 19:17:07]

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

[commit-bot: I haz the power, 2016-03-10 19:17:17]

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

[f(malita), 2016-03-10 19:24:57]

LGTM

https://codereview.chromium.org/1778393002/diff/60001/src/image/SkImage.cpp
File src/image/SkImage.cpp (right):

https://codereview.chromium.org/1778393002/diff/60001/src/image/SkImage.cpp#n...
src/image/SkImage.cpp:159: return generator ?
SkImage::MakeFromGenerator(generator, subset) : nullptr;
On 2016/03/10 19:12:56, reed1 wrote:
> On 2016/03/10 18:12:15, f(malita) wrote:
> > sk_sp<SkImageGenerator> generator =...
> > SkImage::MakeFromGenerator(std::move(generator)...)
> > ?
> > 
> > Maybe also move the null check to SkImage::MakeFromGenerator()?
> 
> Generators are not reference counted.

doh :)

[commit-bot: I haz the power, 2016-03-10 19:33:51]

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

[commit-bot: I haz the power, 2016-03-10 19:33:52]

Dry run: This issue passed the CQ dry run.

[reed1, 2016-03-10 20:44:12]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2016-03-10 20:44:17]

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

[commit-bot: I haz the power, 2016-03-10 20:45:20]

Description was changed from

==========
add Make variations to return SkImage by sk_sp

some internal call-sites update, but not. Will follow-up in future to complete
that.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
add Make variations to return SkImage by sk_sp

some internal call-sites update, but not. Will follow-up in future to complete
that.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

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

[commit-bot: I haz the power, 2016-03-10 20:45:21]

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

[robertphillips, 2016-03-11 00:18:35]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1783063002/ by robertphillips@google.com.

The reason for reverting is: Experimental revert to try to clear up ASAN
failures.

[reed1, 2016-03-11 01:54:00]

seems like it must be related to my changes to pictureshader...

[f(malita), 2016-03-11 13:58:36]

https://codereview.chromium.org/1778393002/diff/80001/src/image/SkImage.cpp
File src/image/SkImage.cpp (right):

https://codereview.chromium.org/1778393002/diff/80001/src/image/SkImage.cpp#n...
src/image/SkImage.cpp:341: return
MakeFromGenerator(SkImageGenerator::NewFromPicture(dimensions,
picture.release(),
Yeah, I think this needs to be picture.get() as SkImageGenerator::NewFromPicture
does not adopt.

(SkImage::SkImageNewFromPicture -> sk_ref_sp(picture) then we release here -> we
leak this extra ref)