Manually generated sRGB mipmaps.

Manually generated sRGB mipmaps, with successively smaller draws.

Dirty GL-generated mipmaps whenever an sRGB texture is used with a new
value for TEXTURE_SRGB_DECODE. Add a new test rectangle to the gamma GM
that tests that textures are correctly converted to linear before
filtering when generating mipmaps.

Added a new unit test that alternates how a texture is interpreted (sRGB
or not), to verify that we rebuild mipmaps when needed, and that we get
the correct results out in both modes.

This test originally failed on four of our bots producing incorrect mips
in three different ways. I'm not real surprised, but it looks like
we can't rely on glGenerateMipmap to do the right thing, in conjunction
with TEXTURE_SRGB_DECODE.

Instead, actually create mip-chains using a series of draw calls.
(My first attempt used glBlitFramebuffer, and that still had bugs on
several bots). This approach appears to work correctly on any device
that fully supports sRGB.

Because the mipmap draws are fairly destructive to state, I had to
hoist them out of bindTexture. That means adding a second pass over
the texture accesses in the processor, at the very beginning of flush.

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

Committed: https://skia.googlesource.com/skia/+/33f6b3f6ee4de24282f5e7f2dc31a5f538bcf40c

[Brian Osman, 2016-05-27 20:30:03]

Description was changed from

==========
Manually generated sRGB mipmaps.

Dirty GL-generated mipmaps whenever a texture is used with a new value
for TEXTURE_SRGB_DECODE. Add a new test rectangle to the gamma GM that
tests that textures are correctly converted to linear before filtering
when generating mipmaps.

Raster backend currently ignores sRGB tag on inputs, so it fails this
new test. GL works correctly. I would like to test that using the same
texture as both sRGB and Linear works correctly (switching back and
forth re-builds mipmaps appropriately). My various attempts at testing
that fell flat - given the number of other conversions that happen, and
the amount of behind-the-scenes code that's running, it's very difficult
to isolate that behavior.

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

patch from issue 1840473002 at patchset 200001
(http://crrev.com/1840473002#ps200001)
==========

to

==========
Manually generated sRGB mipmaps, with successively smaller draws.

Dirty GL-generated mipmaps whenever an sRGB texture is used with a new
value for TEXTURE_SRGB_DECODE. Add a new test rectangle to the gamma GM
that tests that textures are correctly converted to linear before
filtering when generating mipmaps.

Added a new unit test that alternates how a texture is interpreted (sRGB
or not), to verify that we rebuild mipmaps when needed, and that we get
the correct results out in both modes.

This test originally failed on four of our bots producing incorrect mips
in three different ways. I'm not real surprised, but it looks like
we can't rely on glGenerateMipmap to do the right thing, in conjunction
with TEXTURE_SRGB_DECODE.

Instead, actually create mip-chains using a series of draw calls.
(My first attempt used glBlitFramebuffer, and that still had bugs on
several bots). This approach appears to work correctly on any device
that supports sRGB (NOTE: Except for devices without woking sRGB write
control, like Adreno4xx). We will need to opt-in (or out) of this path.

Because the mipmap draws are fairly destructive to state, I had to
hoist them out of bindTexture. That means adding a second pass over
the texture accesses in the processor, at the very beginning of flush.

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

[Brian Osman, 2016-05-27 20:33:06]

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

[Brian Osman, 2016-05-27 20:33:06]

This isn't ready to go yet - I still have some debugging code in there, and we
need to do some performance comparisons (especially on tilers). In addition,
because the draw-call based mips require sRGB write control to produce the right
results, I need to make sure we have that capability - which means not taking
this path on Adreno4xx, and possibly others.

Regardless, I wanted to post this to show you the structural changes I had to
make (mostly hoisting the mipmap generation out to happen before setData and
bindTexture) - it means a second pass over all the textures in the processor. :(

[Brian Osman, 2016-05-27 20:37:12]

https://codereview.chromium.org/2007973002/diff/100001/include/gpu/GrTexture.h
File include/gpu/GrTexture.h (right):

https://codereview.chromium.org/2007973002/diff/100001/include/gpu/GrTexture....
include/gpu/GrTexture.h:56: void dirtyMipMaps(bool mipMapsDirty, bool
sRGBCorrect);
I think Brian had previously mentioned making this a separate API call - I'll
probably do that.

https://codereview.chromium.org/2007973002/diff/100001/src/gpu/gl/GrGLCreateN...
File src/gpu/gl/GrGLCreateNullInterface.cpp (right):

https://codereview.chromium.org/2007973002/diff/100001/src/gpu/gl/GrGLCreateN...
src/gpu/gl/GrGLCreateNullInterface.cpp:261: case GR_GL_READ_FRAMEBUFFER_BINDING:
Leftover from the blit implementation, I'll remove this.

https://codereview.chromium.org/2007973002/diff/100001/src/gpu/gl/GrGLDefines.h
File src/gpu/gl/GrGLDefines.h (right):

https://codereview.chromium.org/2007973002/diff/100001/src/gpu/gl/GrGLDefines...
src/gpu/gl/GrGLDefines.h:852: #define GR_GL_READ_FRAMEBUFFER_BINDING      
0x8CAA
More leftovers from blit version. I'll remove this.

[Brian Osman, 2016-05-31 15:38:24]

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

[commit-bot: I haz the power, 2016-05-31 15:38:35]

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

[commit-bot: I haz the power, 2016-05-31 16:01:36]

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

[commit-bot: I haz the power, 2016-05-31 16:01:37]

Dry run: This issue passed the CQ dry run.

[bsalomon, 2016-05-31 16:35:03]

Wondering whether we can't do the gen and bind in one step. If we gen and then
bind one texture do we mess up state for the next texture?

https://codereview.chromium.org/2007973002/diff/120001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/2007973002/diff/120001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2656: void GrGLGpu::setFramebufferSRGB(bool enable) {
Just for consistency, could we call this flushFramebufferSRGB? (Or if you really
want to rename all the others to set that'd be ok too :) )

https://codereview.chromium.org/2007973002/diff/140001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/2007973002/diff/140001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2650:
setFramebufferSRGB(GrPixelConfigIsSRGB(target->config()) && !disableSRGB);
this->

https://codereview.chromium.org/2007973002/diff/140001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:4147: // We only handle 2D textures for now:
Do we need to check the format? (for renderability).

https://codereview.chromium.org/2007973002/diff/140001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:4183: // Define the next mip:
Aren't we supposed to round up the odds?

https://codereview.chromium.org/2007973002/diff/140001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:4205: this->setTextureUnit(0);
I think we have some notion of a scratch texture unit used for non-GrGLProgram
related things.

https://codereview.chromium.org/2007973002/diff/140001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:4265: this->didWriteToSurface(texture, nullptr,
levelCount);
Do we need this? IIRC we use this to track whether we need to resolve or
generate MIP levels, neither of which should be true here.

[bsalomon, 2016-05-31 16:38:41]

https://codereview.chromium.org/2007973002/diff/140001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/2007973002/diff/140001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:4183: // Define the next mip:
On 2016/05/31 16:35:03, bsalomon wrote:
> Aren't we supposed to round up the odds?

Err, duh, withdrawn.

[Brian Osman, 2016-05-31 17:33:05]

New version addresses a bunch of the feedback.

Re: Doing mipmap and binding in one pass...
I'm not sure there's a place we can safely do it all in one pass. bindTexture
was previously called from setData() in flushGLState, after binding the program
and configuring blend info, etc... It seems like we could move all of the global
state (blending, color write, etc...) around without any trouble, but we need
the program bound before setData for uniform and texture binding, right? Seems
like the simplest single-pass version would still have to support
remembering/restoring the bound program, but leave everything else dirty (and
then re-flushed in flushGLState?)

https://codereview.chromium.org/2007973002/diff/120001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/2007973002/diff/120001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2656: void GrGLGpu::setFramebufferSRGB(bool enable) {
On 2016/05/31 16:35:02, bsalomon wrote:
> Just for consistency, could we call this flushFramebufferSRGB? (Or if you
really
> want to rename all the others to set that'd be ok too :) )

Done.

https://codereview.chromium.org/2007973002/diff/140001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/2007973002/diff/140001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:2650:
setFramebufferSRGB(GrPixelConfigIsSRGB(target->config()) && !disableSRGB);
On 2016/05/31 16:35:02, bsalomon wrote:
> this->

Done.

https://codereview.chromium.org/2007973002/diff/140001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:4147: // We only handle 2D textures for now:
On 2016/05/31 16:35:03, bsalomon wrote:
> Do we need to check the format? (for renderability).

Yes.

https://codereview.chromium.org/2007973002/diff/140001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:4205: this->setTextureUnit(0);
On 2016/05/31 16:35:03, bsalomon wrote:
> I think we have some notion of a scratch texture unit used for non-GrGLProgram
> related things.

We do, although the index of that unit isn't a simple constant:

  fHWBoundTextureUniqueIDs.count() - 1

I was using the scratch unit in some other code (and I could bring it
back here). I was basing this code off copySurfaceAsDraw, which uses
unit zero for the source texture.

https://codereview.chromium.org/2007973002/diff/140001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:4265: this->didWriteToSurface(texture, nullptr,
levelCount);
On 2016/05/31 16:35:03, bsalomon wrote:
> Do we need this? IIRC we use this to track whether we need to resolve or
> generate MIP levels, neither of which should be true here.

Ah, true. I was basing this off the copySurfaceAsXXX methods, and
originally thinking that resolve would be an issue, but it's obviously
not. Good catch.

[bsalomon, 2016-05-31 19:45:47]

https://codereview.chromium.org/2007973002/diff/140001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/2007973002/diff/140001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:4205: this->setTextureUnit(0);
On 2016/05/31 17:33:05, Brian Osman wrote:
> On 2016/05/31 16:35:03, bsalomon wrote:
> > I think we have some notion of a scratch texture unit used for
non-GrGLProgram
> > related things.
> 
> We do, although the index of that unit isn't a simple constant:
> 
>   fHWBoundTextureUniqueIDs.count() - 1
> 
> I was using the scratch unit in some other code (and I could bring it
> back here). I was basing this code off copySurfaceAsDraw, which uses
> unit zero for the source texture.

sgtm

https://codereview.chromium.org/2007973002/diff/160001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/2007973002/diff/160001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:3179: // We were supposed to ensure MipMaps were
up-to-date and built correctly before getting here.
Perhaps here we should just assert that the they're not dirty?

https://codereview.chromium.org/2007973002/diff/160001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:4140: // Uses glBlitFramebuffer to do a series of
downsample operations to successive mips.
comment is out of date

Maybe say that if this returns false then we fall back to calling
glGenerateMipmap?

[Brian Osman, 2016-06-01 13:24:58]

I made a couple fixes, and I'm currently working on two larger enhancements:
Using different filter shapes based on the parity of the width/height (to match
what CPU does, at least for now), and supporting external and rectangle
textures...

https://codereview.chromium.org/2007973002/diff/160001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/2007973002/diff/160001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:3179: // We were supposed to ensure MipMaps were
up-to-date and built correctly before getting here.
On 2016/05/31 19:45:46, bsalomon wrote:
> Perhaps here we should just assert that the they're not dirty?

That's all the code below is doing. (Plus the second check that they're
built correctly wrt gamma).

https://codereview.chromium.org/2007973002/diff/160001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:4140: // Uses glBlitFramebuffer to do a series of
downsample operations to successive mips.
On 2016/05/31 19:45:46, bsalomon wrote:
> comment is out of date
> 
> Maybe say that if this returns false then we fall back to calling
> glGenerateMipmap?

Done.

[Brian Osman, 2016-06-01 15:11:36]

Description was changed from

==========
Manually generated sRGB mipmaps, with successively smaller draws.

Dirty GL-generated mipmaps whenever an sRGB texture is used with a new
value for TEXTURE_SRGB_DECODE. Add a new test rectangle to the gamma GM
that tests that textures are correctly converted to linear before
filtering when generating mipmaps.

Added a new unit test that alternates how a texture is interpreted (sRGB
or not), to verify that we rebuild mipmaps when needed, and that we get
the correct results out in both modes.

This test originally failed on four of our bots producing incorrect mips
in three different ways. I'm not real surprised, but it looks like
we can't rely on glGenerateMipmap to do the right thing, in conjunction
with TEXTURE_SRGB_DECODE.

Instead, actually create mip-chains using a series of draw calls.
(My first attempt used glBlitFramebuffer, and that still had bugs on
several bots). This approach appears to work correctly on any device
that supports sRGB (NOTE: Except for devices without woking sRGB write
control, like Adreno4xx). We will need to opt-in (or out) of this path.

Because the mipmap draws are fairly destructive to state, I had to
hoist them out of bindTexture. That means adding a second pass over
the texture accesses in the processor, at the very beginning of flush.

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

to

==========
Manually generated sRGB mipmaps, with successively smaller draws.

Dirty GL-generated mipmaps whenever an sRGB texture is used with a new
value for TEXTURE_SRGB_DECODE. Add a new test rectangle to the gamma GM
that tests that textures are correctly converted to linear before
filtering when generating mipmaps.

Added a new unit test that alternates how a texture is interpreted (sRGB
or not), to verify that we rebuild mipmaps when needed, and that we get
the correct results out in both modes.

This test originally failed on four of our bots producing incorrect mips
in three different ways. I'm not real surprised, but it looks like
we can't rely on glGenerateMipmap to do the right thing, in conjunction
with TEXTURE_SRGB_DECODE.

Instead, actually create mip-chains using a series of draw calls.
(My first attempt used glBlitFramebuffer, and that still had bugs on
several bots). This approach appears to work correctly on any device
that fully supports sRGB.

Because the mipmap draws are fairly destructive to state, I had to
hoist them out of bindTexture. That means adding a second pass over
the texture accesses in the processor, at the very beginning of flush.

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

[Brian Osman, 2016-06-01 15:19:08]

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

[commit-bot: I haz the power, 2016-06-01 15:19:20]

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

[commit-bot: I haz the power, 2016-06-01 15:36:43]

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

[commit-bot: I haz the power, 2016-06-01 15:36:44]

Dry run: This issue passed the CQ dry run.

[bsalomon, 2016-06-01 15:41:28]

lgtm

https://codereview.chromium.org/2007973002/diff/160001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/2007973002/diff/160001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:3179: // We were supposed to ensure MipMaps were
up-to-date and built correctly before getting here.
On 2016/06/01 13:24:58, Brian Osman wrote:
> On 2016/05/31 19:45:46, bsalomon wrote:
> > Perhaps here we should just assert that the they're not dirty?
> 
> That's all the code below is doing. (Plus the second check that they're
> built correctly wrt gamma).

Oh duh

[Brian Osman, 2016-06-02 12:48:17]

The CQ bit was checked by brianosman@google.com

[commit-bot: I haz the power, 2016-06-02 12:48:28]

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

[commit-bot: I haz the power, 2016-06-02 12:49:26]

Description was changed from

==========
Manually generated sRGB mipmaps, with successively smaller draws.

Dirty GL-generated mipmaps whenever an sRGB texture is used with a new
value for TEXTURE_SRGB_DECODE. Add a new test rectangle to the gamma GM
that tests that textures are correctly converted to linear before
filtering when generating mipmaps.

Added a new unit test that alternates how a texture is interpreted (sRGB
or not), to verify that we rebuild mipmaps when needed, and that we get
the correct results out in both modes.

This test originally failed on four of our bots producing incorrect mips
in three different ways. I'm not real surprised, but it looks like
we can't rely on glGenerateMipmap to do the right thing, in conjunction
with TEXTURE_SRGB_DECODE.

Instead, actually create mip-chains using a series of draw calls.
(My first attempt used glBlitFramebuffer, and that still had bugs on
several bots). This approach appears to work correctly on any device
that fully supports sRGB.

Because the mipmap draws are fairly destructive to state, I had to
hoist them out of bindTexture. That means adding a second pass over
the texture accesses in the processor, at the very beginning of flush.

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

to

==========
Manually generated sRGB mipmaps, with successively smaller draws.

Dirty GL-generated mipmaps whenever an sRGB texture is used with a new
value for TEXTURE_SRGB_DECODE. Add a new test rectangle to the gamma GM
that tests that textures are correctly converted to linear before
filtering when generating mipmaps.

Added a new unit test that alternates how a texture is interpreted (sRGB
or not), to verify that we rebuild mipmaps when needed, and that we get
the correct results out in both modes.

This test originally failed on four of our bots producing incorrect mips
in three different ways. I'm not real surprised, but it looks like
we can't rely on glGenerateMipmap to do the right thing, in conjunction
with TEXTURE_SRGB_DECODE.

Instead, actually create mip-chains using a series of draw calls.
(My first attempt used glBlitFramebuffer, and that still had bugs on
several bots). This approach appears to work correctly on any device
that fully supports sRGB.

Because the mipmap draws are fairly destructive to state, I had to
hoist them out of bindTexture. That means adding a second pass over
the texture accesses in the processor, at the very beginning of flush.

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/+/33f6b3f6ee4de24282f5e7f2dc31a5f538bcf40c
==========

[commit-bot: I haz the power, 2016-06-02 12:49:28]

Committed patchset #12 (id:220001) as
https://skia.googlesource.com/skia/+/33f6b3f6ee4de24282f5e7f2dc31a5f538bcf40c