Adding an SkColorSpace to SkImage_Gpu

Adding an SkColorSpace to SkImage_Gpu

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

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

[Brian Osman, 2016-07-20 15:22:06]

Description was changed from

==========
Adding an SkColorSpace to SkImage_Gpu

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

to

==========
Adding an SkColorSpace to SkImage_Gpu

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

[Brian Osman, 2016-07-20 15:24:10]

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

[commit-bot: I haz the power, 2016-07-20 15:24:17]

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

[Brian Osman, 2016-07-20 15:25:56]

brianosman@google.com changed reviewers:
+ bsalomon@google.com, robertphillips@google.com

[Brian Osman, 2016-07-20 15:25:57]

Brian really didn't want more fields in the BackendDesc structs, so color space
is a new (optional) argument to the public factories. As we discussed, we can
look at fixing external call-sites and removing the older signatures.

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

https://codereview.chromium.org/2165703003/diff/1/src/core/SkSpecialImage.cpp...
src/core/SkSpecialImage.cpp:470: // TODO: Supply correct color space after we're
storing it here
Planning a similar follow-up change to add SkColorSpace to SkSpecialImage (and
fixing these TODOs).

[commit-bot: I haz the power, 2016-07-20 15:38:15]

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

[commit-bot: I haz the power, 2016-07-20 15:38:15]

Dry run: This issue passed the CQ dry run.

[bsalomon, 2016-07-20 17:14:27]

https://codereview.chromium.org/2165703003/diff/1/gm/image_pict.cpp
File gm/image_pict.cpp (right):

https://codereview.chromium.org/2165703003/diff/1/gm/image_pict.cpp#newcode335
gm/image_pict.cpp:335: sk_ref_sp(cache->info().colorSpace()),
This makes me wish colorSpace() returned sk_sp, but maybe its used elsewhere for
ephemeral access?

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

https://codereview.chromium.org/2165703003/diff/1/include/core/SkImage.h#newc...
include/core/SkImage.h:110: static sk_sp<SkImage> MakeFromTexture(GrContext*
ctx, const GrBackendTextureDesc& desc,
Is this used?

https://codereview.chromium.org/2165703003/diff/1/include/core/SkImage.h#newc...
include/core/SkImage.h:113: return MakeFromTexture(ctx, desc, at, NULL, trp,
rc);
nullptr

https://codereview.chromium.org/2165703003/diff/1/src/image/SkImage_Gpu.cpp
File src/image/SkImage_Gpu.cpp (right):

https://codereview.chromium.org/2165703003/diff/1/src/image/SkImage_Gpu.cpp#n...
src/image/SkImage_Gpu.cpp:47: static SkImageInfo make_info(int w, int h, bool
isOpaque, sk_sp<SkColorSpace> colorSpace) {
Not directly related to your CL, but don't we already have an AlphaType, why
take a bool isOpaque and convert back to AlphaType?

[robertphillips, 2016-07-20 17:25:39]

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

https://codereview.chromium.org/2165703003/diff/1/include/core/SkImage.h#newc...
include/core/SkImage.h:92: static sk_sp<SkImage> MakeFromTexture(GrContext* ctx,
const GrBackendTextureDesc& desc) {
make all these nullptrs ?

[Brian Osman, 2016-07-20 17:34:05]

Fixed all the NULLs, added some comments.

https://codereview.chromium.org/2165703003/diff/1/gm/image_pict.cpp
File gm/image_pict.cpp (right):

https://codereview.chromium.org/2165703003/diff/1/gm/image_pict.cpp#newcode335
gm/image_pict.cpp:335: sk_ref_sp(cache->info().colorSpace()),
On 2016/07/20 17:14:27, bsalomon wrote:
> This makes me wish colorSpace() returned sk_sp, but maybe its used elsewhere
for
> ephemeral access?

Yeah, I've had some discussions with reed@ and mtklein@ about this kind of
thing. I think there is still some disagreement about when we should allow
pointers to degrade and/or re-promote to sk_sp. The returned color space (from
info) is definitely used ephemerally elsewhere.

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

https://codereview.chromium.org/2165703003/diff/1/include/core/SkImage.h#newc...
include/core/SkImage.h:110: static sk_sp<SkImage> MakeFromTexture(GrContext*
ctx, const GrBackendTextureDesc& desc,
On 2016/07/20 17:14:27, bsalomon wrote:
> Is this used?

Looks like three hits:
https://cs.chromium.org/search/?q=MakeFromTexture&sq=package:chromium&type=cs
(plus one more in Flutter's engine code).

https://codereview.chromium.org/2165703003/diff/1/include/core/SkImage.h#newc...
include/core/SkImage.h:113: return MakeFromTexture(ctx, desc, at, NULL, trp,
rc);
On 2016/07/20 17:14:27, bsalomon wrote:
> nullptr

Done. (This file had a mix of nullptr and NULL, was trying to match existing
code as much as possible, but I just went ahead and fixed all non-comment
instances of NULL).

https://codereview.chromium.org/2165703003/diff/1/src/image/SkImage_Gpu.cpp
File src/image/SkImage_Gpu.cpp (right):

https://codereview.chromium.org/2165703003/diff/1/src/image/SkImage_Gpu.cpp#n...
src/image/SkImage_Gpu.cpp:47: static SkImageInfo make_info(int w, int h, bool
isOpaque, sk_sp<SkColorSpace> colorSpace) {
On 2016/07/20 17:14:27, bsalomon wrote:
> Not directly related to your CL, but don't we already have an AlphaType, why
> take a bool isOpaque and convert back to AlphaType?

No idea. isOpaque has slightly different semantics (thanks to checking the pixel
config, too), but it sure seems like we could just use the stored alpha type.
I'd like to save that for a separate change, just in case it breaks something.

[Brian Osman, 2016-07-20 17:41:11]

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

[commit-bot: I haz the power, 2016-07-20 17:41:22]

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-20 18:01:44]

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

[commit-bot: I haz the power, 2016-07-20 18:01:45]

Dry run: This issue passed the CQ dry run.

[robertphillips, 2016-07-20 18:04:15]

lgtm

[Brian Osman, 2016-07-20 18:10:00]

The CQ bit was checked by brianosman@google.com

[commit-bot: I haz the power, 2016-07-20 18:10:04]

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-20 18:11:33]

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

[commit-bot: I haz the power, 2016-07-20 18:11:34]

Try jobs failed on following builders:
  skia_presubmit-Trybot on master.client.skia.fyi (JOB_FAILED,
http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)

[Brian Osman, 2016-07-20 18:19:59]

PTAL, bsalomon@ ... public API changes

[bsalomon, 2016-07-20 20:47:26]

lgtm

[Brian Osman, 2016-07-20 20:54:37]

The CQ bit was checked by brianosman@google.com

[commit-bot: I haz the power, 2016-07-20 20:54:45]

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-20 20:55:41]

Description was changed from

==========
Adding an SkColorSpace to SkImage_Gpu

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

to

==========
Adding an SkColorSpace to SkImage_Gpu

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

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

[commit-bot: I haz the power, 2016-07-20 20:55:43]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/dddbe380b0e6b690041a4e020aefe57f1b21691a