Fix text rendering corruption from mid-render atlas bind stomp; font cache hardening

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];
 }
@@ -102,12 +113,13 @@ bool LLFontBitmapCache::nextOpenPos(S32 width, S32 height, S32& pos_x, S32& pos_
 
     const U32 bitmap_idx = static_cast<U32>(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,22 +179,55 @@ 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<S32>(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<S32>(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<U32>(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);
         }
+        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));
-        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 +
@@ -199,7 +244,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<U32>(mCurrentSheet[bitmap_idx]);
 
     mCurrentOffsetX[bitmap_idx] += width + 4;
     // Track tallest glyph placed in the current row so the next Y advance
@@ -295,6 +340,7 @@ void LLFontBitmapCache::reset()
         mCurrentOffsetX[idx] = 4;
         mCurrentOffsetY[idx] = 4;
         mCurrentRowMaxHeight[idx] = 0;
+        mCurrentSheet[idx] = -1;
     }
 
     mBitmapWidth = 0;

indra/llrender/llfontbitmapcache.h

@@ -82,22 +82,24 @@ 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.
-    // 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
@@ -137,6 +139,11 @@ class LLFontBitmapCache
     // glyph heights (text + tall color emoji bitmaps) don't have later
     // rows overwriting earlier rows.
     S32 mCurrentRowMaxHeight[static_cast<U32>(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<U32>(EFontGlyphType::Count)] = { -1, -1 };
     S32 mMaxCharWidth = 0;
     S32 mMaxCharHeight = 0;
     // Globally-unique generation. Each new LLFontBitmapCache instance and

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
 
@@ -247,21 +248,25 @@ 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);
     auto iter = std::find_if(range.first, range.second,
         [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()
@@ -273,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<U32>(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<U32>(EFontGlyphType::Count); ++t)
+    {
+        const EFontGlyphType type = static_cast<EFontGlyphType>(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;

indra/llrender/llfontface.h

@@ -220,13 +220,33 @@ 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.
     template<typename Pred>
     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.
@@ -287,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;