Fix limit on size of glyph paths.

src/ports/SkFontHost_win.cpp

Index: src/ports/SkFontHost_win.cpp
===================================================================
--- src/ports/SkFontHost_win.cpp	(revision 9661)
+++ src/ports/SkFontHost_win.cpp	(working copy)
@@ -19,6 +19,7 @@
 #include "SkPath.h"
 #include "SkStream.h"
 #include "SkString.h"
+#include "SkTemplates.h"
 #include "SkThread.h"
 #include "SkTypeface_win.h"
 #include "SkTypefaceCache.h"
@@ -46,18 +47,11 @@
 // always packed xxRRGGBB
 typedef uint32_t SkGdiRGB;
 
-template <typename T> T* SkTAddByteOffset(T* ptr, size_t byteOffset) {
-    return (T*)((char*)ptr + byteOffset);
-}
-
 // define this in your Makefile or .gyp to enforce AA requests
 // which GDI ignores at small sizes. This flag guarantees AA
 // for rotated text, regardless of GDI's notions.
 //#define SK_ENFORCE_ROTATED_TEXT_AA_ON_WINDOWS
 
-// client3d has to undefine this for now
-#define CAN_USE_LOGFONT_NAME
-
 static bool isLCD(const SkScalerContext::Rec& rec) {
     return SkMask::kLCD16_Format == rec.fMaskFormat ||
            SkMask::kLCD32_Format == rec.fMaskFormat;
@@ -91,9 +85,6 @@
 
 using namespace skia_advanced_typeface_metrics_utils;
 
-static const uint16_t BUFFERSIZE = (16384 - 32);
-static uint8_t glyphbuf[BUFFERSIZE];
-
 /**
  *  Since LOGFONT wants its textsize as an int, and we support fractional sizes,
  *  and since we have a cache of LOGFONTs for our tyepfaces, we always set the
@@ -554,8 +545,6 @@
     return SkFixedToFIXED(SkFloatToFixed(x));
 }
 
-SK_DECLARE_STATIC_MUTEX(gFTMutex);
-
 #define HIRES_TEXTSIZE  2048
 #define HIRES_SHIFT     11
 static inline SkFixed HiResToFixed(int value) {
@@ -589,9 +578,8 @@
         , fFont(0)
         , fSavefont(0)
         , fSC(0)
-        , fGlyphCount(-1) {
-    SkAutoMutexAcquire  ac(gFTMutex);
-
+        , fGlyphCount(-1)
+{
     LogFontTypeface* typeface = reinterpret_cast<LogFontTypeface*>(rawTypeface);
 
     fDDC = ::CreateCompatibleDC(NULL);
@@ -924,10 +912,17 @@
  *  that shifting into other color spaces is imprecise.
  */
 static const uint8_t* getInverseGammaTableGDI() {
+    // Since build_power_table is idempotent, many threads can build gTableGdi
+    // simultaneously.
     static bool gInited;
     static uint8_t gTableGdi[256];
-    if (!gInited) {
+    if (gInited) {
+        // Need a L/L (read) barrier (aquire not needed). If gInited is observed

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

s/aquire/acquire/

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

Done.

+        // true then gTableGdi is observable, but it must be requested.

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

are these comments informational or indicative of future work? Or, through some
magic, does this if (x) else y form introduce the barriers mentioned?

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

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.

+    } else {
         build_power_table(gTableGdi, 2.3f);
+        // Need a S/S (write) barrier (release not needed) here so that this
+        // write to gInited becomes observable after gTableGdi.
         gInited = true;
     }
     return gTableGdi;
@@ -941,15 +936,22 @@
  *  If this value is not specified, the default is a gamma of 1.4.
  */
 static const uint8_t* getInverseGammaTableClearType() {
+    // We don't expect SPI_GETFONTSMOOTHINGCONTRAST to ever change, so building
+    // gTableClearType with build_power_table is effectively idempotent.
     static bool gInited;
     static uint8_t gTableClearType[256];
-    if (!gInited) {
+    if (gInited) {
+        // Need a L/L (read) barrier (aquire not needed). If gInited is observed

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

s/aquire/acquire/

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

Done.

+        // true then gTableClearType is observable, but it must be requested.
+    } else {
         UINT level = 0;
         if (!SystemParametersInfo(SPI_GETFONTSMOOTHINGCONTRAST, 0, &level, 0) || !level) {
             // can't get the data, so use a default
             level = 1400;
         }
         build_power_table(gTableClearType, level / 1000.0f);
+        // Need a S/S (write) barrier (release not needed) here so that this
+        // write to gInited becomes observable after gTableClearType.
         gInited = true;
     }
     return gTableClearType;
@@ -1005,7 +1007,7 @@
                 return false;
             }
         }
-        src = SkTAddByteOffset(src, srcRB);
+        src = SkTAddOffset<const SkGdiRGB>(src, srcRB);
     }
     return true;
 }
@@ -1050,7 +1052,7 @@
             }
             dst[byteCount] = byte;
         }
-        src = SkTAddByteOffset(src, srcRB);
+        src = SkTAddOffset<const SkGdiRGB>(src, srcRB);
         dst -= dstRB;
     }
 }
@@ -1066,7 +1068,7 @@
         for (int i = 0; i < width; i++) {
             dst[i] = rgb_to_a8<APPLY_PREBLEND>(src[i], table8);
         }
-        src = SkTAddByteOffset(src, srcRB);
+        src = SkTAddOffset<const SkGdiRGB>(src, srcRB);
         dst -= dstRB;
     }
 }
@@ -1082,7 +1084,7 @@
         for (int i = 0; i < width; i++) {
             dst[i] = rgb_to_lcd16<APPLY_PREBLEND>(src[i], tableR, tableG, tableB);
         }
-        src = SkTAddByteOffset(src, srcRB);
+        src = SkTAddOffset<const SkGdiRGB>(src, srcRB);
         dst = (uint16_t*)((char*)dst - dstRB);
     }
 }
@@ -1098,7 +1100,7 @@
         for (int i = 0; i < width; i++) {
             dst[i] = rgb_to_lcd32<APPLY_PREBLEND>(src[i], tableR, tableG, tableB);
         }
-        src = SkTAddByteOffset(src, srcRB);
+        src = SkTAddOffset<const SkGdiRGB>(src, srcRB);
         dst = (uint32_t*)((char*)dst - dstRB);
     }
 }
@@ -1109,7 +1111,6 @@
 }
 
 void SkScalerContext_Windows::generateImage(const SkGlyph& glyph) {
-    SkAutoMutexAcquire ac(gFTMutex);
     SkASSERT(fDDC);
 
     const bool isBW = SkMask::kBW_Format == fRec.fMaskFormat;
@@ -1148,7 +1149,7 @@
                 int b = (addr[x] >>  0) & 0xFF;
                 addr[x] = (table[r] << 16) | (table[g] << 8) | table[b];
             }
-            addr = SkTAddByteOffset(addr, srcRB);
+            addr = SkTAddOffset<SkGdiRGB>(addr, srcRB);
         }
     }
 
@@ -1200,23 +1201,45 @@
 }
 
 void SkScalerContext_Windows::generatePath(const SkGlyph& glyph, SkPath* path) {
-
-    SkAutoMutexAcquire ac(gFTMutex);
-
     SkASSERT(&glyph && path);
     SkASSERT(fDDC);
 
     path->reset();
 
     GLYPHMETRICS gm;
-    uint32_t total_size = GetGlyphOutlineW(fDDC, glyph.fID, GGO_NATIVE | GGO_GLYPH_INDEX, &gm, BUFFERSIZE, glyphbuf, &fMat22);
+
+    // Out of all the fonts on a typical Windows box,
+    // 25% of glyphs require more than 2KB.
+    // 1% of glyphs require more than 4KB.
+    // 0.01% of glyphs require more than 8KB.
+    // 8KB is less than 1% of the normal 1MB stack on Windows.
+    // Note that some web fonts glyphs require more than 20KB.
+    static const uint16_t BUFFERSIZE = (1 << 13);
+    SkAutoSTMalloc<BUFFERSIZE, uint8_t> glyphbuf(BUFFERSIZE);
+
+    DWORD total_size = GetGlyphOutlineW(fDDC, glyph.fID, GGO_NATIVE | GGO_GLYPH_INDEX, &gm, BUFFERSIZE, glyphbuf, &fMat22);

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

this and lines below don't fit in 100 chars

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

Done.

     if (GDI_ERROR == total_size) {
-        LogFontTypeface::EnsureAccessible(this->getTypeface());
-        total_size = GetGlyphOutlineW(fDDC, glyph.fID, GGO_NATIVE | GGO_GLYPH_INDEX, &gm, BUFFERSIZE, glyphbuf, &fMat22);
+        total_size = GetGlyphOutlineW(fDDC, glyph.fID, GGO_NATIVE | GGO_GLYPH_INDEX, &gm, 0, NULL, &fMat22);
         if (GDI_ERROR == total_size) {
-            SkASSERT(false);
-            return;
+            LogFontTypeface::EnsureAccessible(this->getTypeface());

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

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:18]

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.
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.

+            total_size = GetGlyphOutlineW(fDDC, glyph.fID, GGO_NATIVE | GGO_GLYPH_INDEX, &gm, 0, NULL, &fMat22);
+            if (GDI_ERROR == total_size) {
+                SkASSERT(false);
+                return;
+            }
         }
+
+        glyphbuf.reset(total_size);
+
+        DWORD ret = GetGlyphOutlineW(fDDC, glyph.fID, GGO_NATIVE | GGO_GLYPH_INDEX, &gm, total_size, glyphbuf, &fMat22);
+        if (GDI_ERROR == ret) {
+            LogFontTypeface::EnsureAccessible(this->getTypeface());
+            ret = GetGlyphOutlineW(fDDC, glyph.fID, GGO_NATIVE | GGO_GLYPH_INDEX, &gm, total_size, glyphbuf, &fMat22);
+            if (GDI_ERROR == ret) {
+                SkASSERT(false);
+                return;
+            }
+        }
     }
 
     const uint8_t* cur_glyph = glyphbuf;