Fix limit on size of glyph paths.

Fix limit on size of glyph paths.

Some web fonts have been observed to contain glyphs which require a buffer
greater than 20KB. In these rare cases we should allocate space on the heap.
Most glyphs require less than 8KB, and Windows has a 1MB stack, so in the
common case just use the stack.

This change also removes the gFTMutex which was both poorly named and used.
The constructor does not appear to have any need for it, and with this change
the path generation code does not need it. The mask generating code does need
memory barriers for correctness, but these bariers are no-ops on x86. We will
need another change to clean up this sort of static initialization code.

R=caryclark@google.com

Committed: https://code.google.com/p/skia/source/detail?r=9700

[caryclark, 2013-06-19 12:21:46]

https://codereview.chromium.org/17435003/diff/1/src/ports/SkFontHost_win.cpp
File src/ports/SkFontHost_win.cpp (right):

https://codereview.chromium.org/17435003/diff/1/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:920: // Need a L/L (read) barrier (aquire not
needed). If gInited is observed
s/aquire/acquire/

https://codereview.chromium.org/17435003/diff/1/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:921: // true then gTableGdi is observable, but it
must be requested.
are these comments informational or indicative of future work? Or, through some
magic, does this if (x) else y form introduce the barriers mentioned?

https://codereview.chromium.org/17435003/diff/1/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:944: // Need a L/L (read) barrier (aquire not
needed). If gInited is observed
s/aquire/acquire/

https://codereview.chromium.org/17435003/diff/1/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:1220: DWORD total_size = GetGlyphOutlineW(fDDC,
glyph.fID, GGO_NATIVE | GGO_GLYPH_INDEX, &gm, BUFFERSIZE, glyphbuf, &fMat22);
this and lines below don't fit in 100 chars

https://codereview.chromium.org/17435003/diff/1/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:1224:
LogFontTypeface::EnsureAccessible(this->getTypeface());
It's unfortunate that this code path can't be tested outside of Chromium as far
as I can tell. Is it time to bring the logic into Skia? 

Also, since GetGlyphOutlineW is overloaded -- it fails if the glyph is too big,
and it fails if the font isn't loaded -- and assuming that failing if the glyph
is too big is very rare, wouldn't it be more common that the failure is because
the font isn't loaded, and call that first before trying a larger buffer size?

[bungeman-skia, 2013-06-19 21:52:17]

We may need to discuss the sanity of EnsureAccessible.

https://codereview.chromium.org/17435003/diff/1/src/ports/SkFontHost_win.cpp
File src/ports/SkFontHost_win.cpp (right):

https://codereview.chromium.org/17435003/diff/1/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:920: // Need a L/L (read) barrier (aquire not
needed). If gInited is observed
On 2013/06/19 12:21:46, caryclark wrote:
> s/aquire/acquire/

Done.

https://codereview.chromium.org/17435003/diff/1/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:921: // true then gTableGdi is observable, but it
must be requested.
On 2013/06/19 12:21:46, caryclark wrote:
> are these comments informational or indicative of future work? Or, through
some
> magic, does this if (x) else y form introduce the barriers mentioned?

These are somewhat TODOs as we also have code like this in at least the Mac as
well (without such comments). The mentioned memory barriers are already implicit
on x86 and x86_64 (an mfence is only needed for StoreLoad), which is the only
place where this code currently runs (I think). On weaker memory architectures
where Windows and GDI might be found (Itanium, ARM) we'd need explicit memory
barriers.

I would fix this now except that Windows has made this difficult. The Windows
API doesn't expose the barriers, and they now expect everyone to use the C++11
atomics which would work very well, except that they're C++11. Chromium also
doesn't have good support for this sort of thing in their atomics library. Also
we aren't running on Windows for Itanium or ARM (as far as I'm aware) and even
then the lack of memory barriers here is extremely unlikely to be observed (at
worst I think one character might be invisible). As a result I'm documenting
what we need while I'm aware of it and hoping to get a pass on fixing it good
until I can figure out a good way to do it.

https://codereview.chromium.org/17435003/diff/1/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:944: // Need a L/L (read) barrier (aquire not
needed). If gInited is observed
On 2013/06/19 12:21:46, caryclark wrote:
> s/aquire/acquire/

Done.

https://codereview.chromium.org/17435003/diff/1/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:1220: DWORD total_size = GetGlyphOutlineW(fDDC,
glyph.fID, GGO_NATIVE | GGO_GLYPH_INDEX, &gm, BUFFERSIZE, glyphbuf, &fMat22);
On 2013/06/19 12:21:46, caryclark wrote:
> this and lines below don't fit in 100 chars

Done.

https://codereview.chromium.org/17435003/diff/1/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:1224:
LogFontTypeface::EnsureAccessible(this->getTypeface());
On 2013/06/19 12:21:46, caryclark wrote:
> It's unfortunate that this code path can't be tested outside of Chromium as
far
> as I can tell. Is it time to bring the logic into Skia? 
> 

I'm not sure what you mean by bringing the logic into Skia, do you mean moving
the logic of PreCacheFont from font_cache_dispatcher_win.cc into Skia? When GDI
calls fail in Chromium we tend to know about it via the layout tests. The only
way to re-create this 'for real' is to run in the Chromium sandbox. It might be
possible to mock this sort of thing out somehow by having a test executable
which mocks out our GDI calls and always or randomly does not have the data the
first time. All of these code blocks are doing the same thing inside the 'if' as
is being done just before the 'if' except after calling EnsureAccessible. It
would be nice to automate this so that we can be sure not to have any copy-paste
errors.

> Also, since GetGlyphOutlineW is overloaded -- it fails if the glyph is too
big,
> and it fails if the font isn't loaded -- and assuming that failing if the
glyph
> is too big is very rare, wouldn't it be more common that the failure is
because
> the font isn't loaded, and call that first before trying a larger buffer size?

Added comment to try to explain. Getting to this point should be fairly rare
(less than 1% of the time for either reason). Also, I expect that
GetGlyphOutlineW will fail faster if it cannot access the data at all. So the
case where we cannot load the data looks like
fail-fast/fail-fast/ensure/find-size-slow and the case where the buffer isn't
big enough looks like fail-slow/find-size-slow(which we have to do anyway). In
the case where we cannot load the data (but can after ensure) and the size turns
out to fit in the stack buffer, we still use the stack.

[caryclark, 2013-06-20 14:02:22]

lgtm

[bungeman-skia, 2013-06-20 15:29:19]

Committed patchset #2 manually as r9700 (presubmit successful).