Some tests around surface creation and snapshotting with color space

Some tests around surface creation and snapshotting with color space

Verify the rules that we're converging on for surfaces:
- For 8888, we only support sRGB-like gamma, or no color space at all.
- For F16, we require a color space, with linear gamma.
- For all other formats, we do not support color spaces.

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

Committed: https://skia.googlesource.com/skia/+/0e22eb8e6efc7d7ab7a601ba555947916d139906

[Brian Osman, 2016-08-23 13:15:02]

Description was changed from

==========
Some tests around surface creation and snapshotting with color space

Verify the rules that we're converging on for surfaces:
- We only support color spaces on 8888 and F16 surfaces.
- For 8888, we only support sRGB-like gamma, or linear gamma.
- For F16, we ignore gamma, so we can technically support any color space.

NOTE: I have doubts about several of these - we need to decide what we're
going to allow or disallow. Comments in the review to explain.

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

to

==========
Some tests around surface creation and snapshotting with color space

Verify the rules that we're converging on for surfaces:
- We only support color spaces on 8888 and F16 surfaces.
- For 8888, we only support sRGB-like gamma, or linear gamma.
- For F16, we ignore gamma, so we can technically support any color space.

NOTE: I have doubts about several of these - we need to decide what we're
going to allow or disallow. Comments in the review to explain.

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

[Brian Osman, 2016-08-23 13:24:03]

Description was changed from

==========
Some tests around surface creation and snapshotting with color space

Verify the rules that we're converging on for surfaces:
- We only support color spaces on 8888 and F16 surfaces.
- For 8888, we only support sRGB-like gamma, or linear gamma.
- For F16, we ignore gamma, so we can technically support any color space.

NOTE: I have doubts about several of these - we need to decide what we're
going to allow or disallow. Comments in the review to explain.

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

to

==========
Some tests around surface creation and snapshotting with color space

Verify the rules that we're converging on for surfaces:
- We only support color spaces on 8888 and F16 surfaces.
- For 8888, we only support sRGB-like gamma, or linear gamma.
- For F16, we ignore gamma, so we can technically support any color space.

NOTE: I have doubts about some of these - we need to decide what we're
going to allow or disallow. Comments in the review to explain.

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

[Brian Osman, 2016-08-23 13:25:55]

brianosman@google.com changed reviewers:
+ bsalomon@google.com, msarett@google.com, mtklein@google.com, reed@google.com

[Brian Osman, 2016-08-23 13:25:57]

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

https://codereview.chromium.org/2270823002/diff/1/src/image/SkSurface_Raster....
src/image/SkSurface_Raster.cpp:65: SkColorSpace::kLinear_GammaNamed !=
info.colorSpace()->gammaNamed()) {
Matt mentioned that if someone constructs a color space with gamma of 1.0
(rather than kLinear_Named), this won't catch that. No one should do that.
Perhaps we should detect it and add a test case, though?

https://codereview.chromium.org/2270823002/diff/1/tests/SurfaceTest.cpp
File tests/SurfaceTest.cpp (right):

https://codereview.chromium.org/2270823002/diff/1/tests/SurfaceTest.cpp#newco...
tests/SurfaceTest.cpp:930: { kN32_SkColorType,       linearColorSpace, true, 
"N32-linear"  }, // TODO: Disallow this?
Starting from the statement: "If there is a color space attached, then rendering
is gamma-correct. Sources are gamma-decoded, etc..."

This can work on GPU, but I'm not sure we have a raster pipeline for this. Do we
have a way to render to linear 8-bit, with gamma-decoding of sRGB sources?

https://codereview.chromium.org/2270823002/diff/1/tests/SurfaceTest.cpp#newco...
tests/SurfaceTest.cpp:934: { kRGBA_F16_SkColorType,  nullptr,          true, 
"F16-nullptr" }, // TODO: Disallow this?
We've mentioned requiring a color space with F16. Again, we can easily support
this on GPU, and it could be useful for auxiliary uses of F16 (shadow maps, high
precision masks, etc...). Does it cause any issues for Raster?

https://codereview.chromium.org/2270823002/diff/1/tests/SurfaceTest.cpp#newco...
tests/SurfaceTest.cpp:938: { kRGBA_F16_SkColorType,  oddColorSpace,    true, 
"F16-odd"     }, // TODO: Disallow this?
Should we allow this? The gamma is ignored, so in some sense it works just fine,
and I think it could be a useful part of a pipeline for eventual down-sampling
to 8 or 10 bit with a different gamma curve. (Where we do a one-time adjustment
in a shader based on the gamma from the attached color space).

https://codereview.chromium.org/2270823002/diff/1/tests/SurfaceTest.cpp#newco...
tests/SurfaceTest.cpp:940: { kAlpha_8_SkColorType,   srgbColorSpace,   false,
"A8-srgb"     }, // TODO: Allow this?
Pretty sure we don't want to support this - obviously the gamma doesn't affect
what's happening in the alpha channel. But if you render to an A8 surface with a
paint that has behavior influenced by gamma-correction (shader / color filter,
etc...), should we respect that? Again, this probably adds a lot of complexity
to raster for an extreme edge case, so my gut says no.

[Brian Osman, 2016-08-23 13:27:00]

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

[commit-bot: I haz the power, 2016-08-23 13:27:13]

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

[msarett, 2016-08-23 13:46:11]

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

https://codereview.chromium.org/2270823002/diff/1/src/image/SkSurface_Raster....
src/image/SkSurface_Raster.cpp:65: SkColorSpace::kLinear_GammaNamed !=
info.colorSpace()->gammaNamed()) {
On 2016/08/23 13:25:56, Brian Osman wrote:
> Matt mentioned that if someone constructs a color space with gamma of 1.0
> (rather than kLinear_Named), this won't catch that. No one should do that.
> Perhaps we should detect it and add a test case, though?

What I was trying to say is that we won't catch ICC profiles with linear gamma
tables (hopefully these don't exist).

We actually will catch 1.0f.  Though the constructor that takes 3 floats is not
in the public API anyway (but used by SkPngCodec).

https://codereview.chromium.org/2270823002/diff/1/tests/SurfaceTest.cpp
File tests/SurfaceTest.cpp (right):

https://codereview.chromium.org/2270823002/diff/1/tests/SurfaceTest.cpp#newco...
tests/SurfaceTest.cpp:930: { kN32_SkColorType,       linearColorSpace, true, 
"N32-linear"  }, // TODO: Disallow this?
On 2016/08/23 13:25:56, Brian Osman wrote:
> Starting from the statement: "If there is a color space attached, then
rendering
> is gamma-correct. Sources are gamma-decoded, etc..."
> 
> This can work on GPU, but I'm not sure we have a raster pipeline for this. Do
we
> have a way to render to linear 8-bit, with gamma-decoding of sRGB sources?

FWIW, image decoders support linear 8-bit.  But it just falls into the slow
path, it's not exactly an intentional feature.

https://codereview.chromium.org/2270823002/diff/1/tests/SurfaceTest.cpp#newco...
tests/SurfaceTest.cpp:934: { kRGBA_F16_SkColorType,  nullptr,          true, 
"F16-nullptr" }, // TODO: Disallow this?
On 2016/08/23 13:25:56, Brian Osman wrote:
> We've mentioned requiring a color space with F16. Again, we can easily support
> this on GPU, and it could be useful for auxiliary uses of F16 (shadow maps,
high
> precision masks, etc...). Does it cause any issues for Raster?

Image decoders will fail (on purpose) if you ask for F16 and the color space is
nullptr.

https://codereview.chromium.org/2270823002/diff/1/tests/SurfaceTest.cpp#newco...
tests/SurfaceTest.cpp:938: { kRGBA_F16_SkColorType,  oddColorSpace,    true, 
"F16-odd"     }, // TODO: Disallow this?
On 2016/08/23 13:25:56, Brian Osman wrote:
> Should we allow this? The gamma is ignored, so in some sense it works just
fine,
> and I think it could be a useful part of a pipeline for eventual down-sampling
> to 8 or 10 bit with a different gamma curve. (Where we do a one-time
adjustment
> in a shader based on the gamma from the attached color space).

Supporting this doesn't bother the image decoders.  I think to answer this, we
need to decide how we want to handle dsts with strange gammas...  AFAICT,
they're rare, but they exist.

https://codereview.chromium.org/2270823002/diff/1/tests/SurfaceTest.cpp#newco...
tests/SurfaceTest.cpp:940: { kAlpha_8_SkColorType,   srgbColorSpace,   false,
"A8-srgb"     }, // TODO: Allow this?
On 2016/08/23 13:25:56, Brian Osman wrote:
> Pretty sure we don't want to support this - obviously the gamma doesn't affect
> what's happening in the alpha channel. But if you render to an A8 surface with
a
> paint that has behavior influenced by gamma-correction (shader / color filter,
> etc...), should we respect that? Again, this probably adds a lot of complexity
> to raster for an extreme edge case, so my gut says no.

Agree with your gut.  It's a nice simplification to say "alpha is always
linear".

[commit-bot: I haz the power, 2016-08-23 13:56:30]

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

[commit-bot: I haz the power, 2016-08-23 13:56:31]

Dry run: This issue passed the CQ dry run.

[mtklein, 2016-08-23 14:24:51]

https://codereview.chromium.org/2270823002/diff/1/tests/SurfaceTest.cpp
File tests/SurfaceTest.cpp (right):

https://codereview.chromium.org/2270823002/diff/1/tests/SurfaceTest.cpp#newco...
tests/SurfaceTest.cpp:930: { kN32_SkColorType,       linearColorSpace, true, 
"N32-linear"  }, // TODO: Disallow this?
On 2016/08/23 13:25:56, Brian Osman wrote:
> Starting from the statement: "If there is a color space attached, then
rendering
> is gamma-correct. Sources are gamma-decoded, etc..."
> 
> This can work on GPU, but I'm not sure we have a raster pipeline for this. Do
we
> have a way to render to linear 8-bit, with gamma-decoding of sRGB sources?

Yes, ban it.  If you're well informed about color spaces and still find yourself
wanting 32-bit buffers, you'll pick N32-srgb.  If not, you'll keep using
N32-nullptr. Even if N32-linear turns out to be easy to support, there's always
a better choice.

https://codereview.chromium.org/2270823002/diff/1/tests/SurfaceTest.cpp#newco...
tests/SurfaceTest.cpp:934: { kRGBA_F16_SkColorType,  nullptr,          true, 
"F16-nullptr" }, // TODO: Disallow this?
On 2016/08/23 13:25:56, Brian Osman wrote:
> We've mentioned requiring a color space with F16. Again, we can easily support
> this on GPU, and it could be useful for auxiliary uses of F16 (shadow maps,
high
> precision masks, etc...). Does it cause any issues for Raster?

Yes, let's ban this.  There are no legacy users of F16.  Anyone who wants to
play with future tech needs to become color space aware.

https://codereview.chromium.org/2270823002/diff/1/tests/SurfaceTest.cpp#newco...
tests/SurfaceTest.cpp:938: { kRGBA_F16_SkColorType,  oddColorSpace,    true, 
"F16-odd"     }, // TODO: Disallow this?
On 2016/08/23 13:25:56, Brian Osman wrote:
> Should we allow this? The gamma is ignored, so in some sense it works just
fine,
> and I think it could be a useful part of a pipeline for eventual down-sampling
> to 8 or 10 bit with a different gamma curve. (Where we do a one-time
adjustment
> in a shader based on the gamma from the attached color space).

Remind me why we'd support non-linear gamma for F16?

If we're dropping down to 8 or 10 bit, won't that format know its own gamma?
Why is it the F16 buffer's responsibility to carry around a notional gamma?

https://codereview.chromium.org/2270823002/diff/1/tests/SurfaceTest.cpp#newco...
tests/SurfaceTest.cpp:940: { kAlpha_8_SkColorType,   srgbColorSpace,   false,
"A8-srgb"     }, // TODO: Allow this?
On 2016/08/23 13:25:56, Brian Osman wrote:
> Pretty sure we don't want to support this - obviously the gamma doesn't affect
> what's happening in the alpha channel. But if you render to an A8 surface with
a
> paint that has behavior influenced by gamma-correction (shader / color filter,
> etc...), should we respect that? Again, this probably adds a lot of complexity
> to raster for an extreme edge case, so my gut says no.

Right, let's ban non-linear alpha entirely.
Grey8-nullptr and Grey8-srgb can probably fill in the gaps, right?

[Brian Osman, 2016-08-23 14:58:33]

https://codereview.chromium.org/2270823002/diff/1/tests/SurfaceTest.cpp
File tests/SurfaceTest.cpp (right):

https://codereview.chromium.org/2270823002/diff/1/tests/SurfaceTest.cpp#newco...
tests/SurfaceTest.cpp:930: { kN32_SkColorType,       linearColorSpace, true, 
"N32-linear"  }, // TODO: Disallow this?
On 2016/08/23 14:24:51, mtklein wrote:
> On 2016/08/23 13:25:56, Brian Osman wrote:
> > Starting from the statement: "If there is a color space attached, then
> rendering
> > is gamma-correct. Sources are gamma-decoded, etc..."
> > 
> > This can work on GPU, but I'm not sure we have a raster pipeline for this.
Do
> we
> > have a way to render to linear 8-bit, with gamma-decoding of sRGB sources?
> 
> Yes, ban it.  If you're well informed about color spaces and still find
yourself
> wanting 32-bit buffers, you'll pick N32-srgb.  If not, you'll keep using
> N32-nullptr. Even if N32-linear turns out to be easy to support, there's
always
> a better choice.

Acknowledged.

https://codereview.chromium.org/2270823002/diff/1/tests/SurfaceTest.cpp#newco...
tests/SurfaceTest.cpp:934: { kRGBA_F16_SkColorType,  nullptr,          true, 
"F16-nullptr" }, // TODO: Disallow this?
On 2016/08/23 14:24:50, mtklein wrote:
> On 2016/08/23 13:25:56, Brian Osman wrote:
> > We've mentioned requiring a color space with F16. Again, we can easily
support
> > this on GPU, and it could be useful for auxiliary uses of F16 (shadow maps,
> high
> > precision masks, etc...). Does it cause any issues for Raster?
> 
> Yes, let's ban this.  There are no legacy users of F16.  Anyone who wants to
> play with future tech needs to become color space aware.

Acknowledged.

https://codereview.chromium.org/2270823002/diff/1/tests/SurfaceTest.cpp#newco...
tests/SurfaceTest.cpp:938: { kRGBA_F16_SkColorType,  oddColorSpace,    true, 
"F16-odd"     }, // TODO: Disallow this?
On 2016/08/23 14:24:50, mtklein wrote:
> On 2016/08/23 13:25:56, Brian Osman wrote:
> > Should we allow this? The gamma is ignored, so in some sense it works just
> fine,
> > and I think it could be a useful part of a pipeline for eventual
down-sampling
> > to 8 or 10 bit with a different gamma curve. (Where we do a one-time
> adjustment
> > in a shader based on the gamma from the attached color space).
> 
> Remind me why we'd support non-linear gamma for F16?
> 
> If we're dropping down to 8 or 10 bit, won't that format know its own gamma?
> Why is it the F16 buffer's responsibility to carry around a notional gamma?

I have several arguments each way:

1) We get an image off the internet, and it's got a gamma we can't directly
support. So we ask the codec to give it to us as F16. The easiest thing for the
codec to do is decode the stored gamma (so we get F16 1.0), but still attach the
original color space object. The alternative is to explicitly make a new version
of the color space with gamma 1.0. Not much work for this case, and this doesn't
really influence the surface creation path, but given that we'll have the same
checks on sources, I think the argument is the same.

2) Re: 8/10-bit, yes, probably. I'm just trying to decide what the world looks
like if someone queries their monitor and learns that it's got a 1.8 gamma. Then
they want to render a surface with that gamma*. We can't do that directly, so
they're going to need to render to something else with a different gamma
(probably F16 1.0). Then they're going to want to push that to the monitor - I
guess you're right that we could have deferred sticking the monitor gamma on
something until this step (we've talked about having a presentation-layer only
surface type that supports arbitrary gamma for this situation). So yeah - may
not matter for this case.

*: I'm still trying to decide if this is actually a thing that we're supposed to
do. It's not obvious to me that color-managed applications are (or aren't)
supposed to compensate for monitor gamma. I feel like one possible world is that
we're supposed to do that. The other world is where the OS contract is that they
accept things in either sRGB or linear gamma, then adapt output to the monitor's
gamma, because that's a function of viewing environment, and they don't (want
to) care about encoding of incoming layers for compositing.

https://codereview.chromium.org/2270823002/diff/1/tests/SurfaceTest.cpp#newco...
tests/SurfaceTest.cpp:940: { kAlpha_8_SkColorType,   srgbColorSpace,   false,
"A8-srgb"     }, // TODO: Allow this?
On 2016/08/23 14:24:50, mtklein wrote:
> On 2016/08/23 13:25:56, Brian Osman wrote:
> > Pretty sure we don't want to support this - obviously the gamma doesn't
affect
> > what's happening in the alpha channel. But if you render to an A8 surface
with
> a
> > paint that has behavior influenced by gamma-correction (shader / color
filter,
> > etc...), should we respect that? Again, this probably adds a lot of
complexity
> > to raster for an extreme edge case, so my gut says no.
> 
> Right, let's ban non-linear alpha entirely.
> Grey8-nullptr and Grey8-srgb can probably fill in the gaps, right?

... actually, we don't support Grey8 as a surface format, at all. Raster doesn't
allow it currently, and GPU actually has bigger problems. We map Grey8 and
Alpha8 to the same backend format, and when I tried to support it properly,
things kind of spiraled out of control. (Grey8-srgb doesn't really exist in
modern GL, sadly. It does exist in *older* GL with some extensions, or in
Vulkan.)

[mtklein, 2016-08-23 15:54:18]

> The alternative is to explicitly make a new version of the color space with
gamma 1.0.

I think this is clearly the right thing to do.  That's the space which most
accurately represents the content of the buffer.

> ... actually, we don't support Grey8 as a surface format, at all. Raster
doesn't
> allow it currently, and GPU actually has bigger problems. We map Grey8 and
> Alpha8 to the same backend format, and when I tried to support it properly,
> things kind of spiraled out of control. (Grey8-srgb doesn't really exist in
> modern GL, sadly. It does exist in *older* GL with some extensions, or in
> Vulkan.)

Ah, fair enough.  Forget I mentioned grey.  Alpha needs to stay linear or we go
insane.

[msarett, 2016-08-23 16:05:03]

On 2016/08/23 15:54:18, mtklein wrote:
> > The alternative is to explicitly make a new version of the color space with
> gamma 1.0.
> 
> I think this is clearly the right thing to do.  That's the space which most
> accurately represents the content of the buffer.
> 

I see what you mean from a clarity perspective.

I was under the impression that we wanted to tag all "dst" buffers with the dst
color space (and "src" buffers with the src color space) for
convenience/efficiency.  IMO, it's not that big of a jump to understand "F16 is
always linear - ignore the final gamma step".  This is how codec behaves now.

> > ... actually, we don't support Grey8 as a surface format, at all. Raster
> doesn't
> > allow it currently, and GPU actually has bigger problems. We map Grey8 and
> > Alpha8 to the same backend format, and when I tried to support it properly,
> > things kind of spiraled out of control. (Grey8-srgb doesn't really exist in
> > modern GL, sadly. It does exist in *older* GL with some extensions, or in
> > Vulkan.)
> 
> Ah, fair enough.  Forget I mentioned grey.  Alpha needs to stay linear or we
go
> insane.

[Brian Osman, 2016-08-23 19:54:46]

This version now requires F16 surfaces to have a colorspace with gamma 1.0. I'm
going to have to change a bunch of other code for that to be land-able, though.
(DM/skiaserve/SampleApp/Viewer all use sRGB F16).

https://codereview.chromium.org/2270823002/diff/40001/src/gpu/GrDrawingManage...
File src/gpu/GrDrawingManager.cpp (right):

https://codereview.chromium.org/2270823002/diff/40001/src/gpu/GrDrawingManage...
src/gpu/GrDrawingManager.cpp:187: static bool
checkConfigAndColorSpace(GrPixelConfig config, SkColorSpace* colorSpace) {
I'm sad that I have this code in three places, now, but this version operates on
GrPixelConfig... sigh.

[Brian Osman, 2016-08-23 19:56:09]

Description was changed from

==========
Some tests around surface creation and snapshotting with color space

Verify the rules that we're converging on for surfaces:
- We only support color spaces on 8888 and F16 surfaces.
- For 8888, we only support sRGB-like gamma, or linear gamma.
- For F16, we ignore gamma, so we can technically support any color space.

NOTE: I have doubts about some of these - we need to decide what we're
going to allow or disallow. Comments in the review to explain.

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

to

==========
Some tests around surface creation and snapshotting with color space

Verify the rules that we're converging on for surfaces:
- For 8888, we only support sRGB-like gamma, or no color space at all.
- For F16, we require a color space, with linear gamma.
- For all other formats, we do not support color spaces.

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

[Brian Osman, 2016-08-23 20:49:11]

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

[commit-bot: I haz the power, 2016-08-23 20:49: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-23 20:57:03]

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

[commit-bot: I haz the power, 2016-08-23 20:57:04]

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

[mtklein, 2016-08-24 00:37:39]

This looks just like I was hoping.

On 2016/08/23 19:54:46, Brian Osman wrote:
> This version now requires F16 surfaces to have a colorspace with gamma 1.0.
I'm
> going to have to change a bunch of other code for that to be land-able,
though.

Thank you for doing the annoying work here.
Please pull me in tomorrow if you need extra eyes to fix up the remaining
failures.

>
https://codereview.chromium.org/2270823002/diff/40001/src/gpu/GrDrawingManage...
> src/gpu/GrDrawingManager.cpp:187: static bool
> checkConfigAndColorSpace(GrPixelConfig config, SkColorSpace* colorSpace) {
> I'm sad that I have this code in three places, now, but this version operates
on
> GrPixelConfig... sigh.

Me too.  :(

[Brian Osman, 2016-08-24 14:04:49]

On 2016/08/24 00:37:39, mtklein wrote:
> This looks just like I was hoping.
> 
> On 2016/08/23 19:54:46, Brian Osman wrote:
> > This version now requires F16 surfaces to have a colorspace with gamma 1.0.
> I'm
> > going to have to change a bunch of other code for that to be land-able,
> though.
> 
> Thank you for doing the annoying work here.
> Please pull me in tomorrow if you need extra eyes to fix up the remaining
> failures.
> 
> >
>
https://codereview.chromium.org/2270823002/diff/40001/src/gpu/GrDrawingManage...
> > src/gpu/GrDrawingManager.cpp:187: static bool
> > checkConfigAndColorSpace(GrPixelConfig config, SkColorSpace* colorSpace) {
> > I'm sad that I have this code in three places, now, but this version
operates
> on
> > GrPixelConfig... sigh.
> 
> Me too.  :(

Trying to figure out how to fix the testing errors without breaking all the
guarantees of safety. Basically, there are numerous code-paths where we
internally
want to make a draw context with a null color space, even for F16 or sRGB
(usually for things like read/write pixels). I can relax the restrictions, but
that risks letting incorrect usage in. I can hoist the checks up to the
top-level
API, but we have so many entry-points that ultimately lead to creating DCs and
devices, that I'm sure I'll miss some. Thinking about a two-pronged approach:

1) Leave a conservative check in place at GrDrawingManager::makeDrawContext.
   This would allow null color spaces in all cases, but enforce all other rules.
2) Add more thorough checks at the common entry-points:
   "SkSurface::MakeFromBackend(RenderTarget|Texture|TextureAsRenderTarget)"

... and then we have four sets of validation code. :(

[Brian Osman, 2016-08-25 16:47:20]

PTAL. Much better version (I think). I moved all the logic to SkSurface_Gpu (to
parallel the raster logic), with the same naming. We validate the full set of
user-facing rules in the four SkSurface (gpu) factories.

To handle all other (mostly internal/testing) code paths, we insert an extra
check in makeDrawContext that allows null color spaces in all cases, but
otherwise reuses the same helper code.

So we have three actual functions that implement the logic, but two of them are
adjacent, and they're all located in SkSurface_(Gpu|Raster).

[Brian Osman, 2016-08-25 16:47:30]

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

[commit-bot: I haz the power, 2016-08-25 16:47:39]

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-25 16:50:08]

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

[commit-bot: I haz the power, 2016-08-25 16:50:09]

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

[Brian Osman, 2016-08-25 17:34:31]

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

[commit-bot: I haz the power, 2016-08-25 17:34:40]

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-25 18:01:34]

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

[commit-bot: I haz the power, 2016-08-25 18:01:35]

Dry run: This issue passed the CQ dry run.

[Brian Osman, 2016-08-29 13:57:00]

I've fixed some more issues with ANGLE, so this should be ready to review/land.

[Brian Osman, 2016-08-29 14:50:22]

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

[commit-bot: I haz the power, 2016-08-29 14:50:31]

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-29 15:29:34]

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

[commit-bot: I haz the power, 2016-08-29 15:29:35]

Dry run: This issue passed the CQ dry run.

[bsalomon, 2016-08-30 13:10:17]

lgtm

https://codereview.chromium.org/2270823002/diff/160001/tests/SurfaceTest.cpp
File tests/SurfaceTest.cpp (right):

https://codereview.chromium.org/2270823002/diff/160001/tests/SurfaceTest.cpp#...
tests/SurfaceTest.cpp:981:
DEF_GPUTEST_FOR_RENDERING_CONTEXTS(SurfaceCreationWithColorSpace_Gpu, reporter,
ctxInfo) {
..FOR_ALL_CONTEXTS?

[Brian Osman, 2016-08-30 13:55:24]

On 2016/08/30 13:10:17, bsalomon wrote:
> lgtm
> 
> https://codereview.chromium.org/2270823002/diff/160001/tests/SurfaceTest.cpp
> File tests/SurfaceTest.cpp (right):
> 
>
https://codereview.chromium.org/2270823002/diff/160001/tests/SurfaceTest.cpp#...
> tests/SurfaceTest.cpp:981:
> DEF_GPUTEST_FOR_RENDERING_CONTEXTS(SurfaceCreationWithColorSpace_Gpu,
reporter,
> ctxInfo) {
> ..FOR_ALL_CONTEXTS?

I had that originally. Our debug GL context asserts in this test.
createTestingOnlyBackendTexture binds the newly created texture, then
SkSurface::MakeFromBackendTexture eventually needs to call FramebufferTexture2D.
The debug GL implementation asserts that the texture isn't bound ("could lead to
a feedback loop" according to the comment). Of course, in this situation, that's
not going to happen, and the reality is more complex, too - mipmap generation
does this (in a way that's blessed by the spec).

[Brian Osman, 2016-08-30 13:55:40]

The CQ bit was checked by brianosman@google.com

[commit-bot: I haz the power, 2016-08-30 13:55:46]

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-30 14:08:02]

Description was changed from

==========
Some tests around surface creation and snapshotting with color space

Verify the rules that we're converging on for surfaces:
- For 8888, we only support sRGB-like gamma, or no color space at all.
- For F16, we require a color space, with linear gamma.
- For all other formats, we do not support color spaces.

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

to

==========
Some tests around surface creation and snapshotting with color space

Verify the rules that we're converging on for surfaces:
- For 8888, we only support sRGB-like gamma, or no color space at all.
- For F16, we require a color space, with linear gamma.
- For all other formats, we do not support color spaces.

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

Committed:
https://skia.googlesource.com/skia/+/0e22eb8e6efc7d7ab7a601ba555947916d139906
==========

[commit-bot: I haz the power, 2016-08-30 14:08:03]

Committed patchset #9 (id:160001) as
https://skia.googlesource.com/skia/+/0e22eb8e6efc7d7ab7a601ba555947916d139906