Always supply a color space (sRGB for now) with F16

Always supply a color space (sRGB for now) with F16

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

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

[Brian Osman, 2016-07-25 19:58:55]

Description was changed from

==========
Always supply a color space (sRGB for now) with F16

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

to

==========
Always supply a color space (sRGB for now) with F16

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

[Brian Osman, 2016-07-25 20:00:16]

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

[commit-bot: I haz the power, 2016-07-25 20:00:26]

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-25 20:04:45]

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

[Brian Osman, 2016-07-25 20:04:45]

As discussed: In preparation for using the color space as the only signal for
gamma-correction (and basically requiring a color space with F16 rendering),
this fixes all of our tools to specify the sRGB color space in F16 mode.

This introduces a handful of changes across GMs and SKPs, but I think they're
all expected. Many raster F16 gms that involve text get "more correct" (which
was also going to happen based on mtklein's upcoming F16-via-new-pipeline
change).

And a few gpuf16 images change, because:
- The color space on the canvas gets propagated to off-screen surfaces used in
things like distance-field text, causing that surface to be sRGB rather than
legacy N32.
- The color space on the canvas triggers my recently added gamut-conversion code
for the mandrill filtering GMs, and a few others that have non-sRGB gamut
images.

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

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

[commit-bot: I haz the power, 2016-07-25 20:18:05]

Dry run: This issue passed the CQ dry run.

[mtklein, 2016-07-25 20:45:36]

> This introduces a handful of changes across GMs and SKPs, but I think they're
> all expected. Many raster F16 gms that involve text get "more correct" (which
> was also going to happen based on mtklein's upcoming F16-via-new-pipeline
> change).
I thought this would be true, but it turns out it's not.

I'm still puzzled by why these changes happen.

I agree f16 looks right after this and not before, but I don't understand why.

[mtklein, 2016-07-25 21:26:48]

On 2016/07/25 20:45:36, mtklein wrote:
> > This introduces a handful of changes across GMs and SKPs, but I think
they're
> > all expected. Many raster F16 gms that involve text get "more correct"
(which
> > was also going to happen based on mtklein's upcoming F16-via-new-pipeline
> > change).
> I thought this would be true, but it turns out it's not.
> 
> I'm still puzzled by why these changes happen.
> 
> I agree f16 looks right after this and not before, but I don't understand why.

Provided that there's not too much diff after
https://codereview.chromium.org/2184443002/, this lgtm.

If there's something we don't understand, I want to try to understand why it was
broken before landing.

[mtklein, 2016-07-26 01:26:07]

On 2016/07/25 21:26:48, mtklein wrote:
> On 2016/07/25 20:45:36, mtklein wrote:
> > > This introduces a handful of changes across GMs and SKPs, but I think
> they're
> > > all expected. Many raster F16 gms that involve text get "more correct"
> (which
> > > was also going to happen based on mtklein's upcoming F16-via-new-pipeline
> > > change).
> > I thought this would be true, but it turns out it's not.
> > 
> > I'm still puzzled by why these changes happen.
> > 
> > I agree f16 looks right after this and not before, but I don't understand
why.
> 
> Provided that there's not too much diff after
> https://codereview.chromium.org/2184443002/, this lgtm.
> 
> If there's something we don't understand, I want to try to understand why it
was
> broken before landing.

I see... 3 diffs, all of which go from having f16 draw wrong to drawing exactly
like sRGB.  Two of the three look like explicit surface allocation shenanigans
in the GM... what's the deal with the change to savelayer_with_backdrop? 
Something we got wrong there with the layer?

[Brian Osman, 2016-07-26 13:43:31]

On 2016/07/26 01:26:07, mtklein wrote:
> On 2016/07/25 21:26:48, mtklein wrote:
> > On 2016/07/25 20:45:36, mtklein wrote:
> > > > This introduces a handful of changes across GMs and SKPs, but I think
> > they're
> > > > all expected. Many raster F16 gms that involve text get "more correct"
> > (which
> > > > was also going to happen based on mtklein's upcoming
F16-via-new-pipeline
> > > > change).
> > > I thought this would be true, but it turns out it's not.
> > > 
> > > I'm still puzzled by why these changes happen.
> > > 
> > > I agree f16 looks right after this and not before, but I don't understand
> why.
> > 
> > Provided that there's not too much diff after
> > https://codereview.chromium.org/2184443002/, this lgtm.
> > 
> > If there's something we don't understand, I want to try to understand why it
> was
> > broken before landing.
> 
> I see... 3 diffs, all of which go from having f16 draw wrong to drawing
exactly
> like sRGB.  Two of the three look like explicit surface allocation shenanigans
> in the GM... what's the deal with the change to savelayer_with_backdrop? 
> Something we got wrong there with the layer?

Not sure. On my laptop, testing all configs (raster, gpu) x (legacy, srgb, f16),
I'm seeing 15 images that are actually different (and 1 with different bits).
But like you - all of them make sense except for the savelayer_with_backdrop.
Going to poke at that one a bit...

[Brian Osman, 2016-07-26 13:59:48]

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

[commit-bot: I haz the power, 2016-07-26 13:59:56]

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-26 14:21:42]

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

[commit-bot: I haz the power, 2016-07-26 14:21:43]

Dry run: This issue passed the CQ dry run.

[Brian Osman, 2016-07-26 15:09:34]

On 2016/07/26 13:43:31, Brian Osman wrote:
> On 2016/07/26 01:26:07, mtklein wrote:
> > On 2016/07/25 21:26:48, mtklein wrote:
> > > On 2016/07/25 20:45:36, mtklein wrote:
> > > > > This introduces a handful of changes across GMs and SKPs, but I think
> > > they're
> > > > > all expected. Many raster F16 gms that involve text get "more correct"
> > > (which
> > > > > was also going to happen based on mtklein's upcoming
> F16-via-new-pipeline
> > > > > change).
> > > > I thought this would be true, but it turns out it's not.
> > > > 
> > > > I'm still puzzled by why these changes happen.
> > > > 
> > > > I agree f16 looks right after this and not before, but I don't
understand
> > why.
> > > 
> > > Provided that there's not too much diff after
> > > https://codereview.chromium.org/2184443002/, this lgtm.
> > > 
> > > If there's something we don't understand, I want to try to understand why
it
> > was
> > > broken before landing.
> > 
> > I see... 3 diffs, all of which go from having f16 draw wrong to drawing
> exactly
> > like sRGB.  Two of the three look like explicit surface allocation
shenanigans
> > in the GM... what's the deal with the change to savelayer_with_backdrop? 
> > Something we got wrong there with the layer?
> 
> Not sure. On my laptop, testing all configs (raster, gpu) x (legacy, srgb,
f16),
> I'm seeing 15 images that are actually different (and 1 with different bits).
> But like you - all of them make sense except for the savelayer_with_backdrop.
> Going to poke at that one a bit...

Okay. I think I've found where things went wrong before (maybe). During the
filter,
we try to snap the source image to a raster-backed SkSpecialImage. That calls
SkSpecialImage::MakrFromRaster, which then tries to copy to an N32 temp:

   if (!bm.copyTo(&tmpStorage, kN32_SkColorType)) { ... }

SkBitmap::copyTo then constructs the info for the destination using this logic:

   const SkImageInfo dstInfo = srcPM.info().makeColorType(dstColorType);

At this point, srcPM is still the F16 source, and in the old code, has no
color space attached, so we end up with a legacy N32 temporary bitmap. I think
that leads us to pick a blitter for one of the early intermediate copies that
doesn't understand F16, leading to a completely empty filtered result.

In the new world (color space attached to F16), dstInfo ends up being sRGB,
so we go down the new world path, with blitters that know how to read F16...

Ultimately, all of this is just a small piece of the "image filters don't
understand F16 or operate at high precision" problem, but I think we can land
this, anyway.

[Brian Osman, 2016-07-26 15:10:45]

The CQ bit was checked by brianosman@google.com

[Brian Osman, 2016-07-26 15:10:46]

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/2177193004/#ps20001
(title: "Fix uploading of F16 textures with color spaces")

[commit-bot: I haz the power, 2016-07-26 15:10:55]

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-26 15:11:52]

Description was changed from

==========
Always supply a color space (sRGB for now) with F16

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

to

==========
Always supply a color space (sRGB for now) with F16

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

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

[commit-bot: I haz the power, 2016-07-26 15:11:53]

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

[mtklein, 2016-07-26 15:11:55]

On 2016/07/26 15:09:34, Brian Osman wrote:
> On 2016/07/26 13:43:31, Brian Osman wrote:
> > On 2016/07/26 01:26:07, mtklein wrote:
> > > On 2016/07/25 21:26:48, mtklein wrote:
> > > > On 2016/07/25 20:45:36, mtklein wrote:
> > > > > > This introduces a handful of changes across GMs and SKPs, but I
think
> > > > they're
> > > > > > all expected. Many raster F16 gms that involve text get "more
correct"
> > > > (which
> > > > > > was also going to happen based on mtklein's upcoming
> > F16-via-new-pipeline
> > > > > > change).
> > > > > I thought this would be true, but it turns out it's not.
> > > > > 
> > > > > I'm still puzzled by why these changes happen.
> > > > > 
> > > > > I agree f16 looks right after this and not before, but I don't
> understand
> > > why.
> > > > 
> > > > Provided that there's not too much diff after
> > > > https://codereview.chromium.org/2184443002/, this lgtm.
> > > > 
> > > > If there's something we don't understand, I want to try to understand
why
> it
> > > was
> > > > broken before landing.
> > > 
> > > I see... 3 diffs, all of which go from having f16 draw wrong to drawing
> > exactly
> > > like sRGB.  Two of the three look like explicit surface allocation
> shenanigans
> > > in the GM... what's the deal with the change to savelayer_with_backdrop? 
> > > Something we got wrong there with the layer?
> > 
> > Not sure. On my laptop, testing all configs (raster, gpu) x (legacy, srgb,
> f16),
> > I'm seeing 15 images that are actually different (and 1 with different
bits).
> > But like you - all of them make sense except for the
savelayer_with_backdrop.
> > Going to poke at that one a bit...
> 
> Okay. I think I've found where things went wrong before (maybe). During the
> filter,
> we try to snap the source image to a raster-backed SkSpecialImage. That calls
> SkSpecialImage::MakrFromRaster, which then tries to copy to an N32 temp:
> 
>    if (!bm.copyTo(&tmpStorage, kN32_SkColorType)) { ... }
> 
> SkBitmap::copyTo then constructs the info for the destination using this
logic:
> 
>    const SkImageInfo dstInfo = srcPM.info().makeColorType(dstColorType);
> 
> At this point, srcPM is still the F16 source, and in the old code, has no
> color space attached, so we end up with a legacy N32 temporary bitmap. I think
> that leads us to pick a blitter for one of the early intermediate copies that
> doesn't understand F16, leading to a completely empty filtered result.
> 
> In the new world (color space attached to F16), dstInfo ends up being sRGB,
> so we go down the new world path, with blitters that know how to read F16...
> 
> Ultimately, all of this is just a small piece of the "image filters don't
> understand F16 or operate at high precision" problem, but I think we can land
> this, anyway.

SGTM