Disabling mipmap generation until anisotropic mipmap levels are generated.

Disabling mipmap generation until anisotropic mipmap levels are generated.

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

Committed: https://skia.googlesource.com/skia/+/89685d9112cfdffb35d14fb18ddff38a18729587

[cblume, 2016-02-29 18:56:40]

Description was changed from

==========
Disabling mipmap generation until anisotropic mipmap levels are generated.

BUG=590804
==========

to

==========
Disabling mipmap generation until anisotropic mipmap levels are generated.

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

[cblume, 2016-02-29 18:57:13]

cblume@chromium.org changed reviewers:
+ bsalomon@google.com

[cblume, 2016-02-29 18:57:25]

The CQ bit was checked by cblume@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-29 18:57:37]

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

[commit-bot: I haz the power, 2016-02-29 18:57:38]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2016-03-01 00:57 UTC

[bsalomon, 2016-02-29 19:19:37]

lgtm

[commit-bot: I haz the power, 2016-02-29 19:20:54]

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

[commit-bot: I haz the power, 2016-02-29 19:20:55]

Dry run: This issue passed the CQ dry run.

[cblume, 2016-02-29 19:26:08]

The CQ bit was checked by cblume@chromium.org

[commit-bot: I haz the power, 2016-02-29 19:26:15]

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

[commit-bot: I haz the power, 2016-02-29 19:27:34]

Description was changed from

==========
Disabling mipmap generation until anisotropic mipmap levels are generated.

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

to

==========
Disabling mipmap generation until anisotropic mipmap levels are generated.

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

Committed:
https://skia.googlesource.com/skia/+/89685d9112cfdffb35d14fb18ddff38a18729587
==========

[commit-bot: I haz the power, 2016-02-29 19:27:34]

Committed patchset #1 (id:1) as
https://skia.googlesource.com/skia/+/89685d9112cfdffb35d14fb18ddff38a18729587

[Brian Osman, 2016-03-24 20:21:11]

On 2016/02/29 19:27:34, commit-bot: I haz the power wrote:
> Committed patchset #1 (id:1) as
> https://skia.googlesource.com/skia/+/89685d9112cfdffb35d14fb18ddff38a18729587

Can we revert this change, now that https://codereview.chromium.org/1750303002
has landed?

[Brian Osman, 2016-03-24 20:21:33]

Description was changed from

==========
Disabling mipmap generation until anisotropic mipmap levels are generated.

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

Committed:
https://skia.googlesource.com/skia/+/89685d9112cfdffb35d14fb18ddff38a18729587
==========

to

==========
Disabling mipmap generation until anisotropic mipmap levels are generated.

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

Committed:
https://skia.googlesource.com/skia/+/89685d9112cfdffb35d14fb18ddff38a18729587
==========

[bsalomon, 2016-03-24 20:38:27]

bsalomon@google.com changed reviewers:
+ cblume@chromium.org

[bsalomon, 2016-03-24 20:38:28]

Think so, but checking with Chris.

[cblume, 2016-03-24 20:54:16]

On 2016/03/24 20:38:28, bsalomon wrote:
> Think so, but checking with Chris.

Oh yes. Definitely. I thought I had already.

[Brian Osman, 2016-03-25 13:57:22]

On 2016/03/24 20:54:16, cblume wrote:
> On 2016/03/24 20:38:28, bsalomon wrote:
> > Think so, but checking with Chris.
> 
> Oh yes. Definitely. I thought I had already.

So I just tested doing this locally... It creates some regressions:

1) The mip-chains don't look identical to the GL ones, which is probably okay -
it would ensure that they're consistent across different GL implementations,
which is likely a net win.
2) It doesn't handle all texture formats. In particular, if it encounters
Index8, it fails, which ultimately results in us not making a texture at all, so
those elements are dropping out completely in various SKPs. The GL path expands
Index8 to RGBA8888 first, and then ends up generating mips of that data.
Ultimately, we probably need to do the expansion - Index8 isn't a texturable
format on many devices these days. (We've also added things like F16 recently,
and that needs to be handled in SkMipMap).
3) It doesn't handle sRGB. This is a bigger issue that I need to discuss with
reed@ when I get in the office later today... I spoke with Brian about it last
night, but it's fairly tricky to get right.

So... my simple workaround proposal:

In the two locations where we attempt to use the CPU mipmap generation path, we
fall back to the GPU path if it fails for any reason. That would include the
current failure when it sees Index8 data). In addition until the CPU mip-mapping
code handles sRGB, we have it fail in that situation, too (triggering the GL
fallback), so that GL can do the right thing there. Make sense?

[bsalomon, 2016-03-25 14:06:42]

On 2016/03/25 13:57:22, Brian Osman wrote:
> On 2016/03/24 20:54:16, cblume wrote:
> > On 2016/03/24 20:38:28, bsalomon wrote:
> > > Think so, but checking with Chris.
> > 
> > Oh yes. Definitely. I thought I had already.
> 
> So I just tested doing this locally... It creates some regressions:
> 
> 1) The mip-chains don't look identical to the GL ones, which is probably okay
-
> it would ensure that they're consistent across different GL implementations,
> which is likely a net win.
> 2) It doesn't handle all texture formats. In particular, if it encounters
> Index8, it fails, which ultimately results in us not making a texture at all,
so
> those elements are dropping out completely in various SKPs. The GL path
expands
> Index8 to RGBA8888 first, and then ends up generating mips of that data.
> Ultimately, we probably need to do the expansion - Index8 isn't a texturable
> format on many devices these days. (We've also added things like F16 recently,
> and that needs to be handled in SkMipMap).
> 3) It doesn't handle sRGB. This is a bigger issue that I need to discuss with
> reed@ when I get in the office later today... I spoke with Brian about it last
> night, but it's fairly tricky to get right.
> 
> So... my simple workaround proposal:
> 
> In the two locations where we attempt to use the CPU mipmap generation path,
we
> fall back to the GPU path if it fails for any reason. That would include the
> current failure when it sees Index8 data). In addition until the CPU
mip-mapping
> code handles sRGB, we have it fail in that situation, too (triggering the GL
> fallback), so that GL can do the right thing there. Make sense?

That sounds like a good plan to me. Will the sRGB failure have adverse effects
on the sw backend? That is, would it cause MIP mapping to not work at all on
sRGB sources for sw, and is sw ok with that if so?

[Brian Osman, 2016-03-25 14:22:10]

On 2016/03/25 14:06:42, bsalomon wrote:
> On 2016/03/25 13:57:22, Brian Osman wrote:
> > On 2016/03/24 20:54:16, cblume wrote:
> > > On 2016/03/24 20:38:28, bsalomon wrote:
> > > > Think so, but checking with Chris.
> > > 
> > > Oh yes. Definitely. I thought I had already.
> > 
> > So I just tested doing this locally... It creates some regressions:
> > 
> > 1) The mip-chains don't look identical to the GL ones, which is probably
okay
> -
> > it would ensure that they're consistent across different GL implementations,
> > which is likely a net win.
> > 2) It doesn't handle all texture formats. In particular, if it encounters
> > Index8, it fails, which ultimately results in us not making a texture at
all,
> so
> > those elements are dropping out completely in various SKPs. The GL path
> expands
> > Index8 to RGBA8888 first, and then ends up generating mips of that data.
> > Ultimately, we probably need to do the expansion - Index8 isn't a texturable
> > format on many devices these days. (We've also added things like F16
recently,
> > and that needs to be handled in SkMipMap).
> > 3) It doesn't handle sRGB. This is a bigger issue that I need to discuss
with
> > reed@ when I get in the office later today... I spoke with Brian about it
last
> > night, but it's fairly tricky to get right.
> > 
> > So... my simple workaround proposal:
> > 
> > In the two locations where we attempt to use the CPU mipmap generation path,
> we
> > fall back to the GPU path if it fails for any reason. That would include the
> > current failure when it sees Index8 data). In addition until the CPU
> mip-mapping
> > code handles sRGB, we have it fail in that situation, too (triggering the GL
> > fallback), so that GL can do the right thing there. Make sense?
> 
> That sounds like a good plan to me. Will the sRGB failure have adverse effects
> on the sw backend? That is, would it cause MIP mapping to not work at all on
> sRGB sources for sw, and is sw ok with that if so?

Ah, yeah. That will definitely cause issues for raster. I can try hoisting the
check one level up so it only affects Ganesh...