From 9c112c768d3d9799180acab9fbe888e6d7e420c9 Mon Sep 17 00:00:00 2001 From: Andros Fenollosa Date: Sun, 31 May 2026 18:40:07 +0200 Subject: [PATCH] Fix text rendering: MTLLoadActionLoad + glyph ID path + rif copy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three critical bugs fixed: 1. MTLLoadActionClear wipes staticTexture every update_begin cycle. Emacs calls update_begin/end multiple times per second (cursor blink, modeline, etc.). Each call was clearing all previously drawn content. Fix: only clear on texture (re)creation; use MTLLoadActionLoad to PRESERVE content between update_begin cycles. Emacs's draw_glyph_string fills backgrounds only for the regions being updated. 2. char2b contains CGGlyph IDs (not Unicode codepoints) in the macfont backend. mtl_draw_glyph_string was calling mtl_cache_glyph() which used CTFontGetGlyphsForCharacters (Unicode→glyph) on values that were already glyph IDs — double conversion, wrong glyphs rendered. Fix: new mtl_cache_glyph_id() uses CTFontDrawGlyphs directly with the glyph ID from char2b, same as ns_draw_glyph_string_foreground does. 3. mtl_patch_terminal_rif replaced the ENTIRE terminal rif (including NULL frame_parm_handlers). init_frame_faces/realize_basic_faces accesses rif->frame_parm_handlers during Fx_create_frame → SIGSEGV. Fix: copy the NS rif (preserving frame_parm_handlers and all management functions), then override ONLY the pixel-drawing functions with Metal. Added: mtl-capture-frame, mtl-draw-stats diagnostics (kept for debugging). --- src/mtlfns.m | 89 +++++++++++++++++++++++++ src/mtlterm.h | 10 +++ src/mtlterm.m | 181 ++++++++++++++++++++++++++++++++++++++------------ 3 files changed, 236 insertions(+), 44 deletions(-) diff --git a/src/mtlfns.m b/src/mtlfns.m index 134e7a5ac60..3f96c0ac0f3 100644 --- a/src/mtlfns.m +++ b/src/mtlfns.m @@ -199,6 +199,81 @@ quads to an off-screen MTLTexture. Returns t on success. */) return mtl_render_text_png (SSDATA (path)) ? Qt : Qnil; } +/* ----------------------------------------------------------------------- + DEFUN: mtl-capture-frame — save the current Metal staticTexture to PNG. + Used for visual regression testing and debugging rendering issues. + ----------------------------------------------------------------------- */ + +DEFUN ("mtl-capture-frame", Fmtl_capture_frame, Smtl_capture_frame, 1, 2, 0, + doc: /* Save the Metal staticTexture for FRAME (or selected frame) to PATH. +Returns t on success. The PNG shows exactly what Metal has rendered into +the intermediate texture, before cursor/animation overlay. */) + (Lisp_Object path, Lisp_Object frame) +{ + CHECK_STRING (path); + struct frame *f = NILP (frame) ? XFRAME (selected_frame) : XFRAME (frame); + if (!f) return Qnil; + + MtlFrameData *fd = mtl_get_frame_data (f); + if (!fd || !fd.staticTexture) return Qnil; + + id src = fd.staticTexture; + NSUInteger W = src.width, H = src.height; + + id dev = mtl_get_device(); + id q = mtl_get_queue(); + if (!dev || !q) return Qnil; + + /* Blit from Private texture to Shared texture for CPU readback */ + MTLTextureDescriptor *td = + [MTLTextureDescriptor texture2DDescriptorWithPixelFormat:MTLPixelFormatBGRA8Unorm + width:W height:H mipmapped:NO]; + td.usage = MTLTextureUsageShaderRead; + td.storageMode = MTLStorageModeShared; + id readback = [dev newTextureWithDescriptor:td]; + if (!readback) return Qnil; + + id cmd = [q commandBuffer]; + id blit = [cmd blitCommandEncoder]; + [blit copyFromTexture:src + sourceSlice:0 sourceLevel:0 + sourceOrigin:MTLOriginMake(0,0,0) + sourceSize:MTLSizeMake(W,H,1) + toTexture:readback + destinationSlice:0 destinationLevel:0 + destinationOrigin:MTLOriginMake(0,0,0)]; + [blit endEncoding]; + [cmd commit]; + [cmd waitUntilCompleted]; + + /* Read pixels (BGRA) and swap to RGBA for PNG */ + NSUInteger bpr = W * 4; + uint8_t *px = malloc (bpr * H); + if (!px) return Qnil; + [readback getBytes:px bytesPerRow:bpr + fromRegion:MTLRegionMake2D(0,0,W,H) mipmapLevel:0]; + + /* BGRA → RGBA */ + for (NSUInteger i = 0; i < W * H; i++) + { uint8_t b=px[i*4]; px[i*4]=px[i*4+2]; px[i*4+2]=b; } + + CGColorSpaceRef cs = CGColorSpaceCreateDeviceRGB(); + CGContextRef ctx = CGBitmapContextCreate(px, W, H, 8, bpr, cs, + (CGBitmapInfo)(kCGImageAlphaPremultipliedLast)); + CGColorSpaceRelease(cs); + CGImageRef img = CGBitmapContextCreateImage(ctx); + CGContextRelease(ctx); + NSString *nspath = [NSString stringWithUTF8String:SSDATA(path)]; + NSURL *url = [NSURL fileURLWithPath:nspath]; + CGImageDestinationRef dst = CGImageDestinationCreateWithURL( + (__bridge CFURLRef)url, kUTTypePNG, 1, NULL); + CGImageDestinationAddImage(dst, img, NULL); + bool ok = CGImageDestinationFinalize(dst); + CFRelease(dst); CGImageRelease(img); free(px); + + return ok ? Qt : Qnil; +} + /* ----------------------------------------------------------------------- DEFUN: mtl-create-frame (stub — use mtl-enable-for-frame instead) ----------------------------------------------------------------------- */ @@ -284,6 +359,18 @@ DEFUN ("mtl-trail-length", Fmtl_trail_length, Smtl_trail_length, 1, 1, 0, return len; } +DEFUN ("mtl-draw-stats", Fmtl_draw_stats, Smtl_draw_stats, 0, 0, 0, + doc: /* Return diagnostic counters for Metal glyph rendering. +Returns an alist with: total-calls, no-fd (no encoder), no-font, glyphs-drawn. */) + (void) +{ + return list4 ( + Fcons (intern ("total-calls"), make_fixnum (mtl_dgs_call_count)), + Fcons (intern ("no-fd"), make_fixnum (mtl_dgs_nofd_count)), + Fcons (intern ("no-font"), make_fixnum (mtl_dgs_nofont_count)), + Fcons (intern ("glyphs-drawn"), make_fixnum (mtl_dgs_drawn_count))); +} + DEFUN ("mtl-animation-status", Fmtl_animation_status, Smtl_animation_status, 0, 0, 0, doc: /* Return an alist with current Metal animation configuration. */) @@ -316,6 +403,8 @@ syms_of_mtlfns (void) defsubr (&Smtl_scroll_duration); defsubr (&Smtl_trail_length); defsubr (&Smtl_animation_status); + defsubr (&Smtl_capture_frame); + defsubr (&Smtl_draw_stats); } #endif /* HAVE_MTL */ diff --git a/src/mtlterm.h b/src/mtlterm.h index e0653d54aa5..54fe6211f43 100644 --- a/src/mtlterm.h +++ b/src/mtlterm.h @@ -251,6 +251,16 @@ extern bool mtl_render_offscreen_png (const char *path, int w, int h, extern bool mtl_render_text_png (const char *path); extern void mtl_patch_terminal_rif (struct frame *f); +/* Device/queue accessors for mtlfns.m (g_device and g_queue are file-static) */ +extern id mtl_get_device (void); +extern id mtl_get_queue (void); + +/* Diagnostic counters for mtl_draw_glyph_string */ +extern int mtl_dgs_call_count; +extern int mtl_dgs_nofd_count; +extern int mtl_dgs_nofont_count; +extern int mtl_dgs_drawn_count; + extern void syms_of_mtlfns (void); #endif /* HAVE_MTL */ diff --git a/src/mtlterm.m b/src/mtlterm.m index 1abc901241d..8e8dab8025c 100644 --- a/src/mtlterm.m +++ b/src/mtlterm.m @@ -295,6 +295,12 @@ ns_color_to_pixel (NSColor *c) Global Metal setup (called once) ----------------------------------------------------------------------- */ +id +mtl_get_device (void) { return g_device; } + +id +mtl_get_queue (void) { return g_queue; } + static BOOL mtl_global_setup (void) { @@ -552,35 +558,23 @@ glyph_cache_insert (uint64_t key) Glyph rasterization — CoreText → R8Unorm atlas patch ----------------------------------------------------------------------- */ -MtlGlyphCacheEntry * -mtl_cache_glyph (CTFontRef font, uint32_t codepoint) +/* Core glyph rasterization: rasterize a CGGlyph (ID already resolved) into + the atlas. This is the single rasterization path used by BOTH: + - mtl_cache_glyph_id (from Emacs glyph_string.char2b — already glyph IDs) + - mtl_cache_glyph (from off-screen PNG test, uses Unicode → glyph lookup) + + The critical insight: glyph_string.char2b stores GLYPH IDs for the macfont + backend (not Unicode codepoints). Always use glyph IDs with CTFont APIs + that accept CGGlyph directly (CTFontGetBoundingRectsForGlyphs, CTFontDrawGlyphs). + Never call CTFontGetGlyphsForCharacters on a value that is already a glyph ID. +*/ +static MtlGlyphCacheEntry * +mtl_rasterize_glyph_id (CTFontRef font, CGGlyph cgGlyph, uint64_t key) { - /* Key: mix font pointer and codepoint */ - uint64_t key = ((uint64_t)(uintptr_t)font << 21) ^ codepoint; - MtlGlyphCacheEntry *entry = glyph_cache_lookup (key); - if (entry) return entry; if (!g_atlas) return NULL; - UniChar chars[2]; - int nc = 1; - if (codepoint >= 0x10000) - { - /* Surrogate pair */ - codepoint -= 0x10000; - chars[0] = 0xD800 | (codepoint >> 10); - chars[1] = 0xDC00 | (codepoint & 0x3FF); - nc = 2; - } - else - { - chars[0] = (UniChar)codepoint; - } + MtlGlyphCacheEntry *entry; - CGGlyph cgGlyph = 0; - if (!CTFontGetGlyphsForCharacters (font, chars, &cgGlyph, nc) || cgGlyph == 0) - return NULL; - - /* Bounding box for this glyph (in font units → pixels) */ CGRect bbox = CTFontGetBoundingRectsForGlyphs (font, kCTFontOrientationDefault, &cgGlyph, NULL, 1); @@ -657,6 +651,50 @@ mtl_cache_glyph (CTFontRef font, uint32_t codepoint) return entry; } +/* mtl_cache_glyph_id — PRIMARY path for Emacs text rendering. + char2b[i] in glyph_string is a GLYPH ID (CGGlyph), not a Unicode codepoint. + We use it directly with CoreText APIs that accept CGGlyph. */ +static MtlGlyphCacheEntry * +mtl_cache_glyph_id (CTFontRef font, CGGlyph glyphId) +{ + if (!font || glyphId == 0) return NULL; + + /* Cache key = (font_ptr × large_prime) XOR glyphId */ + uint64_t key = ((uint64_t)(uintptr_t)font * 6364136223846793005ULL) ^ (uint64_t)glyphId; + + MtlGlyphCacheEntry *e = glyph_cache_lookup (key); + if (e) return e; + if (!g_atlas) return NULL; + + return mtl_rasterize_glyph_id (font, glyphId, key); +} + +/* mtl_cache_glyph — SECONDARY path for off-screen test renders. + Accepts a Unicode codepoint, converts to glyph ID first. */ +MtlGlyphCacheEntry * +mtl_cache_glyph (CTFontRef font, uint32_t codepoint) +{ + if (!font) return NULL; + + UniChar chars[2]; + int nc = 1; + if (codepoint >= 0x10000) + { + codepoint -= 0x10000; + chars[0] = 0xD800 | (codepoint >> 10); + chars[1] = 0xDC00 | (codepoint & 0x3FF); + nc = 2; + } + else + chars[0] = (UniChar)codepoint; + + CGGlyph cgGlyph = 0; + if (!CTFontGetGlyphsForCharacters (font, chars, &cgGlyph, nc) || cgGlyph == 0) + return NULL; + + return mtl_cache_glyph_id (font, cgGlyph); +} + /* ----------------------------------------------------------------------- Per-frame Metal state (stored as ObjC associated object on EmacsView) ----------------------------------------------------------------------- */ @@ -931,9 +969,16 @@ easing_apply (MtlScrollEasing mode, float t) - (void)beginFrame { - /* Ensure staticTexture exists and matches drawable size */ CGSize dsz = self.metalLayer.drawableSize; + + /* Ensure staticTexture exists and matches drawable size. + Bug fix: multiple update_begin/end cycles happen per Emacs redisplay pass + (cursor blink, modeline, etc.). Using MTLLoadActionClear on every beginFrame + wiped out the drawing from the PREVIOUS cycle, leaving only the background. + Fix: only clear when the texture is (re)created; preserve content otherwise. + Emacs's draw_glyph_string already fills backgrounds for updated regions. */ NSUInteger tw = (NSUInteger)dsz.width, th = (NSUInteger)dsz.height; + BOOL needsClear = NO; if (!self.staticTexture || self.staticTexture.width != tw || self.staticTexture.height != th) @@ -944,6 +989,7 @@ easing_apply (MtlScrollEasing mode, float t) td.usage = MTLTextureUsageRenderTarget | MTLTextureUsageShaderRead; td.storageMode = MTLStorageModePrivate; self.staticTexture = [g_device newTextureWithDescriptor:td]; + needsClear = YES; /* New or resized texture: clear to background color */ } NSSize sz = self.metalLayer.frame.size; @@ -957,9 +1003,11 @@ easing_apply (MtlScrollEasing mode, float t) unpack_color (bg, &r, &g, &b); MTLRenderPassDescriptor *rpd = [MTLRenderPassDescriptor renderPassDescriptor]; - rpd.colorAttachments[0].texture = self.staticTexture; - rpd.colorAttachments[0].loadAction = MTLLoadActionClear; - rpd.colorAttachments[0].clearColor = MTLClearColorMake(r, g, b, 1.0); + rpd.colorAttachments[0].texture = self.staticTexture; + /* Load previous content (not clear) unless this is a fresh texture. + The NS backend redraws only dirty regions; our Metal backend must do the same. */ + rpd.colorAttachments[0].loadAction = needsClear ? MTLLoadActionClear : MTLLoadActionLoad; + rpd.colorAttachments[0].clearColor = MTLClearColorMake(r, g, b, 1.0); rpd.colorAttachments[0].storeAction = MTLStoreActionStore; self.cmdBuf = [g_queue commandBuffer]; @@ -1568,12 +1616,19 @@ mtl_draw_image_texture (MtlFrameData *fd, id tex, redisplay_interface — all file-static, called by xdisp.c engine ----------------------------------------------------------------------- */ +int mtl_dgs_call_count = 0; /* total calls */ +int mtl_dgs_nofd_count = 0; /* no fd or encoder */ +int mtl_dgs_nofont_count = 0; /* no ctfont */ +int mtl_dgs_drawn_count = 0; /* glyphs actually drawn */ + static void mtl_draw_glyph_string (struct glyph_string *s) { + mtl_dgs_call_count++; + struct frame *f = s->f; MtlFrameData *fd = mtl_get_frame_data (f); - if (!fd || !fd.encoder) return; + if (!fd || !fd.encoder) { mtl_dgs_nofd_count++; return; } struct face *face = s->face; unsigned long fg = face ? face->foreground : 0xECEFF4; @@ -1617,26 +1672,27 @@ mtl_draw_glyph_string (struct glyph_string *s) /* Get CoreText font */ CTFontRef ctfont = mtl_ctfont_for_face (face); - if (!ctfont) return; + if (!ctfont) { mtl_dgs_nofont_count++; return; } + - /* Draw each glyph at its correct x position. - We use text_extents to get individual advance widths. */ float pen_x = (float)s->x; float baseline_y = (float)s->ybase; for (int i = 0; i < s->nchars; i++) { - unsigned int cp = s->char2b ? s->char2b[i] : 0; - if (!cp) { pen_x += (float)FRAME_COLUMN_WIDTH (f); continue; } + /* char2b contains GLYPH IDs for the macfont backend — NOT Unicode codepoints. + Use mtl_cache_glyph_id which calls CoreText with the ID directly. */ + CGGlyph glyphId = s->char2b ? (CGGlyph)s->char2b[i] : 0; + if (!glyphId) { pen_x += (float)FRAME_COLUMN_WIDTH (f); continue; } - MtlGlyphCacheEntry *ge = mtl_cache_glyph (ctfont, cp); + MtlGlyphCacheEntry *ge = mtl_cache_glyph_id (ctfont, glyphId); if (!ge) continue; - /* Glyph origin: (pen_x, baseline_y). - The cache entry's bearing_y is distance from top-of-bitmap to baseline. - So the top of the bitmap is baseline_y - bearing_y (in top-left coords). */ if (ge->width > 0) { + mtl_dgs_drawn_count++; + /* bearing_y: distance from glyph top-left to baseline. + In our top-left coord system, glyph top = baseline_y - bearing_y. */ CGPoint origin = CGPointMake (pen_x, baseline_y); [fd drawGlyph:ge at:origin color:fg]; } @@ -1956,17 +2012,54 @@ static const char mtl_resize_obs_key; @end +/* One copy of the NS rif, patched with our Metal drawing functions. + Allocated once when mtl_patch_terminal_rif is first called. + CRITICAL: We must NOT replace the entire rif — the NS rif's + frame_parm_handlers, produce_glyphs, and management functions are + called by init_frame_faces / realize_basic_faces / Fx_create_frame. + Replacing them all with NULLs causes SIGSEGV there. + Solution: copy the NS rif, then override ONLY the drawing functions. */ +static struct redisplay_interface *mtl_ns_rif_copy = NULL; + void mtl_patch_terminal_rif (struct frame *f) { struct terminal *term = FRAME_TERMINAL (f); - if (!term) return; + if (!term || !term->rif) return; - /* Replace the redisplay interface so draw_glyph_string etc. use Metal. */ - term->rif = &mtl_redisplay_interface; + /* Build a patched copy of the NS rif (once per process lifetime). + All NS management/frame functions are preserved; only pixel-drawing + functions are replaced with Metal equivalents. */ + if (!mtl_ns_rif_copy) + { + mtl_ns_rif_copy = xmalloc (sizeof (struct redisplay_interface)); + *mtl_ns_rif_copy = *term->rif; /* copy all NS functions as baseline */ - /* Patch render cycle hooks so Metal manages the frame lifecycle. - NS update_begin (lockFocus) and update_end (flushWindow) are replaced. */ + /* Override only the functions that perform pixel drawing. + Everything else (frame_parm_handlers, produce_glyphs, etc.) + stays as the NS implementation — frame management must work. */ + mtl_ns_rif_copy->scroll_run_hook = mtl_scroll_run; + mtl_ns_rif_copy->after_update_window_line_hook = mtl_after_update_window_line; + mtl_ns_rif_copy->flush_display = mtl_flush_display; + mtl_ns_rif_copy->draw_fringe_bitmap = mtl_draw_fringe_bitmap; + mtl_ns_rif_copy->define_fringe_bitmap = mtl_define_fringe_bitmap; + mtl_ns_rif_copy->destroy_fringe_bitmap = mtl_destroy_fringe_bitmap; + mtl_ns_rif_copy->compute_glyph_string_overhangs= mtl_compute_glyph_string_overhangs; + mtl_ns_rif_copy->draw_glyph_string = mtl_draw_glyph_string; + mtl_ns_rif_copy->clear_frame_area = mtl_clear_frame_area; + mtl_ns_rif_copy->clear_under_internal_border = mtl_clear_under_internal_border; + mtl_ns_rif_copy->draw_window_cursor = mtl_draw_window_cursor; + mtl_ns_rif_copy->draw_vertical_window_border = mtl_draw_vertical_window_border; + mtl_ns_rif_copy->draw_window_divider = mtl_draw_window_divider; + mtl_ns_rif_copy->shift_glyphs_for_insert = mtl_shift_glyphs_for_insert; + /* show_hourglass, hide_hourglass, default_font_parameter: keep NS versions */ + } + + term->rif = mtl_ns_rif_copy; + + /* Patch render cycle hooks so Metal manages the frame pixel lifecycle. + The NS backend's update_begin calls [view lockFocus] for CoreGraphics; + we bypass that entirely and use Metal command buffers instead. */ term->update_begin_hook = mtl_update_begin; term->update_end_hook = mtl_update_end; term->clear_frame_hook = mtl_clear_frame;