Moving towards configurable darwin implementation of LocalGlyphRasterizer.

Thin C++ wrappers on C calls.

5 files changed, 109 insertions, 69 deletions

diff --git a/platform/darwin/src/local_glyph_rasterizer.mm b/platform/darwin/src/local_glyph_rasterizer.mm
index 978bda0cb5..fa5d1a9302 100644
--- a/platform/darwin/src/local_glyph_rasterizer.mm
+++ b/platform/darwin/src/local_glyph_rasterizer.mm
@@ -22,6 +22,15 @@ private:
 
 namespace mbgl {
 
+using CGContextHandle = CFHandle<CGContextRef, CGContextRef, CGContextRelease>;
+using CGColorSpaceHandle = CFHandle<CGColorSpaceRef, CGColorSpaceRef, CGColorSpaceRelease>;
+using CTFontDescriptorRefHandle = CFHandle<CTFontDescriptorRef, CFTypeRef, CFRelease>;
+using CTFontRefHandle = CFHandle<CTFontRef, CFTypeRef, CFRelease>;
+using CFStringRefHandle = CFHandle<CFStringRef, CFTypeRef, CFRelease>;
+using CFAttributedStringRefHandle = CFHandle<CFAttributedStringRef, CFTypeRef, CFRelease>;
+using CTLineRefHandle = CFHandle<CTLineRef, CFTypeRef, CFRelease>;
+using CFDictionaryRefHandle = CFHandle<CFDictionaryRef, CFTypeRef, CFRelease>;
+
 /*
     Initial implementation of darwin TinySDF support:
     Draw any CJK glyphs using a default system font
@@ -34,48 +43,66 @@ namespace mbgl {
      - Extract glyph metrics so that this can be used with more than just fixed width glyphs
 */
 
-bool LocalGlyphRasterizer::canRasterizeGlyph(const FontStack&, GlyphID glyphID) {
-    // TODO: This is a rough approximation of the set of glyphs that will work with fixed glyph metrics
-    // Either narrow this down to be conservative, or actually extract glyph metrics in rasterizeGlyph
-    return util::i18n::allowsIdeographicBreaking(glyphID);
-}
-
-using CGContextHandle = CFHandle<CGContextRef, CGContextRef, CGContextRelease>;
-using CGColorSpaceHandle = CFHandle<CGColorSpaceRef, CGColorSpaceRef, CGColorSpaceRelease>;
+class LocalGlyphRasterizer::Impl {
+public:
+    Impl(CTFontRef fontHandle)
+        : font(fontHandle)
+    {}
 
-Glyph LocalGlyphRasterizer::rasterizeGlyph(const FontStack&, GlyphID glyphID) {
-    Glyph fixedMetrics;
-    fixedMetrics.id = glyphID;
-
-    uint32_t width = 35;
-    uint32_t height = 35;
-    
-    fixedMetrics.metrics.width = width;
-    fixedMetrics.metrics.height = height;
-    fixedMetrics.metrics.left = 3;
-    fixedMetrics.metrics.top = -1;
-    fixedMetrics.metrics.advance = 24;
-
-    Size size(width, height);
+    CTFontRefHandle font;
+};
 
-    fixedMetrics.bitmap = AlphaImage(size);
-    
+LocalGlyphRasterizer::LocalGlyphRasterizer(void*)
+{
     NSDictionary *fontAttributes =
               [NSDictionary dictionaryWithObjectsAndKeys:
                       [NSNumber numberWithFloat:24.0], (NSString *)kCTFontSizeAttribute,
                       nil];
-    // Create a descriptor.
-    CTFontDescriptorRef descriptor =
-              CTFontDescriptorCreateWithAttributes((CFDictionaryRef)fontAttributes);
 
-    // Create a font using the descriptor.
-    CTFontRef font = CTFontCreateWithFontDescriptor(descriptor, 0.0, NULL);
-    CFRelease(descriptor);
+    CTFontDescriptorRefHandle descriptor(CTFontDescriptorCreateWithAttributes((CFDictionaryRef)fontAttributes));
+
+    impl = std::make_unique<Impl>(CTFontCreateWithFontDescriptor(*descriptor, 0.0, NULL));
+}
+
+LocalGlyphRasterizer::~LocalGlyphRasterizer()
+{}
+
+bool LocalGlyphRasterizer::canRasterizeGlyph(const FontStack&, GlyphID glyphID) {
+    // TODO: This is a rough approximation of the set of glyphs that will work with fixed glyph metrics
+    // Either narrow this down to be conservative, or actually extract glyph metrics in rasterizeGlyph
+    return util::i18n::allowsIdeographicBreaking(glyphID);
+}
 
+// TODO: In theory we should be able to transform user-coordinate bounding box and advance
+// values into pixel glyph metrics. This would remove the need to use fixed glyph metrics
+// (which will be slightly off depending on the font), and allow us to return non CJK glyphs
+// (which will have variable "advance" values).
+void extractGlyphMetrics(CTFontRef font, CTLineRef line) {
+    CFArrayRef glyphRuns = CTLineGetGlyphRuns(line);
+    CFIndex runCount = CFArrayGetCount(glyphRuns);
+    assert(runCount == 1);
+    CTRunRef run = (CTRunRef)CFArrayGetValueAtIndex(glyphRuns, 0);
+    CFIndex glyphCount = CTRunGetGlyphCount(run);
+    assert(glyphCount == 1);
+    const CGGlyph *glyphs = CTRunGetGlyphsPtr(run);
 
-    CFStringRef string = CFStringCreateWithCharacters(NULL, (UniChar*)&glyphID, 1);
+    CGRect boundingRects[1];
+    boundingRects[0] = CGRectMake(0, 0, 0, 0);
+    CGSize advances[1];
+    advances[0] = CGSizeMake(0,0);
+    CGRect totalBoundingRect = CTFontGetBoundingRectsForGlyphs(font, kCTFontOrientationDefault, glyphs, boundingRects, 1);
+    double totalAdvance = CTFontGetAdvancesForGlyphs(font, kCTFontOrientationDefault, glyphs, advances, 1);
+    
+    // Break in the debugger to see these values: translating from "user coordinates" to bitmap pixel coordinates
+    // should be OK, but a lot of glyphs seem to have empty bounding boxes...?
+    (void)totalBoundingRect;
+    (void)totalAdvance;
+}
 
-    PremultipliedImage image(size);
+PremultipliedImage drawGlyphBitmap(GlyphID glyphID, CTFontRef font, Size size) {
+    PremultipliedImage rgbaBitmap(size);
+    
+    CFStringRefHandle string(CFStringCreateWithCharacters(NULL, reinterpret_cast<UniChar*>(&glyphID), 1));
 
     CGColorSpaceHandle colorSpace(CGColorSpaceCreateDeviceRGB());
     // TODO: Is there a way to just draw a single alpha channel instead of copying it out of an RGB image? Doesn't seem like the grayscale colorspace is what I'm looking for...
@@ -85,12 +112,12 @@ Glyph LocalGlyphRasterizer::rasterizeGlyph(const FontStack&, GlyphID glyphID) {
     
     constexpr const size_t bitsPerComponent = 8;
     constexpr const size_t bytesPerPixel = 4;
-    const size_t bytesPerRow = bytesPerPixel * width;
+    const size_t bytesPerRow = bytesPerPixel * size.width;
 
     CGContextHandle context(CGBitmapContextCreate(
-        image.data.get(),
-        width,
-        height,
+        rgbaBitmap.data.get(),
+        size.width,
+        size.height,
         bitsPerComponent,
         bytesPerRow,
         *colorSpace,
@@ -102,48 +129,44 @@ Glyph LocalGlyphRasterizer::rasterizeGlyph(const FontStack&, GlyphID glyphID) {
     CFStringRef keys[] = { kCTFontAttributeName };
     CFTypeRef values[] = { font };
 
-    CFDictionaryRef attributes =
+    CFDictionaryRefHandle attributes(
         CFDictionaryCreate(kCFAllocatorDefault, (const void**)&keys,
             (const void**)&values, sizeof(keys) / sizeof(keys[0]),
             &kCFTypeDictionaryKeyCallBacks,
-            &kCFTypeDictionaryValueCallBacks);
+            &kCFTypeDictionaryValueCallBacks));
 
-    CFAttributedStringRef attrString =
-        CFAttributedStringCreate(kCFAllocatorDefault, string, attributes);
-    CFRelease(string);
-    CFRelease(attributes);
+    CFAttributedStringRefHandle attrString(CFAttributedStringCreate(kCFAllocatorDefault, *string, *attributes));
 
-    CTLineRef line = CTLineCreateWithAttributedString(attrString); // TODO: Get glyph runs (for metric extraction) and use showGlyphs API instead?
-
-    CFArrayRef glyphRuns = CTLineGetGlyphRuns(line);
-    CFIndex runCount = CFArrayGetCount(glyphRuns);
-    assert(runCount == 1);
-    CTRunRef run = (CTRunRef)CFArrayGetValueAtIndex(glyphRuns, 0);
-    CFIndex glyphCount = CTRunGetGlyphCount(run);
-    assert(glyphCount == 1);
-    const CGGlyph *glyphs = CTRunGetGlyphsPtr(run);
-
-    CGRect boundingRects[1];
-    boundingRects[0] = CGRectMake(0, 0, 0, 0);
-    CGSize advances[1];
-    advances[0] = CGSizeMake(0,0);
-    CGRect totalBoundingRect = CTFontGetBoundingRectsForGlyphs(font, kCTFontOrientationDefault, glyphs, boundingRects, 1);
-    double totalAdvance = CTFontGetAdvancesForGlyphs(font, kCTFontOrientationDefault, glyphs, advances, 1);
+    CTLineRefHandle line(CTLineCreateWithAttributedString(*attrString));
     
-    // Break in the debugger to see these values: translating from "user coordinates" to bitmap pixel coordinates
-    // should be OK, but a lot of glyphs seem to have empty bounding boxes...?
-    (void)totalBoundingRect;
-    (void)totalAdvance;
+    // For debugging only, doesn't get useful metrics yet
+    extractGlyphMetrics(font, *line);
     
     // Set text position and draw the line into the graphics context
     CGContextSetTextPosition(*context, 0.0, 5.0);
-    CTLineDraw(line, *context);
+    CTLineDraw(*line, *context);
     
-    CFRelease(line);
-    CFRelease(font);
+    return rgbaBitmap;
+}
+
+Glyph LocalGlyphRasterizer::rasterizeGlyph(const FontStack&, GlyphID glyphID) {
+    Glyph fixedMetrics;
+    fixedMetrics.id = glyphID;
+
+    Size size(35, 35);
     
-    for (uint32_t i = 0; i < width * height; i++) {
-        fixedMetrics.bitmap.data[i] = image.data[4 * i + 3]; // alpha value
+    fixedMetrics.metrics.width = size.width;
+    fixedMetrics.metrics.height = size.height;
+    fixedMetrics.metrics.left = 3;
+    fixedMetrics.metrics.top = -1;
+    fixedMetrics.metrics.advance = 24;
+
+    PremultipliedImage rgbaBitmap = drawGlyphBitmap(glyphID, *(impl->font), size);
+   
+    // Copy alpha values from RGBA bitmap into the AlphaImage output
+    fixedMetrics.bitmap = AlphaImage(size);
+    for (uint32_t i = 0; i < size.width * size.height; i++) {
+        fixedMetrics.bitmap.data[i] = rgbaBitmap.data[4 * i + 3];
     }
 
     return fixedMetrics;
diff --git a/platform/default/local_glyph_rasterizer.cpp b/platform/default/local_glyph_rasterizer.cpp
index 7ace6cbfb1..514b6a23cb 100644
--- a/platform/default/local_glyph_rasterizer.cpp
+++ b/platform/default/local_glyph_rasterizer.cpp
@@ -2,6 +2,15 @@
 
 namespace mbgl {
 
+class LocalGlyphRasterizer::Impl {
+};
+
+LocalGlyphRasterizer::LocalGlyphRasterizer(void*)
+{}
+
+LocalGlyphRasterizer::~LocalGlyphRasterizer()
+{}
+
 bool LocalGlyphRasterizer::canRasterizeGlyph(const FontStack&, GlyphID) {
     return false;
 }
diff --git a/src/mbgl/text/glyph_manager.hpp b/src/mbgl/text/glyph_manager.hpp
index 194f503ff1..d9a4e64c1d 100644
--- a/src/mbgl/text/glyph_manager.hpp
+++ b/src/mbgl/text/glyph_manager.hpp
@@ -25,7 +25,7 @@ public:
 
 class GlyphManager : public util::noncopyable {
 public:
-    GlyphManager(FileSource&, std::unique_ptr<LocalGlyphRasterizer> = std::make_unique<LocalGlyphRasterizer>());
+    GlyphManager(FileSource&, std::unique_ptr<LocalGlyphRasterizer> = std::make_unique<LocalGlyphRasterizer>((void*)NULL));
     ~GlyphManager();
 
     // Workers send a `getGlyphs` message to the main thread once they have determined
diff --git a/src/mbgl/text/local_glyph_rasterizer.hpp b/src/mbgl/text/local_glyph_rasterizer.hpp
index c2bdbd2840..753ebe372a 100644
--- a/src/mbgl/text/local_glyph_rasterizer.hpp
+++ b/src/mbgl/text/local_glyph_rasterizer.hpp
@@ -32,11 +32,15 @@ namespace mbgl {
 
 class LocalGlyphRasterizer {
 public:
-    virtual ~LocalGlyphRasterizer() = default;
+    virtual ~LocalGlyphRasterizer();
+    LocalGlyphRasterizer(void* configuration);
 
     // virtual so that test harness can override platform-specific behavior
     virtual bool canRasterizeGlyph(const FontStack&, GlyphID);
     virtual Glyph rasterizeGlyph(const FontStack&, GlyphID);
+private:
+    class Impl;
+    std::unique_ptr<Impl> impl;
 };
 
 } // namespace mbgl
diff --git a/test/text/glyph_manager.test.cpp b/test/text/glyph_manager.test.cpp
index a96e1b970c..a50e42236f 100644
--- a/test/text/glyph_manager.test.cpp
+++ b/test/text/glyph_manager.test.cpp
@@ -19,6 +19,10 @@ static constexpr const size_t stubBitmapLength = 900;
 
 class StubLocalGlyphRasterizer : public LocalGlyphRasterizer {
 public:
+    StubLocalGlyphRasterizer()
+        : LocalGlyphRasterizer(0)
+    {}
+
     bool canRasterizeGlyph(const FontStack&, GlyphID glyphID) {
         return util::i18n::allowsIdeographicBreaking(glyphID);
     }