Gamma correction for distance field text.

Gamma correction for distance field text.

Handles both non-LCD and LCD text. Uses a texture to look up the gamma correction values for a given text color or luminance.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/4d517fdbb145cb95e5e935470df331e1b6667cfc

Committed: https://skia.googlesource.com/skia/+/2d2a68c51b4a71bd60760510bf2b2e58bc9890b2

[jvanverth1, 2014-05-28 19:28:49]

After testing on many platforms, I believe this is good to go. The part I'm
feeling a little squirrely about is using the distance field flag in the SkPaint
to drive how we reset the SkScalerContext::Rec for distance fields. However, I
can't think of a better way to pass a flag deep down into the SkScalerContext
setup. Suggestions welcome, and PTAL.

[reed1, 2014-05-28 19:45:16]

https://codereview.chromium.org/258883002/diff/330001/src/core/SkMaskGamma.h
File src/core/SkMaskGamma.h (right):

https://codereview.chromium.org/258883002/diff/330001/src/core/SkMaskGamma.h#...
src/core/SkMaskGamma.h:143: const uint8_t* getGammaTables(int& tableWidth, int&
numTables) const {
style: skia always uses pointers for out-params

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

https://codereview.chromium.org/258883002/diff/330001/src/core/SkPaint.cpp#ne...
src/core/SkPaint.cpp:1820: rec->fFlags &=
~SkScalerContext::kForceAutohinting_Flag;
other than preblend, this rest of this change seems unrelated to gamma. How were
we working before w/o wacking all of these flags?

https://codereview.chromium.org/258883002/diff/330001/src/core/SkPaint.cpp#ne...
src/core/SkPaint.cpp:1994: void* SkScalerContext::getGammaLUTData(int& width,
int& height, SkScalar contrast,
style again: use pointers for out-params

https://codereview.chromium.org/258883002/diff/330001/src/core/SkScalerContext.h
File src/core/SkScalerContext.h (right):

https://codereview.chromium.org/258883002/diff/330001/src/core/SkScalerContex...
src/core/SkScalerContext.h:186: void*       getGammaLUTData(int& width, int&
height, SkScalar contrast,
Need big dox. We almost never return a freshly allocated block, forcing the
caller to accept ownership. We need to document that the caller must call
sk_free on the returned pointer.

https://codereview.chromium.org/258883002/diff/330001/src/gpu/GrDistanceField...
File src/gpu/GrDistanceFieldTextContext.cpp (right):

https://codereview.chromium.org/258883002/diff/330001/src/gpu/GrDistanceField...
src/gpu/GrDistanceFieldTextContext.cpp:389: if (!gGammaTexture) {
Seems like we need thread-protection for writing to this guy...

[bsalomon, 2014-05-28 19:53:53]

https://codereview.chromium.org/258883002/diff/330001/src/gpu/GrDistanceField...
File src/gpu/GrDistanceFieldTextContext.cpp (right):

https://codereview.chromium.org/258883002/diff/330001/src/gpu/GrDistanceField...
src/gpu/GrDistanceFieldTextContext.cpp:38: GrTexture*    gGammaTexture = NULL;
This doesn't seem safe. There can be multiple GrContexts.

[jvanverth1, 2014-05-29 18:53:48]

https://codereview.chromium.org/258883002/diff/330001/src/core/SkMaskGamma.h
File src/core/SkMaskGamma.h (right):

https://codereview.chromium.org/258883002/diff/330001/src/core/SkMaskGamma.h#...
src/core/SkMaskGamma.h:143: const uint8_t* getGammaTables(int& tableWidth, int&
numTables) const {
On 2014/05/28 19:45:17, reed1 wrote:
> style: skia always uses pointers for out-params

Done.

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

https://codereview.chromium.org/258883002/diff/330001/src/core/SkPaint.cpp#ne...
src/core/SkPaint.cpp:1820: rec->fFlags &=
~SkScalerContext::kForceAutohinting_Flag;
On 2014/05/28 19:45:17, reed1 wrote:
> other than preblend, this rest of this change seems unrelated to gamma. How
were
> we working before w/o wacking all of these flags?

This was done in GrDistanceFieldTextContext::init(), by clearing the flags in
the local copy of SkPaint before getting the SkGlyphCache. I think I could do
that and back up the parameters I need from the SkPaint and DeviceProperties
(the current color, the gamma) if that seems better.

https://codereview.chromium.org/258883002/diff/330001/src/core/SkPaint.cpp#ne...
src/core/SkPaint.cpp:1994: void* SkScalerContext::getGammaLUTData(int& width,
int& height, SkScalar contrast,
On 2014/05/28 19:45:17, reed1 wrote:
> style again: use pointers for out-params

Done.

https://codereview.chromium.org/258883002/diff/330001/src/core/SkScalerContext.h
File src/core/SkScalerContext.h (right):

https://codereview.chromium.org/258883002/diff/330001/src/core/SkScalerContex...
src/core/SkScalerContext.h:186: void*       getGammaLUTData(int& width, int&
height, SkScalar contrast,
On 2014/05/28 19:45:17, reed1 wrote:
> Need big dox. We almost never return a freshly allocated block, forcing the
> caller to accept ownership. We need to document that the caller must call
> sk_free on the returned pointer.

Changed to get the size of the table first so we can allocate the data locally,
then an accessor to fill the table.

https://codereview.chromium.org/258883002/diff/330001/src/gpu/GrDistanceField...
File src/gpu/GrDistanceFieldTextContext.cpp (right):

https://codereview.chromium.org/258883002/diff/330001/src/gpu/GrDistanceField...
src/gpu/GrDistanceFieldTextContext.cpp:38: GrTexture*    gGammaTexture = NULL;
On 2014/05/28 19:53:53, bsalomon wrote:
> This doesn't seem safe. There can be multiple GrContexts.

Changed to be a member of GrDistanceFieldTextContext.

https://codereview.chromium.org/258883002/diff/330001/src/gpu/GrDistanceField...
src/gpu/GrDistanceFieldTextContext.cpp:389: if (!gGammaTexture) {
On 2014/05/28 19:45:17, reed1 wrote:
> Seems like we need thread-protection for writing to this guy...

There should be only one GrDistanceFieldTextContext per thread, so I believe
this is safe now (see comment above).

[jvanverth1, 2014-05-30 18:20:27]

I realized that I didn't take into account the contrast value or whether gamma
correction of A8 glyphs is desired, so updated the change to take those into
account.

[bsalomon, 2014-06-02 15:20:46]

https://codereview.chromium.org/258883002/diff/370001/src/gpu/GrDistanceField...
File src/gpu/GrDistanceFieldTextContext.h (right):

https://codereview.chromium.org/258883002/diff/370001/src/gpu/GrDistanceField...
src/gpu/GrDistanceFieldTextContext.h:37: GrTexture*              fGammaTexture;
Where does this get unreffed?

[jvanverth1, 2014-06-03 19:24:58]

https://codereview.chromium.org/258883002/diff/370001/src/gpu/GrDistanceField...
File src/gpu/GrDistanceFieldTextContext.h (right):

https://codereview.chromium.org/258883002/diff/370001/src/gpu/GrDistanceField...
src/gpu/GrDistanceFieldTextContext.h:37: GrTexture*              fGammaTexture;
On 2014/06/02 15:20:46, bsalomon wrote:
> Where does this get unreffed?

Done.

[bsalomon, 2014-06-03 19:30:31]

On 2014/06/03 19:24:58, jvanverth1 wrote:
>
https://codereview.chromium.org/258883002/diff/370001/src/gpu/GrDistanceField...
> File src/gpu/GrDistanceFieldTextContext.h (right):
> 
>
https://codereview.chromium.org/258883002/diff/370001/src/gpu/GrDistanceField...
> src/gpu/GrDistanceFieldTextContext.h:37: GrTexture*             
fGammaTexture;
> On 2014/06/02 15:20:46, bsalomon wrote:
> > Where does this get unreffed?
> 
> Done.

lgtm if it lgt someone who knows the scalercontext/gammamask stuff.

[reed1, 2014-06-03 19:46:44]

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

https://codereview.chromium.org/258883002/diff/400001/src/core/SkPaint.cpp#ne...
src/core/SkPaint.cpp:1816: if (paint.isDistanceFieldTextTEMP()) {
What is the expected lifespan of this TEMP flag?

https://codereview.chromium.org/258883002/diff/400001/src/core/SkScalerContext.h
File src/core/SkScalerContext.h (right):

https://codereview.chromium.org/258883002/diff/400001/src/core/SkScalerContex...
src/core/SkScalerContext.h:189: void        getGammaLUTData(SkScalar contrast,
SkScalar paintGamma, SkScalar deviceGamma,
I presume data is getGammLUTSize bytes big. Can we add some simple dox to make
this clear?

[jvanverth1, 2014-06-05 18:18:23]

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

https://codereview.chromium.org/258883002/diff/400001/src/core/SkPaint.cpp#ne...
src/core/SkPaint.cpp:1816: if (paint.isDistanceFieldTextTEMP()) {
On 2014/06/03 19:46:44, reed1 wrote:
> What is the expected lifespan of this TEMP flag?

I don't know. Though I just realized a problem with using this flag even if it's
decided to make it permanent.  Suppose a client sets the flag requesting
distance field text, but then we fall back to bitmaps (because it's a color
font, or they're using a MaskFilter, or some such thing). Then when we generate
the rec for the GrBitmapTextContext it will also call this code, and you'll end
up with a non-gamma corrected, non-LCD, subpixel result, which may not be what
you want.

I might be able do this by munging my copy of the SkPaint, and also copying the
DeviceProperties and munging those to set the gamma to 1.

https://codereview.chromium.org/258883002/diff/400001/src/core/SkScalerContext.h
File src/core/SkScalerContext.h (right):

https://codereview.chromium.org/258883002/diff/400001/src/core/SkScalerContex...
src/core/SkScalerContext.h:189: void        getGammaLUTData(SkScalar contrast,
SkScalar paintGamma, SkScalar deviceGamma,
On 2014/06/03 19:46:44, reed1 wrote:
> I presume data is getGammLUTSize bytes big. Can we add some simple dox to make
> this clear?

Will do.

[jvanverth1, 2014-06-06 15:34:44]

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

https://codereview.chromium.org/258883002/diff/400001/src/core/SkPaint.cpp#ne...
src/core/SkPaint.cpp:1816: if (paint.isDistanceFieldTextTEMP()) {
On 2014/06/05 18:18:23, jvanverth1 wrote:
> On 2014/06/03 19:46:44, reed1 wrote:
> > What is the expected lifespan of this TEMP flag?
> 
> I don't know. Though I just realized a problem with using this flag even if
it's
> decided to make it permanent.  Suppose a client sets the flag requesting
> distance field text, but then we fall back to bitmaps (because it's a color
> font, or they're using a MaskFilter, or some such thing). Then when we
generate
> the rec for the GrBitmapTextContext it will also call this code, and you'll
end
> up with a non-gamma corrected, non-LCD, subpixel result, which may not be what
> you want.
> 
> I might be able do this by munging my copy of the SkPaint, and also copying
the
> DeviceProperties and munging those to set the gamma to 1.

Done. I was not able to use the DeviceProperties because the contrast also has
to be 0. However, there was an old 'ignoreGamma' parameter to descriptorProc()
that I was able to make use of, and create SkAutoGlyphCacheNoGamma, that
actually sets it.

https://codereview.chromium.org/258883002/diff/400001/src/core/SkScalerContext.h
File src/core/SkScalerContext.h (right):

https://codereview.chromium.org/258883002/diff/400001/src/core/SkScalerContex...
src/core/SkScalerContext.h:189: void        getGammaLUTData(SkScalar contrast,
SkScalar paintGamma, SkScalar deviceGamma,
On 2014/06/05 18:18:23, jvanverth1 wrote:
> On 2014/06/03 19:46:44, reed1 wrote:
> > I presume data is getGammLUTSize bytes big. Can we add some simple dox to
make
> > this clear?
> 
> Will do.

Done.

[jvanverth1, 2014-06-09 14:19:50]

The CQ bit was checked by jvanverth@google.com

[commit-bot: I haz the power, 2014-06-09 14:20:11]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/jvanverth@google.com/258883002/420001

[commit-bot: I haz the power, 2014-06-09 14:41:22]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Build-Win7-VS2010-x86-Debug-Trybot on tryserver.chromium
(http://108.170.220.76:10117/builders/Build-Win7-VS2010-x86-Debug-Trybot/build...)

[commit-bot: I haz the power, 2014-06-09 14:59:54]

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

[commit-bot: I haz the power, 2014-06-09 14:59:55]

Try jobs failed on following builders:
  Build-Win7-VS2010-x86-Debug-Trybot on tryserver.chromium
(http://108.170.220.76:10117/builders/Build-Win7-VS2010-x86-Debug-Trybot/build...)

[bungeman-skia, 2014-06-09 15:17:22]

https://codereview.chromium.org/258883002/diff/420001/src/core/SkScalerContext.h
File src/core/SkScalerContext.h (right):

https://codereview.chromium.org/258883002/diff/420001/src/core/SkScalerContex...
src/core/SkScalerContext.h:189: size_t      getGammaLUTSize(SkScalar contrast,
SkScalar paintGamma, SkScalar deviceGamma,
Can these be static? I see nothing in the implementation or interface here which
necessitates an object. This would mirror the existing GetMaskPreBlend.

https://codereview.chromium.org/258883002/diff/420001/src/gpu/effects/GrDista...
File src/gpu/effects/GrDistanceFieldTextureEffect.cpp (right):

https://codereview.chromium.org/258883002/diff/420001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:18: // To get proper gamma
correction, we need to slightly bold each glyph.
This is just bogus. Maybe "In order to get optical sizes people don't complain
about when we blit correctly..." or something like that.

[jvanverth1, 2014-06-09 17:26:14]

https://codereview.chromium.org/258883002/diff/420001/src/core/SkScalerContext.h
File src/core/SkScalerContext.h (right):

https://codereview.chromium.org/258883002/diff/420001/src/core/SkScalerContex...
src/core/SkScalerContext.h:189: size_t      getGammaLUTSize(SkScalar contrast,
SkScalar paintGamma, SkScalar deviceGamma,
On 2014/06/09 15:17:22, bungeman1 wrote:
> Can these be static? I see nothing in the implementation or interface here
which
> necessitates an object. This would mirror the existing GetMaskPreBlend.

Done.

https://codereview.chromium.org/258883002/diff/420001/src/gpu/effects/GrDista...
File src/gpu/effects/GrDistanceFieldTextureEffect.cpp (right):

https://codereview.chromium.org/258883002/diff/420001/src/gpu/effects/GrDista...
src/gpu/effects/GrDistanceFieldTextureEffect.cpp:18: // To get proper gamma
correction, we need to slightly bold each glyph.
On 2014/06/09 15:17:22, bungeman1 wrote:
> This is just bogus. Maybe "In order to get optical sizes people don't complain
> about when we blit correctly..." or something like that.

Done.

[jvanverth1, 2014-06-09 17:59:39]

The CQ bit was checked by jvanverth@google.com

[commit-bot: I haz the power, 2014-06-09 18:00:13]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/jvanverth@google.com/258883002/460001

[jvanverth1, 2014-06-09 18:09:15]

The CQ bit was unchecked by jvanverth@google.com

[jvanverth1, 2014-06-09 19:04:17]

The CQ bit was checked by jvanverth@google.com

[commit-bot: I haz the power, 2014-06-09 19:04:46]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/jvanverth@google.com/258883002/460001

[commit-bot: I haz the power, 2014-06-09 19:05:00]

Change committed as 4d517fdbb145cb95e5e935470df331e1b6667cfc

[scroggo, 2014-06-09 20:01:45]

A revert of this CL has been created in
https://codereview.chromium.org/323513005/ by scroggo@google.com.

The reason for reverting is: Memory leaks. I suspect this is due to the lack of
destructors in SkAutoGlyphCache.

[jvanverth1, 2014-06-10 13:22:34]

The CQ bit was checked by jvanverth@google.com

[commit-bot: I haz the power, 2014-06-10 13:23:13]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/jvanverth@google.com/258883002/480001

[commit-bot: I haz the power, 2014-06-10 13:43:01]

Change committed as 2d2a68c51b4a71bd60760510bf2b2e58bc9890b2