Fix Weird Behavior in CoreText Shaper (#9002)

You can pretty simply reproduce a crash on `main` in `Debug` mode by
running `printf "مرحبًا \n"` with your primary font set to one that
supports Arabic such as Cascadia Code/Mono or Kawkab Mono, which will
cause CoreText to output the shaped glyphs non-monotonically which hits
the assert we have in the renderer.

In `ReleaseFast` this assert is skipped and because we already moved
ahead to the space glyph (which belongs at the end but is emitted first)
all of the glyphs up to that point are lost. I believe this is probably
the cause of #8280, I tested and this change seems to fix it at least.

Included in this PR is a little optimization: we were allocating buffers
to copy glyphs etc. from runs to every time, even though CoreText
provides `CTRunGet*Ptr` functions which get *pointers* to the internal
storage of these values- these aren't guaranteed to return a usable
pointer but in that case we can always fall back to allocating again.
Also avoided allocation while processing glyphs by ensuring capacity
beforehand immediately after creating the `CTLine`.

The performance impact of this PR is negligible on my machine and
actually seems to be positive, probably due to avoiding allocations if I
had to guess.

pkg/macos/text.zig

@@ -4,6 +4,7 @@ const font_descriptor = @import("text/font_descriptor.zig");
 const font_manager = @import("text/font_manager.zig");
 const frame = @import("text/frame.zig");
 const framesetter = @import("text/framesetter.zig");
+const typesetter = @import("text/typesetter.zig");
 const line = @import("text/line.zig");
 const paragraph_style = @import("text/paragraph_style.zig");
 const run = @import("text/run.zig");
@@ -23,6 +24,7 @@ pub const createFontDescriptorsFromData = font_manager.createFontDescriptorsFrom
 pub const createFontDescriptorFromData = font_manager.createFontDescriptorFromData;
 pub const Frame = frame.Frame;
 pub const Framesetter = framesetter.Framesetter;
+pub const Typesetter = typesetter.Typesetter;
 pub const Line = line.Line;
 pub const ParagraphStyle = paragraph_style.ParagraphStyle;
 pub const ParagraphStyleSetting = paragraph_style.ParagraphStyleSetting;

pkg/macos/text/run.zig

@@ -15,10 +15,13 @@ pub const Run = opaque {
         return @intCast(c.CTRunGetGlyphCount(@ptrCast(self)));
     }
 
-    pub fn getGlyphsPtr(self: *Run) []const graphics.Glyph {
+    pub fn getGlyphsPtr(self: *Run) ?[]const graphics.Glyph {
         const len = self.getGlyphCount();
         if (len == 0) return &.{};
-        const ptr = c.CTRunGetGlyphsPtr(@ptrCast(self)) orelse &.{};
+        const ptr: [*c]const graphics.Glyph = @ptrCast(
+            c.CTRunGetGlyphsPtr(@ptrCast(self)),
+        );
+        if (ptr == null) return null;
         return ptr[0..len];
     }
 
@@ -34,10 +37,13 @@ pub const Run = opaque {
         return ptr;
     }
 
-    pub fn getPositionsPtr(self: *Run) []const graphics.Point {
+    pub fn getPositionsPtr(self: *Run) ?[]const graphics.Point {
         const len = self.getGlyphCount();
         if (len == 0) return &.{};
-        const ptr = c.CTRunGetPositionsPtr(@ptrCast(self)) orelse &.{};
+        const ptr: [*c]const graphics.Point = @ptrCast(
+            c.CTRunGetPositionsPtr(@ptrCast(self)),
+        );
+        if (ptr == null) return null;
         return ptr[0..len];
     }
 
@@ -53,10 +59,13 @@ pub const Run = opaque {
         return ptr;
     }
 
-    pub fn getAdvancesPtr(self: *Run) []const graphics.Size {
+    pub fn getAdvancesPtr(self: *Run) ?[]const graphics.Size {
         const len = self.getGlyphCount();
         if (len == 0) return &.{};
-        const ptr = c.CTRunGetAdvancesPtr(@ptrCast(self)) orelse &.{};
+        const ptr: [*c]const graphics.Size = @ptrCast(
+            c.CTRunGetAdvancesPtr(@ptrCast(self)),
+        );
+        if (ptr == null) return null;
         return ptr[0..len];
     }
 
@@ -72,10 +81,13 @@ pub const Run = opaque {
         return ptr;
     }
 
-    pub fn getStringIndicesPtr(self: *Run) []const usize {
+    pub fn getStringIndicesPtr(self: *Run) ?[]const usize {
         const len = self.getGlyphCount();
         if (len == 0) return &.{};
-        const ptr = c.CTRunGetStringIndicesPtr(@ptrCast(self)) orelse &.{};
+        const ptr: [*c]const usize = @ptrCast(
+            c.CTRunGetStringIndicesPtr(@ptrCast(self)),
+        );
+        if (ptr == null) return null;
         return ptr[0..len];
     }
 
@@ -90,4 +102,16 @@ pub const Run = opaque {
         );
         return ptr;
     }
+
+    pub fn getStatus(self: *Run) Status {
+        return @bitCast(c.CTRunGetStatus(@ptrCast(self)));
+    }
+};
+
+/// https://developer.apple.com/documentation/coretext/ctrunstatus?language=objc
+pub const Status = packed struct(u32) {
+    right_to_left: bool,
+    non_monotonic: bool,
+    has_non_identity_matrix: bool,
+    _pad: u29 = 0,
 };

pkg/macos/text/typesetter.zig

@@ -0,0 +1,36 @@
+const std = @import("std");
+const assert = std.debug.assert;
+const Allocator = std.mem.Allocator;
+const foundation = @import("../foundation.zig");
+const graphics = @import("../graphics.zig");
+const text = @import("../text.zig");
+const c = @import("c.zig").c;
+
+pub const Typesetter = opaque {
+    pub fn createWithAttributedStringAndOptions(
+        str: *foundation.AttributedString,
+        opts: *foundation.Dictionary,
+    ) Allocator.Error!*Typesetter {
+        return @as(
+            ?*Typesetter,
+            @ptrFromInt(@intFromPtr(c.CTTypesetterCreateWithAttributedStringAndOptions(
+                @ptrCast(str),
+                @ptrCast(opts),
+            ))),
+        ) orelse Allocator.Error.OutOfMemory;
+    }
+
+    pub fn release(self: *Typesetter) void {
+        foundation.CFRelease(self);
+    }
+
+    pub fn createLine(
+        self: *Typesetter,
+        range: foundation.c.CFRange,
+    ) *text.Line {
+        return @ptrFromInt(@intFromPtr(c.CTTypesetterCreateLine(
+            @ptrCast(self),
+            range,
+        )));
+    }
+};

src/font/shaper/coretext.zig

@@ -52,10 +52,10 @@ pub const Shaper = struct {
     /// The shared memory used for shaping results.
     cell_buf: CellBuf,
 
-    /// The cached writing direction value for shaping. This isn't
+    /// Cached attributes dict for creating CTTypesetter objects.
-    /// configurable we just use this as a cache to avoid creating
+    /// The values in this never change so we can avoid overhead
-    /// and releasing many objects when shaping.
+    /// by just creating it once and saving it for re-use.
-    writing_direction: *macos.foundation.Array,
+    typesetter_attr_dict: *macos.foundation.Dictionary,
 
     /// List where we cache fonts, so we don't have to remake them for
     /// every single shaping operation.
@@ -174,21 +174,28 @@ pub const Shaper = struct {
         //
         // See: https://github.com/mitchellh/ghostty/issues/1737
         // See: https://github.com/mitchellh/ghostty/issues/1442
-        const writing_direction = array: {
+        //
-            const dir: macos.text.WritingDirection = .lro;
+        // We used to do this by setting the writing direction attribute
-            const num = try macos.foundation.Number.create(
+        // on the attributed string we used, but it seems like that will
-                .int,
+        // still allow some weird results, for example a single space at
-                &@intFromEnum(dir),
+        // the end of a line composed of RTL characters will be cause it
-            );
+        // to output a run containing just that space, BEFORE it outputs
+        // the rest of the line as a separate run, very weirdly with the
+        // "right to left" flag set in the single space run's run status...
+        //
+        // So instead what we do is use a CTTypesetter to create our line,
+        // using the kCTTypesetterOptionForcedEmbeddingLevel attribute to
+        // force CoreText not to try doing any sort of BiDi, instead just
+        // treat all text as embedding level 0 (left to right).
+        const typesetter_attr_dict = dict: {
+            const num = try macos.foundation.Number.create(.int, &0);
             defer num.release();
-
+            break :dict try macos.foundation.Dictionary.create(
-            var arr_init = [_]*const macos.foundation.Number{num};
+                &.{macos.c.kCTTypesetterOptionForcedEmbeddingLevel},
-            break :array try macos.foundation.Array.create(
+                &.{num},
-                macos.foundation.Number,
-                &arr_init,
             );
         };
-        errdefer writing_direction.release();
+        errdefer typesetter_attr_dict.release();
 
         // Create the CF release thread.
         var cf_release_thread = try alloc.create(CFReleaseThread);
@@ -210,7 +217,7 @@ pub const Shaper = struct {
             .run_state = run_state,
             .features = features,
             .features_no_default = features_no_default,
-            .writing_direction = writing_direction,
+            .typesetter_attr_dict = typesetter_attr_dict,
             .cached_fonts = .{},
             .cached_font_grid = 0,
             .cf_release_pool = .{},
@@ -224,7 +231,7 @@ pub const Shaper = struct {
         self.run_state.deinit(self.alloc);
         self.features.release();
         self.features_no_default.release();
-        self.writing_direction.release();
+        self.typesetter_attr_dict.release();
 
         {
             for (self.cached_fonts.items) |ft| {
@@ -346,8 +353,8 @@ pub const Shaper = struct {
             run.font_index,
         );
 
-        // Make room for the attributed string and the CTLine.
+        // Make room for the attributed string, CTTypesetter, and CTLine.
-        try self.cf_release_pool.ensureUnusedCapacity(self.alloc, 3);
+        try self.cf_release_pool.ensureUnusedCapacity(self.alloc, 4);
 
         const str = macos.foundation.String.createWithCharactersNoCopy(state.unichars.items);
         self.cf_release_pool.appendAssumeCapacity(str);
@@ -359,8 +366,17 @@ pub const Shaper = struct {
         );
         self.cf_release_pool.appendAssumeCapacity(attr_str);
 
-        // We should always have one run because we do our own run splitting.
+        // Create a typesetter from the attributed string and the cached
-        const line = try macos.text.Line.createWithAttributedString(attr_str);
+        // attr dict. (See comment in init for more info on the attr dict.)
+        const typesetter =
+            try macos.text.Typesetter.createWithAttributedStringAndOptions(
+                attr_str,
+                self.typesetter_attr_dict,
+            );
+        self.cf_release_pool.appendAssumeCapacity(typesetter);
+
+        // Create a line from the typesetter
+        const line = typesetter.createLine(.{ .location = 0, .length = 0 });
         self.cf_release_pool.appendAssumeCapacity(line);
 
         // This keeps track of the current offsets within a single cell.
@@ -369,7 +385,12 @@ pub const Shaper = struct {
             x: f64 = 0,
             y: f64 = 0,
         } = .{};
+
+        // Clear our cell buf and make sure we have enough room for the whole
+        // line of glyphs, so that we can just assume capacity when appending
+        // instead of maybe allocating.
         self.cell_buf.clearRetainingCapacity();
+        try self.cell_buf.ensureTotalCapacity(self.alloc, line.getGlyphCount());
 
         // CoreText may generate multiple runs even though our input to
         // CoreText is already split into runs by our own run iterator.
@@ -381,9 +402,9 @@ pub const Shaper = struct {
             const ctrun = runs.getValueAtIndex(macos.text.Run, i);
 
             // Get our glyphs and positions
-            const glyphs = try ctrun.getGlyphs(alloc);
+            const glyphs = ctrun.getGlyphsPtr() orelse try ctrun.getGlyphs(alloc);
-            const advances = try ctrun.getAdvances(alloc);
+            const advances = ctrun.getAdvancesPtr() orelse try ctrun.getAdvances(alloc);
-            const indices = try ctrun.getStringIndices(alloc);
+            const indices = ctrun.getStringIndicesPtr() orelse try ctrun.getStringIndices(alloc);
             assert(glyphs.len == advances.len);
             assert(glyphs.len == indices.len);
 
@@ -406,7 +427,7 @@ pub const Shaper = struct {
                     cell_offset = .{ .cluster = cluster };
                 }
 
-                try self.cell_buf.append(self.alloc, .{
+                self.cell_buf.appendAssumeCapacity(.{
                     .x = @intCast(cluster),
                     .x_offset = @intFromFloat(@round(cell_offset.x)),
                     .y_offset = @intFromFloat(@round(cell_offset.y)),
@@ -511,15 +532,10 @@ pub const Shaper = struct {
         // Get our font and use that get the attributes to set for the
         // attributed string so the whole string uses the same font.
         const attr_dict = dict: {
-            var keys = [_]?*const anyopaque{
+            break :dict try macos.foundation.Dictionary.create(
-                macos.text.StringAttribute.font.key(),
+                &.{macos.text.StringAttribute.font.key()},
-                macos.text.StringAttribute.writing_direction.key(),
+                &.{run_font},
-            };
+            );
-            var values = [_]?*const anyopaque{
-                run_font,
-                self.writing_direction,
-            };
-            break :dict try macos.foundation.Dictionary.create(&keys, &values);
         };
 
         self.cached_fonts.items[index_int] = attr_dict;