-
Notifications
You must be signed in to change notification settings - Fork 621
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix coretext shaper on how to decide fallbacks happen #36
Conversation
This is more or less what my initial patch was doing but it does not work with user features. |
What Khaled said. |
Guess we will have to find a better solution then, as the current code simply doesn't work as intended. |
May be we should compare the PS Name of the fonts instead? Looks a bit hacky but may be it would be more reliable here. |
Here is an attempt to compare PS names: diff --git a/src/hb-coretext.cc b/src/hb-coretext.cc
index 5705136..2a01322 100644
--- a/src/hb-coretext.cc
+++ b/src/hb-coretext.cc
@@ -699,11 +699,10 @@ _hb_coretext_shape (hb_shape_plan_t *shape_plan,
*/
CFDictionaryRef attributes = CTRunGetAttributes (run);
CTFontRef run_ct_font = static_cast<CTFontRef>(CFDictionaryGetValue (attributes, kCTFontAttributeName));
- CGFontRef run_cg_font = CTFontCopyGraphicsFont (run_ct_font, 0);
- if (!CFEqual (run_cg_font, face_data->cg_font))
+ CFStringRef ps_name1 = CTFontCopyName (run_ct_font, kCTFontPostScriptNameKey);
+ CFStringRef ps_name2 = CTFontCopyName (font_data->ct_font, kCTFontPostScriptNameKey);
+ if (CFStringCompare (ps_name1, ps_name2, 0) != kCFCompareEqualTo)
{
- CFRelease (run_cg_font);
-
CFRange range = CTRunGetStringRange (run);
buffer->ensure (buffer->len + range.length);
if (buffer->in_error)
@@ -739,7 +738,6 @@ _hb_coretext_shape (hb_shape_plan_t *shape_plan,
}
continue;
}
- CFRelease (run_cg_font);
unsigned int num_glyphs = CTRunGetGlyphCount (run);
if (num_glyphs == 0) |
Before we do that I like to make sure we fully understand the issue. @jjgod, so, you are saying that the CGFont you get back from the CTFont is different from the CGFont we used to create it? |
Maybe we should get the cg_font back from the CTFont we create and save that? |
Looks like it, the |
BTW, the PSName comparison looks fine to me. |
I mean something like this: diff --git a/src/hb-coretext.cc b/src/hb-coretext.cc
index 5705136..c42a2d9 100644
--- a/src/hb-coretext.cc
+++ b/src/hb-coretext.cc
@@ -686,6 +686,7 @@ _hb_coretext_shape (hb_shape_plan_t *shape_plan,
buffer->len = 0;
const CFRange range_all = CFRangeMake (0, 0);
+ CGFontRef orig_cg_font = CTFontCopyGraphicsFont (font_data->ct_font, 0);
for (unsigned int i = 0; i < num_runs; i++)
{
@@ -700,7 +701,7 @@ _hb_coretext_shape (hb_shape_plan_t *shape_plan,
CFDictionaryRef attributes = CTRunGetAttributes (run);
CTFontRef run_ct_font = static_cast<CTFontRef>(CFDictionaryGetValue (attributes, kCTFontAttributeName));
CGFontRef run_cg_font = CTFontCopyGraphicsFont (run_ct_font, 0);
- if (!CFEqual (run_cg_font, face_data->cg_font))
+ if (!CFEqual (run_cg_font, orig_cg_font))
{
CFRelease (run_cg_font);
@@ -794,6 +795,8 @@ _hb_coretext_shape (hb_shape_plan_t *shape_plan,
}
}
+ CFRelease (orig_cg_font);
+
buffer->clear_positions ();
unsigned int count = buffer->len; |
Ok, I asked Ned (from Apple) and he suggests the PostScriptName also. Maybe we can do the simple pointer equality test first, and if that fails, try PSName? Or, do a pointer equality check on the CTFont, and if that fails, do the above. That should make it fast for the common case of no font features and no fallback fonts. |
Yes, that sounds fine to me. |
coretext shaper returns all 0 glyph IDs even for non-fallback fonts. It's cause it currently compares the CGFont copied from the CTRun with the CGFont used when creating the original CTFont, a technique shown in [1]. However, CFEqual() seemed to return false even for identical fonts getting from single CTRun without any fallback happening. [2] does show a different method, i.e. comparing the CTFonts directly instead, which seemed to work for me. (On OS X 10.9) [1] https://github.com/WebKit/webkit/blob/master/Source/WebCore/platform/graphics/mac/GlyphPageTreeNodeMac.cpp#L134 [2] https://github.com/WebKit/webkit/blob/master/Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm#L249
Ok, I think we can do something along those lines. I'm not fully comfortable relying on PS names, but I think that's the best heuristic we have for now... I'll push something along those lines out soon. Thanks for the patch. |
Some patches are waiting for this one to be processed (not yet uploaded due to potential path conflicts). |
Here is [https://qt.gitorious.org/qt/qtbase/source/bae184532dde3b8d46bd90d060377ab06dc62644:src/3rdparty/harfbuzz-ng/src/hb-coretext.cc#L729] how we're doing it in Qt. It was tested in about a half of year already on Mac, w/o any instabilities/regressions found so far. Does it look acceptable to you guys? |
@KonstantinRitt That one fails with, eg: hb-view /Library/Fonts/Zapfino.ttf ZapfinoZapfino --features=-dlig[7:] --shaper coretext I'm working on a proper fix. Hold on. |
I managed to push a real fix out that doesn't use PS-names (which I'm not sure are guaranteed to unique). |
Fixed by 25f4fb9 |
Thanks. |
FWIW, here's how webkit does it: cascade to LastResort, and detect LastResort: |
And... even what I pushed in is not good enough. I'm trying to get vertical text out of CoreText, and if I set vertical, the ct_font points don't match anymore... |
Huh? Yet another undocumented behavior issue that eats our time and gives no real gain once resolved? I wonder if implementing support for AAT wouldn't cost less than using the CoreText API :P |
I was reading AAT (again!) over the weekend. Will happen later this year if I was to guess. |
Looks nice. |
The only other things I see in your list are:
Anything else? |
The removed PDF at end of run caused an assertion in the layouting code. I don't remember all the details now. After you've introduced the use of run properties, we'll have to re-check if that "fix" is still needed.
Which ones? I don't think we had any assertions due to other removed formatting characters so far. |
Joiners, etc. I think the PDF assertion might have been caused by the direction issue. Definitely try without it first, if it doesn't work, send me full details (CC me on the bug, email, whatever works for you). |
Fix Windows build
coretext shaper returns all 0 glyph IDs even for non-fallback fonts. It's cause it currently compares the
CGFont
copied from theCTRun
with theCGFont
used when creating the originalCTFont
, a technique shown in WebKit. However,CFEqual()
seemed to return false even for identical fonts getting from singleCTRun
without any fallback happening. A different place in WebKit does show a different method, i.e. comparing theCTFont
s directly instead, which seemed to work for me. (On OS X 10.9)