From f04a566bb466b75faaf428751bd113eba3bc17f5 Mon Sep 17 00:00:00 2001 From: Rye Date: Wed, 10 Jun 2026 22:35:32 -0400 Subject: [PATCH 1/5] Fix mid-render atlas bind stomp corrupting batched text Rasterizing a cache-missed glyph inside the render loop rebinds texture unit 0 to the upload target and leaves it bound (LLImageGL::setSubImage uploads with skip_unbind; nextOpenPos binds brand-new sheets to create their GL textures). Quads already queued for the previous atlas then flushed under the stomped binding and sampled another atlas page, rendering fragments of unrelated glyphs - the occasional "text shows other text / broken glyphs" corruption reported since the font rework. The legacy per-codepoint `last_char != wch` flush removed in 7c05b2de82 had bounded the misdraw to ~1 glyph, hiding it; idle-sheet eviction made re-rasterization (and thus the stomp window) recurrent. llfontgl.cpp: - render: route every batch submit through a flush_batch helper that re-asserts the batch's own atlas texture before drawing. bind() is a cached no-op when the binding didn't move, so the common path pays nothing. Applied to the shaped path, codepoint path, end-of-pass flush, and pass B. - render: skip quad emission when the slot's atlas page can't resolve (released sheet) instead of emitting quads against whatever texture happens to be bound. Pen advance still runs so layout is preserved. llfontbitmapcache.cpp: - Zero-clear Color (BGRA) sheets on allocation. LLImageRaw doesn't zero its buffer, so emoji atlas borders and inter-glyph gutters uploaded heap garbage - latent today under point sampling, visible the moment the shadow shader's dilated taps or linear filtering sample past a glyph edge. Grayscale sheets were already cleared. llfontfreetype.cpp: - getGlyphInfo / getGlyphInfoByIndex: treat Unspecified as match-any at cache probe time and resolve to Grayscale only when rasterizing. Resolving before the probe made every measurement-path lookup of an already-rendered color glyph miss and rasterize a duplicate grayscale copy (a full COLRv1 paint walk plus a wasted Grayscale atlas slot per emoji). llvertexbuffer.h: - LLVertexBufferData's value constructor cross-assigned model_view into mProjection and projection into mModelView. No live caller (drawWithMatrix has none), but wrong regardless. tests/llfontgl_test.cpp: - Regression test: capture a render of a Latin prefix plus a cold U+1F995 through gGL.beginList and require the prefix batch's recorded mTexName to be the grayscale atlas page its UVs were built against; pre-fix it carried the emoji sheet bound by the mid-render upload. Headless-only (OSMesa) like the rest of the render group. Co-Authored-By: Claude Fable 5 --- indra/llrender/llfontbitmapcache.cpp | 8 + indra/llrender/llfontfreetype.cpp | 27 ++- indra/llrender/llfontgl.cpp | 287 +++++++++++++------------ indra/llrender/llvertexbuffer.h | 4 +- indra/llrender/tests/llfontgl_test.cpp | 86 ++++++++ 5 files changed, 265 insertions(+), 147 deletions(-) diff --git a/indra/llrender/llfontbitmapcache.cpp b/indra/llrender/llfontbitmapcache.cpp index cf9621c201..60dc599c28 100644 --- a/indra/llrender/llfontbitmapcache.cpp +++ b/indra/llrender/llfontbitmapcache.cpp @@ -176,6 +176,14 @@ bool LLFontBitmapCache::nextOpenPos(S32 width, S32 height, S32& pos_x, S32& pos_ { image_raw->clear(255, 0); } + else + { + // LLImageRaw doesn't zero its allocation. The BGRA sheet's + // borders and inter-glyph gutters are sampled by the shadow + // shader's dilated taps (and by any future linear filtering), + // so they must be transparent black, not heap garbage. + image_raw->clear(0, 0, 0, 0); + } // Make corresponding GL image. mImageGLVec[bitmap_idx].emplace_back(new LLImageGL(image_raw, false, false)); diff --git a/indra/llrender/llfontfreetype.cpp b/indra/llrender/llfontfreetype.cpp index 948b33296c..e461e2eabf 100644 --- a/indra/llrender/llfontfreetype.cpp +++ b/indra/llrender/llfontfreetype.cpp @@ -835,14 +835,15 @@ LLFontGlyphInfo* LLFontFreetype::getGlyphInfo(llwchar wch, EFontGlyphType glyph_ return nullptr; llassert(!mIsFallback); - const EFontGlyphType resolve_type = (EFontGlyphType::Unspecified != glyph_type) - ? glyph_type : EFontGlyphType::Grayscale; + // glyph_type passes through unresolved: getGlyphInfoByIndex treats + // Unspecified as match-any at lookup and only pins a concrete type + // when it has to rasterize. // Hot path: codepoint exists on this head's face. One cmap lookup // (cached) + one (face, glyph_index) hash lookup. U32 glyph_index = getCharGlyphIndex(wch); if (glyph_index != 0) - return getGlyphInfoByIndex(this, glyph_index, resolve_type); + return getGlyphInfoByIndex(this, glyph_index, glyph_type); // Cold path: walk fallbacks in codepoint-priority order, which // differs from the shape-path priority in selectShapingFace: @@ -865,25 +866,25 @@ LLFontGlyphInfo* LLFontFreetype::getGlyphInfo(llwchar wch, EFontGlyphType glyph_ [&](const char_functor_t& f) { return f && f(wch); }); hit.first) { - return getGlyphInfoByIndex(hit.first, hit.second, resolve_type); + return getGlyphInfoByIndex(hit.first, hit.second, glyph_type); } } if (auto hit = find_fallback_hit(mFallbackFonts, wch, [](const char_functor_t& f) { return !f; }); hit.first) { - return getGlyphInfoByIndex(hit.first, hit.second, resolve_type); + return getGlyphInfoByIndex(hit.first, hit.second, glyph_type); } if (auto hit = find_fallback_hit(mFallbackFonts, wch, [](const char_functor_t& f) { return (bool)f; }); hit.first) { - return getGlyphInfoByIndex(hit.first, hit.second, resolve_type); + return getGlyphInfoByIndex(hit.first, hit.second, glyph_type); } // No face in our chain has this codepoint. Use the head face's // notdef (glyph_index=0) — pre-warmed during loadFace. - return getGlyphInfoByIndex(this, 0, resolve_type); + return getGlyphInfoByIndex(this, 0, glyph_type); } LLFontGlyphInfo* LLFontFreetype::getGlyphInfoByIndex(const LLFontFreetype* fontp, U32 glyph_index, EFontGlyphType glyph_type) const @@ -900,12 +901,20 @@ LLFontGlyphInfo* LLFontFreetype::getGlyphInfoByIndex(const LLFontFreetype* fontp // is observed consistently by every freetype that ever rendered the // glyph: the face's findGlyphInfo returns null after eviction, so we // fall through to addShapedGlyphFromFont and re-rasterize. - const EFontGlyphType resolve_type = (EFontGlyphType::Unspecified != glyph_type) ? glyph_type : EFontGlyphType::Grayscale; + // + // Unspecified probes the cache as match-any (findGlyphInfo returns the + // first entry of either type) and only resolves to Grayscale when the + // glyph has to be rasterized. Measurement paths pass Unspecified — they + // need metrics, not a particular atlas — and resolving before the probe + // forced a duplicate Grayscale rasterization (a full COLRv1 paint walk + // plus a Grayscale atlas slot) for every color glyph that had already + // been rendered through the Color atlas. if (fontp && fontp->mFace) { - if (LLFontGlyphInfo* gi = fontp->mFace->findGlyphInfo(glyph_index, resolve_type)) + if (LLFontGlyphInfo* gi = fontp->mFace->findGlyphInfo(glyph_index, glyph_type)) return gi; } + const EFontGlyphType resolve_type = (EFontGlyphType::Unspecified != glyph_type) ? glyph_type : EFontGlyphType::Grayscale; return addShapedGlyphFromFont(fontp, glyph_index, resolve_type); } diff --git a/indra/llrender/llfontgl.cpp b/indra/llrender/llfontgl.cpp index 726995f0a0..24b9ebc9d3 100644 --- a/indra/llrender/llfontgl.cpp +++ b/indra/llrender/llfontgl.cpp @@ -507,6 +507,35 @@ S32 LLFontGL::render(const LLWString &wstr, S32 begin_offset, F32 x, F32 y, cons std::pair bitmap_entry = std::make_pair(EFontGlyphType::Grayscale, -1); S32 glyph_count = 0; + // Atlas texture the pending batch's UVs were built against. Every batch + // submit re-asserts this binding: rasterizing a cache-missed glyph between + // accumulation and flush rebinds unit 0 to the upload target and leaves it + // bound (LLImageGL::setSubImage uploads with skip_unbind, and nextOpenPos + // binds brand-new sheets to create their GL textures), so the texture + // bound at flush time is NOT necessarily the one the pending quads were + // built for. Submitting under the stomped binding samples another atlas + // page — glyphs from unrelated text — which is exactly the legacy + // "CJK/emoji on first render" corruption the old per-codepoint + // `last_char != wch` flush band-aided around (the per-char flush kept the + // pending batch to ~1 glyph, making the misdraw practically invisible). + // bind() is a cached no-op when the binding didn't move, so the re-assert + // costs nothing on the common path. + LLImageGL* batch_image = nullptr; + auto flush_batch = [&]() + { + if (glyph_count > 0) + { + if (batch_image) + { + gGL.getTexUnit(0)->bind(batch_image); + } + gGL.begin(LLRender::TRIANGLES); + gGL.vertexBatchPreTransformed(vertices, uvs, colors, glyph_count * 6); + gGL.end(); + glyph_count = 0; + } + }; + // Itemize + shape the slice via the shared helper. Strict-monospace // gets emoji-cluster ranges so ASCII keeps the codepoint path's exact // metrics (visual parity with toggle-off) while embedded emoji @@ -596,13 +625,9 @@ S32 LLFontGL::render(const LLWString &wstr, S32 begin_offset, F32 x, F32 y, cons std::pair next_bitmap_entry = slot.mBitmapEntry; if (glyph_face != current_face || next_bitmap_entry != bitmap_entry) { - if (glyph_count > 0) - { - gGL.begin(LLRender::TRIANGLES); - gGL.vertexBatchPreTransformed(vertices, uvs, colors, glyph_count * 6); - gGL.end(); - glyph_count = 0; - } + // Drain the queued glyphs under their own texture + // before switching the batch to the new one. + flush_batch(); bitmap_entry = next_bitmap_entry; if (glyph_face != current_face) { @@ -614,11 +639,17 @@ S32 LLFontGL::render(const LLWString &wstr, S32 begin_offset, F32 x, F32 y, cons inv_height = 1.f / font_bitmap_cache->getBitmapHeight(); } } - if (font_bitmap_cache) + // Null when the slot's sheet has been released + // (shouldn't happen mid-render — eviction runs at the + // frame boundary and purges glyph entries first). The + // emission guard below skips the quad rather than + // sampling whatever texture happens to be bound. + batch_image = font_bitmap_cache + ? font_bitmap_cache->getImageGL(bitmap_entry.first, bitmap_entry.second) + : nullptr; + if (batch_image) { - LLImageGL* font_image = font_bitmap_cache->getImageGL(bitmap_entry.first, bitmap_entry.second); - if (font_image) - gGL.getTexUnit(0)->bind(font_image); + gGL.getTexUnit(0)->bind(batch_image); } } @@ -632,46 +663,46 @@ S32 LLFontGL::render(const LLWString &wstr, S32 begin_offset, F32 x, F32 y, cons break; } - LLRectf uv_rect(slot.mXBitmapOffset * inv_width, - (slot.mYBitmapOffset + slot.mHeight + PAD_UVY) * inv_height, - (slot.mXBitmapOffset + slot.mWidth) * inv_width, - (slot.mYBitmapOffset - PAD_UVY) * inv_height); - LLRectf screen_rect(glyph_x, - glyph_y, - glyph_x + (F32)slot.mWidth, - glyph_y - (F32)slot.mHeight); - - if (glyph_count >= GLYPH_BATCH_SIZE) + if (batch_image) { - gGL.begin(LLRender::TRIANGLES); - gGL.vertexBatchPreTransformed(vertices, uvs, colors, glyph_count * 6); - gGL.end(); - glyph_count = 0; - } + LLRectf uv_rect(slot.mXBitmapOffset * inv_width, + (slot.mYBitmapOffset + slot.mHeight + PAD_UVY) * inv_height, + (slot.mXBitmapOffset + slot.mWidth) * inv_width, + (slot.mYBitmapOffset - PAD_UVY) * inv_height); + LLRectf screen_rect(glyph_x, + glyph_y, + glyph_x + (F32)slot.mWidth, + glyph_y - (F32)slot.mHeight); + + if (glyph_count >= GLYPH_BATCH_SIZE) + { + flush_batch(); + } - // Grayscale glyphs tint with text_color (the bitmap is a - // luminance / coverage mask). Color glyphs tint with - // emoji_color (white, preserving CPAL palette colors baked - // into the bitmap). - const LLColor4U& col = bitmap_entry.first == EFontGlyphType::Grayscale - ? text_color : emoji_color; - if (needs_two_pass) - { - // BOLD suppresses shadow per the legacy drawGlyph contract - // (see FIXME at drawGlyphForeground): the bold doubled quad - // and the shadow taps are mutually exclusive. drawGlyphShadow - // doesn't see `style`, so gate the call here. - if (!(style_to_add & BOLD)) + // Grayscale glyphs tint with text_color (the bitmap is a + // luminance / coverage mask). Color glyphs tint with + // emoji_color (white, preserving CPAL palette colors baked + // into the bitmap). + const LLColor4U& col = bitmap_entry.first == EFontGlyphType::Grayscale + ? text_color : emoji_color; + if (needs_two_pass) { - drawGlyphShadow(glyph_count, vertices, uvs, colors, screen_rect, uv_rect, - precomputed_shadow_color, shadow, slant_offset); + // BOLD suppresses shadow per the legacy drawGlyph contract + // (see FIXME at drawGlyphForeground): the bold doubled quad + // and the shadow taps are mutually exclusive. drawGlyphShadow + // doesn't see `style`, so gate the call here. + if (!(style_to_add & BOLD)) + { + drawGlyphShadow(glyph_count, vertices, uvs, colors, screen_rect, uv_rect, + precomputed_shadow_color, shadow, slant_offset); + } + deferred.push_back({screen_rect, uv_rect, bitmap_entry, current_face, col}); + } + else + { + drawGlyphForeground(glyph_count, vertices, uvs, colors, screen_rect, uv_rect, + col, style_to_add, slant_offset); } - deferred.push_back({screen_rect, uv_rect, bitmap_entry, current_face, col}); - } - else - { - drawGlyphForeground(glyph_count, vertices, uvs, colors, screen_rect, uv_rect, - col, style_to_add, slant_offset); } cur_x += sg.x_advance; @@ -742,27 +773,19 @@ S32 LLFontGL::render(const LLWString &wstr, S32 begin_offset, F32 x, F32 y, cons const auto& cp_slot = fgi->mPhaseSlots[cp_phase]; // Per-glyph bitmap texture. Flush + rebind only when the atlas - // slot actually changes; the legacy `last_char != wch` clause - // forced a per-codepoint flush as a band-aid for an undiagnosed - // CJK/emoji-on-first-render issue. Moving atlas eviction off the - // render path (sweepGlyphCaches between frames) eliminated the - // underlying race, and Latin text now batches up to GLYPH_BATCH_SIZE - // glyphs per draw the way it should. + // slot actually changes; flush_batch re-asserts the batch's own + // texture, so glyph rasterization between flushes (which leaves the + // upload target bound) can't misdirect the queued quads. The legacy + // `last_char != wch` clause forced a per-codepoint flush as a + // band-aid over exactly that misdirection; Latin text now batches + // up to GLYPH_BATCH_SIZE glyphs per draw the way it should. const LLFontFace* cp_glyph_face = fgi->mSourceFace; std::pair next_bitmap_entry = cp_slot.mBitmapEntry; if (cp_glyph_face != current_face || next_bitmap_entry != bitmap_entry) { // Actually draw the queued glyphs before switching their texture; // otherwise the queued glyphs will be taken from wrong textures. - if (glyph_count > 0) - { - gGL.begin(LLRender::TRIANGLES); - { - gGL.vertexBatchPreTransformed(vertices, uvs, colors, glyph_count * 6); - } - gGL.end(); - glyph_count = 0; - } + flush_batch(); bitmap_entry = next_bitmap_entry; if (cp_glyph_face != current_face) @@ -775,16 +798,17 @@ S32 LLFontGL::render(const LLWString &wstr, S32 begin_offset, F32 x, F32 y, cons inv_height = 1.f / font_bitmap_cache->getBitmapHeight(); } } - if (font_bitmap_cache) + // Defensive: getImageGL returns null when a sheet has been + // released (collectGarbage) or when the slot index is out + // of range. The emission guard below skips the glyph in that + // case — emitting quads without a known binding would sample + // whatever texture is currently bound. + batch_image = font_bitmap_cache + ? font_bitmap_cache->getImageGL(bitmap_entry.first, bitmap_entry.second) + : nullptr; + if (batch_image) { - LLImageGL* font_image = font_bitmap_cache->getImageGL(bitmap_entry.first, bitmap_entry.second); - // Defensive: getImageGL returns null when a sheet has been - // released (collectGarbage) or when the slot index is out - // of range. Skip the bind in that case; the glyph won't - // render this frame but we don't crash inside LLTexUnit::bind - // dereferencing a null texture pointer. - if (font_image) - gGL.getTexUnit(0)->bind(font_image); + gGL.getTexUnit(0)->bind(batch_image); } } @@ -794,52 +818,49 @@ S32 LLFontGL::render(const LLWString &wstr, S32 begin_offset, F32 x, F32 y, cons break; } - // Draw the text at the appropriate location - //Specify vertices and texture coordinates - LLRectf uv_rect((cp_slot.mXBitmapOffset) * inv_width, - (cp_slot.mYBitmapOffset + cp_slot.mHeight + PAD_UVY) * inv_height, - (cp_slot.mXBitmapOffset + cp_slot.mWidth) * inv_width, - (cp_slot.mYBitmapOffset - PAD_UVY) * inv_height); - // Integer dest derived from quantized pen + per-phase bearing. - const F32 cp_glyph_x = (F32)(cp_dest_int_x + cp_slot.mXBearing); - const F32 cp_glyph_y = (F32)ll_round(cur_render_y) + (F32)cp_slot.mYBearing; - LLRectf screen_rect(cp_glyph_x, - cp_glyph_y, - cp_glyph_x + (F32)cp_slot.mWidth, - cp_glyph_y - (F32)cp_slot.mHeight); - - if (glyph_count >= GLYPH_BATCH_SIZE) + if (batch_image) { - gGL.begin(LLRender::TRIANGLES); + // Draw the text at the appropriate location + //Specify vertices and texture coordinates + LLRectf uv_rect((cp_slot.mXBitmapOffset) * inv_width, + (cp_slot.mYBitmapOffset + cp_slot.mHeight + PAD_UVY) * inv_height, + (cp_slot.mXBitmapOffset + cp_slot.mWidth) * inv_width, + (cp_slot.mYBitmapOffset - PAD_UVY) * inv_height); + // Integer dest derived from quantized pen + per-phase bearing. + const F32 cp_glyph_x = (F32)(cp_dest_int_x + cp_slot.mXBearing); + const F32 cp_glyph_y = (F32)ll_round(cur_render_y) + (F32)cp_slot.mYBearing; + LLRectf screen_rect(cp_glyph_x, + cp_glyph_y, + cp_glyph_x + (F32)cp_slot.mWidth, + cp_glyph_y - (F32)cp_slot.mHeight); + + if (glyph_count >= GLYPH_BATCH_SIZE) { - gGL.vertexBatchPreTransformed(vertices, uvs, colors, glyph_count * 6); + flush_batch(); } - gGL.end(); - - glyph_count = 0; - } - // Grayscale tints with text_color; Color tints with emoji_color - // (white, preserving CPAL palette colors baked into the bitmap). - const LLColor4U& col = bitmap_entry.first == EFontGlyphType::Grayscale - ? text_color : emoji_color; - if (needs_two_pass) - { - // BOLD suppresses shadow per the legacy drawGlyph contract - // (see FIXME at drawGlyphForeground): the bold doubled quad - // and the shadow taps are mutually exclusive. drawGlyphShadow - // doesn't see `style`, so gate the call here. - if (!(style_to_add & BOLD)) + // Grayscale tints with text_color; Color tints with emoji_color + // (white, preserving CPAL palette colors baked into the bitmap). + const LLColor4U& col = bitmap_entry.first == EFontGlyphType::Grayscale + ? text_color : emoji_color; + if (needs_two_pass) { - drawGlyphShadow(glyph_count, vertices, uvs, colors, screen_rect, uv_rect, - precomputed_shadow_color, shadow, slant_offset); + // BOLD suppresses shadow per the legacy drawGlyph contract + // (see FIXME at drawGlyphForeground): the bold doubled quad + // and the shadow taps are mutually exclusive. drawGlyphShadow + // doesn't see `style`, so gate the call here. + if (!(style_to_add & BOLD)) + { + drawGlyphShadow(glyph_count, vertices, uvs, colors, screen_rect, uv_rect, + precomputed_shadow_color, shadow, slant_offset); + } + deferred.push_back({screen_rect, uv_rect, bitmap_entry, current_face, col}); + } + else + { + drawGlyphForeground(glyph_count, vertices, uvs, colors, screen_rect, uv_rect, + col, style_to_add, slant_offset); } - deferred.push_back({screen_rect, uv_rect, bitmap_entry, current_face, col}); - } - else - { - drawGlyphForeground(glyph_count, vertices, uvs, colors, screen_rect, uv_rect, - col, style_to_add, slant_offset); } chars_drawn++; @@ -873,12 +894,7 @@ S32 LLFontGL::render(const LLWString &wstr, S32 begin_offset, F32 x, F32 y, cons // End-of-pass flush. In single-pass mode this drains the foreground batch // and we're done. In two-pass mode this drains the shadow batch; pass B // below then walks the deferred metadata to emit foreground geometry. - gGL.begin(LLRender::TRIANGLES); - { - gGL.vertexBatchPreTransformed(vertices, uvs, colors, glyph_count * 6); - } - gGL.end(); - glyph_count = 0; + flush_batch(); if (needs_two_pass) { @@ -901,39 +917,41 @@ S32 LLFontGL::render(const LLWString &wstr, S32 begin_offset, F32 x, F32 y, cons // Pass B: emit foreground geometry from deferred metadata. Reset the // atlas-binding tracker; the first deferred glyph forces a (possibly // redundant, GL-driver-cheap) rebind to begin the foreground stream. + // Only glyphs whose batch_image resolved in pass A made it into + // `deferred`, so the getImageGL lookups below can only go null if a + // sheet vanished mid-render — which the frame-boundary eviction + // discipline rules out — but keep the same guard shape regardless. bitmap_entry = std::make_pair(EFontGlyphType::Grayscale, -1); current_face = nullptr; + batch_image = nullptr; for (const DeferredGlyph& dg : deferred) { if (dg.face != current_face || dg.bitmap_entry != bitmap_entry) { - if (glyph_count > 0) - { - gGL.begin(LLRender::TRIANGLES); - gGL.vertexBatchPreTransformed(vertices, uvs, colors, glyph_count * 6); - gGL.end(); - glyph_count = 0; - } + flush_batch(); bitmap_entry = dg.bitmap_entry; if (dg.face != current_face) { current_face = dg.face; font_bitmap_cache = current_face ? current_face->getBitmapCache() : nullptr; } - if (font_bitmap_cache) + batch_image = font_bitmap_cache + ? font_bitmap_cache->getImageGL(bitmap_entry.first, bitmap_entry.second) + : nullptr; + if (batch_image) { - LLImageGL* font_image = font_bitmap_cache->getImageGL(bitmap_entry.first, bitmap_entry.second); - if (font_image) - gGL.getTexUnit(0)->bind(font_image); + gGL.getTexUnit(0)->bind(batch_image); } } + if (!batch_image) + { + continue; + } + if (glyph_count >= GLYPH_BATCH_SIZE) { - gGL.begin(LLRender::TRIANGLES); - gGL.vertexBatchPreTransformed(vertices, uvs, colors, glyph_count * 6); - gGL.end(); - glyph_count = 0; + flush_batch(); } drawGlyphForeground(glyph_count, vertices, uvs, colors, @@ -941,10 +959,7 @@ S32 LLFontGL::render(const LLWString &wstr, S32 begin_offset, F32 x, F32 y, cons dg.color, style_to_add, slant_offset); } - gGL.begin(LLRender::TRIANGLES); - gGL.vertexBatchPreTransformed(vertices, uvs, colors, glyph_count * 6); - gGL.end(); - glyph_count = 0; + flush_batch(); } if (right_x) diff --git a/indra/llrender/llvertexbuffer.h b/indra/llrender/llvertexbuffer.h index f24d75e41d..6a2e1d9045 100644 --- a/indra/llrender/llvertexbuffer.h +++ b/indra/llrender/llvertexbuffer.h @@ -73,8 +73,8 @@ class LLVertexBufferData , mMode(mode) , mCount(count) , mTexName(tex_name) - , mProjection(model_view) - , mModelView(projection) + , mProjection(projection) + , mModelView(model_view) , mTexture0(texture0) {} void drawWithMatrix(); diff --git a/indra/llrender/tests/llfontgl_test.cpp b/indra/llrender/tests/llfontgl_test.cpp index aeb8d9a916..a7bb9e9664 100644 --- a/indra/llrender/tests/llfontgl_test.cpp +++ b/indra/llrender/tests/llfontgl_test.cpp @@ -768,4 +768,90 @@ namespace tut ensure_equals("DROP_SHADOW returns 2", n_drop, 2); ensure_equals("DROP_SHADOW_SOFT returns 2", n_soft, 2); } + + // Mid-render glyph rasterization must not misdirect the pending + // batch's texture. Rasterizing a cache-missed glyph uploads into its + // atlas via LLImageGL::setSubImage, which binds the upload target on + // unit 0 and leaves it bound; if the quads queued before it then + // flush under that binding, their UVs sample a different atlas page + // and the run renders fragments of other glyphs ("text shows other + // text"). The legacy per-codepoint `last_char != wch` flush kept the + // queue to ~1 glyph and hid this; with real batching every flush must + // re-assert the batch's own texture (flush_batch in LLFontGL::render). + // + // Captured display lists record the texture bound at flush time + // (LLVertexBufferData::mTexName), which makes the misdirection + // directly observable: render a Latin prefix plus a never-yet- + // rasterized emoji and require the prefix batch to carry the + // grayscale atlas texture its UVs were built against. + template<> template<> + void llfontgl_render_object::test<6>() + { + if (!fileExists(kFontsXml)) + skip("fonts.xml not found"); + LLFontGL* font = LLFontGL::getFontSansSerif(); + ensure("font resolves", font != nullptr); + const LLFontFreetype* head = font->getFontFreetype(); + ensure("head freetype resolves", head != nullptr); + + // U+1F995 SAUROPOD — obscure enough that no other test in this + // binary rasterizes it. The mid-render upload only fires on a + // cache miss, so the emoji must be cold going in. + const llwchar emoji_cp = 0x1F995; + U32 emoji_idx = 0; + const LLFontFreetype* emoji_face = head->selectShapingFace(emoji_cp, emoji_idx); + if (!emoji_face || emoji_face == head || emoji_idx == 0) + skip("no emoji fallback covers U+1F995 in this harness"); + ensure("emoji fallback has a face wrapper", + emoji_face->getFontFace() != nullptr); + if (emoji_face->getFontFace()->findGlyphInfo(emoji_idx, EFontGlyphType::Color)) + skip("U+1F995 already rasterized; mid-render upload won't fire"); + + LLWString s = utf8str_to_wstring("stomp \xF0\x9F\xA6\x95 check"); + + std::list capture; + gGL.beginList(&capture); + const S32 n = font->render(s, 0, 50.f, 100.f, LLColor4::white, + LLFontGL::LEFT, LLFontGL::BASELINE, + LLFontGL::NORMAL, LLFontGL::NO_SHADOW, + (S32)s.size()); + gGL.endList(); + ensure_equals("render consumed the whole string", n, (S32)s.size()); + + // Expected textures, resolved through the glyph entries the render + // itself produced. use_color defaulted true, so the render looked + // glyphs up as Color; querying the same way returns the same + // entries (and therefore the same atlas pages) it drew from. + const LLFontGlyphInfo* latin_gi = head->getGlyphInfo((llwchar)'s', EFontGlyphType::Color); + ensure("latin glyph cached", latin_gi && latin_gi->mSourceFace); + const auto& latin_entry = latin_gi->mPhaseSlots[0].mBitmapEntry; + LLImageGL* latin_img = latin_gi->mSourceFace->getBitmapCache()->getImageGL( + latin_entry.first, (U32)latin_entry.second); + ensure("latin atlas page live", latin_img != nullptr); + const U32 latin_tex = latin_img->getTexName(); + + const LLFontGlyphInfo* emoji_gi = head->getGlyphInfo(emoji_cp, EFontGlyphType::Color); + ensure("emoji glyph cached", emoji_gi && emoji_gi->mSourceFace); + const auto& emoji_entry = emoji_gi->mPhaseSlots[0].mBitmapEntry; + LLImageGL* emoji_img = emoji_gi->mSourceFace->getBitmapCache()->getImageGL( + emoji_entry.first, (U32)emoji_entry.second); + ensure("emoji atlas page live", emoji_img != nullptr); + const U32 emoji_tex = emoji_img->getTexName(); + + ensure("string spans two distinct atlas textures", latin_tex != emoji_tex); + ensure("capture produced batches", !capture.empty()); + + // The first captured batch is the Latin prefix, flushed when the + // glyph stream switches to the emoji's atlas — exactly the batch + // the mid-render upload used to misdirect. + ensure_equals("prefix batch carries its own atlas texture", + capture.front().mTexName, latin_tex); + // And every batch in the capture must reference one of the two + // atlas pages this string actually draws from. + for (const LLVertexBufferData& entry : capture) + { + ensure("batch texture is one of the string's atlas pages", + entry.mTexName == latin_tex || entry.mTexName == emoji_tex); + } + } } From 04a8989347899a240875ce018df01a2bb436992d Mon Sep 17 00:00:00 2001 From: Rye Date: Wed, 10 Jun 2026 23:44:35 -0400 Subject: [PATCH 2/5] Font cache hardening: per-font VB invalidation, dedup fixes, lifecycle guards Follow-ups from the corruption audit, all in the glyph/shape cache layer. llfontfreetype.cpp: - getGlyphInfoByIndex: degrade Color requests to Grayscale on faces with neither a color table (CBDT/sbix/COLR) nor OT-SVG before the cache probe. FT_LOAD_COLOR on such a face rasterizes the same grayscale outline anyway, but the typed probe missed the existing Grayscale entry and rasterized a duplicate gray bitmap into fresh atlas slots published under a Color-typed entry. use_color=true is the UI default for every glyph, and reload paths pre-warm ASCII as Grayscale, so the whole ASCII set was stored twice per font - x8 phase slots on subpixel faces. llfontgl.cpp: - getCacheGeneration: return the sum of the per-instance atlas generations of the head face plus every fallback face instead of the global counter. Glyphs can only come from those atlases, so the sum ticks for exactly the mutations that can stale this font's captured UVs. The global counter invalidated EVERY cached text vertex/width buffer viewer-wide whenever any font rasterized a glyph; during glyph churn (first CJK chat fill, post-eviction warm-up) each regen rasterized more glyphs and re-invalidated everything again for several frames. Monotonic by construction - every component only takes fresh values from the shared counter, so no A+1/B-1 aliasing. - Holders of shapeLine references (render, getWidthF32, charFromPixelOffset, maxDrawableChars, firstDrawableChar) snapshot LLFontShaping::cacheMutationCount() after shaping and llassert equality after their last dereference, so a use-after-invalidation trips a debug assert instead of reading freed glyph runs. build_shape_layout snapshots up front too so the empty-slice early return carries the live count - a default 0 tripped the assert on every empty-label measurement (LLButton::resize with "") once anything had ever shaped. llfontshaping.cpp/.h: - New cacheMutationCount(): bumped by miss-inserts (covering their evictions) and clears. Cache hits don't bump - an LRU splice never touches the index or a glyph vector. - clearCacheForFace: also drop entries rooted at OTHER heads whose glyph runs reference the dying face. Every fallback-sourced LLShapedGlyph stores a raw pointer to the fallback that owns its glyph_id, while invalidation was keyed only on the entry's root. Today's lifecycle makes the sweep defensive (fallbacks outlive heads via LLPointer chains; reloads clear globally), but enforcing it here keeps a future runtime fallback-removal path correct by construction. llfontface.cpp/.h: - insertGlyphInfo: keep the already-published entry on a duplicate (glyph_index, type) publish, delete the incoming one, and return the survivor. The old replace-in-place deleted an entry that callers up the stack (render loop, kerning prefetch) may still hold. Duplicate publishes assert in debug - they mean an upstream dedup probe was skipped. addShapedGlyphFromFont continues with the return value. llfontbitmapcache.h: - Update getGlobalGeneration's comment: production no longer compares against it directly; it remains the uniqueness source per-instance generations draw from, plus a test/diagnostic accessor. Co-Authored-By: Claude Fable 5 --- indra/llrender/llfontbitmapcache.h | 15 +++--- indra/llrender/llfontface.cpp | 18 ++++--- indra/llrender/llfontface.h | 11 +++- indra/llrender/llfontfreetype.cpp | 22 +++++++- indra/llrender/llfontgl.cpp | 86 ++++++++++++++++++++++++++---- indra/llrender/llfontshaping.cpp | 42 ++++++++++++++- indra/llrender/llfontshaping.h | 24 +++++++-- 7 files changed, 186 insertions(+), 32 deletions(-) diff --git a/indra/llrender/llfontbitmapcache.h b/indra/llrender/llfontbitmapcache.h index 304a3b241f..f705275f4e 100644 --- a/indra/llrender/llfontbitmapcache.h +++ b/indra/llrender/llfontbitmapcache.h @@ -82,14 +82,13 @@ class LLFontBitmapCache S32 getCacheGeneration() const { return mGeneration; } // Snapshot of the global atlas-mutation counter. Every nextOpenPos / - // releaseSheet / reset / injectPage / new LLFontBitmapCache anywhere - // bumps this. Vertex/width-buffer caches that sample from a font's - // head face AND its fallback faces (each owning its own atlas with a - // separate per-instance mGeneration) need a counter that ticks on - // mutations to ANY of them — comparing only the head's mGeneration - // misses fallback rasterization (e.g. emoji glyphs added during a - // genBuffers walk), leaving the captured UVs pointing at uninitialized - // atlas slots until something else triggers a rebuild. + // releaseSheet / reset / new LLFontBitmapCache anywhere bumps this; it + // is the uniqueness source every per-instance mGeneration draws from, + // which is what makes summing per-instance generations a monotonic + // per-font invalidation stamp (see LLFontGL::getCacheGeneration — + // production no longer compares against the global value directly, + // since that invalidated every cached text buffer viewer-wide on any + // font's glyph churn). Kept public for tests and diagnostics. static S32 getGlobalGeneration() { return sNextGeneration; } // Drop the underlying images for a sheet, freeing the GPU and CPU memory. diff --git a/indra/llrender/llfontface.cpp b/indra/llrender/llfontface.cpp index 7d8232ee14..b475600bef 100644 --- a/indra/llrender/llfontface.cpp +++ b/indra/llrender/llfontface.cpp @@ -247,7 +247,7 @@ LLFontGlyphInfo* LLFontFace::findGlyphInfo(U32 glyph_index, EFontGlyphType type) return (iter != range.second) ? iter->second : nullptr; } -void LLFontFace::insertGlyphInfo(U32 glyph_index, LLFontGlyphInfo* gi) const +LLFontGlyphInfo* LLFontFace::insertGlyphInfo(U32 glyph_index, LLFontGlyphInfo* gi) const { llassert(gi->mGlyphType < EFontGlyphType::Count); auto range = mGlyphInfoMap.equal_range(glyph_index); @@ -255,13 +255,17 @@ void LLFontFace::insertGlyphInfo(U32 glyph_index, LLFontGlyphInfo* gi) const [gi](const glyph_info_map_t::value_type& e) { return e.second->mGlyphType == gi->mGlyphType; }); if (iter != range.second) { - delete iter->second; - iter->second = gi; - } - else - { - mGlyphInfoMap.insert(std::make_pair(glyph_index, gi)); + // Keep the already-published entry — pointers to it may be live up + // the stack, and swapping it out would free memory in active use. + // Reaching here means an upstream dedup probe was skipped; the + // duplicate's atlas slots stay orphaned (we have no slot-level + // reclaim), which is a leak of atlas space but not of memory safety. + llassert(false); + delete gi; + return iter->second; } + mGlyphInfoMap.insert(std::make_pair(glyph_index, gi)); + return gi; } void LLFontFace::resetBitmapCache() diff --git a/indra/llrender/llfontface.h b/indra/llrender/llfontface.h index ca545b0b41..a1e84b3811 100644 --- a/indra/llrender/llfontface.h +++ b/indra/llrender/llfontface.h @@ -220,7 +220,16 @@ class LLFontFace : public LLRefCount // either path lives in one atlas slot. LLFontGlyphInfo entries are // owned here and deleted in ~LLFontFace. LLFontGlyphInfo* findGlyphInfo(U32 glyph_index, EFontGlyphType type) const; - void insertGlyphInfo(U32 glyph_index, LLFontGlyphInfo* gi) const; + // Publish `gi` (ownership transfers to the cache) and return the + // published entry. If a (glyph_index, type) entry already exists, the + // EXISTING one is kept and returned and `gi` is deleted — published + // pointers may be held up the stack (render loop, kerning prefetch), + // so replacing in place would free memory in active use. Callers must + // continue with the return value, never with `gi`. A duplicate publish + // means an upstream dedup probe was skipped (addShapedGlyphFromFont + // checks findGlyphInfo first) and asserts in debug; the duplicate's + // atlas slots are orphaned, not reclaimed. + LLFontGlyphInfo* insertGlyphInfo(U32 glyph_index, LLFontGlyphInfo* gi) const; // Iterate and conditionally erase entries. Used by collectGarbage to // purge entries that referenced an evicted atlas sheet. diff --git a/indra/llrender/llfontfreetype.cpp b/indra/llrender/llfontfreetype.cpp index e461e2eabf..52cdcad55a 100644 --- a/indra/llrender/llfontfreetype.cpp +++ b/indra/llrender/llfontfreetype.cpp @@ -811,7 +811,9 @@ LLFontGlyphInfo* LLFontFreetype::addShapedGlyphFromFont(const LLFontFreetype* fo if (!gi) return nullptr; - fontp->mFace->insertGlyphInfo(glyph_index, gi); + // insertGlyphInfo keeps an already-published entry over the incoming + // one (deleting the duplicate), so continue with its return value. + gi = fontp->mFace->insertGlyphInfo(glyph_index, gi); // Optimization: when the rendered pixel format differs from what the // caller requested (e.g. Color requested but the file is monochrome), @@ -909,6 +911,24 @@ LLFontGlyphInfo* LLFontFreetype::getGlyphInfoByIndex(const LLFontFreetype* fontp // forced a duplicate Grayscale rasterization (a full COLRv1 paint walk // plus a Grayscale atlas slot) for every color glyph that had already // been rendered through the Color atlas. + // + // A face with neither a color table (CBDT/sbix/COLR) nor OT-SVG can + // never produce a Color bitmap — FT_LOAD_COLOR on it rasterizes the + // same grayscale outline. Degrade the request up front so Color + // lookups on plain text faces (use_color=true is the UI default for + // every glyph, not just emoji) hit the existing Grayscale entry + // instead of missing and rasterizing a duplicate gray bitmap into + // fresh atlas slots published under a Color-typed entry. Reload paths + // that pre-warm ASCII as Grayscale made that the common case: the + // whole ASCII set stored twice per font, x8 phase slots on subpixel + // faces. + if (glyph_type == EFontGlyphType::Color + && fontp && fontp->mFace + && !fontp->mFace->hasColor() + && !fontp->mFace->hasSvg()) + { + glyph_type = EFontGlyphType::Grayscale; + } if (fontp && fontp->mFace) { if (LLFontGlyphInfo* gi = fontp->mFace->findGlyphInfo(glyph_index, glyph_type)) diff --git a/indra/llrender/llfontgl.cpp b/indra/llrender/llfontgl.cpp index 24b9ebc9d3..9fd798dbff 100644 --- a/indra/llrender/llfontgl.cpp +++ b/indra/llrender/llfontgl.cpp @@ -102,6 +102,12 @@ namespace { std::vector> ranges; std::vector*> glyphs; + // Shape-cache mutation count at build time. The glyph pointers are + // valid only while this matches LLFontShaping::cacheMutationCount(); + // holders llassert equality after their last dereference so a + // use-after-invalidation trips a debug assert instead of reading + // freed glyph runs. + size_t mutation_snapshot = 0; }; // Build the shape layout for `slice` against `root_face`. One @@ -114,6 +120,11 @@ namespace LLWStringView slice) { ShapeLayout out; + // Snapshot up front so the empty-layout early return below carries + // the live mutation count too — a default 0 would trip the holders' + // validity assert on every empty-label measurement (LLButton::resize + // with "" at startup) once anything had ever shaped. + out.mutation_snapshot = LLFontShaping::cacheMutationCount(); if (!root_face || slice.empty()) return out; @@ -126,6 +137,13 @@ namespace out.ranges[s].first, out.ranges[s].second); } + // Re-snapshot AFTER all shapeLine calls — each call may itself + // mutate (miss-insert), which is fine: only mutations after this + // point invalidate the pointers collected above. (With a single + // range there's nothing to invalidate mid-build; with several, + // entries just shaped sit at the LRU front, out of eviction's + // reach.) + out.mutation_snapshot = LLFontShaping::cacheMutationCount(); return out; } } @@ -172,16 +190,40 @@ S32 LLFontGL::getCacheGeneration() const { // Every render() call may sample from this font's head atlas AND from // each fallback face's atlas (e.g. emoji glyphs in an otherwise - // grayscale string). Each LLFontBitmapCache has its own per-instance - // generation that only ticks on mutations to that cache; the head - // atlas's local counter is blind to a fallback rasterizing a new - // glyph mid-frame, so vertex/width buffers comparing against it - // would miss the invalidation and keep the stale UVs that point at - // uninitialized atlas slots. The global counter ticks on any atlas - // mutation across every face — slight over-invalidation, but the - // only correct answer when the cache key has to cover all sampled - // atlases without enumerating them. - return LLFontBitmapCache::getGlobalGeneration(); + // grayscale string) — and from nothing else: glyphs route through + // getGlyphInfo* whose mSourceFace is always the head face or a chain + // member. Summing the per-instance generation of exactly those caches + // gives a per-font invalidation stamp: it ticks for any mutation that + // can stale this font's captured UVs and stays put for unrelated + // fonts' glyph churn. + // + // Monotonic: each component only ever takes fresh values from the + // shared LLFontBitmapCache::sNextGeneration counter, so every mutation + // (including a face wrapper swap on reload, whose new cache starts at + // a value above everything previously issued) strictly increases the + // sum — no A+1/B-1 aliasing. addFallbackFont growing the chain adds a + // component, which also only increases it. + // + // This used to return the global counter, which invalidated EVERY + // cached text buffer viewer-wide whenever any font rasterized a glyph; + // during glyph churn (first CJK chat fill, post-eviction warm-up) each + // regen rasterized more glyphs and re-invalidated everything again for + // several frames. + const LLFontFreetype* ft = mFontFreetype.get(); + if (!ft) + return 0; + S32 gen = 0; + if (const LLFontBitmapCache* cache = ft->getFontBitmapCache()) + gen += cache->getCacheGeneration(); + for (const auto& fb : ft->getFallbackFonts()) + { + if (fb.first) + { + if (const LLFontBitmapCache* cache = fb.first->getFontBitmapCache()) + gen += cache->getCacheGeneration(); + } + } + return gen; } S32 LLFontGL::render(const LLWString &wstr, S32 begin_offset, const LLRect& rect, const LLColor4 &color, HAlign halign, VAlign valign, U8 style, @@ -891,6 +933,10 @@ S32 LLFontGL::render(const LLWString &wstr, S32 begin_offset, F32 x, F32 y, cons cur_render_y = cur_y; } + // The layout's glyph pointers reach into the shape LRU; nothing inside + // the loop may shape (glyph rasterization doesn't), or they dangle. + llassert(layout.mutation_snapshot == LLFontShaping::cacheMutationCount()); + // End-of-pass flush. In single-pass mode this drains the foreground batch // and we're done. In two-pass mode this drains the shadow batch; pass B // below then walks the deferred metadata to emit foreground geometry. @@ -1203,6 +1249,9 @@ F32 LLFontGL::getWidthF32(const llwchar* wchars, S32 begin_offset, S32 max_chars cur_x = (F32)ll_round(cur_x); } + // Layout pointers must have stayed valid for the whole measurement walk. + llassert(layout.mutation_snapshot == LLFontShaping::cacheMutationCount()); + if (!no_padding) { // add in extra pixels for last character's width past its xadvance @@ -1270,6 +1319,9 @@ S32 LLFontGL::maxDrawableChars(const llwchar* wchars, F32 max_pixels, S32 max_ch } } const bool use_shaped = !shape_glyphs->empty(); + // shape_glyphs points into the shape LRU until the loop's last use. + const size_t shape_gen = LLFontShaping::cacheMutationCount(); + (void)shape_gen; S32 i; for (i=0; (i < max_chars); i++) @@ -1384,6 +1436,10 @@ S32 LLFontGL::maxDrawableChars(const llwchar* wchars, F32 max_pixels, S32 max_ch cur_x = (F32)ll_round(cur_x); } + // No shape* call may fire while shape_glyphs is held (see the comment + // at the shapeLine call above). + llassert(shape_gen == LLFontShaping::cacheMutationCount()); + if( clip ) { switch (end_on_word_boundary) @@ -1435,6 +1491,8 @@ S32 LLFontGL::firstDrawableChar(const llwchar* wchars, F32 max_pixels, S32 text_ // (begin=0), which equals 0..start here. LLWStringView slice(wchars, (size_t)(start + 1)); const auto& shape_glyphs = LLFontShaping::shapeLine(mFontFreetype, slice, 0, (size_t)(start + 1)); + const size_t shape_gen = LLFontShaping::cacheMutationCount(); + (void)shape_gen; if (!shape_glyphs.empty()) { per_cp_advance.assign(start + 1, 0.f); @@ -1454,6 +1512,9 @@ S32 LLFontGL::firstDrawableChar(const llwchar* wchars, F32 max_pixels, S32 text_ } } } + // The fill above is shape_glyphs' last dereference — it must not + // have been invalidated by a shape-cache mutation mid-hold. + llassert(shape_gen == LLFontShaping::cacheMutationCount()); } const bool use_shaped = !per_cp_advance.empty(); @@ -1663,6 +1724,11 @@ S32 LLFontGL::charFromPixelOffset(const llwchar* wchars, S32 begin_offset, F32 t cur_x = (F32)ll_round(cur_x); } + // Hit-test walk holds the layout's glyph pointers; early returns inside + // the loop skip this check, which is fine — the assert is a tripwire + // for shape-cache mutation mid-hold, not exhaustive coverage. + llassert(layout.mutation_snapshot == LLFontShaping::cacheMutationCount()); + return llmin(max_chars, pos - begin_offset); } diff --git a/indra/llrender/llfontshaping.cpp b/indra/llrender/llfontshaping.cpp index 9f6434a312..84f4ea9c2b 100644 --- a/indra/llrender/llfontshaping.cpp +++ b/indra/llrender/llfontshaping.cpp @@ -143,6 +143,12 @@ namespace ShapeLru sShapeLru; ShapeIndex sShapeIndex; + // Bumped by every mutation that can invalidate a reference returned by + // shapeLine (miss-insert + its evictions, clears). LRU splices on cache + // hit don't count — they never touch the index or a glyph vector. See + // cacheMutationCount() in the header for the holder-side contract. + size_t sShapeCacheMutations = 0; + // Shape a single sub-run through its owning face and append the // resulting glyphs to `out_glyphs`. Clusters are written in wstr // coordinates relative to `sub_begin_in_slice` — i.e. local to the @@ -751,6 +757,10 @@ const std::vector& LLFontShaping::shapeLine( key.codepoints.assign(slice.data(), slice.size()); key.root_face = root_face; + // One bump covers the insert and any evictions below — either way, + // previously returned references may now dangle. + ++sShapeCacheMutations; + auto [ins, inserted] = sShapeIndex.try_emplace(std::move(key)); // Just-missed lookup means inserted is true; no duplicate to merge. sShapeLru.push_front(&ins->first); @@ -791,6 +801,8 @@ void LLFontShaping::shapeRun(const LLFontFreetype* root_face, void LLFontShaping::clearCache() { + if (!sShapeIndex.empty()) + ++sShapeCacheMutations; sShapeIndex.clear(); sShapeLru.clear(); } @@ -800,6 +812,11 @@ size_t LLFontShaping::cacheSize() return sShapeIndex.size(); } +size_t LLFontShaping::cacheMutationCount() +{ + return sShapeCacheMutations; +} + void LLFontShaping::clearCacheForFace(const LLFontFreetype* face) { if (!face) @@ -808,16 +825,39 @@ void LLFontShaping::clearCacheForFace(const LLFontFreetype* face) // Walk the index — its iterator gives us O(1) erase from the LRU via // the back-reference stored in each entry. Walking the LRU instead // would force a per-key index lookup on every match. + bool erased_any = false; for (auto it = sShapeIndex.begin(); it != sShapeIndex.end(); ) { - if (it->first.root_face == face) + bool references_face = (it->first.root_face == face); + if (!references_face) + { + // Entries rooted at OTHER heads can still hold this face inside + // their glyph runs: every fallback-sourced glyph stores a raw + // pointer to the fallback that owns its glyph_id. Sweep those + // too so a dying face can't leave sibling-rooted entries whose + // sg.face dangles — see the header note on clearCacheForFace. + // O(glyphs) per surviving entry, but this only runs on face + // teardown / reload, never on the shape or render hot path. + for (const LLShapedGlyph& sg : it->second.glyphs) + { + if (sg.face == face) + { + references_face = true; + break; + } + } + } + if (references_face) { sShapeLru.erase(it->second.lru_pos); it = sShapeIndex.erase(it); + erased_any = true; } else { ++it; } } + if (erased_any) + ++sShapeCacheMutations; } diff --git a/indra/llrender/llfontshaping.h b/indra/llrender/llfontshaping.h index 09d2adce5c..db2afcdef7 100644 --- a/indra/llrender/llfontshaping.h +++ b/indra/llrender/llfontshaping.h @@ -93,10 +93,17 @@ namespace LLFontShaping // Single-threaded; the shape path is main-thread only. void clearCache(); - // Drop only the entries owned by `face`. Called from ~LLFontFreetype - // and from loadFace() reload, so a face teardown doesn't blow away - // unrelated entries cached for siblings. No-op when `face` is null - // or has no entries. Single-threaded. + // Drop every entry that references `face` — keyed on it (root_face) OR + // holding glyphs sourced from it (LLShapedGlyph::face stores a raw + // pointer to whichever fallback owns each glyph_id, and those entries + // are keyed by the HEAD that shaped them, not the fallback). Called + // from ~LLFontFreetype and from loadFace() reload, so a face teardown + // can't leave sibling-rooted entries whose glyph runs dangle into the + // destroyed face. Today's lifecycle makes the glyph-run sweep + // defensive — fallbacks outlive their heads via LLPointer chains and + // registry reloads clear globally — but enforcing it here keeps a + // future runtime fallback-removal path correct by construction. + // No-op when `face` is null or unreferenced. Single-threaded. void clearCacheForFace(const LLFontFreetype* face); // Number of entries currently held in the LRU. Cheap (boost:: @@ -104,6 +111,15 @@ namespace LLFontShaping // the cache contract — production callers shouldn't be branching // on this. Use clearCache to get to a known-empty state. size_t cacheSize(); + + // Monotonic count of cache mutations that can invalidate references + // returned by shapeLine: miss-inserts (including their evictions), + // clearCache, and clearCacheForFace when they erased anything. Cache + // HITS don't bump it — an LRU splice moves list nodes without touching + // the index or any glyph vector. Holders snapshot this after shaping + // and llassert equality after their last dereference, turning a + // use-after-invalidation into a debug assert instead of silent garbage. + size_t cacheMutationCount(); } #endif // LL_LLFONTSHAPING_H From ffcf3d51b5ae834bee334eeb02fbb734b200fc6f Mon Sep 17 00:00:00 2001 From: Rye Date: Thu, 11 Jun 2026 00:46:52 -0400 Subject: [PATCH 3/5] Font audit follow-ups: slot recycling, per-face GC, kern-face fix, registry tidy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The remaining low-priority items from the corruption audit. llfontbitmapcache.cpp/.h: - Recycle released sheet slots. nextOpenPos tracks the active allocation target explicitly (mCurrentSheet, per atlas type) instead of assuming it's the last slot, and a new sheet reuses the first released slot before growing the vectors. The purge-before-release contract (glyph entries erased before releaseSheet, generation bump rebuilds captured vertex buffers) guarantees nothing references a released index, so reuse is safe — and a long session cycling glyph working sets through eviction no longer grows the slot vectors monotonically. llfontface.cpp/.h, llfontfreetype.cpp/.h: - Move the atlas eviction sweep (collectGarbage) and its throttle clock onto LLFontFace. The atlas and glyph map are face state; with the throttle per-head, N freetypes sharing one face re-swept the same atlas once per head per interval. LLFontFreetype::collectGarbage forwards, so sweepGlyphCaches callers are unchanged and siblings no-op inside the shared interval. llfontfreetype.cpp: - getXKerning: probe the kern table of the face that owns BOTH glyphs instead of always the head's. Legacy 'kern' tables map pairs of that face's own glyph indices; feeding the head's table foreign fallback indices was benign for GPOS-era fonts (no legacy table) but garbage-prone on old fonts, and silently dropped real kerning for pairs sharing a fallback face. Mixed-face pairs have no defined kerning and return 0. Pen policy (UNFITTED vs DEFAULT) stays the head's — it owns the layout loop the result feeds. llfontregistry.cpp/.h, llfontgl.cpp, llfontfreetype.h: - getNumFaces becomes a static probe; createFont no longer allocates a throwaway LLFontGL per search path just to count collection faces, and allocates the load wrapper lazily inside the face loop. - New storeFont helper: mFontMap values are owned raw pointers, so assigning into an occupied slot leaked the incumbent; reload()'s re-seat paths and createFont now route through it. - createFont also publishes the head under its canonical (template-name + normalized-size) key when free, so a differently-spelled descriptor that normalizes to the same font shares the freetype through a thin wrapper instead of re-walking every font file. tests/llfontbitmapcache_test.cpp, tests/llfontfreetype_test.cpp: - Re-pin the eviction tests to the recycling contract: released slots come back live instead of staying nullptr placeholders, slot counts stay bounded across release cycles, generation still advances. Co-Authored-By: Claude Fable 5 (1M context) --- indra/llrender/llfontbitmapcache.cpp | 53 +++++++--- indra/llrender/llfontbitmapcache.h | 18 +++- indra/llrender/llfontface.cpp | 65 ++++++++++++ indra/llrender/llfontface.h | 17 ++++ indra/llrender/llfontfreetype.cpp | 98 ++++++------------- indra/llrender/llfontfreetype.h | 21 ++-- indra/llrender/llfontgl.cpp | 9 +- indra/llrender/llfontregistry.cpp | 80 +++++++++++---- indra/llrender/llfontregistry.h | 7 ++ .../llrender/tests/llfontbitmapcache_test.cpp | 91 ++++++++--------- indra/llrender/tests/llfontfreetype_test.cpp | 17 ++-- 11 files changed, 300 insertions(+), 176 deletions(-) diff --git a/indra/llrender/llfontbitmapcache.cpp b/indra/llrender/llfontbitmapcache.cpp index 60dc599c28..dc5390fd49 100644 --- a/indra/llrender/llfontbitmapcache.cpp +++ b/indra/llrender/llfontbitmapcache.cpp @@ -102,12 +102,13 @@ bool LLFontBitmapCache::nextOpenPos(S32 width, S32 height, S32& pos_x, S32& pos_ const U32 bitmap_idx = static_cast(bitmap_type); - // The last sheet is the active allocation target. If it was released by - // the eviction sweep, force a fresh sheet — its in-flight pen offsets are + // mCurrentSheet is the active allocation target. If the eviction sweep + // released it, force a fresh sheet — the in-flight pen offsets are // pointing into a freed image. - const bool last_sheet_released = !mImageRawVec[bitmap_idx].empty() - && mImageRawVec[bitmap_idx].back().isNull(); - bool need_new_sheet = mImageRawVec[bitmap_idx].empty() || last_sheet_released; + const S32 active_sheet = mCurrentSheet[bitmap_idx]; + const bool active_sheet_released = (active_sheet >= 0) + && mImageRawVec[bitmap_idx][active_sheet].isNull(); + bool need_new_sheet = (active_sheet < 0) || active_sheet_released; if (!need_new_sheet && (mCurrentOffsetX[bitmap_idx] + width + 4) > mBitmapWidth) { @@ -167,11 +168,35 @@ bool LLFontBitmapCache::nextOpenPos(S32 width, S32 height, S32& pos_x, S32& pos_ mBitmapWidth = image_width; mBitmapHeight = image_height; + // Prefer recycling a released slot over growing the sheet vectors. + // The nullptr placeholder only existed for index stability, and the + // purge-before-release contract (every glyph entry referencing a + // sheet is erased before releaseSheet, which also bumps the + // generation so captured vertex buffers rebuild) guarantees nothing + // references the released index anymore. Without recycling, a long + // session that cycles glyph working sets through eviction grows the + // slot vectors monotonically. + S32 slot = -1; + for (size_t i = 0, n = mImageRawVec[bitmap_idx].size(); i < n; ++i) + { + if (mImageRawVec[bitmap_idx][i].isNull()) + { + slot = static_cast(i); + break; + } + } + if (slot < 0) + { + mImageRawVec[bitmap_idx].emplace_back(); + mImageGLVec[bitmap_idx].emplace_back(); + mLastUsedTime[bitmap_idx].push_back(0.0); + slot = static_cast(mImageRawVec[bitmap_idx].size()) - 1; + } + S32 num_components = getNumComponents(bitmap_type); - mImageRawVec[bitmap_idx].emplace_back(new LLImageRaw(mBitmapWidth, mBitmapHeight, num_components)); - bitmap_num = static_cast(mImageRawVec[bitmap_idx].size()) - 1; + mImageRawVec[bitmap_idx][slot] = new LLImageRaw(mBitmapWidth, mBitmapHeight, num_components); - LLImageRaw* image_raw = mImageRawVec[bitmap_idx][bitmap_num]; + LLImageRaw* image_raw = mImageRawVec[bitmap_idx][slot]; if (EFontGlyphType::Grayscale == bitmap_type) { image_raw->clear(255, 0); @@ -186,11 +211,12 @@ bool LLFontBitmapCache::nextOpenPos(S32 width, S32 height, S32& pos_x, S32& pos_ } // Make corresponding GL image. - mImageGLVec[bitmap_idx].emplace_back(new LLImageGL(image_raw, false, false)); - LLImageGL* image_gl = mImageGLVec[bitmap_idx][bitmap_num]; + mImageGLVec[bitmap_idx][slot] = new LLImageGL(image_raw, false, false); + LLImageGL* image_gl = mImageGLVec[bitmap_idx][slot]; - // Track per-sheet last-used time alongside the image vectors. - mLastUsedTime[bitmap_idx].push_back(0.0); + // Fresh sheet hasn't been read or written yet. + mLastUsedTime[bitmap_idx][slot] = 0.0; + mCurrentSheet[bitmap_idx] = slot; // Start at beginning of the new image. 4px border guarantees that // the shadow shader's worst-case sample reach (2px screen dilation + @@ -207,7 +233,7 @@ bool LLFontBitmapCache::nextOpenPos(S32 width, S32 height, S32& pos_x, S32& pos_ pos_x = mCurrentOffsetX[bitmap_idx]; pos_y = mCurrentOffsetY[bitmap_idx]; - bitmap_num = getNumBitmaps(bitmap_type) - 1; + bitmap_num = static_cast(mCurrentSheet[bitmap_idx]); mCurrentOffsetX[bitmap_idx] += width + 4; // Track tallest glyph placed in the current row so the next Y advance @@ -303,6 +329,7 @@ void LLFontBitmapCache::reset() mCurrentOffsetX[idx] = 4; mCurrentOffsetY[idx] = 4; mCurrentRowMaxHeight[idx] = 0; + mCurrentSheet[idx] = -1; } mBitmapWidth = 0; diff --git a/indra/llrender/llfontbitmapcache.h b/indra/llrender/llfontbitmapcache.h index f705275f4e..8dd12bb10c 100644 --- a/indra/llrender/llfontbitmapcache.h +++ b/indra/llrender/llfontbitmapcache.h @@ -92,11 +92,14 @@ class LLFontBitmapCache static S32 getGlobalGeneration() { return sNextGeneration; } // Drop the underlying images for a sheet, freeing the GPU and CPU memory. - // The slot index remains valid (kept as a nullptr placeholder) so existing - // sheet-index references stay numerically stable; callers must purge any - // glyph cache entries that referenced this sheet *before* releasing it, - // otherwise the next render will try to draw from a null texture. Bumps - // the cache generation so vertex buffers invalidate. + // The slot index stays valid as a nullptr placeholder so surviving + // sheet-index references remain numerically stable; callers must purge + // any glyph cache entries that referenced this sheet *before* releasing + // it, otherwise the next render will try to draw from a null texture. + // Released slots are recycled by the next nextOpenPos that needs a new + // sheet (the purge contract guarantees nothing references the index by + // then), so the slot vectors don't grow monotonically across eviction + // cycles. Bumps the cache generation so vertex buffers invalidate. void releaseSheet(EFontGlyphType bitmap_type, U32 bitmap_num); // Wall-clock seconds (LLFrameTimer::getTotalSeconds) when this sheet was @@ -136,6 +139,11 @@ class LLFontBitmapCache // glyph heights (text + tall color emoji bitmaps) don't have later // rows overwriting earlier rows. S32 mCurrentRowMaxHeight[static_cast(EFontGlyphType::Count)] = { 0, 0 }; + // Slot index the pen offsets above point into, per atlas type; -1 + // until the first sheet is created. Explicit because the active sheet + // is no longer necessarily the last slot: nextOpenPos recycles + // released slots, so the allocation target can sit mid-vector. + S32 mCurrentSheet[static_cast(EFontGlyphType::Count)] = { -1, -1 }; S32 mMaxCharWidth = 0; S32 mMaxCharHeight = 0; // Globally-unique generation. Each new LLFontBitmapCache instance and diff --git a/indra/llrender/llfontface.cpp b/indra/llrender/llfontface.cpp index b475600bef..2cbdfa1647 100644 --- a/indra/llrender/llfontface.cpp +++ b/indra/llrender/llfontface.cpp @@ -33,6 +33,7 @@ #include "llfontfreetype.h" // for LLFontGlyphInfo, LLFontManager, ll::fonts::LoadedFont #include "llfontgl.h" // for sUseDarkEmojiPalette #include "llfontregistry.h" // for EFontHinting full definition +#include "llframetimer.h" // collectGarbage throttle clock #include "llimage.h" // LLImageRaw, LLImageDataLock #include "llmath.h" // ll_round, llclamp @@ -277,6 +278,70 @@ void LLFontFace::resetBitmapCache() mFontBitmapCachep->reset(); } +void LLFontFace::collectGarbage() const +{ + if (!mFTFace || !mFontBitmapCachep) + return; + + // Sweep cadence: cheap enough to run at the top of every frame, with + // GC_INTERVAL_SEC bounding actual work. Idle threshold sized for "real + // user idle" — roughly the time after which a chat scrollback or panel of + // unique-codepoint text has stopped being displayed. Long enough not to + // churn during normal interaction; short enough that an hour-long session + // doesn't accumulate every transient code page ever shown. + constexpr F64 GC_INTERVAL_SEC = 5.0; + constexpr F64 IDLE_THRESHOLD_SEC = 60.0 * 15.0; + + const F64 now = LLFrameTimer::getTotalSeconds(); + if (now < mNextGcTime) + return; + mNextGcTime = now + GC_INTERVAL_SEC; + + auto glyph_uses_sheet = [](const LLFontGlyphInfo* gi, EFontGlyphType type, U32 num) -> bool + { + for (U8 p = 0; p < gi->mPhaseCount; ++p) + { + const auto& entry = gi->mPhaseSlots[p].mBitmapEntry; + if (entry.first == type && entry.second >= 0 && static_cast(entry.second) == num) + return true; + } + return false; + }; + + // Shaped runs in LLFontShaping's cache hold only metric/glyph_id data — no + // atlas references — so they survive eviction; getGlyphInfoByIndex on the + // next frame re-rasterizes whichever glyphs were dropped here. Cache + // generation bumps inside releaseSheet so LLFontVertexBuffer rebuilds. + for (U32 t = 0; t < static_cast(EFontGlyphType::Count); ++t) + { + const EFontGlyphType type = static_cast(t); + const U32 sheet_count = mFontBitmapCachep->getNumBitmaps(type); + for (U32 num = 0; num < sheet_count; ++num) + { + if (mFontBitmapCachep->isSheetReleased(type, num)) + continue; + const F64 last_used = mFontBitmapCachep->getSheetLastUsedTime(type, num); + // last_used == 0 means the sheet was allocated but not yet drawn + // from — skip it for one cycle so a brand-new sheet gets at least + // a frame to be touched before it's a candidate. + if (last_used <= 0.0) + continue; + if ((now - last_used) <= IDLE_THRESHOLD_SEC) + continue; + + // Delete the glyph entries that reference this sheet, then + // release the sheet itself. There is no head-side cache to + // invalidate: getGlyphInfoByIndex routes every lookup through + // findGlyphInfo here, so every freetype sharing this face + // observes the deletion on its next render and re-rasterizes. + auto matches = [&](const LLFontGlyphInfo* gi) { return glyph_uses_sheet(gi, type, num); }; + erase_glyph_entries(matches); + + mFontBitmapCachep->releaseSheet(type, num); + } + } +} + void LLFontFace::destroyGlyphInfo(LLFontGlyphInfo* gi) { delete gi; diff --git a/indra/llrender/llfontface.h b/indra/llrender/llfontface.h index a1e84b3811..a99ef28926 100644 --- a/indra/llrender/llfontface.h +++ b/indra/llrender/llfontface.h @@ -236,6 +236,17 @@ class LLFontFace : public LLRefCount template void erase_glyph_entries(Pred should_erase) const; + // Release atlas sheets that haven't been read or written within the + // idle threshold, dropping the glyph entries that pointed into them + // first. Self-throttled — repeat calls inside the GC interval are + // cheap no-ops. Lives on the face because the atlas and glyph map do: + // N LLFontFreetype heads sharing this face cost one sweep per + // interval, not N (the throttle used to sit per-head, so siblings + // re-swept the same shared atlas). NOT safe to call mid-render while + // a glyph pointer is held: call only at frame boundaries / before any + // glyph lookups (LLFontGL::sweepGlyphCaches does). + void collectGarbage() const; + // Drop all rasterized glyphs and reset the atlas. Used by the registry // when DPI changes and the wrapper survives but its atlas state needs // to be rebuilt. @@ -296,6 +307,12 @@ class LLFontFace : public LLRefCount LLFontBitmapCache* mFontBitmapCachep = nullptr; mutable glyph_info_map_t mGlyphInfoMap; + // Earliest wall-clock time (seconds) at which collectGarbage() should + // do real work. Throttle gate so the per-frame sweep is essentially + // free between intervals. Per-face (not per-head) so siblings sharing + // this face share one cadence. + mutable F64 mNextGcTime = 0.0; + bool mUseSubpixelPen = false; bool mHasColor = false; bool mHasSvg = false; diff --git a/indra/llrender/llfontfreetype.cpp b/indra/llrender/llfontfreetype.cpp index 52cdcad55a..d6a65b737d 100644 --- a/indra/llrender/llfontfreetype.cpp +++ b/indra/llrender/llfontfreetype.cpp @@ -50,7 +50,6 @@ #include "lldir.h" #include "llerror.h" -#include "llframetimer.h" #include "llimage.h" #include "llimagepng.h" //#include "llimagej2c.h" @@ -556,24 +555,42 @@ F32 LLFontFreetype::getXKerning(const LLFontGlyphInfo* left_glyph_info, const LL if (getFTFace() == nullptr) return 0.0; - U32 left_glyph = left_glyph_info ? left_glyph_info->mGlyphIndex : 0; - U32 right_glyph = right_glyph_info ? right_glyph_info->mGlyphIndex : 0; + // Kerning is defined within one face: the legacy 'kern' table maps + // pairs of THAT face's glyph indices. Probe the table of the face that + // owns both glyphs — historically this always probed the head's table, + // which fed it foreign indices whenever the glyphs came from a fallback + // (benign for GPOS-era fonts, which ship no legacy kern table, but + // wrong in principle and garbage-prone on old fonts — and it silently + // dropped real kerning for pairs that DO share a fallback face, e.g. + // two CJK glyphs). A mixed-face pair has no defined kerning at all. + if (!left_glyph_info || !right_glyph_info) + return 0.f; + const LLFontFace* source_face = left_glyph_info->mSourceFace; + if (!source_face || source_face != right_glyph_info->mSourceFace) + return 0.f; + LLFT_Face kern_face = source_face->face(); + if (!kern_face) + return 0.f; + + U32 left_glyph = left_glyph_info->mGlyphIndex; + U32 right_glyph = right_glyph_info->mGlyphIndex; FT_Vector delta; // UNFITTED gives subpixel-precise kerning when callers maintain a // fractional pen accumulator (mUseSubpixelPen — autohinted/unhinted // faces). DEFAULT grid-fits to integer pixels, which is what callers - // want when they round per glyph (native-hinted faces). + // want when they round per glyph (native-hinted faces). The pen policy + // is the head's: it owns the layout loop the result feeds. const FT_UInt kern_mode = mUseSubpixelPen ? FT_KERNING_UNFITTED : FT_KERNING_DEFAULT; - llverify(!FT_Get_Kerning(getFTFace(), left_glyph, right_glyph, kern_mode, &delta)); + llverify(!FT_Get_Kerning(kern_face, left_glyph, right_glyph, kern_mode, &delta)); // Apply the FreeType auto-hinter's subpixel side-bearing correction between // adjacent glyphs. The lsb/rsb deltas are populated only when the autohinter // ran; for native-hinted (DEFAULT) and unhinted (NO_HINTING) loads they're // always zero and the correction is meaningless. F32 delta_correction = 0.0f; - if (mHinting == EFontHinting::FORCE_AUTOHINT && left_glyph_info && right_glyph_info) + if (mHinting == EFontHinting::FORCE_AUTOHINT) { // delta_diff is in 26.6 fixed point: the autohinter's net shift in // inter-glyph spacing (positive = hinter pushed glyphs apart). @@ -1202,70 +1219,11 @@ const LLFontBitmapCache* LLFontFreetype::getFontBitmapCache() const void LLFontFreetype::collectGarbage() const { - if (!getFTFace()) - return; - - // Sweep cadence: cheap enough to run at the top of every render call, with - // GC_INTERVAL_SEC bounding actual work. Idle threshold sized for "real - // user idle" — roughly the time after which a chat scrollback or panel of - // unique-codepoint text has stopped being displayed. Long enough not to - // churn during normal interaction; short enough that an hour-long session - // doesn't accumulate every transient code page ever shown. - constexpr F64 GC_INTERVAL_SEC = 5.0; - constexpr F64 IDLE_THRESHOLD_SEC = 60.0 * 15.0; - - const F64 now = LLFrameTimer::getTotalSeconds(); - if (now < mNextGcTime) - return; - mNextGcTime = now + GC_INTERVAL_SEC; - - auto glyph_uses_sheet = [](const LLFontGlyphInfo* gi, EFontGlyphType type, U32 num) -> bool - { - for (U8 p = 0; p < gi->mPhaseCount; ++p) - { - const auto& entry = gi->mPhaseSlots[p].mBitmapEntry; - if (entry.first == type && entry.second >= 0 && static_cast(entry.second) == num) - return true; - } - return false; - }; - - // Shaped runs in LLFontShaping's cache hold only metric/glyph_id data — no - // atlas references — so they survive eviction; getGlyphInfoByIndex on the - // next frame re-rasterizes whichever glyphs were dropped here. Cache - // generation bumps inside releaseSheet so LLFontVertexBuffer rebuilds. - for (U32 t = 0; t < static_cast(EFontGlyphType::Count); ++t) - { - const EFontGlyphType type = static_cast(t); - const U32 sheet_count = getBitmapCache()->getNumBitmaps(type); - for (U32 num = 0; num < sheet_count; ++num) - { - if (getBitmapCache()->isSheetReleased(type, num)) - continue; - const F64 last_used = getBitmapCache()->getSheetLastUsedTime(type, num); - // last_used == 0 means the sheet was allocated but not yet drawn - // from — skip it for one cycle so a brand-new sheet gets at least - // a frame to be touched before it's a candidate. - if (last_used <= 0.0) - continue; - if ((now - last_used) <= IDLE_THRESHOLD_SEC) - continue; - - // Delete the face-owned glyph entries that reference this sheet, - // then release the sheet itself. There is no head-side cache to - // invalidate: getGlyphInfoByIndex routes every lookup through the - // face's findGlyphInfo, so siblings sharing this face (other - // heads, or heads using this face as a fallback) automatically - // observe the deletion on their next render and re-rasterize. - auto matches = [&](const LLFontGlyphInfo* gi) { return glyph_uses_sheet(gi, type, num); }; - if (mFace) - { - mFace->erase_glyph_entries(matches); - } - - getBitmapCache()->releaseSheet(type, num); - } - } + // The sweep (and its throttle) lives on the shared face wrapper — the + // atlas and glyph map are face state, and siblings wrapping the same + // face should cost one sweep per interval, not one per head. + if (mFace) + mFace->collectGarbage(); } U8 LLFontFreetype::getStyle() const diff --git a/indra/llrender/llfontfreetype.h b/indra/llrender/llfontfreetype.h index f36c3f26a8..432e3bb5fb 100644 --- a/indra/llrender/llfontfreetype.h +++ b/indra/llrender/llfontfreetype.h @@ -154,7 +154,10 @@ class LLFontFreetype : public LLRefCount // to render directly (Unicode backup, primarily) bool loadFace(const std::string& filename, F32 point_size, F32 vert_dpi, F32 horz_dpi, bool is_fallback, S32 face_n, EFontHinting hinting, S32 flags, const LLFontVarAxes& var_axes = {}); - S32 getNumFaces(const std::string& filename); + // Count the faces in a font file (TTC/OTC collections). Pure probe — + // opens a temporary FT face from a locally-read buffer and closes it; + // touches no instance state, so callers don't need a live freetype. + static S32 getNumFaces(const std::string& filename); typedef std::function char_functor_t; void addFallbackFont(const LLPointer& fallback_font, const char_functor_t& functor = nullptr); @@ -281,10 +284,11 @@ class LLFontFreetype : public LLRefCount // Run a maintenance pass that releases bitmap atlas sheets which haven't // been read or written within the idle threshold, recovering their CPU // and GPU memory and dropping any LLFontGlyphInfo entries that pointed - // into them. Self-throttled — repeat calls inside the GC interval are - // cheap no-ops, so it's fine for every render() invocation to call this - // unconditionally. NOT safe to call mid-render while a glyph pointer is - // held: call only at frame boundaries / before any glyph lookups. + // into them. Forwards to LLFontFace::collectGarbage — the sweep and its + // throttle live on the shared face wrapper, so N heads sharing one face + // cost one sweep per interval, not N. NOT safe to call mid-render while + // a glyph pointer is held: call only at frame boundaries / before any + // glyph lookups. void collectGarbage() const; // Return the FT glyph index for `wch` on this face, caching the result so @@ -394,10 +398,9 @@ class LLFontFreetype : public LLRefCount mutable S32 mRenderGlyphCount; - // Earliest wall-clock time (seconds) at which collectGarbage() should - // do real work. Throttle gate so the per-render call from LLFontGL::render - // is essentially free between sweeps. - mutable F64 mNextGcTime = 0.0; + // (collectGarbage's throttle clock moved to LLFontFace::mNextGcTime — + // per-head throttles made siblings sharing a face re-sweep the same + // atlas once per head per interval.) }; #endif // LL_FONTFREETYPE_H diff --git a/indra/llrender/llfontgl.cpp b/indra/llrender/llfontgl.cpp index 9fd798dbff..c04d327e3d 100644 --- a/indra/llrender/llfontgl.cpp +++ b/indra/llrender/llfontgl.cpp @@ -178,12 +178,9 @@ bool LLFontGL::loadFace(const std::string& filename, F32 point_size, const F32 v S32 LLFontGL::getNumFaces(const std::string& filename) { - if (mFontFreetype == reinterpret_cast(NULL)) - { - mFontFreetype = new LLFontFreetype; - } - - return mFontFreetype->getNumFaces(filename); + // Pure file probe — no instance state involved, so don't allocate a + // throwaway LLFontFreetype just to count collection faces. + return LLFontFreetype::getNumFaces(filename); } S32 LLFontGL::getCacheGeneration() const diff --git a/indra/llrender/llfontregistry.cpp b/indra/llrender/llfontregistry.cpp index 243bfe20ac..83c7079e27 100644 --- a/indra/llrender/llfontregistry.cpp +++ b/indra/llrender/llfontregistry.cpp @@ -1597,6 +1597,21 @@ bool LLFontRegistry::nameToSize(const std::string& size_name, F32& size) } +void LLFontRegistry::storeFont(const LLFontDescriptor& desc, LLFontGL* fontp) +{ + auto it = mFontMap.find(desc); + if (it != mFontMap.end()) + { + if (it->second != fontp) + { + delete it->second; + it->second = fontp; + } + return; + } + mFontMap[desc] = fontp; +} + LLFontGL *LLFontRegistry::createFont(const LLFontDescriptor& desc) { // Name should hold a font name recognized as a setting; the value @@ -1642,7 +1657,7 @@ LLFontGL *LLFontRegistry::createFont(const LLFontDescriptor& desc) LLFontGL *font = new LLFontGL; font->mFontDescriptor = desc; font->mFontFreetype = it->second->mFontFreetype; - mFontMap[desc] = font; + storeFont(desc, font); return font; } @@ -1729,8 +1744,11 @@ LLFontGL *LLFontRegistry::createFont(const LLFontDescriptor& desc) { const std::string font_path = *font_search_path_it + font_file_it->FileName; - fontp = new LLFontGL; - S32 num_faces = font_file_it->mLoadCollection ? fontp->getNumFaces(font_path) : 1; + // Static probe — counting collection faces needs no LLFontGL. + // The lazy `new LLFontGL` inside the face loop below allocates + // only when a face actually loads, so cache-hit-only files and + // missing search paths allocate nothing. + S32 num_faces = font_file_it->mLoadCollection ? LLFontFreetype::getNumFaces(font_path) : 1; for (S32 i = 0; i < num_faces; i++) { // Fallback dedup: if the same (filename, face_index, sized @@ -1808,10 +1826,13 @@ LLFontGL *LLFontRegistry::createFont(const LLFontDescriptor& desc) { LL_INFOS_ONCE("LLFontRegistry") << "Couldn't load font " << font_file_it->FileName << LL_ENDL; } - // fontp is non-NULL here when every face of this file hit - // mFallbackInstanceCache (the inner loop's `continue` skips the - // delete branches), or when all search paths failed. Either way - // ownership wasn't transferred to result/cache, so free it. + // fontp is non-NULL here only when an allocated wrapper wasn't + // consumed — its loadFace succeeded for a cache-inserted fallback + // but a later face of the same file hit mFallbackInstanceCache and + // `continue`d past the delete branches. Ownership wasn't + // transferred to result, so free it. (All-cache-hit files and + // missing search paths never allocate now — the wrapper is created + // lazily inside the face loop.) delete fontp; fontp = NULL; } @@ -1819,7 +1840,22 @@ LLFontGL *LLFontRegistry::createFont(const LLFontDescriptor& desc) if (result) { result->mFontDescriptor = desc; - mFontMap[desc] = result; + storeFont(desc, result); + // Also publish the result under its canonical (template-name + + // normalized-size) key when that differs from the requested desc + // and isn't taken. The matching-font-exists shortcut at the top + // probes exactly this key, so the next differently-spelled + // descriptor that normalizes to the same font shares this + // freetype through a thin wrapper instead of re-walking every + // font file. The alias is a full map citizen: owned by its slot, + // deleted in clear(), rebuilt by reload() like any head. + if (mFontMap.find(nearest_exact_desc) == mFontMap.end()) + { + LLFontGL* canonical = new LLFontGL; + canonical->mFontDescriptor = nearest_exact_desc; + canonical->mFontFreetype = result->mFontFreetype; + mFontMap[nearest_exact_desc] = canonical; + } } else { @@ -1943,10 +1979,11 @@ bool LLFontRegistry::reload(const LLSD& font_overrides) { LL_WARNS() << "Font reload: parseFontInfo failed; restoring previous fallback cache" << LL_ENDL; mFallbackInstanceCache = std::move(pinned_old_fallbacks); - // Re-insert the snapshot heads so widgets continue rendering - // against their previous freetype state. + // Re-seat the snapshot heads so widgets continue rendering + // against their previous freetype state. storeFont guards against + // anything the partial parse left in a colliding slot. for (const auto& [desc, head] : heads) - mFontMap[desc] = head; + storeFont(desc, head); return false; } @@ -1962,17 +1999,19 @@ bool LLFontRegistry::reload(const LLSD& font_overrides) for (auto& [desc, head] : heads) { // createFont allocates a fresh LLFontGL with newly-loaded - // mFontFreetype and inserts it into mFontMap[desc]. We then steal - // its mFontFreetype LLPointer onto the existing head and replace - // the map entry with the original pointer so widget caches stay - // valid. + // mFontFreetype and stores it at mFontMap[desc]. We steal its + // mFontFreetype LLPointer onto the existing head, then re-seat the + // original pointer so widget caches stay valid — storeFont deletes + // the `fresh` wrapper sitting in the slot. The snapshot can hold + // canonical aliases too; when their turn comes, createFont's + // matching-font-exists shortcut shares the already-rebuilt + // freetype instead of re-walking font files. LLFontGL* fresh = createFont(desc); if (fresh) { head->mFontFreetype = fresh->mFontFreetype; head->mFontDescriptor = desc; - mFontMap[desc] = head; - delete fresh; + storeFont(desc, head); head->generateASCIIglyphs(); } else @@ -1981,9 +2020,10 @@ bool LLFontRegistry::reload(const LLSD& font_overrides) << desc.getName() << " size " << desc.getSize() << " style " << (S32)desc.getStyle() << "; keeping previous freetype" << LL_ENDL; - // Re-insert old head so widgets still render its previous - // (now-orphaned) glyphs rather than nothing at all. - mFontMap[desc] = head; + // Re-seat the old head so widgets still render its previous + // (now-orphaned) glyphs rather than nothing at all. storeFont + // guards against an interim alias occupying the slot. + storeFont(desc, head); } } diff --git a/indra/llrender/llfontregistry.h b/indra/llrender/llfontregistry.h index 511a6abc86..1db5e5e1e4 100644 --- a/indra/llrender/llfontregistry.h +++ b/indra/llrender/llfontregistry.h @@ -311,6 +311,13 @@ class LLFontRegistry private: LLFontRegistry(const LLFontRegistry& other); // no-copy LLFontGL *createFont(const LLFontDescriptor& desc); + // Assign mFontMap[desc] = fontp, deleting any different LLFontGL the + // slot already owns. Map values are owned raw pointers, so plain + // assignment on an occupied slot leaks; occupied slots happen when + // reload() drives createFont directly over a map that already holds + // interim wrappers (canonical aliases, fresh heads from earlier + // iterations). Null-safe on both the incumbent and `fontp`. + void storeFont(const LLFontDescriptor& desc, LLFontGL* fontp); // Resolve cross-family references, then per-family inherit="true" // style variants, in that order. Idempotent — consumes mFamilyUses and // mInheritFlags so re-running over a partially-resolved registry is safe. diff --git a/indra/llrender/tests/llfontbitmapcache_test.cpp b/indra/llrender/tests/llfontbitmapcache_test.cpp index 38426c90fb..7365e537ae 100644 --- a/indra/llrender/tests/llfontbitmapcache_test.cpp +++ b/indra/llrender/tests/llfontbitmapcache_test.cpp @@ -358,10 +358,13 @@ namespace tut c.getNumBitmaps(EFontGlyphType::Grayscale), 1u); } - // After releasing the trailing sheet, the next nextOpenPos must - // allocate a fresh sheet rather than try to write into the freed - // image — pins llfontbitmapcache.cpp:105-107 (last_sheet_released - // check). + // After releasing the active sheet, the next nextOpenPos must build a + // fresh image rather than write into the freed one — and it recycles + // the released slot instead of growing the sheet vector. The purge- + // before-release contract guarantees nothing references the released + // index, so reuse is safe and bounds slot growth across eviction + // cycles. Pins the active_sheet_released check + slot recycling in + // nextOpenPos. template<> template<> void llfontbitmapcache_gl_object::test<10>() { @@ -371,85 +374,83 @@ namespace tut c.nextOpenPos(8, 8, px, py, EFontGlyphType::Grayscale, sheet); c.releaseSheet(EFontGlyphType::Grayscale, sheet); - S32 px2 = 0, py2 = 0; U32 sheet2 = 0; - ensure("nextOpenPos succeeds after trailing sheet release", + S32 px2 = 0, py2 = 0; U32 sheet2 = 99; + ensure("nextOpenPos succeeds after active sheet release", c.nextOpenPos(8, 8, px2, py2, EFontGlyphType::Grayscale, sheet2)); - ensure_equals("new sheet is index 1 (slot 0 kept as nullptr)", - (S32)sheet2, 1); + ensure_equals("released slot 0 is recycled for the new sheet", + (S32)sheet2, 0); ensure_equals("new sheet starts at posX=4", px2, 4); ensure_equals("new sheet starts at posY=4", py2, 4); - ensure("released sheet 0 is still released", - c.isSheetReleased(EFontGlyphType::Grayscale, 0)); + ensure("recycled slot 0 is live again", + !c.isSheetReleased(EFontGlyphType::Grayscale, 0)); + ensure_equals("slot vector did not grow", + c.getNumBitmaps(EFontGlyphType::Grayscale), 1u); } // Multi-cycle release+re-alloc: alloc, release, alloc, release, alloc - // produces three sheet slots. Only the most recent is live; slots 0 - // and 1 are released-nullptr placeholders that stay reserved (no slot - // index re-use). Pins index stability under repeat cycles — - // llfontbitmapcache.cpp:105-107. + // keeps recycling slot 0 — the slot vector never grows across eviction + // cycles, and the generation counter still advances every cycle so + // captured vertex buffers rebuild. Pins bounded slot growth under + // repeat cycles. template<> template<> void llfontbitmapcache_gl_object::test<12>() { LLFontBitmapCache c; c.init(2, 2); S32 px = 0, py = 0; - U32 s0 = 0, s1 = 0, s2 = 0; + U32 s0 = 0, s1 = 99, s2 = 99; c.nextOpenPos(8, 8, px, py, EFontGlyphType::Grayscale, s0); ensure_equals("first alloc lands on sheet 0", (S32)s0, 0); + const S32 gen0 = c.getCacheGeneration(); c.releaseSheet(EFontGlyphType::Grayscale, s0); c.nextOpenPos(8, 8, px, py, EFontGlyphType::Grayscale, s1); - ensure_equals("second alloc lands on sheet 1", (S32)s1, 1); + ensure_equals("second alloc recycles slot 0", (S32)s1, 0); + const S32 gen1 = c.getCacheGeneration(); + ensure("generation advanced across release+recycle", gen1 > gen0); c.releaseSheet(EFontGlyphType::Grayscale, s1); c.nextOpenPos(8, 8, px, py, EFontGlyphType::Grayscale, s2); - ensure_equals("third alloc lands on sheet 2", (S32)s2, 2); - - ensure_equals("getNumBitmaps == 3 (released slots stay reserved)", - c.getNumBitmaps(EFontGlyphType::Grayscale), 3u); - ensure("sheet 0 still released", - c.isSheetReleased(EFontGlyphType::Grayscale, 0)); - ensure("sheet 1 still released", - c.isSheetReleased(EFontGlyphType::Grayscale, 1)); - ensure("sheet 2 live (not released)", - !c.isSheetReleased(EFontGlyphType::Grayscale, 2)); + ensure_equals("third alloc recycles slot 0 again", (S32)s2, 0); + ensure("generation advanced again", c.getCacheGeneration() > gen1); + + ensure_equals("getNumBitmaps == 1 (slots recycled, not reserved)", + c.getNumBitmaps(EFontGlyphType::Grayscale), 1u); + ensure("slot 0 live after the final alloc", + !c.isSheetReleased(EFontGlyphType::Grayscale, 0)); } - // Mid-list released sheet stays dead: once a non-trailing sheet is - // released, the allocator never re-uses its index. Subsequent allocs - // continue using the trailing live sheet (if it has room) until that - // sheet is itself released or fills. Pins the "back-only re-allocation" - // invariant — llfontbitmapcache.cpp:105-107 only inspects back(). + // A live active sheet with room keeps absorbing allocations — a + // release+recycle cycle doesn't perturb the pen, and no spurious + // sheets are created while the active sheet has space. Pins the + // active-sheet tracking (mCurrentSheet) staying put across allocs. template<> template<> void llfontbitmapcache_gl_object::test<13>() { LLFontBitmapCache c; c.init(2, 2); S32 px = 0, py = 0; - U32 s0 = 0, s1 = 0; + U32 s0 = 0, s1 = 99; - // Alloc sheet 0, release (trailing), alloc → sheet 1 (live). + // Alloc sheet 0, release it, alloc → recycled slot 0 (live). c.nextOpenPos(8, 8, px, py, EFontGlyphType::Grayscale, s0); c.releaseSheet(EFontGlyphType::Grayscale, s0); c.nextOpenPos(8, 8, px, py, EFontGlyphType::Grayscale, s1); - ensure_equals("after release+alloc, on sheet 1", (S32)s1, 1); + ensure_equals("after release+alloc, recycled onto slot 0", (S32)s1, 0); - // Sheet 0 is now mid-list released; sheet 1 is trailing live. - // Further allocs must keep landing on sheet 1 (it has plenty of - // room — 1024² atlas, only one 8x8 glyph placed). + // Further allocs must keep landing on the recycled sheet (it has + // plenty of room — only one 8x8 glyph placed since the rebuild). for (int i = 0; i < 4; ++i) { U32 si = 99; c.nextOpenPos(8, 8, px, py, EFontGlyphType::Grayscale, si); - ensure_equals("subsequent alloc stays on sheet 1 (mid-list 0 untouched)", - (S32)si, 1); + ensure_equals("subsequent alloc stays on the recycled sheet", + (S32)si, 0); } - ensure_equals("still 2 sheet slots (no extra allocation)", - c.getNumBitmaps(EFontGlyphType::Grayscale), 2u); - ensure("sheet 0 stays released", - c.isSheetReleased(EFontGlyphType::Grayscale, 0)); - ensure("sheet 1 stays live", - !c.isSheetReleased(EFontGlyphType::Grayscale, 1)); + ensure_equals("still 1 sheet slot (no extra allocation)", + c.getNumBitmaps(EFontGlyphType::Grayscale), 1u); + ensure("recycled sheet stays live", + !c.isSheetReleased(EFontGlyphType::Grayscale, 0)); } // Cross-instance global generation: two LLFontBitmapCache instances diff --git a/indra/llrender/tests/llfontfreetype_test.cpp b/indra/llrender/tests/llfontfreetype_test.cpp index 00ef2199c2..64eee94bab 100644 --- a/indra/llrender/tests/llfontfreetype_test.cpp +++ b/indra/llrender/tests/llfontfreetype_test.cpp @@ -986,9 +986,10 @@ namespace tut // Sheet re-entry through the rasterizer: rasterize onto sheet 0, // release sheet 0 directly, rasterize a new codepoint — the new - // glyph allocates sheet 1 (slot 0 holds a nullptr placeholder) - // and getCacheGeneration advances. Pins index stability through - // the rasterizer layer (Phase 11 pins it at the cache layer). + // glyph rebuilds into the recycled slot (slot growth stays bounded + // across eviction cycles) and getCacheGeneration advances so + // captured vertex buffers rebuild. Pins slot recycling through the + // rasterizer layer (the cache-layer tests pin it at nextOpenPos). template<> template<> void llfontfreetype_render_object::test<6>() { @@ -1012,14 +1013,14 @@ namespace tut ensure("sheet 0 reports released", cache->isSheetReleased(EFontGlyphType::Grayscale, 0)); - // New glyph forces a fresh sheet because slot 0 is no longer - // re-allocatable (only trailing released slots get reused; slot - // 0 here is mid-list once sheet 1 exists). Picking a glyph not + // New glyph rebuilds into the recycled slot. Picking a glyph not // already cached avoids hitting the cmap-cache branch. LLFontGlyphInfo* g2 = ft->getGlyphInfo(L'B', EFontGlyphType::Grayscale); ensure("second glyph allocated post-release", g2 != nullptr); - ensure("getNumBitmaps grew (new sheet allocated)", - cache->getNumBitmaps(EFontGlyphType::Grayscale) >= 2u); + ensure_equals("released slot recycled (no sheet-vector growth)", + cache->getNumBitmaps(EFontGlyphType::Grayscale), 1u); + ensure("recycled slot is live again", + !cache->isSheetReleased(EFontGlyphType::Grayscale, 0)); ensure("cache generation advanced after release+alloc", cache->getCacheGeneration() > gen_before); } From c7bce2fc97c49129dda35f8d4bbd4bd5d2d0e5e8 Mon Sep 17 00:00:00 2001 From: Rye Date: Thu, 11 Jun 2026 00:59:50 -0400 Subject: [PATCH 4/5] Shader text shadow: derive atlas texel size in-shader; drop dead uniforms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the mixed-atlas limitation noted when the shader shadow path landed. atlasTexelSize was pushed once per string from the HEAD face's atlas dimensions, but one shadowed string can batch glyphs from differently-sized atlases (a 512px head sheet vs a 1024px fallback emoji sheet), putting the fallback glyphs' shadow taps at the wrong distance — and captured vertex buffers replay per-batch texture binds but cannot replay per-batch uniforms, so a uniform could never be right for mixed strings. uiF.glsl: - Compute atlasTexelSize from textureSize(diffuseMap, 0) inside the shadow branch. Exact for whichever atlas each draw actually binds, including captured-buffer replay. shadowMode is now the shader's only shadow uniform, per-pass constant by design. - The channel half of the old limitation was already solved when sampleAtlasAlpha switched to .a sampling (the grayscale atlas's RG swizzle maps coverage to .a; BGRA is native) — the C++ side just never caught up. llfontgl.cpp, llfontvertexbuffer.cpp/.h, llviewershadermgr.cpp: - Drop the atlasTexelSize pushes and the genBuffers texel-size capture (mLastAtlasTexelW/H), and the grayscaleAtlas pushes — that uniform no longer exists in the shader, so all three sites were setting nothing. Only shadowMode remains: pushed before pass A, reset before pass B, re-pushed on captured replay, defaulted to 0 at shader creation. Known pre-existing edge for the shader-shadow re-enable checklist (inert while sEnableShaderShadow is off): with shadow != NO_SHADOW the underline quad is emitted before pass A, so capture stores it in mShadowBufferList and replay draws it under shadowMode != 0 — DROP is visually identical (white-texture taps collapse to passthrough math), SOFT renders the underline slightly denser than direct mode. Co-Authored-By: Claude Fable 5 (1M context) --- indra/llrender/llfontgl.cpp | 29 ++++++++----------- indra/llrender/llfontvertexbuffer.cpp | 24 +++------------ indra/llrender/llfontvertexbuffer.h | 12 ++++---- .../shaders/class1/interface/uiF.glsl | 21 ++++++++++---- indra/newview/llviewershadermgr.cpp | 10 +++---- 5 files changed, 42 insertions(+), 54 deletions(-) diff --git a/indra/llrender/llfontgl.cpp b/indra/llrender/llfontgl.cpp index c04d327e3d..debfe1e83a 100644 --- a/indra/llrender/llfontgl.cpp +++ b/indra/llrender/llfontgl.cpp @@ -449,35 +449,30 @@ S32 LLFontGL::render(const LLWString &wstr, S32 begin_offset, F32 x, F32 y, cons // bind the atlas of whichever face produced each glyph. Track the current // (face, atlas) pair as we walk glyphs and flip on transitions. The // initial (face, cache, inv_width, inv_height) captures are deferred to - // the first glyph; the head's primary cache is used as a fallback for - // shadow-uniform setup below. + // the first glyph. const LLFontFace* current_face = nullptr; const LLFontBitmapCache* font_bitmap_cache = mFontFreetype->getBitmapCache(); F32 inv_width = font_bitmap_cache ? 1.f / font_bitmap_cache->getBitmapWidth() : 0.f; F32 inv_height = font_bitmap_cache ? 1.f / font_bitmap_cache->getBitmapHeight() : 0.f; - // Shader-shadow uniforms are pushed once before pass A starts and reset - // to passthrough (shadowMode = 0) before pass B's foreground emission. - // Captured into mShadowBufferList only by virtue of the shadow geometry - // they're set for; LLFontVertexBuffer::renderBuffers re-pushes the same - // values on cache replay since LLVertexBufferData doesn't capture - // uniforms. Skipped when sEnableShaderShadow is off (legacy multi-quad - // emission still drives shadow geometry — shader stays at shadowMode=0). + // shadowMode is pushed once before pass A starts and reset to + // passthrough (0) before pass B's foreground emission. It is the only + // shadow uniform by design: per-pass constant, so the captured-buffer + // replay (LLFontVertexBuffer::renderBuffers re-pushes it; LLVertexBufferData + // doesn't capture uniforms) stays correct. Everything that varies per + // batch in a mixed-atlas string — texel size of the bound atlas, alpha + // channel layout — derives from the bound texture inside uiF.glsl + // (textureSize + the RG-swizzle/.a sampling), so glyphs from + // differently-sized head and fallback atlases all shadow correctly. + // Skipped when sEnableShaderShadow is off (legacy multi-quad emission + // still drives shadow geometry — shader stays at shadowMode=0). const bool push_shader_shadow_uniforms = sEnableShaderShadow && (shadow != NO_SHADOW) && LLGLSLShader::sCurBoundShaderPtr; static const LLStaticHashedString sShadowMode("shadowMode"); - static const LLStaticHashedString sAtlasTexelSize("atlasTexelSize"); - static const LLStaticHashedString sGrayscaleAtlas("grayscaleAtlas"); if (push_shader_shadow_uniforms) { const int mode = (shadow == DROP_SHADOW) ? 1 : 2; // SOFT LLGLSLShader::sCurBoundShaderPtr->uniform1i(sShadowMode, mode); - LLGLSLShader::sCurBoundShaderPtr->uniform2f(sAtlasTexelSize, inv_width, inv_height); - // Per-batch atlas type would be needed for correct mixed-atlas shadow - // sampling; for now treat all shadowed strings as grayscale text. - // Mixed text+emoji shadows may sample the wrong channel — acceptable - // limitation while sEnableShaderShadow is opt-in. - LLGLSLShader::sCurBoundShaderPtr->uniform1i(sGrayscaleAtlas, 1); } bool draw_ellipses = false; diff --git a/indra/llrender/llfontvertexbuffer.cpp b/indra/llrender/llfontvertexbuffer.cpp index 00e934248c..6decdc80d9 100644 --- a/indra/llrender/llfontvertexbuffer.cpp +++ b/indra/llrender/llfontvertexbuffer.cpp @@ -254,23 +254,11 @@ void LLFontVertexBuffer::genBuffers( // Snapshot shader-shadow state for the cache. The static flag could flip // between gen and replay, so we cache it alongside the captured streams - // and use the snapshot in renderBuffers. + // and use the snapshot in renderBuffers. shadowMode is the only shadow + // uniform — atlas texel size and channel layout derive from the bound + // texture inside uiF.glsl, so per-batch texture rebinds on replay carry + // everything that varies. mLastUsedShaderShadow = LLFontGL::sEnableShaderShadow && (shadow != LLFontGL::NO_SHADOW); - mLastAtlasTexelW = 0.f; - mLastAtlasTexelH = 0.f; - if (mLastUsedShaderShadow) - { - if (const LLFontFreetype* face = fontp->getFontFreetype()) - { - if (const LLFontBitmapCache* cache = face->getFontBitmapCache()) - { - const F32 w = (F32)cache->getBitmapWidth(); - const F32 h = (F32)cache->getBitmapHeight(); - mLastAtlasTexelW = (w > 0.f) ? 1.f / w : 0.f; - mLastAtlasTexelH = (h > 0.f) ? 1.f / h : 0.f; - } - } - } // Two-pass capture: pass A (shadow geometry) lands in mShadowBufferList, // pass B (foreground geometry) lands in mForegroundBufferList. The @@ -440,15 +428,11 @@ void LLFontVertexBuffer::renderBuffers() // the original interleaved-per-glyph emission's net visual stacking — shadow // contributions sit beneath glyph foregrounds rather than between them. static const LLStaticHashedString sShadowMode("shadowMode"); - static const LLStaticHashedString sAtlasTexelSize("atlasTexelSize"); - static const LLStaticHashedString sGrayscaleAtlas("grayscaleAtlas"); if (mLastUsedShaderShadow && LLGLSLShader::sCurBoundShaderPtr) { const int mode = (mLastShadow == LLFontGL::DROP_SHADOW) ? 1 : 2; // SOFT LLGLSLShader::sCurBoundShaderPtr->uniform1i(sShadowMode, mode); - LLGLSLShader::sCurBoundShaderPtr->uniform2f(sAtlasTexelSize, mLastAtlasTexelW, mLastAtlasTexelH); - LLGLSLShader::sCurBoundShaderPtr->uniform1i(sGrayscaleAtlas, 1); } for (LLVertexBufferData& buffer : mShadowBufferList) { diff --git a/indra/llrender/llfontvertexbuffer.h b/indra/llrender/llfontvertexbuffer.h index 3f911699a5..f22d718738 100644 --- a/indra/llrender/llfontvertexbuffer.h +++ b/indra/llrender/llfontvertexbuffer.h @@ -127,13 +127,13 @@ class LLFontVertexBuffer // Snapshot of LLFontGL::sEnableShaderShadow at genBuffers time. Required // because LLVertexBufferData doesn't capture shader uniforms; renderBuffers - // must re-push shadowMode / atlasTexelSize before replaying mShadowBufferList - // and reset shadowMode = 0 before mForegroundBufferList. If the static flag - // flips between gen and replay, the captured stream still replays with the - // shader state it was built for. + // must re-push shadowMode before replaying mShadowBufferList and reset + // shadowMode = 0 before mForegroundBufferList. If the static flag flips + // between gen and replay, the captured stream still replays with the + // shader state it was built for. shadowMode is the only shadow uniform — + // atlas texel size / channel layout derive from the bound texture in + // uiF.glsl, which the captured streams DO rebind per batch. bool mLastUsedShaderShadow = false; - F32 mLastAtlasTexelW = 0.f; - F32 mLastAtlasTexelH = 0.f; S32 mChars = 0; const LLFontGL *mLastFont = nullptr; S32 mLastOffset = 0; diff --git a/indra/newview/app_settings/shaders/class1/interface/uiF.glsl b/indra/newview/app_settings/shaders/class1/interface/uiF.glsl index 4db8c3629f..aadf8cc1df 100644 --- a/indra/newview/app_settings/shaders/class1/interface/uiF.glsl +++ b/indra/newview/app_settings/shaders/class1/interface/uiF.glsl @@ -27,12 +27,14 @@ out vec4 frag_color; uniform sampler2D diffuseMap; -// Shadow shader path. Default values (shadowMode == 0, others ignored) make -// every non-text UI surface and every NO_SHADOW text glyph hit the early-return +// Shadow shader path. The default value (shadowMode == 0) makes every +// non-text UI surface and every NO_SHADOW text glyph hit the early-return // branch below — bytewise equivalent to the pre-change shader. Only text -// rendering with active shadow geometry sets shadowMode > 0 and pushes -// atlasTexelSize before its draw range. -uniform vec2 atlasTexelSize; // 1.0 / vec2(atlas_w, atlas_h) +// rendering with active shadow geometry sets shadowMode > 0 before its +// draw range. shadowMode is deliberately the ONLY shadow uniform: per-pass +// constant, so captured vertex buffers (which replay texture binds but not +// uniforms) stay correct. Everything that varies per batch — atlas texel +// size, alpha channel layout — derives from the bound texture itself. uniform int shadowMode; // 0 = passthrough, 1 = drop, 2 = soft in vec2 vary_texcoord0; @@ -74,6 +76,15 @@ void main() // background gives total_alpha = 1 − Π(1 − α_i). Using max() instead // collapses to uniform vertex_color.a wherever any sample hits, which // produces a blocky outline-shaped shadow with no gradient. + // + // Atlas texel size comes from the bound texture, not a uniform: one + // shadowed string can batch glyphs from differently-sized atlases (the + // head face's sheets vs a fallback emoji face's), and a per-string + // uniform sized off the head atlas put the fallback glyphs' taps at the + // wrong distance. textureSize always reflects the atlas this draw + // actually samples — including on captured-buffer replay, which rebinds + // textures per batch. + vec2 atlasTexelSize = 1.0 / vec2(textureSize(diffuseMap, 0)); float vc_a = vertex_color.a; float p; if (shadowMode == 1) diff --git a/indra/newview/llviewershadermgr.cpp b/indra/newview/llviewershadermgr.cpp index dea4bd25cb..69d981c7c1 100644 --- a/indra/newview/llviewershadermgr.cpp +++ b/indra/newview/llviewershadermgr.cpp @@ -3332,17 +3332,15 @@ bool LLViewerShaderMgr::loadShadersInterface() success = gUIProgram.createShader(); if (success) { - // Initialize the shadow-path uniforms to passthrough so non-text UI + // Initialize the shadow-path uniform to passthrough so non-text UI // and NO_SHADOW text take the early-return branch in uiF.glsl. GLSL - // already zero-initializes uniforms, but pushing explicit defaults + // already zero-initializes uniforms, but pushing an explicit default // documents the contract and protects against driver quirks. + // shadowMode is the shader's only shadow uniform — atlas texel size + // and channel layout derive from the bound texture in uiF.glsl. static LLStaticHashedString sShadowMode("shadowMode"); - static LLStaticHashedString sGrayscaleAtlas("grayscaleAtlas"); - static LLStaticHashedString sAtlasTexelSize("atlasTexelSize"); gUIProgram.bind(); gUIProgram.uniform1i(sShadowMode, 0); - gUIProgram.uniform1i(sGrayscaleAtlas, 1); - gUIProgram.uniform2f(sAtlasTexelSize, 0.f, 0.f); gUIProgram.unbind(); } } From 9d019a75e098f5a73ecb42ba516c86b52c4ce97e Mon Sep 17 00:00:00 2001 From: Rye Date: Thu, 11 Jun 2026 01:31:42 -0400 Subject: [PATCH 5/5] Address PR #302 review: widen gen stamp, transactional reload restore, touch guards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit findings, all four taken: - LLFontGL::getCacheGeneration (and the mLastFontCacheGen consumers in LLFontVertexBuffer / LLFontWidthBuffer) widen to U64. The stamp is a sum of per-face S32 generations; an S32 accumulator could in principle overflow (signed UB) and break the strictly-increases contract. The per-face generations stay S32 — they're raw draws from the global counter, summed into the wide accumulator. - LLFontRegistry::reload snapshots the full parse-time state (font map incl. templates, size tables, family metadata, applied-overrides LLSD) and restores it wholesale when parseFontInfo fails. Previously only the heads were re-seated, leaving nameToSize / getAvailableFamilies / uncached getFont running against an emptied registry until the next successful reload. The map copy is safe: a failed parse only adds nullptr template placeholders, so the restore can't double-own or leak. - LLFontBitmapCache::getImageRaw/getImageGL no longer touchSheet before the released-slot check. releaseSheet zeroes the slot's last-used time and recycling relies on it staying zero; a null read of a released sheet shouldn't make the slot look recently used. - llfontfreetype_test test<6> purges the face-owned glyph entries referencing sheet 0 before its direct releaseSheet, matching the purge-before-release contract the production sweep maintains (and the idiom tests 8/9 already use). Co-Authored-By: Claude Fable 5 (1M context) --- indra/llrender/llfontbitmapcache.cpp | 11 ++++++ indra/llrender/llfontgl.cpp | 12 ++++--- indra/llrender/llfontgl.h | 5 ++- indra/llrender/llfontregistry.cpp | 36 ++++++++++++++++---- indra/llrender/llfontvertexbuffer.h | 7 ++-- indra/llrender/tests/llfontfreetype_test.cpp | 16 +++++++++ 6 files changed, 73 insertions(+), 14 deletions(-) diff --git a/indra/llrender/llfontbitmapcache.cpp b/indra/llrender/llfontbitmapcache.cpp index dc5390fd49..57f5026505 100644 --- a/indra/llrender/llfontbitmapcache.cpp +++ b/indra/llrender/llfontbitmapcache.cpp @@ -78,6 +78,13 @@ LLImageRaw *LLFontBitmapCache::getImageRaw(EFontGlyphType bitmap_type, U32 bitma if (bitmap_type >= EFontGlyphType::Count || bitmap_num >= mImageRawVec[bitmap_idx].size()) return nullptr; + // Released slots must NOT be touched: releaseSheet zeroes the slot's + // last-used time and the eviction sweep / recycling logic rely on it + // staying zero. A diagnostic or defensive probe of a released sheet + // returning null shouldn't make the slot look recently used. + if (mImageRawVec[bitmap_idx][bitmap_num].isNull()) + return nullptr; + touchSheet(bitmap_type, bitmap_num); return mImageRawVec[bitmap_idx][bitmap_num]; } @@ -88,6 +95,10 @@ LLImageGL *LLFontBitmapCache::getImageGL(EFontGlyphType bitmap_type, U32 bitmap_ if (bitmap_type >= EFontGlyphType::Count || bitmap_num >= mImageGLVec[bitmap_idx].size()) return nullptr; + // Same released-slot guard as getImageRaw — null reads don't touch. + if (mImageGLVec[bitmap_idx][bitmap_num].isNull()) + return nullptr; + touchSheet(bitmap_type, bitmap_num); return mImageGLVec[bitmap_idx][bitmap_num]; } diff --git a/indra/llrender/llfontgl.cpp b/indra/llrender/llfontgl.cpp index debfe1e83a..ba9d738281 100644 --- a/indra/llrender/llfontgl.cpp +++ b/indra/llrender/llfontgl.cpp @@ -183,7 +183,7 @@ S32 LLFontGL::getNumFaces(const std::string& filename) return LLFontFreetype::getNumFaces(filename); } -S32 LLFontGL::getCacheGeneration() const +U64 LLFontGL::getCacheGeneration() const { // Every render() call may sample from this font's head atlas AND from // each fallback face's atlas (e.g. emoji glyphs in an otherwise @@ -209,15 +209,19 @@ S32 LLFontGL::getCacheGeneration() const const LLFontFreetype* ft = mFontFreetype.get(); if (!ft) return 0; - S32 gen = 0; + // U64 accumulator: components are non-negative S32s drawn from the + // monotonic global counter, so the unsigned 64-bit sum can't overflow + // within any reachable session and the comparison contract in the + // vertex/width buffer caches stays exact. + U64 gen = 0; if (const LLFontBitmapCache* cache = ft->getFontBitmapCache()) - gen += cache->getCacheGeneration(); + gen += (U64)cache->getCacheGeneration(); for (const auto& fb : ft->getFallbackFonts()) { if (fb.first) { if (const LLFontBitmapCache* cache = fb.first->getFontBitmapCache()) - gen += cache->getCacheGeneration(); + gen += (U64)cache->getCacheGeneration(); } } return gen; diff --git a/indra/llrender/llfontgl.h b/indra/llrender/llfontgl.h index 36e048ecb0..9d7fcb639b 100644 --- a/indra/llrender/llfontgl.h +++ b/indra/llrender/llfontgl.h @@ -93,7 +93,10 @@ class LLFontGL bool loadFace(const std::string& filename, F32 point_size, const F32 vert_dpi, const F32 horz_dpi, bool is_fallback, S32 face_n, EFontHinting hinting, S32 flags, const LLFontVarAxes& var_axes = {}); S32 getNumFaces(const std::string& filename); - S32 getCacheGeneration() const; + // U64: the stamp is a SUM of per-face S32 generations; a 64-bit unsigned + // accumulator keeps the strictly-increases contract without any overflow + // horizon a real session could reach (an S32 sum could wrap UB-style). + U64 getCacheGeneration() const; const LLFontFreetype* getFontFreetype() const { return mFontFreetype.get(); } S32 render(const LLWString &text, S32 begin_offset, diff --git a/indra/llrender/llfontregistry.cpp b/indra/llrender/llfontregistry.cpp index 83c7079e27..044bfd6134 100644 --- a/indra/llrender/llfontregistry.cpp +++ b/indra/llrender/llfontregistry.cpp @@ -1961,6 +1961,23 @@ bool LLFontRegistry::reload(const LLSD& font_overrides) auto pinned_old_fallbacks = std::move(mFallbackInstanceCache); mFallbackInstanceCache.clear(); + // Snapshot the full parse-time state alongside the heads. parseFontInfo + // can fail partway (malformed fonts.xml / override file) AFTER the wipes + // below have run; restoring only the heads would leave nameToSize, + // getAvailableFamilies, and uncached getFont calls running against an + // emptied registry until the next successful reload. mFontMap values are + // owned raw pointers, but the copy is safe: a failed parse only ever + // adds nullptr template placeholders (mergeFontEntry), so restoring the + // snapshot over the partial map can't double-own or leak a live + // LLFontGL, and on success the snapshot copies die without deleting. + auto saved_font_map = mFontMap; + auto saved_font_sizes = mFontSizes; + auto saved_family_sizes = mFamilySizes; + auto saved_family_uses = mFamilyUses; + auto saved_inherit_flags = mInheritFlags; + auto saved_family_meta = mFamilyMeta; + auto saved_last_overrides = mLastFontOverrides; + // Wipe parse-time state. mFontSizes.clear(); mFamilySizes.clear(); @@ -1977,13 +1994,20 @@ bool LLFontRegistry::reload(const LLSD& font_overrides) if (!parseFontInfo("fonts.xml", font_overrides)) { - LL_WARNS() << "Font reload: parseFontInfo failed; restoring previous fallback cache" << LL_ENDL; + LL_WARNS() << "Font reload: parseFontInfo failed; restoring previous registry state" << LL_ENDL; mFallbackInstanceCache = std::move(pinned_old_fallbacks); - // Re-seat the snapshot heads so widgets continue rendering - // against their previous freetype state. storeFont guards against - // anything the partial parse left in a colliding slot. - for (const auto& [desc, head] : heads) - storeFont(desc, head); + // Restore the registry wholesale — templates + heads, size tables, + // family metadata, and the applied-overrides snapshot — so every + // lookup path behaves exactly as before the attempt. The partial + // parse's nullptr placeholders in mFontMap are discarded by the + // assignment (nothing non-null to free; see the snapshot note). + mFontMap = std::move(saved_font_map); + mFontSizes = std::move(saved_font_sizes); + mFamilySizes = std::move(saved_family_sizes); + mFamilyUses = std::move(saved_family_uses); + mInheritFlags = std::move(saved_inherit_flags); + mFamilyMeta = std::move(saved_family_meta); + mLastFontOverrides = saved_last_overrides; return false; } diff --git a/indra/llrender/llfontvertexbuffer.h b/indra/llrender/llfontvertexbuffer.h index f22d718738..127b79e73e 100644 --- a/indra/llrender/llfontvertexbuffer.h +++ b/indra/llrender/llfontvertexbuffer.h @@ -158,7 +158,8 @@ class LLFontVertexBuffer // Adding new characters to bitmap cache can alter value from getBitmapWidth(); // which alters whole string. So rerender when new characters were added to cache. - S32 mLastFontCacheGen = 0; + // U64 to match LLFontGL::getCacheGeneration's summed stamp. + U64 mLastFontCacheGen = 0; static bool sEnableBufferCollection; @@ -206,8 +207,8 @@ class LLFontWidthBuffer F32 mLastHorizDPI = 0.f; S32 mLastResGeneration = 0; - // Cache generation tracking - S32 mLastFontCacheGen = 0; + // Cache generation tracking. U64 to match LLFontGL::getCacheGeneration. + U64 mLastFontCacheGen = 0; static bool sEnableBufferCollection; }; diff --git a/indra/llrender/tests/llfontfreetype_test.cpp b/indra/llrender/tests/llfontfreetype_test.cpp index 64eee94bab..bdbffa71d1 100644 --- a/indra/llrender/tests/llfontfreetype_test.cpp +++ b/indra/llrender/tests/llfontfreetype_test.cpp @@ -1009,6 +1009,22 @@ namespace tut cache->getNumBitmaps(EFontGlyphType::Grayscale), 1u); const S32 gen_before = cache->getCacheGeneration(); + // Honor the purge-before-release contract the production sweep + // maintains (LLFontFace::collectGarbage): delete the face-owned + // glyph entries referencing sheet 0 before releasing it. Without + // this, 'A''s stale entry would survive pointing into the slot the + // next allocation recycles — exactly the state recycling forbids. + ft->getFontFace()->erase_glyph_entries( + [](const LLFontGlyphInfo* gi) + { + for (U8 p = 0; p < gi->mPhaseCount; ++p) + { + const auto& e = gi->mPhaseSlots[p].mBitmapEntry; + if (e.first == EFontGlyphType::Grayscale && e.second == 0) + return true; + } + return false; + }); cache->releaseSheet(EFontGlyphType::Grayscale, 0); ensure("sheet 0 reports released", cache->isSheetReleased(EFontGlyphType::Grayscale, 0));