Decouple contrast boost from fake gamma.

Decouple contrast boost from fake gamma.

Replace FakeGamma with FontWeightFlags. Largely mechanical, except for
the changes in ignorePreBlend. With this change, text GMs generally look
the same (or at least more similar) in 8888/srgb/gpu/gpusrgb configs.

No public API changes.
TBR=reed@google.com

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

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

[Brian Osman, 2016-04-07 16:52:20]

Description was changed from

==========
Decouple contrast boost from fake gamma.

Replace FakeGamma with FontWeightFlags. Largely mechanical, except for
the changes in ignorePreBlend. With this change, text GMs generally look
the same in gpu and gpusrgb configs.

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

to

==========
Decouple contrast boost from fake gamma.

Replace FakeGamma with FontWeightFlags. Largely mechanical, except for
the changes in ignorePreBlend. With this change, text GMs generally look
the same in gpu and gpusrgb configs.

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

[Brian Osman, 2016-04-07 16:53:37]

Description was changed from

==========
Decouple contrast boost from fake gamma.

Replace FakeGamma with FontWeightFlags. Largely mechanical, except for
the changes in ignorePreBlend. With this change, text GMs generally look
the same in gpu and gpusrgb configs.

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

to

==========
Decouple contrast boost from fake gamma.

Replace FakeGamma with FontWeightFlags. Largely mechanical, except for
the changes in ignorePreBlend. With this change, text GMs generally look
the same (or at least more similar) in 8888/srgb/gpu/gpusrgb configs.

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

[Brian Osman, 2016-04-07 17:08:31]

brianosman@google.com changed reviewers:
+ bsalomon@google.com, bungeman@google.com, jvanverth@google.com

[Brian Osman, 2016-04-07 17:08:32]

More improvements to sRGB font rendering. This keeps the contrast hack intact.
Largely mechanical, although there are a couple spots (especially the SkFontHost
calls) I'm not sure about. We have gamma-applied-to-A8, so that code doesn't
even run. If it *did*, then we'd be choosing not to apply the fake-gamma ramp to
those masks (why is this option separate from other non-A8 fake-gamma choices in
the first place?). And in that case, should we really be ignoring the contrast
hack, too? I assume so - presumably whatever client wants the A8 masks built
like that really wants them "pure"?

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

https://codereview.chromium.org/1866293003/diff/1/src/core/SkPaint.cpp#newcod...
src/core/SkPaint.cpp:2252: fCache = fPaint.detachCache(nullptr,
SkPaint::kFakeGammaAndContrast_FontWeightFlag, nullptr);
It seems like the choice of flags here may be arbitrary? SkTextInterceptsIter
looks like it just relies on the font metrics, and SkTextToPathIter just gets
the pathified version of glyphs. It doesn't look like either of those care about
the mask?

[Brian Osman, 2016-04-07 17:32:44]

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

[commit-bot: I haz the power, 2016-04-07 17:32:49]

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

[commit-bot: I haz the power, 2016-04-07 17:52:00]

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

[commit-bot: I haz the power, 2016-04-07 17:52:01]

Dry run: This issue passed the CQ dry run.

[bungeman-skia, 2016-04-07 18:35:09]

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

https://codereview.chromium.org/1866293003/diff/1/include/core/SkPaint.h#newc...
include/core/SkPaint.h:1088: enum FontWeightFlags {
Eck... the name. I liked the term 'FakeGamma' because while this is used with
fonts, it could just as well be applied to any other drawing operation as well.
Indeed, if reed had more time for such things, this would be applied to
hairlines. In other words, attaching 'Font' to the name sees overly specific. I
liked my old 'leakyProperties' name because it better stated that these were all
things that the device was supposed to handle but wasn't going to, so try to do
the best one can to make up for it. So I think I would prefer the name to be
something like

enum ThingsTheBlitterShouldHandleButCant {
  Gamma,
  BoostedContrast,
}

In other words, I don't want anyone ever thinking that the scaler context / font
system *should* ever know any of this information; it very much should not, it's
completely a hack that it does.

Note that none of the above is quite right, I'll probably have to talk about it.

https://codereview.chromium.org/1866293003/diff/1/include/core/SkPaint.h#newc...
include/core/SkPaint.h:1093: kFakeGammaAndContrast_FontWeightFlag =
nit: the name says flag... but it isn't really a flag. Ick.

[Brian Osman, 2016-04-07 19:48:03]

Renamed everything after discussing with bungeman.

[bungeman-skia, 2016-04-07 20:15:45]

https://codereview.chromium.org/1866293003/diff/40001/include/core/SkPaint.h
File include/core/SkPaint.h (right):

https://codereview.chromium.org/1866293003/diff/40001/include/core/SkPaint.h#...
include/core/SkPaint.h:1088: enum ScalerContextFlags {
It seems we're always passing these around as uint32_t, but this is signed int.
Why not make it 'enum ScalerContextFlags : uint32_t' since we can?

https://codereview.chromium.org/1866293003/diff/40001/src/core/SkPaint.cpp
File src/core/SkPaint.cpp (right):

https://codereview.chromium.org/1866293003/diff/40001/src/core/SkPaint.cpp#ne...
src/core/SkPaint.cpp:1520: rec->ignorePreBlend(false);
At the call site it's annoying that the bool parameter is non-descriptively
'false'. We could make this an enum, or we could split this into ignoreGamma and
setContrast(0), with ignorePreBlend calling both ignoreGamma and setContrast(0).
That way all of these ignorePreBlend(false) turn into ignorePreBlend(). Then in
fill_out_rec, the current ignorePreBlend becomes ignoreGamma, and then adds 'if
(boostContrast) { setContrast(0); }' right after.

https://codereview.chromium.org/1866293003/diff/40001/src/ports/SkFontHost_Fr...
File src/ports/SkFontHost_FreeType.cpp (right):

https://codereview.chromium.org/1866293003/diff/40001/src/ports/SkFontHost_Fr...
src/ports/SkFontHost_FreeType.cpp:717: // SRGBTODO: Is this correct?
Yeah... so by default we don't apply the gamma stuff to A8, but that means we
also don't apply the contrast. The idea being that if you didn't ask for gamma
you were going to do the linear blit anyway, and that linear blit would provide
the 'contrast' (at least for the common case of black on white). I don't know
what the right thing is here either.

https://codereview.chromium.org/1866293003/diff/40001/src/ports/SkFontHost_ma...
File src/ports/SkFontHost_mac.cpp (right):

https://codereview.chromium.org/1866293003/diff/40001/src/ports/SkFontHost_ma...
src/ports/SkFontHost_mac.cpp:2033: // SRGBTODO: Is this correct?
This one is similar and was designed around mimicking the system here, which is
that it blits non-lcd text from original outlines but with a gamma of 1.0.

[Brian Osman, 2016-04-07 21:23:46]

Minor tweaks from review.

https://codereview.chromium.org/1866293003/diff/40001/include/core/SkPaint.h
File include/core/SkPaint.h (right):

https://codereview.chromium.org/1866293003/diff/40001/include/core/SkPaint.h#...
include/core/SkPaint.h:1088: enum ScalerContextFlags {
On 2016/04/07 20:15:45, bungeman1 wrote:
> It seems we're always passing these around as uint32_t, but this is signed
int.
> Why not make it 'enum ScalerContextFlags : uint32_t' since we can?

Done. (I like this idea, although we don't currently do that anywhere else that
I can find. We have lots of flag enums that are stored/manipulated as uint32_t
everywhere, but where no storage class is assigned to the enum itself).

https://codereview.chromium.org/1866293003/diff/40001/src/core/SkPaint.cpp
File src/core/SkPaint.cpp (right):

https://codereview.chromium.org/1866293003/diff/40001/src/core/SkPaint.cpp#ne...
src/core/SkPaint.cpp:1520: rec->ignorePreBlend(false);
On 2016/04/07 20:15:45, bungeman1 wrote:
> At the call site it's annoying that the bool parameter is non-descriptively
> 'false'. We could make this an enum, or we could split this into ignoreGamma
and
> setContrast(0), with ignorePreBlend calling both ignoreGamma and
setContrast(0).
> That way all of these ignorePreBlend(false) turn into ignorePreBlend(). Then
in
> fill_out_rec, the current ignorePreBlend becomes ignoreGamma, and then adds
'if
> (boostContrast) { setContrast(0); }' right after.

Done. (Yeah, I like the broken-out API better).

[bungeman-skia, 2016-04-07 22:03:52]

I have now stared at this until I have acquired Stockholm Syndrome and think it
looks good. So if it also looks good, lgtm.

[bungeman-skia, 2016-04-07 22:04:06]

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

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

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

[commit-bot: I haz the power, 2016-04-07 22:14:28]

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

[commit-bot: I haz the power, 2016-04-07 22:14:28]

Dry run: This issue passed the CQ dry run.

[Brian Osman, 2016-04-08 13:29:12]

The CQ bit was checked by brianosman@google.com

[commit-bot: I haz the power, 2016-04-08 13:29:24]

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

[commit-bot: I haz the power, 2016-04-08 13:30:54]

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

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

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

[Brian Osman, 2016-04-08 13:46:33]

Description was changed from

==========
Decouple contrast boost from fake gamma.

Replace FakeGamma with FontWeightFlags. Largely mechanical, except for
the changes in ignorePreBlend. With this change, text GMs generally look
the same (or at least more similar) in 8888/srgb/gpu/gpusrgb configs.

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

to

==========
Decouple contrast boost from fake gamma.

Replace FakeGamma with FontWeightFlags. Largely mechanical, except for
the changes in ignorePreBlend. With this change, text GMs generally look
the same (or at least more similar) in 8888/srgb/gpu/gpusrgb configs.

No public API changes.
TBR=reed@google.com

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

[Brian Osman, 2016-04-08 13:46:46]

The CQ bit was checked by brianosman@google.com

[commit-bot: I haz the power, 2016-04-08 13:46:57]

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

[commit-bot: I haz the power, 2016-04-08 13:48:10]

Description was changed from

==========
Decouple contrast boost from fake gamma.

Replace FakeGamma with FontWeightFlags. Largely mechanical, except for
the changes in ignorePreBlend. With this change, text GMs generally look
the same (or at least more similar) in 8888/srgb/gpu/gpusrgb configs.

No public API changes.
TBR=reed@google.com

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

to

==========
Decouple contrast boost from fake gamma.

Replace FakeGamma with FontWeightFlags. Largely mechanical, except for
the changes in ignorePreBlend. With this change, text GMs generally look
the same (or at least more similar) in 8888/srgb/gpu/gpusrgb configs.

No public API changes.
TBR=reed@google.com

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/+/a1e8f8d8f1a0b7141136d7d49df1cc2fec0528df
==========

[commit-bot: I haz the power, 2016-04-08 13:48:13]

Committed patchset #4 (id:50016) as
https://skia.googlesource.com/skia/+/a1e8f8d8f1a0b7141136d7d49df1cc2fec0528df