From 3fa253c15dcfc07a58bee458fdc78c11616144d9 Mon Sep 17 00:00:00 2001 From: Rye Date: Tue, 13 Oct 2026 08:33:49 -0400 Subject: [PATCH 1/5] llrender's font registry tests check what they say they check Old test bugs TM22's idiom pass found (bb3f752d28), fixed. - llfontregistry: - "operator< orders descriptors that differ only in style" gave both the same size, so whether style is compared before size was never tested; the lower style now has the later size; - getMatchingFontDesc's normalization was never queried: a BOLD style and a "SansSerifBold" query now need it. The comment's "SansSerifHuge" could not work: normalizing moves Huge into the size, so the query names a head, not a template; - a second resolveFontReferences promised to keep the size table and the source-family tags and checked only the file count; it checks both now; - the sweep case's CHECK(true) is gone: the case passes by returning, and now checks the map holds the entry it walks; - a family that s itself was tested twice, once under a comment naming a 'self' alias there is none of; the second is gone; - templateFor() results and getFontFiles()[0] are required before they are read. - llfontregistry_gl: - each case requires its font's freetype before reading through it, and case 1's existing REQUIRE moved ahead of the first read; - after an eviction, a glyph's entry naming no sheet (Unspecified, -1) passed the "not released" checks; the entry is checked to name a Color sheet first. Each new or corrected check fails under a small change to the test's own input or expectation, at the check and not by crashing. Co-Authored-By: Claude Opus 5.5 --- .../llrender/tests/llfontregistry_gl_test.cpp | 60 ++++--- indra/llrender/tests/llfontregistry_test.cpp | 160 ++++++++++++------ 2 files changed, 149 insertions(+), 71 deletions(-) diff --git a/indra/llrender/tests/llfontregistry_gl_test.cpp b/indra/llrender/tests/llfontregistry_gl_test.cpp index 7e96537ebe..2ff298cbd0 100644 --- a/indra/llrender/tests/llfontregistry_gl_test.cpp +++ b/indra/llrender/tests/llfontregistry_gl_test.cpp @@ -165,12 +165,12 @@ namespace tut LLFontGL* font = reg.getFont(LLFontDescriptor("Inter", "Small", 0)); REQUIRE(font != nullptr); - - font->generateASCIIglyphs(); - + // generateASCIIglyphs rasterizes through the freetype. const LLFontFreetype* ft = font->getFontFreetype(); REQUIRE(ft != nullptr); + font->generateASCIIglyphs(); + const LLFontBitmapCache* cache = ft->getFontBitmapCache(); REQUIRE(cache != nullptr); CHECK(cache->getNumBitmaps(EFontGlyphType::Grayscale) >= 1u); @@ -218,12 +218,17 @@ namespace tut LLFontGL* font = reg.getFont(LLFontDescriptor("Inter", "Small", 0)); REQUIRE(font != nullptr); + const LLFontFreetype* destroy_ft = font->getFontFreetype(); + REQUIRE(destroy_ft != nullptr); font->generateASCIIglyphs(); - const LLFontBitmapCache* cache = font->getFontFreetype()->getFontBitmapCache(); + const LLFontBitmapCache* cache = destroy_ft->getFontBitmapCache(); + REQUIRE(cache != nullptr); REQUIRE(cache->getNumBitmaps(EFontGlyphType::Grayscale) >= 1u); - CHECK(cache->getImageGL(EFontGlyphType::Grayscale, 0)->getTexName() != 0); + LLImageGL* page = cache->getImageGL(EFontGlyphType::Grayscale, 0); + REQUIRE(page != nullptr); + CHECK(page->getTexName() != 0); reg.destroyGL(); @@ -379,6 +384,9 @@ namespace tut resolve(); LLFontGL* font = reg.getFont(LLFontDescriptor("Inter", "Large", 0)); REQUIRE(font != nullptr); + // getAscenderHeight reads through the freetype too. + const LLFontFreetype* head_ft = font->getFontFreetype(); + REQUIRE(head_ft != nullptr); // Head is Inter at chain's 16pt. F32 head_ascender = font->getAscenderHeight(); @@ -386,7 +394,7 @@ namespace tut // Find the DejaVu fallback in the chain. Its mPointSize should be // the per-family pin 8.0, NOT the chain's 16.0. - const auto& fallbacks = font->getFontFreetype()->getFallbackFonts(); + const auto& fallbacks = head_ft->getFallbackFonts(); const LLFontFreetype* dejavu_fallback = nullptr; for (const auto& fb : fallbacks) { @@ -549,9 +557,12 @@ namespace tut LLFontGL* font = reg.getFont(LLFontDescriptor("Inter", "Small", 0)); REQUIRE(font != nullptr); + const LLFontFreetype* sweep_ft = font->getFontFreetype(); + REQUIRE(sweep_ft != nullptr); font->generateASCIIglyphs(); - const LLFontBitmapCache* cache = font->getFontFreetype()->getFontBitmapCache(); + const LLFontBitmapCache* cache = sweep_ft->getFontBitmapCache(); + REQUIRE(cache != nullptr); const U32 pages_before = cache->getNumBitmaps(EFontGlyphType::Grayscale); CHECK(pages_before >= 1u); @@ -618,19 +629,23 @@ namespace tut LLFontGL* head_b = reg.getFont(LLFontDescriptor("HeadB", "Small", 0)); REQUIRE(head_a != nullptr); REQUIRE(head_b != nullptr); + const LLFontFreetype* ft_a = head_a->getFontFreetype(); + const LLFontFreetype* ft_b = head_b->getFontFreetype(); + REQUIRE(ft_a != nullptr); + REQUIRE(ft_b != nullptr); // Locate the Noto-COLRv1 fallback inside each head's chain. - auto find_emoji_fallback = [](LLFontGL* head) -> const LLFontFreetype* + auto find_emoji_fallback = [](const LLFontFreetype* head_ft) -> const LLFontFreetype* { - for (const auto& fb : head->getFontFreetype()->getFallbackFonts()) + for (const auto& fb : head_ft->getFallbackFonts()) { if (fb.first && fb.first->getName().find("Noto-COLRv1") != std::string::npos) return fb.first.get(); } return nullptr; }; - const LLFontFreetype* emoji_a = find_emoji_fallback(head_a); - const LLFontFreetype* emoji_b = find_emoji_fallback(head_b); + const LLFontFreetype* emoji_a = find_emoji_fallback(ft_a); + const LLFontFreetype* emoji_b = find_emoji_fallback(ft_b); REQUIRE(emoji_a != nullptr); REQUIRE(emoji_b != nullptr); UNSCOPED_INFO("siblings share the fallback freetype instance " @@ -642,16 +657,16 @@ namespace tut // walk picks Noto-COLRv1 → face's findGlyphInfo allocates and // caches on the FIRST call; the SECOND head hits the face cache. constexpr llwchar kFire = 0x1F525; - const LLFontGlyphInfo* gi_a = head_a->getFontFreetype()->getGlyphInfo( - kFire, EFontGlyphType::Color); - const LLFontGlyphInfo* gi_b = head_b->getFontFreetype()->getGlyphInfo( - kFire, EFontGlyphType::Color); + const LLFontGlyphInfo* gi_a = ft_a->getGlyphInfo(kFire, EFontGlyphType::Color); + const LLFontGlyphInfo* gi_b = ft_b->getGlyphInfo(kFire, EFontGlyphType::Color); REQUIRE(gi_a != nullptr); REQUIRE(gi_b != nullptr); // Face-level dedup → both heads see the same face-owned entry. UNSCOPED_INFO("face dedup returns the same entry to both heads"); REQUIRE(gi_a == gi_b); + // Slot 0 is always there: mPhaseSlots is a fixed array, and every + // rasterized glyph fills its first mPhaseCount (1 or kNumPhases). const auto target = gi_a->mPhaseSlots[0].mBitmapEntry; { INFO("emoji glyph references a real Color sheet"); @@ -687,14 +702,21 @@ namespace tut // 1. not crash on a freed face entry, // 2. produce a non-null glyph entry, // 3. land that entry on a LIVE sheet (not the released one). - const LLFontGlyphInfo* gi_a2 = head_a->getFontFreetype()->getGlyphInfo( - kFire, EFontGlyphType::Color); - const LLFontGlyphInfo* gi_b2 = head_b->getFontFreetype()->getGlyphInfo( - kFire, EFontGlyphType::Color); + const LLFontGlyphInfo* gi_a2 = ft_a->getGlyphInfo(kFire, EFontGlyphType::Color); + const LLFontGlyphInfo* gi_b2 = ft_b->getGlyphInfo(kFire, EFontGlyphType::Color); REQUIRE(gi_a2 != nullptr); REQUIRE(gi_b2 != nullptr); const auto post_a = gi_a2->mPhaseSlots[0].mBitmapEntry; const auto post_b = gi_b2->mPhaseSlots[0].mBitmapEntry; + // isSheetReleased is false for a sheet that does not exist, so an + // entry naming none, (Unspecified, -1), would pass the checks below. + { + INFO("re-rasterized glyphs reference real Color sheets"); + CHECK(post_a.first == EFontGlyphType::Color); + CHECK(post_a.second >= 0); + CHECK(post_b.first == EFontGlyphType::Color); + CHECK(post_b.second >= 0); + } CHECK_FALSE(cache->isSheetReleased(post_a.first, static_cast(post_a.second))); CHECK_FALSE(cache->isSheetReleased(post_b.first, static_cast(post_b.second))); } diff --git a/indra/llrender/tests/llfontregistry_test.cpp b/indra/llrender/tests/llfontregistry_test.cpp index ec1fcc68b5..1eaa882f76 100644 --- a/indra/llrender/tests/llfontregistry_test.cpp +++ b/indra/llrender/tests/llfontregistry_test.cpp @@ -249,11 +249,13 @@ namespace tut } // operator< orders lexicographically by (name, style, size). Style - // comparisons happen ahead of size — verify by holding name fixed. - TEST_CASE_METHOD(llfontregistry_data, "LLFontRegistry: operator< orders descriptors that differ only in style", "[LLFontRegistry]") + // comparisons happen ahead of size — verify by holding name fixed and + // giving the lower style the later size: "Small" sorts after "Huge", + // so comparing size first would put b ahead of a. + TEST_CASE_METHOD(llfontregistry_data, "LLFontRegistry: operator< compares style before size", "[LLFontRegistry]") { - LLFontDescriptor a("SansSerif", "Medium", 0); - LLFontDescriptor b("SansSerif", "Medium", LLFontGL::BOLD); + LLFontDescriptor a("SansSerif", "Small", 0); + LLFontDescriptor b("SansSerif", "Huge", LLFontGL::BOLD); CHECK(a < b); UNSCOPED_INFO("strict weak ordering"); CHECK_FALSE(b < a); @@ -312,6 +314,8 @@ namespace tut const LLFontDescriptor* bold = templateFor("Inter", LLFontGL::BOLD); REQUIRE(normal != nullptr); REQUIRE(bold != nullptr); + REQUIRE_FALSE(normal->getFontFiles().empty()); + REQUIRE_FALSE(bold->getFontFiles().empty()); CHECK(normal->getFontFiles()[0].mVarAxes.wght_set); CHECK(normal->getFontFiles()[0].mVarAxes.wght == 400.f); CHECK((S32)normal->getFontFiles()[0].mHinting == (S32)EFontHinting::LIGHT); @@ -356,6 +360,7 @@ namespace tut CHECK(loadXml(xml)); const LLFontDescriptor* d = templateFor("Mono", LLFontGL::NORMAL); REQUIRE(d != nullptr); + REQUIRE_FALSE(d->getFontFiles().empty()); CHECK(d->getFontFiles()[0].mMonospaceLigatures); } @@ -373,10 +378,16 @@ namespace tut " " ""; CHECK(loadXml(xml)); + const LLFontDescriptor* cjk = templateFor("CJK"); + const LLFontDescriptor* mono = templateFor("Mono"); + REQUIRE(cjk != nullptr); + REQUIRE(mono != nullptr); + REQUIRE_FALSE(cjk->getFontFiles().empty()); + REQUIRE_FALSE(mono->getFontFiles().empty()); UNSCOPED_INFO("family-level load_collection inherited"); - CHECK(templateFor("CJK")->getFontFiles()[0].mLoadCollection); + CHECK(cjk->getFontFiles()[0].mLoadCollection); UNSCOPED_INFO("file-level load_collection works alone"); - CHECK(templateFor("Mono")->getFontFiles()[0].mLoadCollection); + CHECK(mono->getFontFiles()[0].mLoadCollection); } // flags="bold" forces LLFontGL::BOLD into the file's flags so that @@ -395,6 +406,7 @@ namespace tut CHECK(loadXml(xml)); const LLFontDescriptor* d = templateFor("Inter", LLFontGL::BOLD); REQUIRE(d != nullptr); + REQUIRE_FALSE(d->getFontFiles().empty()); CHECK((d->getFontFiles()[0].mFlags & LLFontGL::BOLD) == (S32)LLFontGL::BOLD); } @@ -411,7 +423,10 @@ namespace tut " " ""; CHECK(loadXml(xml)); - const auto& cf = templateFor("Emoji")->getFontFiles()[0].CharFunctor; + const LLFontDescriptor* d = templateFor("Emoji"); + REQUIRE(d != nullptr); + REQUIRE_FALSE(d->getFontFiles().empty()); + const auto& cf = d->getFontFiles()[0].CharFunctor; REQUIRE(static_cast(cf)); UNSCOPED_INFO("U+2600 inside range"); CHECK(cf(0x2600)); @@ -440,7 +455,10 @@ namespace tut " " ""; CHECK(loadXml(xml)); - const auto& cf = templateFor("Frag")->getFontFiles()[0].CharFunctor; + const LLFontDescriptor* d = templateFor("Frag"); + REQUIRE(d != nullptr); + REQUIRE_FALSE(d->getFontFiles().empty()); + const auto& cf = d->getFontFiles()[0].CharFunctor; REQUIRE(static_cast(cf)); CHECK(cf(0x2700)); } @@ -458,7 +476,10 @@ namespace tut " " ""; CHECK(loadXml(xml)); - CHECK(templateFor("X")->getFontFiles()[0].mSizeDelta == -1.5f); + const LLFontDescriptor* d = templateFor("X"); + REQUIRE(d != nullptr); + REQUIRE_FALSE(d->getFontFiles().empty()); + CHECK(d->getFontFiles()[0].mSizeDelta == -1.5f); } // Family metadata: label / user_selectable / monospace flow into @@ -619,7 +640,8 @@ namespace tut CHECK(countFile("A", "C.woff2") == 1); } - // Self-reference logs and is skipped. + // A naming the family itself ( inside A) logs + // and is skipped: the chain holds only A's own file, once. TEST_CASE_METHOD(llfontregistry_data, "LLFontRegistry: a of the family itself is skipped", "[LLFontRegistry]") { const char* xml = @@ -728,11 +750,12 @@ namespace tut // mInheritFlags are consumed at the end of the first call. Per- // family sizes (mFamilySizes) and per-file source-family tags // survive — those are runtime data, not parse state. - TEST_CASE_METHOD(llfontregistry_data, "LLFontRegistry: a second resolveFontReferences call adds nothing", "[LLFontRegistry]") + TEST_CASE_METHOD(llfontregistry_data, "LLFontRegistry: a second resolveFontReferences adds nothing and keeps size and source", "[LLFontRegistry]") { const char* xml = "" " " + " " " " " " " " @@ -746,6 +769,16 @@ namespace tut resolve(); // second call must not re-append auto second = fileNames("A"); CHECK((S32)first.size() == (S32)second.size()); + + F32 pin = 0.f; + CHECK(familySizePt("A", "Large", pin)); + CHECK(pin == 9.0f); + const LLFontDescriptor* a = templateFor("A"); + REQUIRE(a != nullptr); + const auto& files = a->getFontFiles(); + REQUIRE((S32)files.size() == 2); + CHECK(files[0].mSourceFamily == std::string("A")); + CHECK(files[1].mSourceFamily == std::string("B")); } // =================================================================== @@ -794,7 +827,10 @@ namespace tut LLSD overrides; overrides["UI"] = "user-supplied.ttf"; applyOverrides(overrides); - const auto& files = templateFor("UI")->getFontFiles(); + const LLFontDescriptor* ui = templateFor("UI"); + REQUIRE(ui != nullptr); + const auto& files = ui->getFontFiles(); + REQUIRE_FALSE(files.empty()); CHECK(files[0].FileName == std::string("user-supplied.ttf")); CHECK((S32)files[0].mHinting == (S32)EFontHinting::LIGHT); CHECK(files[0].mVarAxes.wght == 400.f); @@ -893,7 +929,9 @@ namespace tut // The override DID prepend Alt's file onto UI's chain, with // mSourceFamily=Alt — so per-file pin lookup at render time // would use Alt's for that prepended file. - const auto& files = templateFor("UI")->getFontFiles(); + const LLFontDescriptor* ui = templateFor("UI"); + REQUIRE(ui != nullptr); + const auto& files = ui->getFontFiles(); REQUIRE_FALSE(files.empty()); CHECK(files[0].FileName == std::string("A.woff2")); CHECK(files[0].mSourceFamily == std::string("Alt")); @@ -916,19 +954,26 @@ namespace tut // =================================================================== // getMatchingFontDesc applies normalize() to its argument, so passing - // an unnormalized "SansSerifHuge" finds the {SansSerif, Huge} template. - TEST_CASE_METHOD(llfontregistry_data, "LLFontRegistry: getMatchingFontDesc finds a declared template and misses an absent one", "[LLFontRegistry]") + // an unnormalized "SansSerifBold" finds the {SansSerif, BOLD} template. + // (A size suffix such as "Huge" would not: normalize moves it into the + // size, over "TEMPLATE", so the query names a head, not a template.) + TEST_CASE_METHOD(llfontregistry_data, "LLFontRegistry: getMatchingFontDesc normalizes its query and misses an absent family", "[LLFontRegistry]") { const char* xml = "" " " " " + " " " " ""; CHECK(loadXml(xml)); resolve(); LLFontDescriptor query("SansSerif", "TEMPLATE", 0); CHECK(reg.getMatchingFontDesc(query) != nullptr); + const LLFontDescriptor* bold = reg.getMatchingFontDesc(LLFontDescriptor("SansSerifBold", "TEMPLATE", 0)); + REQUIRE(bold != nullptr); + CHECK(bold->getName() == std::string("SansSerif")); + CHECK((S32)bold->getStyle() == (S32)LLFontGL::BOLD); LLFontDescriptor miss("Nope", "TEMPLATE", 0); CHECK(reg.getMatchingFontDesc(miss) == nullptr); } @@ -1075,26 +1120,6 @@ namespace tut CHECK(fams[0].label == std::string("Plain")); } - // inside — literal self-reference - // by name, distinct from the 'self' alias path (Group 3). The cycle - // detector must skip both. The post-resolve chain should contain - // only A's own files, with no duplicate insertion. - TEST_CASE_METHOD(llfontregistry_data, "LLFontRegistry: a naming the family itself leaves only the family's own file", "[LLFontRegistry]") - { - const char* xml = - "" - " " - " " - " " - " " - ""; - CHECK(loadXml(xml)); - resolve(); - auto names = fileNames("Mono"); - REQUIRE((S32)names.size() == 1); - CHECK(names[0] == std::string("M.woff2")); - } - // Empty root with no children is a degenerate but valid // input — init_from_xml + resolve must complete without crashing // and leave the registry's font map empty. Pins safe handling of @@ -1140,7 +1165,9 @@ namespace tut LLSD ovr1; ovr1["UI"] = "AltA"; applyOverrides(ovr1); - const auto& files_a = templateFor("UI")->getFontFiles(); + const LLFontDescriptor* ui_a = templateFor("UI"); + REQUIRE(ui_a != nullptr); + const auto& files_a = ui_a->getFontFiles(); REQUIRE_FALSE(files_a.empty()); CHECK(files_a[0].FileName == std::string("A.woff2")); CHECK(files_a[0].mSourceFamily == std::string("AltA")); @@ -1150,7 +1177,9 @@ namespace tut LLSD ovr2; ovr2["UI"] = "AltB"; applyOverrides(ovr2); - const auto& files_b = templateFor("UI")->getFontFiles(); + const LLFontDescriptor* ui_b = templateFor("UI"); + REQUIRE(ui_b != nullptr); + const auto& files_b = ui_b->getFontFiles(); REQUIRE_FALSE(files_b.empty()); CHECK(files_b[0].FileName == std::string("B.woff2")); CHECK(files_b[0].mSourceFamily == std::string("AltB")); @@ -1657,7 +1686,9 @@ namespace tut ""; CHECK(loadXml(xml)); resolve(); - const auto& a_files = templateFor("A")->getFontFiles(); + const LLFontDescriptor* a = templateFor("A"); + REQUIRE(a != nullptr); + const auto& a_files = a->getFontFiles(); REQUIRE((S32)a_files.size() == 2); CHECK(a_files[0].mSourceFamily == std::string("A")); CHECK(a_files[1].mSourceFamily == std::string("B")); @@ -2053,7 +2084,9 @@ namespace tut ""; CHECK(loadXml(xml)); resolve(); - const auto& files = templateFor("A")->getFontFiles(); + const LLFontDescriptor* a = templateFor("A"); + REQUIRE(a != nullptr); + const auto& files = a->getFontFiles(); REQUIRE((S32)files.size() == 2); CHECK(files[0].mSourceFamily == std::string("A")); CHECK(files[1].mSourceFamily == std::string("B")); @@ -2095,7 +2128,9 @@ namespace tut LLSD ovr_a; ovr_a["Target"] = "SourceA"; resolve(ovr_a); - const auto& a_files = templateFor("Target")->getFontFiles(); + const LLFontDescriptor* target_a = templateFor("Target"); + REQUIRE(target_a != nullptr); + const auto& a_files = target_a->getFontFiles(); REQUIRE_FALSE(a_files.empty()); CHECK(a_files[0].FileName == std::string("SourceA.woff2")); CHECK(a_files[0].mSourceFamily == std::string("SourceA")); @@ -2109,7 +2144,9 @@ namespace tut LLSD ovr_b; ovr_b["Target"] = "SourceB"; resolve(ovr_b); - const auto& b_files = templateFor("Target")->getFontFiles(); + const LLFontDescriptor* target_b = templateFor("Target"); + REQUIRE(target_b != nullptr); + const auto& b_files = target_b->getFontFiles(); REQUIRE_FALSE(b_files.empty()); CHECK(b_files[0].FileName == std::string("SourceB.woff2")); CHECK(b_files[0].mSourceFamily == std::string("SourceB")); @@ -2224,9 +2261,12 @@ namespace tut ""; CHECK(loadXml(xml)); resolve(); - // No heads instantiated, no fallback-cache entries. + // No heads instantiated, no fallback-cache entries: the map holds + // the UI template alone, with no font behind it. + CHECK(templateFor("UI") != nullptr); + // The case passes by returning: the sweep walks that entry without + // dereferencing its NULL font. reg.sweepGlyphCaches(); - CHECK(true); } // OpenType variation axes plumbed through XML parse. Family-level @@ -2284,6 +2324,7 @@ namespace tut CHECK(loadXml(xml)); const LLFontDescriptor* d = templateFor("Plain", LLFontGL::NORMAL); REQUIRE(d != nullptr); + REQUIRE_FALSE(d->getFontFiles().empty()); const auto& f = d->getFontFiles()[0]; CHECK_FALSE(f.mVarAxes.wght_set); CHECK_FALSE(f.mVarAxes.opsz_set); @@ -2556,8 +2597,12 @@ namespace tut CHECK(loadXml(xml)); resolve(); - const auto& bf = templateFor("Broad")->getFontFiles(); - const auto& nf = templateFor("Narrow")->getFontFiles(); + const LLFontDescriptor* broad = templateFor("Broad"); + const LLFontDescriptor* narrow = templateFor("Narrow"); + REQUIRE(broad != nullptr); + REQUIRE(narrow != nullptr); + const auto& bf = broad->getFontFiles(); + const auto& nf = narrow->getFontFiles(); REQUIRE((S32)bf.size() == 1); REQUIRE((S32)nf.size() == 1); // Broad accepts U+1F100; Narrow doesn't. @@ -2597,8 +2642,12 @@ namespace tut overrides["EmojiBase"] = "Twemoji.ttf"; resolve(overrides); - const auto& bf = templateFor("Broad")->getFontFiles(); - const auto& nf = templateFor("Narrow")->getFontFiles(); + const LLFontDescriptor* broad = templateFor("Broad"); + const LLFontDescriptor* narrow = templateFor("Narrow"); + REQUIRE(broad != nullptr); + REQUIRE(narrow != nullptr); + const auto& bf = broad->getFontFiles(); + const auto& nf = narrow->getFontFiles(); // Each consumer sees BOTH files (Twemoji prepended onto the // EmojiBase chain by the override; Noto preserved behind it), // each with the consumer's filter applied. @@ -2638,8 +2687,12 @@ namespace tut overrides["EmojiBase"] = entry; resolve(overrides); - const auto& bf = templateFor("Broad")->getFontFiles(); - const auto& nf = templateFor("Narrow")->getFontFiles(); + const LLFontDescriptor* broad = templateFor("Broad"); + const LLFontDescriptor* narrow = templateFor("Narrow"); + REQUIRE(broad != nullptr); + REQUIRE(narrow != nullptr); + const auto& bf = broad->getFontFiles(); + const auto& nf = narrow->getFontFiles(); // replace_first dropped Noto from EmojiBase's chain; consumers // pull just Twemoji through their respective use-level filters. REQUIRE((S32)bf.size() == 1); @@ -2811,7 +2864,9 @@ namespace tut ""; CHECK(loadXml(xml)); resolve(); - const auto& files = templateFor("Emoji")->getFontFiles(); + const LLFontDescriptor* emoji = templateFor("Emoji"); + REQUIRE(emoji != nullptr); + const auto& files = emoji->getFontFiles(); REQUIRE((S32)files.size() == 1); CHECK(files[0].FileName == std::string("Noto.ttf")); CHECK_FALSE(static_cast(files[0].CharFunctor)); @@ -2858,7 +2913,8 @@ namespace tut overrides["EmojiBase"] = entry; resolve(overrides); - const auto& files = templateFor("Emoji")->getFontFiles(); - CHECK((S32)files.size() == 1); + const LLFontDescriptor* emoji = templateFor("Emoji"); + REQUIRE(emoji != nullptr); + CHECK((S32)emoji->getFontFiles().size() == 1); } } From b4e74993c979cc0b45804d5eada3712a3f56623f Mon Sep 17 00:00:00 2001 From: Rye Date: Tue, 13 Oct 2026 08:34:01 -0400 Subject: [PATCH 2/5] LLFontGL's and LLFontTextCache's tests check what they say they check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Old test bugs TM22's idiom pass found (bb3f752d28), fixed. - llfontgl: - the vertical alignment case's comment named VCENTER, which it never rendered; it renders it now; - the reflow bound took a wrap's shape count for its line count, so "two shapes a line" was looser than it said; each width's wrap now counts its lines; - the run-end case passed with no assertion if no sample had a width; it checks that some were measured; - two frame comparisons compared 256x256 pixel vectors with ==, so a failure printed some 262,000 values; they count the pixels that differ and show the first; - freetypes, bitmap caches and fonts are required before they are read; - the file's header said each case had a registry of its own; the registry is the binary's. - llfonttextcache: - five CHECK(true)s: two cases now pass by returning, as they meant; the toggles case reads each toggle's effect off regenCount() and a draw; - "resets twice and outlives a toggle" repeated another case and the toggles case, and is gone; - the generation-tick case rasterized É, which an earlier case could already have drawn, and never checked that the cache measured again; it takes a glyph nothing has drawn and reads the shaper; - "a reset leaves another cache alone" measured the same text in both, so a shared slot could not show; they measure different texts; - the five render cases checked only counts. Each now shows which way the draw went, through regenCount() and the pixels it left: a repeat replays, a colour change recolors in place (and draws red), a move rebuilds by 100 pixels, a style flip rebuilds, and with the fast path off a colour change rebuilds. The file's header no longer says the harness cannot test that. Each new or corrected check fails under a small change to the test's own input or expectation, at the check and not by crashing. Co-Authored-By: Claude Opus 5.5 --- indra/llrender/tests/llfontgl_test.cpp | 111 ++++-- indra/llrender/tests/llfonttextcache_test.cpp | 354 +++++++++++++----- 2 files changed, 338 insertions(+), 127 deletions(-) diff --git a/indra/llrender/tests/llfontgl_test.cpp b/indra/llrender/tests/llfontgl_test.cpp index 044359ca6f..a7b47a1e96 100644 --- a/indra/llrender/tests/llfontgl_test.cpp +++ b/indra/llrender/tests/llfontgl_test.cpp @@ -6,10 +6,11 @@ * LLFontGL::initClass through the explicit-fonts.xml-path overload, * and verifies the GL-backed atlas pipeline produces a live texture. * - * The single fixture instance shared across test methods (see - * llheadlessgl_fixture.h) keeps the context creation cost - * out of every test — but each test method still gets a fresh - * sFontRegistry, so font-state mutations don't leak. + * The context is the binary's shared one (see llheadlessgl_fixture.h), + * and so is LLFontGL's registry: the first case to call initClass makes + * it, and it stays for every case after, llfonttextcache's too, since + * LLFontGL's getters cache the fonts it holds. A case leaves the fonts, + * their DPI and their scale as it found them. * * $LicenseInfo:firstyear=2026&license=viewerlgpl$ * Alchemy Viewer Source Code @@ -43,6 +44,7 @@ #include +#include #include #include @@ -187,6 +189,8 @@ namespace REQUIRE(big != nullptr); const LLFontFreetype* fs = small->getFontFreetype(); const LLFontFreetype* fb = big->getFontFreetype(); + REQUIRE(fs != nullptr); + REQUIRE(fb != nullptr); CHECK(fs->getLineHeight() > 0.f); CHECK(fb->getLineHeight() > 0.f); CHECK(fb->getLineHeight() > fs->getLineHeight()); @@ -222,7 +226,9 @@ namespace font->generateASCIIglyphs(); const LLFontFreetype* ft = font->getFontFreetype(); + REQUIRE(ft != nullptr); const LLFontBitmapCache* bc = ft->getFontBitmapCache(); + REQUIRE(bc != nullptr); const U32 sheets_after_first = bc->getNumBitmaps(EFontGlyphType::Grayscale); CHECK(sheets_after_first >= 1u); @@ -1034,10 +1040,16 @@ namespace "times at any sensible chat width at all."; // One pass of LLTextBase's wrap loop over one segment, counting the - // shapes it costs. Mirrors LLNormalTextSegment::getNumBytes followed - // by getDimensionsF32. - auto wrap_once = [&](F32 width) -> size_t + // shapes it costs and the lines it makes. Mirrors + // LLNormalTextSegment::getNumBytes followed by getDimensionsF32. + struct Wrap + { + size_t shapes = 0; + size_t lines = 0; + }; + auto wrap_once = [&](F32 width) -> Wrap { + Wrap wrap; const size_t before = ALFontShaping::cacheMutationCount(); S32 at = 0; while (at < (S32)paragraph.size()) @@ -1054,19 +1066,21 @@ namespace // asks for next to place the line. (void)font->getWidthF32Bytes(paragraph, at, fits, true); at += fits; + ++wrap.lines; } - return ALFontShaping::cacheMutationCount() - before; + wrap.shapes = ALFontShaping::cacheMutationCount() - before; + return wrap; }; // Cold, so every distinct slice counts. ALFontShaping::clearCache(); - const size_t first = wrap_once(220.f); + const size_t first = wrap_once(220.f).shapes; UNSCOPED_INFO("wrapping shapes something"); CHECK(first > 0); // The same width again: every slice is already cached, so a repeated // reflow at an unchanged width must cost nothing. - const size_t again = wrap_once(220.f); + const size_t again = wrap_once(220.f).shapes; UNSCOPED_INFO("re-wrapping at the same width re-shapes nothing"); CHECK(again == size_t(0)); @@ -1075,21 +1089,24 @@ namespace // last frame cached. This is what a resize costs per frame per // segment, and it is the number worth watching. size_t dragging = 0; + size_t drag_lines = 0; for (S32 px = 219; px >= 200; --px) { - dragging += wrap_once((F32)px); + const Wrap wrap = wrap_once((F32)px); + dragging += wrap.shapes; + drag_lines += wrap.lines; } // Not an assertion about a good number -- an assertion that the number // is bounded by the work there is to do. Twenty widths over a // paragraph that wraps into a handful of lines: if this ever exceeds // two shapes per line per width, the two callers have stopped agreeing - // on their slices again. - const size_t lines_per_wrap = first; - std::printf("[reflow shaping] cold=%zu same-width=%zu drag-20px=%zu\n", - first, again, dragging); + // on their slices again. The lines are each width's own, counted as + // the wrap makes them. + std::printf("[reflow shaping] cold=%zu same-width=%zu drag-20px=%zu over %zu lines\n", + first, again, dragging, drag_lines); UNSCOPED_INFO("the drag stays within two shapes per line per width"); - CHECK(dragging <= 2 * lines_per_wrap * 20); + CHECK(dragging <= 2 * drag_lines); } // First end-to-end render() call with LEFT/BASELINE places glyph @@ -1150,6 +1167,7 @@ namespace if (!fileExists(kFontsXml)) SKIP("fonts.xml not found"); LLFontGL* font = LLFontGL::getFontSansSerif(); + REQUIRE(font != nullptr); const std::string s = "ABC"; const S32 n_left = font->renderBytes(s, 0, 100.f, 100.f, LLColor4::white, @@ -1169,18 +1187,22 @@ namespace // VAlign branches: render() must complete through TOP, VCENTER, // BASELINE, and BOTTOM without crashing; return value matches - // requested char count for each. Pins the valign switch at - // llfonttextcache.cpp:123-138. + // requested char count for each. Pins the valign switch in + // LLFontGL::renderBytes. TEST_CASE_METHOD(llfontgl_render_data, "LLFontGLRender: renderBytes draws every byte whatever the vertical alignment", "[LLFontGLRender]") { if (!fileExists(kFontsXml)) SKIP("fonts.xml not found"); LLFontGL* font = LLFontGL::getFontSansSerif(); + REQUIRE(font != nullptr); const std::string s = "Hi"; const S32 n_top = font->renderBytes(s, 0, 100.f, 100.f, LLColor4::white, LLFontGL::LEFT, LLFontGL::TOP, LLFontGL::NORMAL, LLFontGL::NO_SHADOW, 2); + const S32 n_vcen = font->renderBytes(s, 0, 100.f, 100.f, LLColor4::white, + LLFontGL::LEFT, LLFontGL::VCENTER, + LLFontGL::NORMAL, LLFontGL::NO_SHADOW, 2); const S32 n_bs = font->renderBytes(s, 0, 100.f, 100.f, LLColor4::white, LLFontGL::LEFT, LLFontGL::BASELINE, LLFontGL::NORMAL, LLFontGL::NO_SHADOW, 2); @@ -1189,6 +1211,7 @@ namespace LLFontGL::NORMAL, LLFontGL::NO_SHADOW, 2); gGL.flush(); CHECK(n_top == 2); + CHECK(n_vcen == 2); CHECK(n_bs == 2); CHECK(n_btm == 2); } @@ -1201,6 +1224,7 @@ namespace if (!fileExists(kFontsXml)) SKIP("fonts.xml not found"); LLFontGL* font = LLFontGL::getFontSansSerif(); + REQUIRE(font != nullptr); const std::string s = "Hello"; const S32 n_norm = font->renderBytes(s, 0, 50.f, 100.f, LLColor4::white, @@ -1230,6 +1254,7 @@ namespace if (!fileExists(kFontsXml)) SKIP("fonts.xml not found"); LLFontGL* font = LLFontGL::getFontSansSerif(); + REQUIRE(font != nullptr); const std::string s = "Hi"; const S32 n_no = font->renderBytes(s, 0, 100.f, 100.f, LLColor4::white, @@ -1306,8 +1331,9 @@ namespace REQUIRE(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); + const LLFontBitmapCache* latin_cache = latin_gi->mSourceFace->getBitmapCache(); + REQUIRE(latin_cache != nullptr); + LLImageGL* latin_img = latin_cache->getImageGL(latin_entry.first, (U32)latin_entry.second); UNSCOPED_INFO("latin atlas page live"); REQUIRE(latin_img != nullptr); const U32 latin_tex = latin_img->getTexName(); @@ -1319,8 +1345,9 @@ namespace REQUIRE(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); + const LLFontBitmapCache* emoji_cache = emoji_gi->mSourceFace->getBitmapCache(); + REQUIRE(emoji_cache != nullptr); + LLImageGL* emoji_img = emoji_cache->getImageGL(emoji_entry.first, (U32)emoji_entry.second); UNSCOPED_INFO("emoji atlas page live"); REQUIRE(emoji_img != nullptr); const U32 emoji_tex = emoji_img->getTexName(); @@ -1501,6 +1528,7 @@ namespace std::string("."), }; + S32 measured = 0; for (const std::string& s : samples) { CAPTURE(s); @@ -1510,7 +1538,8 @@ namespace // what the padded form adds on top. const F32 expect = font->getWidthF32Bytes(s, 0, total, true); if (expect <= 0.f) - continue; + continue; // a script the test's fonts do not cover + ++measured; F32 right_x = 0.f; font->renderBytes(s, 0, 40.f, 60.f, LLColor4::white, @@ -1523,6 +1552,8 @@ namespace UNSCOPED_INFO("the drawn run ends where the measurement says"); REQUIRE(right_x == 40.f + expect); } + UNSCOPED_INFO("some sample had a width to hold the drawn run against"); + CHECK(measured > 0); } // An ellipsized draw buys itself padding by measuring "....", and that @@ -1597,6 +1628,32 @@ namespace } return false; }; + // Where two frames differ: how many pixels, and the first of them, so + // a failure names a place rather than printing both frames whole. + struct FrameDiff + { + size_t pixels = 0; + S32 first_x = -1; + S32 first_y = -1; + }; + const auto differ = [&](const std::vector& a, const std::vector& b) { + FrameDiff d; + const size_t common = std::min(a.size(), b.size()); + for (size_t i = 0; i + 3 < common; i += 4) + { + if (a[i] != b[i] || a[i + 1] != b[i + 1] || a[i + 2] != b[i + 2] || a[i + 3] != b[i + 3]) + { + if (d.pixels++ == 0) + { + d.first_x = (S32)((i / 4) % (size_t)w); + d.first_y = (S32)((i / 4) / (size_t)w); + } + } + } + // Frames of different sizes are different frames. + d.pixels += (std::max(a.size(), b.size()) - common + 3) / 4; + return d; + }; const LLFontGL::HAlign aligns[] = { LLFontGL::LEFT, LLFontGL::RIGHT, LLFontGL::HCENTER }; for (LLFontGL* font : { LLFontGL::getFontMonospace(), LLFontGL::getFontSansSerif() }) { @@ -1615,8 +1672,10 @@ namespace const std::vector drawn = frame([&] { font->renderGlyphs(placed.data(), colors.data(), placed.size(), 128.f + shift, 100.f); }); UNSCOPED_INFO("drawn at all"); REQUIRE(lit(said)); + const FrameDiff d = differ(said, drawn); + CAPTURE(d.first_x, d.first_y); UNSCOPED_INFO("the same pixels"); - REQUIRE(said == drawn); + REQUIRE(d.pixels == 0u); } } } @@ -1636,7 +1695,9 @@ namespace const std::vector together = frame([&] { font->renderGlyphRuns(runs, 2); }); UNSCOPED_INFO("runs drawn at all"); CHECK(lit(apiece)); + const FrameDiff d = differ(apiece, together); + CAPTURE(d.first_x, d.first_y); UNSCOPED_INFO("runs in one call are the runs apiece"); - CHECK(apiece == together); + CHECK(d.pixels == 0u); } } diff --git a/indra/llrender/tests/llfonttextcache_test.cpp b/indra/llrender/tests/llfonttextcache_test.cpp index bae46234e5..c70d48577d 100644 --- a/indra/llrender/tests/llfonttextcache_test.cpp +++ b/indra/llrender/tests/llfonttextcache_test.cpp @@ -1,23 +1,14 @@ /** * @file llfonttextcache_test.cpp - * @brief Smoke tests for LLFontTextCache + LLFontTextCache. + * @brief Tests for LLFontTextCache, which keeps what one piece of text + * costs to draw and to measure. * - * The deeper cache-invalidation invariants (geometry vs color - * regen, mLastFontCacheGen, mLastUsesColorAtlas) are private to - * LLFontTextCache, and the public render() entry point dives into - * gGL.pushUIMatrix and drawGlyph — code paths that the - * headless harness doesn't have populated (no UI matrix stack, - * no shader bindings to draw with). A test for the recolor fast - * path proper would need a full viewer-scale GL bring-up that the - * llrender harness can't supply. - * - * What this file does instead is smoke the API surface that doesn't - * touch the render-time path: construct/destruct, reset(), - * enable*() static toggles. That at least catches link-time - * regressions and obvious lifecycle bugs (double-free in dtor, - * etc.). LLFontTextCache's measurement path is also covered - * lightly — it's invoked via fontp->getWidthF32, which uses the - * shape layout but doesn't hit gGL drawing primitives. + * Its lifecycle (construct, reset, destroy) and its static toggles; its + * width slots, held against the font's own measurement; and its draws, + * through the shared context's UI program. Which way a draw went -- a + * replay, a recolor in place, or a rebuild -- is private to the cache, + * and is read off LLFontTextCache::regenCount(), which only a rebuild + * moves, and off the pixels the draw left. * * $LicenseInfo:firstyear=2026&license=viewerlgpl$ * Alchemy Viewer Source Code @@ -97,6 +88,26 @@ namespace initialized = true; } + // Draws until a draw replays rather than rebuilds, as the viewer's frames + // do, at most eight times. A first draw can rasterize a glyph that shaping + // substituted, which moves the font's cache generation under the geometry + // it just captured, and that geometry is deliberately not trusted for a + // second frame. A cache that never replays is left for the checks after + // this to report. + template + void settleDraws(DRAW&& draw) + { + for (int i = 0; i < 8; ++i) + { + const U64 before = LLFontTextCache::regenCount(); + draw(); + if (LLFontTextCache::regenCount() == before) + { + return; + } + } + } + // Fixture: minimal — just gates the static toggles and ensures the // shared GL fixture is up. The actual HeadlessGL instance lives in // ll_test::sharedHeadlessGL() and stays alive for the whole @@ -122,61 +133,85 @@ namespace // Default construction must not crash and must leave the buffer // in a destructible state. Catches uninitialized-member regressions // that surface as a crash in ~LLFontTextCache trying to free - // a garbage list. + // a garbage list. There is nothing to ask of it: the case passes by + // returning, and a crash in the destructor is its failure. TEST_CASE_METHOD(llfonttextcache_data, "LLFontTextCache: a default-constructed cache destroys cleanly", "[LLFontTextCache]") { LLFontTextCache vb; - CHECK(true); - // Implicit destruct at scope exit; assertion above catches any - // crash during dtor by being unreached. } - // reset() on a freshly-constructed buffer must be a safe no-op. + // reset() on a freshly-constructed buffer must be a safe no-op. The case + // passes by returning. TEST_CASE_METHOD(llfonttextcache_data, "LLFontTextCache: reset on a fresh cache is safe to call twice", "[LLFontTextCache]") { LLFontTextCache vb; vb.reset(); vb.reset(); // idempotent - CHECK(true); } - // Static toggles round-trip without affecting later test setup. - // The assertions confirm the calls compile and link; observable - // round-trip would require accessing the private statics. + // The static toggles take effect, and turning one back on restores what + // it turned off, which every later case relies on. Their statics are + // private, so each is read off what a draw does: with buffer collection + // off a draw goes straight to the font and captures nothing, so nothing + // is rebuilt; with the colour-only fast path on, a colour change is + // recolored in place rather than rebuilt. What the fast path does when it + // is off is the render group's to check. TEST_CASE_METHOD(llfonttextcache_data, "LLFontTextCache: enableBufferCollection and enableColorOnlyRegen toggle off and on", "[LLFontTextCache]") { + if (!fileExists(kFontsXml)) + SKIP("fonts.xml not present in test data dir"); + LLFontGL* font = LLFontGL::getFontSansSerif(); + REQUIRE(font != nullptr); + font->generateASCIIglyphs(); + const std::string s = "Hi"; + + auto draw = [&](LLFontTextCache& cache, const LLColor4& color) + { + cache.renderBytes(font, s, 0, 100.f, 100.f, color, + LLFontGL::LEFT, LLFontGL::BASELINE, + LLFontGL::NORMAL, LLFontGL::NO_SHADOW, 2); + // An uncollected draw leaves its quads in gGL's batch. + gGL.flush(); + }; + LLFontTextCache::enableBufferCollection(false); + LLFontTextCache off; + const U64 uncollected = LLFontTextCache::regenCount(); + draw(off, LLColor4::white); + UNSCOPED_INFO("with buffer collection off a draw captures nothing"); + CHECK(LLFontTextCache::regenCount() == uncollected); + + // A fresh cache, whose first draw is a capture whenever collection is on. LLFontTextCache::enableBufferCollection(true); + LLFontTextCache on; + const U64 collected = LLFontTextCache::regenCount(); + draw(on, LLColor4::white); + UNSCOPED_INFO("turned back on, a draw is captured"); + CHECK(LLFontTextCache::regenCount() > collected); + LLFontTextCache::enableColorOnlyRegen(false); LLFontTextCache::enableColorOnlyRegen(true); - CHECK(true); - } - - // LLFontTextCache parallel: construct + reset. - TEST_CASE_METHOD(llfonttextcache_data, "LLFontTextCache: a cache resets twice and outlives a buffer collection toggle", "[LLFontTextCache]") - { - LLFontTextCache wb; - wb.reset(); - wb.reset(); - CHECK(true); - LLFontTextCache::enableBufferCollection(false); - LLFontTextCache::enableBufferCollection(true); + settleDraws([&] { draw(on, LLColor4::white); }); + const U64 before_recolor = LLFontTextCache::regenCount(); + draw(on, LLColor4::red); + UNSCOPED_INFO("turned back on, the colour-only fast path recolors without a rebuild"); + CHECK(LLFontTextCache::regenCount() == before_recolor); } - // LLFontTextCache constructed alongside LLFontTextCache: - // multiple coexisting instances don't share static state in a way - // that breaks lifecycle. Pin two buffers in flight at once. + // Several caches in flight at once: none shares state with another in a + // way that breaks construction, reset or destruction. The case passes by + // returning; that one cache's reset leaves another's answers alone is + // checked in the width group. TEST_CASE_METHOD(llfonttextcache_data, "LLFontTextCache: four caches in flight at once each reset cleanly", "[LLFontTextCache]") { LLFontTextCache a; LLFontTextCache b; - LLFontTextCache w1; - LLFontTextCache w2; + LLFontTextCache c; + LLFontTextCache d; a.reset(); b.reset(); - w1.reset(); - w2.reset(); - CHECK(true); + c.reset(); + d.reset(); } // =================================================================== @@ -251,23 +286,57 @@ namespace } // Cache miss when the font's cache generation advances between - // calls (a fallback face's atlas mutation, for example). Pins - // mLastFontCacheGen at llfonttextcache.cpp:524. - TEST_CASE_METHOD(llfonttextcache_data, "LLFontTextCache: getWidthBytes agrees with the font across an atlas generation tick", "[LLFontTextCache]") + // calls (a fallback face's atlas mutation, for example): the width the + // cache holds is measured again. Whether it was is read off the shaper, + // which a measurement asks and an answer from the cache's own slot does + // not -- so with the shape cache emptied, a measurement shows as a shape. + TEST_CASE_METHOD(llfonttextcache_data, "LLFontTextCache: getWidthBytes measures again after a generation tick and agrees", "[LLFontTextCache]") { if (!fileExists(kFontsXml)) SKIP("fonts.xml not present in test data dir"); LLFontGL* font = LLFontGL::getFontSansSerif(); REQUIRE(font != nullptr); + const LLFontFreetype* ft = font->getFontFreetype(); + REQUIRE(ft != nullptr); + REQUIRE(ft->getFontFace() != nullptr); const std::string s = "Hi"; + // A glyph of the font's own face that nothing has rasterized yet, so + // that asking for it has to take an atlas slot. A fixed one could + // already have been drawn by any case before this. + llwchar fresh = 0; + for (llwchar cp = 0x100; cp < 0x180 && fresh == 0; ++cp) + { + const U32 index = ft->getCharGlyphIndex(cp); + if (index != 0 && !ft->getFontFace()->findGlyphInfo(index, EFontGlyphType::Grayscale)) + { + fresh = cp; + } + } + if (fresh == 0) + SKIP("every Latin Extended-A glyph is already rasterized"); + LLFontTextCache wb; const F32 first = wb.getWidthBytes(font, s, 0, 2, false); - // Force a generation tick by rasterizing a fresh glyph through - // the font (atlas allocation bumps the global counter). - font->getFontFreetype()->getGlyphInfo(L'É', // 'É' - EFontGlyphType::Grayscale); + + // Nothing moved, so a repeat is answered from the cache's slot. + ALFontShaping::clearCache(); + const size_t shaped_before_repeat = ALFontShaping::cacheMutationCount(); + (void)wb.getWidthBytes(font, s, 0, 2, false); + UNSCOPED_INFO("with nothing changed the cache answers without measuring"); + CHECK(ALFontShaping::cacheMutationCount() == shaped_before_repeat); + + // Force a generation tick by rasterizing the fresh glyph through the + // font: its atlas allocation moves the font's cache generation. + const U64 gen_before = font->getCacheGeneration(); + ft->getGlyphInfo(fresh, EFontGlyphType::Grayscale); + UNSCOPED_INFO("rasterizing a new glyph moves the font's cache generation"); + CHECK(font->getCacheGeneration() > gen_before); + + const size_t shaped_before_tick = ALFontShaping::cacheMutationCount(); const F32 second = wb.getWidthBytes(font, s, 0, 2, false); + UNSCOPED_INFO("after the tick the cache measures again"); + CHECK(ALFontShaping::cacheMutationCount() > shaped_before_tick); // Held against the font rather than against the buffer's own earlier // answer. Comparing a cache to itself passes whether it invalidated, // recomputed, or returned a stale value -- the only reading that can @@ -302,21 +371,38 @@ namespace } // Multiple LLFontTextCache instances stay independent: cache - // state in instance A doesn't affect instance B. + // state in instance A doesn't affect instance B. The two hold different + // texts over the same span, and width slots are keyed on the span, so a + // slot the two shared would hand one of them the other's width. TEST_CASE_METHOD(llfonttextcache_data, "LLFontTextCache: one cache's reset leaves another's width unchanged", "[LLFontTextCache]") { if (!fileExists(kFontsXml)) SKIP("fonts.xml not present in test data dir"); LLFontGL* font = LLFontGL::getFontSansSerif(); REQUIRE(font != nullptr); - const std::string s = "Hi"; + const std::string sa = "Hi"; + const std::string sb = "WW"; LLFontTextCache a, b; - const F32 wa = a.getWidthBytes(font, s, 0, 2, false); + const F32 wa = a.getWidthBytes(font, sa, 0, 2, false); + const F32 wb_val = b.getWidthBytes(font, sb, 0, 2, false); + UNSCOPED_INFO("the two texts differ in width, so a shared slot would show"); + CHECK(wa != wb_val); + UNSCOPED_INFO("instance B answers for its own text"); + CHECK(wb_val == font->getWidthF32Bytes(sb, 0, 2, false)); + + // Whether B kept its answer is read off the shaper: with the shape + // cache emptied, B measuring again would shape, and B answering from + // its own slot does not. a.reset(); - const F32 wb_val = b.getWidthBytes(font, s, 0, 2, false); - UNSCOPED_INFO("instance B unaffected by A's reset"); - CHECK(wa == wb_val); + ALFontShaping::clearCache(); + const size_t shaped = ALFontShaping::cacheMutationCount(); + const F32 wb_again = b.getWidthBytes(font, sb, 0, 2, false); + UNSCOPED_INFO("instance B unaffected by A's reset: it answers from its own slot"); + CHECK(ALFontShaping::cacheMutationCount() == shaped); + CHECK(wb_again == wb_val); + UNSCOPED_INFO("and A answers for its own text again"); + CHECK(a.getWidthBytes(font, sa, 0, 2, false) == wa); } // The bound the cache is keyed on, over text where a byte is not a @@ -449,12 +535,11 @@ namespace CHECK(n == 5); } - // Color-only fast path (b317778477): same render twice with same - // geometry but different color. Second call goes through - // recolorBuffers — no genBuffers, no atlas alloc. The global - // generation counter must NOT advance between the first and second - // calls (atlas already populated; recolor doesn't allocate). - TEST_CASE_METHOD(llfonttextcache_render_data, "LLFontTextCacheRender: a repeated identical render allocates nothing in the atlas", "[LLFontTextCacheRender]") + // An identical render replays: same geometry, same colour, so the second + // call has nothing to rebuild and nothing to recolor, and the atlas is + // not touched. The global generation counter must NOT advance across the + // repeat, and the rebuild count must not move. + TEST_CASE_METHOD(llfonttextcache_render_data, "LLFontTextCacheRender: a repeated identical render replays and allocates nothing", "[LLFontTextCacheRender]") { if (!fileExists(kFontsXml)) SKIP("fonts.xml not present in test data dir"); @@ -463,31 +548,37 @@ namespace const std::string s = "Hello"; LLFontTextCache vb; - // Pre-rasterize all glyphs of "Hello" so neither call grows - // the atlas (otherwise both calls would bump the gen counter - // and the test would be a no-op). + // Pre-rasterize all glyphs of "Hello" so the repeat cannot grow + // the atlas (otherwise it would bump the gen counter and the + // test would be a no-op). font->generateASCIIglyphs(); - vb.renderBytes(font, s, 0, 100.f, 100.f, LLColor4::white, - LLFontGL::LEFT, LLFontGL::BASELINE, - LLFontGL::NORMAL, LLFontGL::NO_SHADOW, 5); + settleDraws([&] { + vb.renderBytes(font, s, 0, 100.f, 100.f, LLColor4::white, + LLFontGL::LEFT, LLFontGL::BASELINE, + LLFontGL::NORMAL, LLFontGL::NO_SHADOW, 5); + }); const S32 gen_before = LLFontBitmapCache::getGlobalGeneration(); - // Second call: same geometry, same color. Should be a no-regen - // replay — neither genBuffers nor recolorBuffers fires. + const U64 regen_before = LLFontTextCache::regenCount(); + // Same geometry, same color. Should be a no-regen replay — + // neither genBuffers nor recolorBuffers fires. vb.renderBytes(font, s, 0, 100.f, 100.f, LLColor4::white, LLFontGL::LEFT, LLFontGL::BASELINE, LLFontGL::NORMAL, LLFontGL::NO_SHADOW, 5); const S32 gen_after = LLFontBitmapCache::getGlobalGeneration(); + UNSCOPED_INFO("the repeat replays: nothing is rebuilt"); + CHECK(LLFontTextCache::regenCount() == regen_before); UNSCOPED_INFO("no atlas alloc on repeat render (gen counter stable)"); CHECK(gen_after == gen_before); } // Color change with same geometry: takes the recolor fast path — // recolorBuffers walks the captured streams and rewrites the color - // attribute, no genBuffers. Pins llfonttextcache.cpp:203-210. - // Observable: global generation counter doesn't advance (no atlas - // mutation), and the call returns the same character count. - TEST_CASE_METHOD(llfonttextcache_render_data, "LLFontTextCacheRender: a color change alone keeps the count and allocates nothing", "[LLFontTextCacheRender]") + // attribute, no genBuffers. Observable: nothing is rebuilt, the global + // generation counter doesn't advance (no atlas mutation), the call + // returns the same character count, and the text comes out in the new + // colour -- which a replay of the white capture would not. + TEST_CASE_METHOD(llfonttextcache_render_data, "LLFontTextCacheRender: a color change alone recolors in place without rebuilding or allocating", "[LLFontTextCacheRender]") { if (!fileExists(kFontsXml)) SKIP("fonts.xml not present in test data dir"); @@ -497,26 +588,59 @@ namespace LLFontTextCache vb; font->generateASCIIglyphs(); - const S32 n_white = vb.renderBytes(font, s, 0, 100.f, 100.f, LLColor4::white, - LLFontGL::LEFT, LLFontGL::BASELINE, - LLFontGL::NORMAL, LLFontGL::NO_SHADOW, 5); + S32 n_white = 0; + settleDraws([&] { + n_white = vb.renderBytes(font, s, 0, 100.f, 100.f, LLColor4::white, + LLFontGL::LEFT, LLFontGL::BASELINE, + LLFontGL::NORMAL, LLFontGL::NO_SHADOW, 5); + }); + + gl.clearFramebuffer(); const S32 gen_before = LLFontBitmapCache::getGlobalGeneration(); + const U64 regen_before = LLFontTextCache::regenCount(); const S32 n_red = vb.renderBytes(font, s, 0, 100.f, 100.f, LLColor4::red, LLFontGL::LEFT, LLFontGL::BASELINE, LLFontGL::NORMAL, LLFontGL::NO_SHADOW, 5); const S32 gen_after = LLFontBitmapCache::getGlobalGeneration(); + const U64 regen_after = LLFontTextCache::regenCount(); + + // Over a black frame, red text leaves only red; anything with green or + // blue in it is the white the geometry was captured in. + glFinish(); + const std::vector px = ll_test::readFramebufferRGBA(ll_test::HeadlessGL::WIDTH, + ll_test::HeadlessGL::HEIGHT); + size_t red = 0; + size_t not_red = 0; + for (size_t i = 0; i + 3 < px.size(); i += 4) + { + if (px[i + 1] != 0 || px[i + 2] != 0) + { + ++not_red; + } + else if (px[i] != 0) + { + ++red; + } + } + UNSCOPED_INFO("color change preserves char count"); CHECK(n_red == n_white); + UNSCOPED_INFO("recolor fast path: nothing rebuilt"); + CHECK(regen_after == regen_before); UNSCOPED_INFO("recolor fast path: no atlas mutation"); CHECK(gen_after == gen_before); + UNSCOPED_INFO("the text is drawn in the new colour"); + CHECK(red > 0u); + UNSCOPED_INFO("and nothing is left of the colour it was captured in"); + CHECK(not_red == 0u); } // Geometry change forces full regen: render at x=100, then at - // x=200. Both calls succeed; the global generation counter still - // shouldn't advance (nothing about geometry change touches the - // atlas), but the buffer cache had to rebuild. The post-render - // render-target reflects the second position. - TEST_CASE_METHOD(llfonttextcache_render_data, "LLFontTextCacheRender: a render moved to another x returns the same count", "[LLFontTextCacheRender]") + // x=200. The global generation counter still shouldn't advance + // (nothing about geometry change touches the atlas), but the buffer + // cache has to rebuild, and the run it draws ends where the second + // position puts it (right_x) rather than where the first capture did. + TEST_CASE_METHOD(llfonttextcache_render_data, "LLFontTextCacheRender: a render moved to another x rebuilds there and allocates nothing", "[LLFontTextCacheRender]") { if (!fileExists(kFontsXml)) SKIP("fonts.xml not present in test data dir"); @@ -527,22 +651,35 @@ namespace LLFontTextCache vb; font->generateASCIIglyphs(); - const S32 n1 = vb.renderBytes(font, s, 0, 100.f, 100.f, LLColor4::white, - LLFontGL::LEFT, LLFontGL::BASELINE, - LLFontGL::NORMAL, LLFontGL::NO_SHADOW, 2); + S32 n1 = 0; + F32 right1 = 0.f; + settleDraws([&] { + n1 = vb.renderBytes(font, s, 0, 100.f, 100.f, LLColor4::white, + LLFontGL::LEFT, LLFontGL::BASELINE, + LLFontGL::NORMAL, LLFontGL::NO_SHADOW, 2, S32_MAX, &right1); + }); + const S32 gen_before = LLFontBitmapCache::getGlobalGeneration(); + const U64 regen_before = LLFontTextCache::regenCount(); + F32 right2 = 0.f; const S32 n2 = vb.renderBytes(font, s, 0, 200.f, 100.f, LLColor4::white, LLFontGL::LEFT, LLFontGL::BASELINE, - LLFontGL::NORMAL, LLFontGL::NO_SHADOW, 2); + LLFontGL::NORMAL, LLFontGL::NO_SHADOW, 2, S32_MAX, &right2); UNSCOPED_INFO("char count stable after geometry change"); CHECK(n1 == n2); CHECK(n1 > 0); + UNSCOPED_INFO("moving the text rebuilds it"); + CHECK(LLFontTextCache::regenCount() > regen_before); + UNSCOPED_INFO("a move allocates nothing in the atlas"); + CHECK(LLFontBitmapCache::getGlobalGeneration() == gen_before); + UNSCOPED_INFO("the moved run ends 100 px further right"); + CHECK(right2 == right1 + 100.f); } // enableColorOnlyRegen(false) disables the fast path entirely: // a same-geometry color-change call falls through to genBuffers - // (full regen). The static toggle pins the flag-controlled branch - // at llfonttextcache.cpp:210. Restore the flag in dtor. - TEST_CASE_METHOD(llfonttextcache_render_data, "LLFontTextCacheRender: with enableColorOnlyRegen off a color change still draws every byte", "[LLFontTextCacheRender]") + // (full regen), where with it on the change is recolored in place. + // The fixture puts the flag back whatever happens here. + TEST_CASE_METHOD(llfonttextcache_render_data, "LLFontTextCacheRender: with enableColorOnlyRegen off a color change rebuilds every byte", "[LLFontTextCacheRender]") { if (!fileExists(kFontsXml)) SKIP("fonts.xml not present in test data dir"); @@ -552,25 +689,32 @@ namespace LLFontTextCache vb; font->generateASCIIglyphs(); - vb.renderBytes(font, s, 0, 100.f, 100.f, LLColor4::white, - LLFontGL::LEFT, LLFontGL::BASELINE, - LLFontGL::NORMAL, LLFontGL::NO_SHADOW, 2); + settleDraws([&] { + vb.renderBytes(font, s, 0, 100.f, 100.f, LLColor4::white, + LLFontGL::LEFT, LLFontGL::BASELINE, + LLFontGL::NORMAL, LLFontGL::NO_SHADOW, 2); + }); LLFontTextCache::enableColorOnlyRegen(false); // Same geometry, different color — with fast path off, this - // should still complete cleanly through genBuffers. + // goes through genBuffers. + const U64 regen_before = LLFontTextCache::regenCount(); const S32 n = vb.renderBytes(font, s, 0, 100.f, 100.f, LLColor4::red, LLFontGL::LEFT, LLFontGL::BASELINE, LLFontGL::NORMAL, LLFontGL::NO_SHADOW, 2); + const U64 regen_after = LLFontTextCache::regenCount(); LLFontTextCache::enableColorOnlyRegen(true); + UNSCOPED_INFO("with the fast path off a colour change rebuilds"); + CHECK(regen_after > regen_before); UNSCOPED_INFO("render with fast path disabled still returns 2"); CHECK(n == 2); } // Style flip NORMAL → BOLD forces full regen — different glyph - // index stream. Pins 00f3a4e93d (style invalidation in geometry- - // invalid check at llfonttextcache.cpp:175). - TEST_CASE_METHOD(llfonttextcache_render_data, "LLFontTextCacheRender: a style flip from NORMAL to BOLD keeps the count", "[LLFontTextCacheRender]") + // index stream. Pins 00f3a4e93d (style invalidation in the cache's + // geometry check); where the registry has a bold face the flip also + // changes the font drawing it, which rebuilds as well. + TEST_CASE_METHOD(llfonttextcache_render_data, "LLFontTextCacheRender: a style flip from NORMAL to BOLD rebuilds and keeps the count", "[LLFontTextCacheRender]") { if (!fileExists(kFontsXml)) SKIP("fonts.xml not present in test data dir"); @@ -580,12 +724,18 @@ namespace LLFontTextCache vb; font->generateASCIIglyphs(); - const S32 n_norm = vb.renderBytes(font, s, 0, 100.f, 100.f, LLColor4::white, - LLFontGL::LEFT, LLFontGL::BASELINE, - LLFontGL::NORMAL, LLFontGL::NO_SHADOW, 2); + S32 n_norm = 0; + settleDraws([&] { + n_norm = vb.renderBytes(font, s, 0, 100.f, 100.f, LLColor4::white, + LLFontGL::LEFT, LLFontGL::BASELINE, + LLFontGL::NORMAL, LLFontGL::NO_SHADOW, 2); + }); + const U64 regen_before = LLFontTextCache::regenCount(); const S32 n_bold = vb.renderBytes(font, s, 0, 100.f, 100.f, LLColor4::white, LLFontGL::LEFT, LLFontGL::BASELINE, LLFontGL::BOLD, LLFontGL::NO_SHADOW, 2); + UNSCOPED_INFO("the style flip rebuilds"); + CHECK(LLFontTextCache::regenCount() > regen_before); UNSCOPED_INFO("char count stable across style flip"); CHECK(n_norm == n_bold); CHECK(n_norm > 0); From 585512b52b2e66d8ed9869d50d8bf8a39c43489f Mon Sep 17 00:00:00 2001 From: Rye Date: Tue, 13 Oct 2026 08:34:15 -0400 Subject: [PATCH 3/5] llrender's shaping and COLRv1 tests check what they say they check Old test bugs TM22's idiom pass found (bb3f752d28), fixed. - alfontshaping: - three cases asked for SourceHanSans-Regular.woff2, which the font package does not ship, so they always skipped; they load face 0 of SourceHanSans-VF.otf.ttc, which fonts.xml's CJK font loads, and run; - the cache cases checked a repeat's glyph count, never that it hit the cache, nor that a cleared face's entry was dropped; they read the shape cache's size and mutation count, and compare the glyphs themselves. a_addr, set and never read, is gone; - the tag-sequence bound compared a byte count where it meant codepoints; - the combining-mark case's GPOS branch never ran (DejaVuSansMono composes a + U+0301), and checked no offset; a q + U+0301 probe, which cannot compose, now reaches it, and its x offset is checked; - the load-flags check compared FT_Int32 with int, which cannot differ; it loads at light hinting and checks HarfBuzz is given it; - fonts' faces are required before they are read, and the variation case's FT_MM_Var is owned, so a failure does not leak it. - alfontshaping_gl: the kerning check followed a SKIP for the same condition, so it could not fail; DejaVuSans kerns, and the SKIP is gone. - alfontcolrv1: - "a second paint" never compared the two; it compares the bytes with a fresh painter's; - the size bound said three times the point size and checked four; - the no-scale case zeroed the ppem, already 0, not the scale its comment named, so it never reached the 2-em fallback; it zeroes the scale now and checks the surface the fallback sizes; - the black-floor case skipped for want of a pure-black pixel and checked only the first; it paints U+1F576, which has 28, and checks every one; - the palette case's count check followed a SKIP for the same condition; the count is checked before painting. Noto-COLRv1 ships one palette, so the case still skips here; - faces, HarfBuzz fonts and bitmaps are required before they are read. - alfontcolrv1_gl: the force-monochrome case checked its own ternary; it asks for Grayscale directly, and sForceMonochromeEmoji is put back however the case ends. Caches, faces and HarfBuzz fonts are required before they are read. Each new or corrected check fails under a small change to the test's own input or expectation, at the check and not by crashing. Co-Authored-By: Claude Opus 5.5 --- indra/llrender/tests/alfontcolrv1_gl_test.cpp | 39 ++-- indra/llrender/tests/alfontcolrv1_test.cpp | 178 +++++++++++------- .../llrender/tests/alfontshaping_gl_test.cpp | 8 +- indra/llrender/tests/alfontshaping_test.cpp | 160 ++++++++++------ 4 files changed, 234 insertions(+), 151 deletions(-) diff --git a/indra/llrender/tests/alfontcolrv1_gl_test.cpp b/indra/llrender/tests/alfontcolrv1_gl_test.cpp index 510858dd25..56daacc40c 100644 --- a/indra/llrender/tests/alfontcolrv1_gl_test.cpp +++ b/indra/llrender/tests/alfontcolrv1_gl_test.cpp @@ -110,6 +110,7 @@ namespace REQUIRE(ft->getGlyphInfo(rocket, EFontGlyphType::Color) != nullptr); const LLFontBitmapCache* cache = ft->getFontBitmapCache(); + REQUIRE(cache != nullptr); CHECK(cache->getNumBitmaps(EFontGlyphType::Color) == 1u); } @@ -146,6 +147,7 @@ namespace REQUIRE(ft.notNull()); const LLFontBitmapCache* cache = ft->getFontBitmapCache(); + REQUIRE(cache != nullptr); const S32 gen_before = cache->getCacheGeneration(); LLFontGlyphInfo* gi = ft->getGlyphInfo(0x1F525, EFontGlyphType::Color); @@ -171,7 +173,9 @@ namespace SKIP("Noto-COLRv1.ttf not present"); LLPointer face = gFontManagerp->getOrCreateFace(makeKey(path)); + REQUIRE(face.notNull()); hb_font_t* hb_font = face->getHbFont(); + REQUIRE(hb_font != nullptr); hb_codepoint_t fire = 0; hb_font_get_nominal_glyph(hb_font, 0x1F525, &fire); if (fire == 0) @@ -215,37 +219,32 @@ namespace CHECK(page->getTexName() != 0); } - // Force-monochrome static is wired in at the LLFontGL render-call layer. - // We can't directly drive a render call from here without the full UI - // bringup, but we *can* pin the static's existence + mutability and - // verify that the Grayscale routing works while it's set, which is - // what the call-site code does at llfontgl.cpp:560/715/862. Save and - // restore the static so the test doesn't perturb sibling tests. - TEST_CASE_METHOD(alfontcolrv1_render_data, "ALFontColrV1Render: with sForceMonochromeEmoji set the emoji is looked up as Grayscale", "[ALFontColrV1Render]") + // Force-monochrome static is wired in at the LLFontGL render-call layer: + // renderUTF8 and renderGlyphRuns ask for Grayscale glyphs rather than + // Color while it is set, and getGlyphInfo never reads it. We can't + // drive a render call from here without the full UI bringup, so the + // case pins the static's existence + mutability and that the Grayscale + // routing those call sites go on to ask for works while it's set. The + // static is put back however the case ends, so it doesn't perturb + // sibling tests. + TEST_CASE_METHOD(alfontcolrv1_render_data, "ALFontColrV1Render: with sForceMonochromeEmoji set a Grayscale lookup paints Grayscale", "[ALFontColrV1Render]") { const std::string path = std::string(kFontDir) + "Noto-COLRv1.ttf"; if (!fileExists(path)) SKIP("Noto-COLRv1.ttf not present"); - const bool saved = LLFontGL::sForceMonochromeEmoji; + struct ForceMonoRestore + { + const bool mSaved = LLFontGL::sForceMonochromeEmoji; + ~ForceMonoRestore() { LLFontGL::sForceMonochromeEmoji = mSaved; } + } restore; LLFontGL::sForceMonochromeEmoji = true; LLPointer ft = loadFtHead(path); REQUIRE(ft.notNull()); - // Simulate the render-call selector at llfontgl.cpp:560 with - // use_color=true and force_mono=true → request Grayscale. - const EFontGlyphType requested = - (!/*use_color*/true || LLFontGL::sForceMonochromeEmoji) - ? EFontGlyphType::Grayscale : EFontGlyphType::Color; - UNSCOPED_INFO("force-mono toggle downgrades Color to Grayscale at " - "the request layer"); - CHECK((S32)requested == (S32)EFontGlyphType::Grayscale); - - LLFontGlyphInfo* gi = ft->getGlyphInfo(0x1F525, requested); + LLFontGlyphInfo* gi = ft->getGlyphInfo(0x1F525, EFontGlyphType::Grayscale); REQUIRE(gi != nullptr); CHECK((S32)gi->mGlyphType == (S32)EFontGlyphType::Grayscale); - - LLFontGL::sForceMonochromeEmoji = saved; } } diff --git a/indra/llrender/tests/alfontcolrv1_test.cpp b/indra/llrender/tests/alfontcolrv1_test.cpp index 191907b650..978e13e8e0 100644 --- a/indra/llrender/tests/alfontcolrv1_test.cpp +++ b/indra/llrender/tests/alfontcolrv1_test.cpp @@ -36,8 +36,10 @@ #include #include +#include #include +#include #include #include @@ -232,7 +234,9 @@ namespace // call's result must reflect the second glyph (not the first // — a staging-buffer leak would surface as the second result // pointing at stale first-glyph bytes when dimensions match). - TEST_CASE_METHOD(alfontcolrv1_data, "ALFontColrV1: one painter paints two different emoji one after the other", "[ALFontColrV1]") + // The reference is the second glyph painted by a painter that + // never painted the first. + TEST_CASE_METHOD(alfontcolrv1_data, "ALFontColrV1: one painter paints a second emoji exactly as a fresh painter does", "[ALFontColrV1]") { const std::string path = std::string(kFontDir) + "Noto-COLRv1.ttf"; if (!fileExists(path)) @@ -244,12 +248,13 @@ namespace REQUIRE(face.notNull()); CHECK(face->isValid()); hb_font_t* hb_font = face->getHbFont(); + REQUIRE(hb_font != nullptr); // Two distinct emoji: fire (U+1F525) and red heart (U+2764). // Both should rasterize cleanly through Noto-COLRv1's paint - // walker. We don't assert the dimensions match (different - // glyphs render to different surfaces) — just that both - // succeed and produce live bitmaps. + // walker. Their surfaces differ in size (different glyphs + // render to different surfaces), so the second result is + // compared with the heart painted alone, not with the fire. const LLColor4U fg(255, 255, 255, 255); hb_codepoint_t fire = 0; @@ -270,19 +275,27 @@ namespace REQUIRE(r2.mBitmap != nullptr); CHECK(r2.mWidth > 0); CHECK(r2.mHeight > 0); + + ALFontColrV1Painter fresh; + ALFontColrV1Painter::Result ref; + REQUIRE(fresh.paintGlyph(hb_font, heart, 14.f, fg, 0, ref)); + REQUIRE(ref.mBitmap != nullptr); + CHECK(r2.mWidth == ref.mWidth); + // The memcmp below reads r2 for ref's pitch times height. + REQUIRE(r2.mHeight == ref.mHeight); + REQUIRE(r2.mPitch == ref.mPitch); + UNSCOPED_INFO("the second paint holds the heart, byte for byte"); + CHECK(std::memcmp(r2.mBitmap, ref.mBitmap, (size_t)ref.mPitch * ref.mHeight) == 0); } // 2-em-square surface fallback (88585ff86d): a glyph with no // ClipBox AND no FT extents lands the painter on a synthetic // 2-em surface so it still produces SOMETHING rather than - // returning a 0×0 bitmap. We can't naturally trigger this with - // Noto-COLRv1 (every emoji has extents); the test instead pins - // the public contract via a positive-extent glyph and confirms - // the surface size is on the order of (point_size, point_size) - // rather than 0×0 — i.e. the path that runs when extents work - // also produces a sized bitmap. A negative version (synthetic - // glyph forcing the fallback) requires a hand-built TTF and - // is left for the integration suite. + // returning a 0×0 bitmap. Noto-COLRv1 gives every emoji extents, + // so this case cannot reach the fallback; the no-scale case below + // drives it by taking the font's scale away. This one pins the + // path that runs when extents work: the surface size is on the + // order of (point_size, point_size) rather than 0×0. TEST_CASE_METHOD(alfontcolrv1_data, "ALFontColrV1: the fire emoji's surface is sized on the order of the point size", "[ALFontColrV1]") { const std::string path = std::string(kFontDir) + "Noto-COLRv1.ttf"; @@ -295,6 +308,7 @@ namespace REQUIRE(face.notNull()); CHECK(face->isValid()); hb_font_t* hb_font = face->getHbFont(); + REQUIRE(hb_font != nullptr); hb_codepoint_t fire = 0; hb_font_get_nominal_glyph(hb_font, 0x1F525, &fire); @@ -312,11 +326,11 @@ namespace // size. A regression where the path returned 0×0 would fail // the lower bound; a regression that ballooned to 2× the // 2-em square (32×32 fallback for 14pt) would fail the upper - // bound (~3× point_size). + // bound (3× point_size). CHECK(result.mWidth >= 4); - CHECK(result.mWidth <= (S32)(point_size * 4.f)); + CHECK(result.mWidth <= (S32)(point_size * 3.f)); CHECK(result.mHeight >= 4); - CHECK(result.mHeight <= (S32)(point_size * 4.f)); + CHECK(result.mHeight <= (S32)(point_size * 3.f)); } // (Test 9 removed: pinned the mForegroundOnly flag on the painter @@ -335,7 +349,9 @@ namespace SKIP("Noto-COLRv1.ttf not present"); LLPointer face = gFontManagerp->getOrCreateFace(makeKey(path)); + REQUIRE(face.notNull()); hb_font_t* hb_font = face->getHbFont(); + REQUIRE(hb_font != nullptr); hb_codepoint_t fire = 0; hb_font_get_nominal_glyph(hb_font, 0x1F525, &fire); if (fire == 0) @@ -376,8 +392,8 @@ namespace // the fire emoji once with palette 0 and once with palette 1. If // Noto-COLRv1 ships >= 2 CPAL palettes, the resulting bytes must // differ at non-zero-alpha positions. Skip if only one palette is - // present (no observable difference, but the code path is exercised - // either way). + // present: the painter clamps palette 1 to 0 then, so the two paints + // match and say nothing. TEST_CASE_METHOD(alfontcolrv1_data, "ALFontColrV1: palette_index reaches hb_font_paint_glyph: palette 1 paints differently", "[ALFontColrV1]") { const std::string path = std::string(kFontDir) + "Noto-COLRv1.ttf"; @@ -385,11 +401,15 @@ namespace SKIP("Noto-COLRv1.ttf not present"); LLPointer face = gFontManagerp->getOrCreateFace(makeKey(path)); + REQUIRE(face.notNull()); hb_font_t* hb_font = face->getHbFont(); + REQUIRE(hb_font != nullptr); hb_codepoint_t fire = 0; hb_font_get_nominal_glyph(hb_font, 0x1F525, &fire); if (fire == 0) SKIP("Noto-COLRv1 has no fire emoji"); + if (hb_ot_color_palette_get_count(hb_font_get_face(hb_font)) < 2) + SKIP("Noto-COLRv1 ships only one CPAL palette"); const LLColor4U fg(255, 255, 255, 255); @@ -403,9 +423,8 @@ namespace REQUIRE(r0.mWidth == r1.mWidth); REQUIRE(r0.mHeight == r1.mHeight); - // Compare bytes — if the font has only one palette, hb resolves - // both calls against palette 0 and the bitmaps match. Skip in - // that case; the test would be vacuous. + // Compare bytes — palette 1 exists, so the two paints must differ + // somewhere either of them covers. bool any_diff = false; for (S32 y = 0; y < r0.mHeight && !any_diff; ++y) { @@ -422,9 +441,6 @@ namespace } } } - if (!any_diff) - SKIP("Noto-COLRv1 ships only one CPAL palette"); - CHECK(any_diff); } @@ -439,7 +455,9 @@ namespace SKIP("Noto-COLRv1.ttf not present"); LLPointer face = gFontManagerp->getOrCreateFace(makeKey(path)); + REQUIRE(face.notNull()); hb_font_t* hb_font = face->getHbFont(); + REQUIRE(hb_font != nullptr); hb_codepoint_t fire = 0; hb_font_get_nominal_glyph(hb_font, 0x1F525, &fire); if (fire == 0) @@ -452,6 +470,7 @@ namespace /*palette=*/0, ALFontColrV1Painter::OutputFormat::Gray, result)); + REQUIRE(result.mBitmap != nullptr); CHECK((S32)result.mFormat == (S32)ALFontColrV1Painter::OutputFormat::Gray); CHECK(result.mWidth > 0); CHECK(result.mHeight > 0); @@ -484,7 +503,9 @@ namespace SKIP("Noto-COLRv1.ttf not present"); LLPointer face = gFontManagerp->getOrCreateFace(makeKey(path)); + REQUIRE(face.notNull()); hb_font_t* hb_font = face->getHbFont(); + REQUIRE(hb_font != nullptr); hb_codepoint_t fire = 0; hb_font_get_nominal_glyph(hb_font, 0x1F525, &fire); if (fire == 0) @@ -494,6 +515,7 @@ namespace ALFontColrV1Painter::Result rgba_r, gray_r; const LLColor4U fg(255, 255, 255, 255); REQUIRE(painter.paintGlyph(hb_font, fire, 14.f, fg, 0, rgba_r)); + REQUIRE(rgba_r.mBitmap != nullptr); // Stash the BGRA bytes — the next paintGlyph call invalidates // mBitmap (it points back into mStaging which we overwrite below). std::vector bgra_bytes(rgba_r.mBitmap, @@ -504,6 +526,7 @@ namespace REQUIRE(painter.paintGlyph(hb_font, fire, 14.f, fg, 0, ALFontColrV1Painter::OutputFormat::Gray, gray_r)); + REQUIRE(gray_r.mBitmap != nullptr); // The comparison below walks the Gray bitmap at the BGRA size. REQUIRE(gray_r.mWidth == bgra_w); REQUIRE(gray_r.mHeight == bgra_h); @@ -533,12 +556,13 @@ namespace } // Hybrid floor pin: scan the BGRA result for fully-saturated palette - // black pixels (R=G=B=0, A>200) and assert the corresponding Gray + // black pixels (R=G=B=0, A>200) and assert every corresponding Gray // byte is >= floor (≈ 38 * a / 256) rather than zero. If no such // pixel exists across the test glyphs, skip — the floor invariant - // still holds, just isn't observable here. The point of the test is - // to lock the constant against future tweaks that would silently - // crush dark CPAL details. + // still holds, just isn't observable here. The dark sunglasses' + // lenses are palette black, so Noto-COLRv1 has some. The point of + // the test is to lock the constant against future tweaks that would + // silently crush dark CPAL details. TEST_CASE_METHOD(alfontcolrv1_data, "ALFontColrV1: Gray keeps an alpha floor under saturated black palette pixels", "[ALFontColrV1]") { const std::string path = std::string(kFontDir) + "Noto-COLRv1.ttf"; @@ -546,15 +570,23 @@ namespace SKIP("Noto-COLRv1.ttf not present"); LLPointer face = gFontManagerp->getOrCreateFace(makeKey(path)); + REQUIRE(face.notNull()); hb_font_t* hb_font = face->getHbFont(); + REQUIRE(hb_font != nullptr); // Sweep a handful of likely candidates: emoji that commonly carry - // black eye/outline detail in Noto's palette. First one with a - // fully-saturated black pixel wins; otherwise the test skips. + // black eye/outline detail in Noto's palette. Every + // fully-saturated black pixel of every one is checked; if none + // has one, the test skips. const hb_codepoint_t cps[] = { 0x1F600 /*grinning face*/, 0x1F602 /*face with tears of joy*/, 0x1F525 /*fire*/, - 0x2764 /*red heart*/ }; + 0x2764 /*red heart*/, + 0x1F576 /*dark sunglasses*/ }; + S32 black = 0; // fully-saturated black pixels seen + S32 under_floor = 0; // of those, Gray bytes below the floor + hb_codepoint_t first_cp = 0; + S32 first_x = -1, first_y = -1; for (hb_codepoint_t cp : cps) { hb_codepoint_t gid = 0; @@ -566,6 +598,7 @@ namespace const LLColor4U fg(255, 255, 255, 255); if (!painter.paintGlyph(hb_font, gid, 32.f, fg, 0, rgba_r)) continue; + REQUIRE(rgba_r.mBitmap != nullptr); std::vector bgra_bytes(rgba_r.mBitmap, rgba_r.mBitmap + (size_t)rgba_r.mPitch * rgba_r.mHeight); const S32 bgra_w = rgba_r.mWidth, bgra_h = rgba_r.mHeight; @@ -574,6 +607,11 @@ namespace ALFontColrV1Painter::OutputFormat::Gray, gray_r)) continue; + REQUIRE(gray_r.mBitmap != nullptr); + // The scan below walks the Gray bitmap at the BGRA size. + CAPTURE(cp); + REQUIRE(gray_r.mWidth == bgra_w); + REQUIRE(gray_r.mHeight == bgra_h); for (S32 y = 0; y < bgra_h; ++y) { @@ -587,15 +625,23 @@ namespace const U32 a = bgra_row[x*4 + 3]; if (r == 0 && g == 0 && b == 0 && a > 200) { + ++black; const U32 expected_floor = (a * 38u) >> 8; - CAPTURE(cp, x, y); - REQUIRE((U32)gray_row[x] + 1u >= expected_floor); - return; + if ((U32)gray_row[x] + 1u < expected_floor && under_floor++ == 0) + { + first_cp = cp; + first_x = x; + first_y = y; + } } } } } - SKIP("no fully-saturated-black palette pixel found across test glyphs"); + if (black == 0) + SKIP("no fully-saturated-black palette pixel found across test glyphs"); + // The first pixel under the floor, if any. + CAPTURE(black, first_cp, first_x, first_y); + CHECK(under_floor == 0); } // Out-of-range palette_index clamps to 0 deterministically. Paint @@ -609,7 +655,9 @@ namespace SKIP("Noto-COLRv1.ttf not present"); LLPointer face = gFontManagerp->getOrCreateFace(makeKey(path)); + REQUIRE(face.notNull()); hb_font_t* hb_font = face->getHbFont(); + REQUIRE(hb_font != nullptr); hb_codepoint_t fire = 0; hb_font_get_nominal_glyph(hb_font, 0x1F525, &fire); if (fire == 0) @@ -620,6 +668,8 @@ namespace const LLColor4U fg(255, 255, 255, 255); REQUIRE(painter_a.paintGlyph(hb_font, fire, 14.f, fg, /*palette=*/0u, r_zero)); REQUIRE(painter_b.paintGlyph(hb_font, fire, 14.f, fg, /*palette=*/99u, r_huge)); + REQUIRE(r_zero.mBitmap != nullptr); + REQUIRE(r_huge.mBitmap != nullptr); CHECK(r_huge.mWidth == r_zero.mWidth); // The memcmp below reads r_huge for r_zero's pitch times height. REQUIRE(r_huge.mHeight == r_zero.mHeight); @@ -629,28 +679,32 @@ namespace CHECK(std::memcmp(r_zero.mBitmap, r_huge.mBitmap, bytes) == 0); } - // Fallback point_size kicks in when the hb_font has ppem unset. - // Synthesize that condition by zeroing the hb_font scale before - // calling paintGlyph; the painter must still produce a sized - // bitmap rather than returning false. Specifically targets the - // 2-em fallback inside the empty-bbox branch — without the - // fallback point_size override, that branch would return false. - TEST_CASE_METHOD(alfontcolrv1_data, "ALFontColrV1: with ppem unset paintGlyph sizes from the fallback point size or fails", "[ALFontColrV1]") + // Fallback point_size kicks in when the hb_font has ppem unset and + // gives the glyph no extents. Synthesize that condition by zeroing + // the hb_font scale before calling paintGlyph (a fresh hb_font has + // no ppem already); the painter must still produce a bitmap sized + // from the fallback rather than returning false. Specifically + // targets the 2-em fallback inside the empty-bbox branch — without + // the fallback point_size override, that branch would return false. + // Nothing is painted at scale 0; the surface is what is checked. + TEST_CASE_METHOD(alfontcolrv1_data, "ALFontColrV1: with no scale or ppem paintGlyph sizes 2 em of the fallback point size", "[ALFontColrV1]") { const std::string path = std::string(kFontDir) + "Noto-COLRv1.ttf"; if (!fileExists(path)) SKIP("Noto-COLRv1.ttf not present"); LLPointer face = gFontManagerp->getOrCreateFace(makeKey(path)); - // Build a dedicated hb_font on top of the same FT_Face so we can - // mutate its ppem without affecting the face cache shared with - // other tests in this group. + REQUIRE(face.notNull()); + // Build a dedicated hb_font on top of the same hb_face so we can + // mutate its scale without affecting the face cache shared with + // other tests in this group. Destroyed however the case ends. hb_font_t* shared = face->getHbFont(); REQUIRE(shared != nullptr); hb_face_t* hb_face = hb_font_get_face(shared); - hb_font_t* hb_font = hb_font_create(hb_face); + const std::unique_ptr owned(hb_font_create(hb_face), hb_font_destroy); + hb_font_t* hb_font = owned.get(); REQUIRE(hb_font != nullptr); - hb_font_set_ppem(hb_font, 0, 0); + hb_font_set_scale(hb_font, 0, 0); unsigned px = 1, py = 1; hb_font_get_ppem(hb_font, &px, &py); CHECK(px == 0); @@ -663,31 +717,21 @@ namespace hb_codepoint_t fire = 0; hb_font_get_nominal_glyph(shared, 0x1F525, &fire); if (fire == 0) - { - hb_font_destroy(hb_font); SKIP("Noto-COLRv1 has no fire emoji"); - } ALFontColrV1Painter painter; ALFontColrV1Painter::Result result; const LLColor4U fg(255, 255, 255, 255); - // fallback_point_size = 14: should drive the empty-bbox branch - // to size against ~14 px instead of returning false. - const bool ok = painter.paintGlyph(hb_font, fire, - /*fallback_point_size=*/14.f, fg, - /*palette=*/0u, result); - // The walker may still bail on other grounds for this unscaled - // font (some paint commands depend on scale-derived coords), so - // the strict assertion is "doesn't crash and returns either a - // valid bitmap or a clean false." A regression that returned - // false when fallback_point_size would have rescued it is the - // failure mode we're protecting against. - if (ok) - { - CHECK(result.mWidth > 0); - CHECK(result.mHeight > 0); - } - hb_font_destroy(hb_font); + // fallback_point_size = 14: drives the empty-bbox branch to size + // against 14 px instead of returning false. + constexpr F32 fallback_point_size = 14.f; + REQUIRE(painter.paintGlyph(hb_font, fire, fallback_point_size, fg, + /*palette=*/0u, result)); + // Two ems of the fallback size, and the painter's 2 px pad on + // every side. + constexpr S32 expected = 2 * (S32)fallback_point_size + 2 * 2; + CHECK(result.mWidth == expected); + CHECK(result.mHeight == expected); } // GLYPH_PAD bump (§10.2): outermost row/column of a rendered glyph @@ -703,7 +747,9 @@ namespace SKIP("Noto-COLRv1.ttf not present"); LLPointer face = gFontManagerp->getOrCreateFace(makeKey(path)); + REQUIRE(face.notNull()); hb_font_t* hb_font = face->getHbFont(); + REQUIRE(hb_font != nullptr); hb_codepoint_t fire = 0; hb_font_get_nominal_glyph(hb_font, 0x1F525, &fire); if (fire == 0) diff --git a/indra/llrender/tests/alfontshaping_gl_test.cpp b/indra/llrender/tests/alfontshaping_gl_test.cpp index 379af02f3b..a6dbfc009e 100644 --- a/indra/llrender/tests/alfontshaping_gl_test.cpp +++ b/indra/llrender/tests/alfontshaping_gl_test.cpp @@ -119,8 +119,8 @@ namespace // (not the legacy `kern` table), so the observable signal is that // the AV-as-pair advance differs from the sum of solo-A and solo-V // advances. A regression that disabled GPOS unconditionally would - // make these equal. Skip if no Latin kern pair fires (test - // verifies nothing then). + // make these equal for every pair. DejaVuSans's GPOS kerns several + // of them (AV among them), so none kerning fails the case. TEST_CASE_METHOD(alfontshaping_gl_data, "ALFontShapingGL: GPOS kerning reaches HB shaping for some Latin pair in DejaVuSans", "[ALFontShapingGL]") { const std::string path = std::string(kFontDir) + "DejaVuSans.woff2"; @@ -159,9 +159,7 @@ namespace break; } } - if (!found_kerned) - SKIP("DejaVuSans lacks a measurable Latin kern pair"); - + UNSCOPED_INFO("some pair advances differently shaped together than its halves shaped alone"); CHECK(found_kerned); } } diff --git a/indra/llrender/tests/alfontshaping_test.cpp b/indra/llrender/tests/alfontshaping_test.cpp index 7d7a336870..ffeb6b115f 100644 --- a/indra/llrender/tests/alfontshaping_test.cpp +++ b/indra/llrender/tests/alfontshaping_test.cpp @@ -53,6 +53,7 @@ #include #include #include +#include #include #include @@ -182,11 +183,12 @@ namespace } // Cache hit contract: shaping the same (slice, root_face) twice - // yields a glyph stream that's bit-for-bit identical to the first. - // This is what callers depend on across frames (pen positions and - // glyph ids must not drift), and it's the load-bearing invariant - // for the LRU's reuse semantics. - TEST_CASE_METHOD(alfontshaping_data, "ALFontShaping: shaping the same slice twice gives a bit-identical glyph stream", "[ALFontShaping]") + // hits the cache and yields a glyph stream that's bit-for-bit + // identical to the first. This is what callers depend on across + // frames (pen positions and glyph ids must not drift), and it's the + // load-bearing invariant for the LRU's reuse semantics. A hit is told + // by cacheMutationCount, which every miss advances and no hit does. + TEST_CASE_METHOD(alfontshaping_data, "ALFontShaping: shaping the same slice again hits the cache with identical glyphs", "[ALFontShaping]") { const std::string path = std::string(kFontDir) + "DejaVuSans.woff2"; if (!fileExists(path)) @@ -197,9 +199,13 @@ namespace const Text s = text('h','e','l','l','o'); std::vector first; ALFontShaping::shapeRun(ft, s, 0, s.size(), first); + REQUIRE_FALSE(first.empty()); + const size_t mutations = ALFontShaping::cacheMutationCount(); std::vector second; ALFontShaping::shapeRun(ft, s, 0, s.size(), second); + UNSCOPED_INFO("the second shape is a cache hit"); + CHECK(ALFontShaping::cacheMutationCount() == mutations); REQUIRE(second.size() == first.size()); for (size_t i = 0; i < first.size(); ++i) { @@ -213,13 +219,13 @@ namespace // clearCacheForFace purges only the entries owned by the named // face — entries from other heads must persist. Verify by populating // the cache from two distinct faces and checking both round-trip - // via shapeLine; clear face A's cache and verify A's cluster - // identity is gone (next shape rebuilds) while B's persists. + // via shapeLine; clear face A's cache and verify A's entry is gone + // (the next shape of A misses and rebuilds it) while B's persists. // - // Without instrumentation we observe "cache cleared" indirectly - // by checking the returned reference identity from shapeLine — - // a cleared entry rebuilds to a fresh storage location. - TEST_CASE_METHOD(alfontshaping_data, "ALFontShaping: clearCacheForFace on one face leaves another face's entry in place", "[ALFontShaping]") + // A miss is told by cacheMutationCount, which every miss advances, + // not by the rebuilt entry's address: the allocator may hand the + // freed storage straight back. + TEST_CASE_METHOD(alfontshaping_data, "ALFontShaping: clearCacheForFace drops one face's entry and leaves another's", "[ALFontShaping]") { const std::string a_path = std::string(kFontDir) + "DejaVuSans.woff2"; const std::string b_path = std::string(kFontDir) + "InterVariable.woff2"; @@ -245,13 +251,17 @@ namespace CHECK(a_second.data() == a_addr0); ALFontShaping::clearCacheForFace(a); + UNSCOPED_INFO("clearCacheForFace(A) dropped one of the two entries"); + CHECK(ALFontShaping::cacheSize() == 1u); - // After clearing A: B's entry survives — its storage address - // is unchanged. (Whether A's entry got reused at the same - // address by the allocator after clear is implementation- - // defined and not part of the contract.) + // After clearing A: shaping A again misses, so A's entry was + // the one dropped, and B's entry survives — its storage address + // is unchanged. + const size_t mutations = ALFontShaping::cacheMutationCount(); const auto& a_after = ALFontShaping::shapeLine(a, s, 0, s.size()); CHECK_FALSE(a_after.empty()); + UNSCOPED_INFO("A's entry was dropped: shaping A again rebuilt it"); + CHECK(ALFontShaping::cacheMutationCount() != mutations); const auto& b_after = ALFontShaping::shapeLine(b, s, 0, s.size()); UNSCOPED_INFO("B cache survived clearCacheForFace(A)"); @@ -335,11 +345,11 @@ namespace REQUIRE_FALSE(first.empty()); UNSCOPED_INFO("first shape leaves one entry"); CHECK(ALFontShaping::cacheSize() == 1u); - // Snapshot the size BEFORE clearCache invalidates `first` — + // Copy the glyphs BEFORE clearCache invalidates `first` — // shapeLine returns a reference into the LRU, so the entry's // storage is gone the moment the LRU is cleared. Reading - // first.size() afterward would be a use-after-free. - const size_t first_size = first.size(); + // `first` afterward would be a use-after-free. + const std::vector first_glyphs = first; const void* addr0 = first.data(); // Cache hit: same storage AND the entry count stays at one. @@ -360,7 +370,14 @@ namespace UNSCOPED_INFO("post-clear miss repopulated to one entry"); CHECK(ALFontShaping::cacheSize() == 1u); UNSCOPED_INFO("post-clear shape has as many glyphs as the first"); - CHECK(after.size() == first_size); + REQUIRE(after.size() == first_glyphs.size()); + for (size_t i = 0; i < after.size(); ++i) + { + CAPTURE(i); + CHECK(after[i].glyph_id == first_glyphs[i].glyph_id); + CHECK(after[i].cluster == first_glyphs[i].cluster); + CHECK(after[i].x_advance == first_glyphs[i].x_advance); + } } // Multi-hop shape run: ASCII + CJK + Emoji through a head (DejaVu) @@ -368,10 +385,11 @@ namespace // itemizes by face coverage so each cluster routes to the right // face. Pins gap audit #3 — multi-hop selectShapingFace was // untested, the existing fallback tests use a single hop only. + // SourceHanSans ships as a collection; loadFt opens its first face. TEST_CASE_METHOD(alfontshaping_data, "ALFontShaping: ASCII then CJK then emoji route to the head and each of two fallbacks", "[ALFontShaping]") { const std::string dejavu = std::string(kFontDir) + "DejaVuSans.woff2"; - const std::string han = std::string(kFontDir) + "SourceHanSans-Regular.woff2"; + const std::string han = std::string(kFontDir) + "SourceHanSans-VF.otf.ttc"; const std::string emoji = std::string(kFontDir) + "Noto-COLRv1.ttf"; if (!fileExists(dejavu) || !fileExists(han) || !fileExists(emoji)) SKIP("DejaVuSans + SourceHanSans + Noto-COLRv1 required"); @@ -446,10 +464,10 @@ namespace // dropped by clearCacheForFace(A). The shape LRU is keyed on root // face only (alfontshaping.cpp:62-64); B's own entries (if any) // survive — we don't double-purge across heads sharing fallbacks. - TEST_CASE_METHOD(alfontshaping_data, "ALFontShaping: clearCacheForFace on a head keeps the entry of its fallback face", "[ALFontShaping]") + TEST_CASE_METHOD(alfontshaping_data, "ALFontShaping: clearCacheForFace on a head keeps its fallback face's entry", "[ALFontShaping]") { const std::string a_path = std::string(kFontDir) + "DejaVuSans.woff2"; - const std::string b_path = std::string(kFontDir) + "SourceHanSans-Regular.woff2"; + const std::string b_path = std::string(kFontDir) + "SourceHanSans-VF.otf.ttc"; if (!fileExists(a_path) || !fileExists(b_path)) SKIP("DejaVuSans + SourceHanSans required"); @@ -466,27 +484,28 @@ namespace const auto& b_first = ALFontShaping::shapeLine(b, s, 0, s.size()); REQUIRE_FALSE(a_first.empty()); REQUIRE_FALSE(b_first.empty()); - const void* a_addr = a_first.data(); const void* b_addr = b_first.data(); + // One of the two entries goes. Which one is told by B's shape + // below hitting, not by A's address: whether A's rebuilt entry + // lands where the old one was is the allocator's business. ALFontShaping::clearCacheForFace(a); + UNSCOPED_INFO("clearCacheForFace(A) dropped one of the two entries"); + CHECK(ALFontShaping::cacheSize() == 1u); - // B's entry survives clearCacheForFace(A): its storage stays - // at the same address. (Whether A's entry got reused by the - // allocator at the same address is implementation-defined and - // not load-bearing for the contract; B's survival is.) const auto& a_after = ALFontShaping::shapeLine(a, s, 0, s.size()); - const auto& b_after = ALFontShaping::shapeLine(b, s, 0, s.size()); CHECK_FALSE(a_after.empty()); - // b_first is a reference into the cache: it dangles unless B's - // entry survived, so this REQUIRE guards the read below. - UNSCOPED_INFO("B's entry survived clearCacheForFace(A)"); - REQUIRE(b_after.data() == b_addr); - // Sanity: the data we read out of the survived B entry is the - // same B stream we observed before — a hidden cross-purge would - // have reset it. - UNSCOPED_INFO("B output unchanged across A's cache flush"); - CHECK(b_after.size() == b_first.size()); + + // B's entry survives clearCacheForFace(A): shaping B again is a + // hit, which leaves cacheMutationCount alone where a purged and + // rebuilt entry would have advanced it, and finds B's storage at + // the same address. + const size_t mutations = ALFontShaping::cacheMutationCount(); + const auto& b_after = ALFontShaping::shapeLine(b, s, 0, s.size()); + UNSCOPED_INFO("B's entry survived clearCacheForFace(A): shaping B again hit it"); + CHECK(ALFontShaping::cacheMutationCount() == mutations); + UNSCOPED_INFO("B's entry survived clearCacheForFace(A): same storage"); + CHECK(b_after.data() == b_addr); } // notdef contract: shaping a codepoint through a head with NO @@ -775,7 +794,7 @@ namespace // up to N glyphs, but never MORE than the input length (which // would mean glyph duplication). UNSCOPED_INFO("output glyph count never exceeds input codepoint count"); - CHECK(out.size() <= s.size()); + CHECK(out.size() <= s.codepoints()); } // Cell-alignment probe. For each monospace face shipped in @@ -832,7 +851,6 @@ namespace // sets the canonical cell. Deliberately don't use // ft->getXAdvance — that path asserts !mIsFallback and // the probe loads as fallback to skip GL pre-warm. - REQUIRE_FALSE(out.empty()); const F32 ref_advance = out[0].x_advance; REQUIRE(ref_advance > 0.f); @@ -942,11 +960,14 @@ namespace // routing monospace through HB, the mark / mkmk features stay enabled // (they're not in kFixedWidthStrict), so HB applies GPOS to position // the mark over its base. Pin: shape "a" + U+0301 (combining acute - // accent) on a monospace face; the cluster has 2 glyphs sharing one - // cluster index, the second has zero x_advance (mark consumes no - // cell), and the mark has non-zero positioning (offset OR cluster - // composition through ccmp into a precomposed 'á'). - TEST_CASE_METHOD(alfontshaping_data, "ALFontShaping: a combining acute on monospace composes or sits as a zero-advance mark", "[ALFontShaping]") + // accent) on a monospace face; either it composes into a precomposed + // 'á', or the cluster has 2 glyphs sharing one cluster index, the + // second has zero x_advance (mark consumes no cell), and GPOS moves + // the mark back over its base (a negative x_offset). HarfBuzz's + // normaliser composes "a" + U+0301 whenever the face has U+00E1, as + // DejaVuSansMono does, so the mark path is pinned again with "q" + + // U+0301, which has no precomposed form. + TEST_CASE_METHOD(alfontshaping_data, "ALFontShaping: a monospace combining acute composes or sits as a GPOS-placed mark", "[ALFontShaping]") { const std::string path = std::string(kFontDir) + "DejaVuSansMono.woff2"; if (!fileExists(path)) @@ -969,7 +990,7 @@ namespace } REQUIRE_FALSE(out.empty()); // Expected outcomes: - // (a) ccmp composed to precomposed 'á' → 1 glyph, full cell advance. + // (a) composed to precomposed 'á' → 1 glyph, full cell advance. // (b) Two glyphs (a + acute) in one HB cluster → first carries // full advance, second has zero advance and the mark // positioned via GPOS. @@ -979,7 +1000,7 @@ namespace CHECK(ref_advance > 0.f); if (out.size() == 1) { - // ccmp composition path. One cell, no second glyph. + // Composition path. One cell, no second glyph. UNSCOPED_INFO("cluster index is 0 (base)"); CHECK(out[0].cluster == 0); } @@ -992,12 +1013,25 @@ namespace CHECK(out[1].cluster == out[0].cluster); UNSCOPED_INFO("mark glyph has zero advance"); CHECK(out[1].x_advance == 0.f); + UNSCOPED_INFO("GPOS moved the mark back over its base"); + CHECK(out[1].x_offset < 0.f); } else { UNSCOPED_INFO("combining cluster produced unexpected glyph count"); CHECK(false); } + + // "q" + U+0301 cannot compose, so it always takes path (b). + const Text q = text(L'q', 0x0301); + ALFontShaping::shapeRun(ft, q, 0, q.size(), out); + REQUIRE(out.size() == 2u); + UNSCOPED_INFO("mark shares base cluster"); + CHECK(out[1].cluster == out[0].cluster); + UNSCOPED_INFO("mark glyph has zero advance"); + CHECK(out[1].x_advance == 0.f); + UNSCOPED_INFO("GPOS moved the mark back over its base"); + CHECK(out[1].x_offset < 0.f); } // ----------------------------------------------------------------- @@ -1077,6 +1111,7 @@ namespace REQUIRE(ft.notNull()); const ALFontFace* face = ft->getFontFace(); + REQUIRE(face != nullptr); FT_Face ftf = face->face(); const FT_Int32 load_flags = static_cast(face->hinting()); @@ -1099,29 +1134,31 @@ namespace } // HB's reported load flags after construction must equal what the - // FT renderGlyph path uses. Both are casts of mHinting; any - // refactor that splits the casts asymmetrically breaks shaped vs - // codepoint advance consistency. hb_ft_font_get_load_flags has - // existed since HB 1.7 — the codebase requires newer than that. + // FT renderGlyph path uses. Both are casts of the hinting the face + // was loaded with; any refactor that splits the casts asymmetrically + // breaks shaped vs codepoint advance consistency. The face loads at + // LIGHT, not the default, so a hinting dropped on its way to either + // cannot pass for it. hb_ft_font_get_load_flags has existed since + // HB 1.7 — the codebase requires newer than that. TEST_CASE_METHOD(alfontshaping_data, "ALFontShaping: HB's load flags are the hinting FT renders glyphs with", "[ALFontShaping]") { const std::string path = std::string(kFontDir) + "DejaVuSans.woff2"; if (!fileExists(path)) SKIP("DejaVuSans.woff2 not present"); - LLPointer ft = loadFt(path); + constexpr EFontHinting hinting = EFontHinting::LIGHT; + LLPointer ft = loadFt(path, /*is_fallback=*/true, /*point_size=*/14.f, /*weight=*/-1, hinting); REQUIRE(ft.notNull()); const ALFontFace* face = ft->getFontFace(); + REQUIRE(face != nullptr); hb_font_t* hbf = face->getHbFont(); REQUIRE(hbf != nullptr); const int hb_flags = hb_ft_font_get_load_flags(hbf); - const int expected = static_cast(face->hinting()); - CHECK(hb_flags == expected); - // And confirm the cast equivalence between the two surfaces - // (HB takes int, FT takes FT_Int32) hasn't drifted. - CHECK(static_cast(face->hinting()) == - (int)static_cast(face->hinting())); + CHECK(hb_flags == static_cast(face->hinting())); + // LLFontFreetype::renderGlyph loads with (FT_Int32) of the hinting + // the face was loaded with. + CHECK(hb_flags == static_cast(static_cast(hinting))); } // HB must see the same OT-VAR design coordinates that FT was @@ -1149,6 +1186,7 @@ namespace /*flags=*/0, va)); const ALFontFace* face = ft->getFontFace(); + REQUIRE(face != nullptr); FT_Face ftf = face->face(); hb_font_t* hbf = face->getHbFont(); REQUIRE(hbf != nullptr); @@ -1156,6 +1194,9 @@ namespace FT_MM_Var* mm = nullptr; CHECK(FT_Get_MM_Var(ftf, &mm) == 0); REQUIRE(mm != nullptr); + // Freed however the case ends, a failed REQUIRE below included. + const std::unique_ptr mm_owner( + mm, [](FT_MM_Var* p) { FT_Done_MM_Var(gFTLibrary, p); }); const FT_UInt num_axis = mm->num_axis; CHECK(num_axis >= 1u); @@ -1177,8 +1218,6 @@ namespace // 1/65536 to cover any internal float-conversion rounding. REQUIRE(delta < (1.f / 65536.f) + 1e-6f); } - - FT_Done_MM_Var(gFTLibrary, mm); } // Stem darkening must be disabled on every FT hinter module — the @@ -1221,6 +1260,7 @@ namespace REQUIRE(ft.notNull()); const ALFontFace* face = ft->getFontFace(); + REQUIRE(face != nullptr); FT_Face ftf = face->face(); REQUIRE(ftf->charmap != nullptr); CHECK((int)ftf->charmap->encoding == (int)FT_ENCODING_UNICODE); @@ -1400,7 +1440,7 @@ namespace TEST_CASE_METHOD(alfontshaping_data, "ALFontShaping: addFallbackFont drops cached shapes so the new fallback is used", "[ALFontShaping]") { const std::string a_path = std::string(kFontDir) + "DejaVuSans.woff2"; - const std::string b_path = std::string(kFontDir) + "SourceHanSans-Regular.woff2"; + const std::string b_path = std::string(kFontDir) + "SourceHanSans-VF.otf.ttc"; if (!fileExists(a_path) || !fileExists(b_path)) SKIP("DejaVuSans + SourceHanSans required"); From 8e022b58831317b24d8b8c7c914c4ac206246fbf Mon Sep 17 00:00:00 2001 From: Rye Date: Tue, 13 Oct 2026 08:34:39 -0400 Subject: [PATCH 4/5] llrender's FreeType, bitmap cache and UI transform tests check what they say they check Old test bugs TM22's idiom pass found (bb3f752d28), fixed. - llfontfreetype: - CHECK(face->isValid()) could not fail: getOrCreateFace returns null for a face that does not load. Where nothing required the face, it is a REQUIRE that it is not null; where something did, it is gone. One stays, after resetBitmapCache, which a broken reset would fail; - faces loadFt returned are required before they are read; - the dark-palette check followed a SKIP for the same condition; the expected index now comes from FreeType's palette flags, and the global flag is put back however the case ends; - the CJK fallback case asked for a font the package does not ship, so it always skipped; it loads face 0 of SourceHanSans-VF.otf.ttc; - hasSvg had no positive case; TwemojiSVG is one; - the functor case's 'a' was not in TwemojiSVG at all, so the functor was never what kept the fallback from it; it uses U+2600, which both fonts have; - the refcount case compared addresses, which a freed and reused block can repeat; a reload is read off the atlas generation; - comments promised a match with FT_Get_Char_Index and a cleared atlas; both are checked now, and a third comment is corrected; - a CHECK(true) is gone. - llfontfreetype_gl: - the throttle case could not see the throttle, since nothing was idle; it moves the frame clock (and puts it back) so the second call comes past the idle threshold but within the interval; - two "not released" checks also held for an index past the end; the index is checked to name a sheet. - llfontbitmapcache: the generation was read before init(), whose reset bumps it, so "a reset bumps it" passed even if reset did not. - llfontbitmapcache_gl: two comments still said vertex buffers compare the global generation. - lluitransform: after a pop only the X scale was checked. Each new or corrected check fails under a small change to the test's own input or expectation, at the check and not by crashing. Co-Authored-By: Claude Opus 5.5 --- .../tests/llfontbitmapcache_gl_test.cpp | 9 +- .../llrender/tests/llfontbitmapcache_test.cpp | 6 +- .../llrender/tests/llfontfreetype_gl_test.cpp | 64 +++++-- indra/llrender/tests/llfontfreetype_test.cpp | 181 +++++++++++------- indra/llrender/tests/lluitransform_test.cpp | 7 +- 5 files changed, 167 insertions(+), 100 deletions(-) diff --git a/indra/llrender/tests/llfontbitmapcache_gl_test.cpp b/indra/llrender/tests/llfontbitmapcache_gl_test.cpp index ccabdaa6f3..c8a0917ab4 100644 --- a/indra/llrender/tests/llfontbitmapcache_gl_test.cpp +++ b/indra/llrender/tests/llfontbitmapcache_gl_test.cpp @@ -379,10 +379,11 @@ namespace } // Cross-instance global generation: two LLFontBitmapCache instances - // mutating in interleaved order each bump the global counter. Pins - // that vertex buffers checking `mLastFontCacheGen` against - // getGlobalGeneration() can detect changes that fired on any cache, - // not just the one they captured against. + // mutating in interleaved order each bump the global counter. Every + // cache draws its generation from it, and LLFontGL::getCacheGeneration + // keys its memo on it, so a change to any cache of a font's chain has + // to move it. (Vertex buffers no longer compare against it directly: + // llfontbitmapcache.h:84-92.) TEST_CASE_METHOD(llfontbitmapcache_gl_data, "LLFontBitmapCacheGL: two caches' allocations and releases each bump the global generation", "[LLFontBitmapCacheGL]") { LLFontBitmapCache c1; c1.init(2, 2); diff --git a/indra/llrender/tests/llfontbitmapcache_test.cpp b/indra/llrender/tests/llfontbitmapcache_test.cpp index 796e66b2b1..8672f9a6ee 100644 --- a/indra/llrender/tests/llfontbitmapcache_test.cpp +++ b/indra/llrender/tests/llfontbitmapcache_test.cpp @@ -67,17 +67,17 @@ namespace TEST_CASE("LLFontBitmapCache: init sizes the sheets; reset clears them and bumps the generation", "[LLFontBitmapCache]") { LLFontBitmapCache c; - const S32 gen_init0 = c.getCacheGeneration(); - c.init(/*max_char_width=*/2, /*max_char_height=*/2); // 2*40=80 → next pow2 ≥ 80 → 128, capped at 1024. CHECK(c.getBitmapWidth() == 128); CHECK(c.getBitmapHeight() == 128); + // Read after init, which resets and so bumps it too. + const S32 gen_before_reset = c.getCacheGeneration(); c.reset(); CHECK(c.getBitmapWidth() == 0); CHECK(c.getBitmapHeight() == 0); - CHECK(c.getCacheGeneration() > gen_init0); + CHECK(c.getCacheGeneration() > gen_before_reset); } // Out-of-range queries on a freshly-constructed (or reset) cache diff --git a/indra/llrender/tests/llfontfreetype_gl_test.cpp b/indra/llrender/tests/llfontfreetype_gl_test.cpp index d6cfddfce0..be8d135689 100644 --- a/indra/llrender/tests/llfontfreetype_gl_test.cpp +++ b/indra/llrender/tests/llfontfreetype_gl_test.cpp @@ -34,6 +34,8 @@ #include "llfonttest_helpers.h" #include "llheadlessgl_fixture.h" +#include "llframetimer.h" + #include #include @@ -54,6 +56,20 @@ namespace ll_test::FontStateScope font_scope; }; + // The frame clock, which the atlas stamps a sheet's use with and the + // glyph sweep reads, set by a case that needs time to have passed. + struct FrameClock : LLFrameTimer + { + static void set(F64 seconds) { sTotalSeconds = seconds; } + }; + + // The clock is the process's: put back however the case ends. + struct FrameClockRestore + { + const F64 mSaved = LLFrameTimer::getTotalSeconds(); + ~FrameClockRestore() { FrameClock::set(mSaved); } + }; + // After getGlyphInfo for an ASCII glyph, the bitmap cache has at // least one Grayscale page, and the returned info has positive @@ -76,14 +92,17 @@ namespace CHECK(cache->getNumBitmaps(EFontGlyphType::Grayscale) >= 1u); } - // collectGarbage is throttled — calling it twice in immediate - // succession does not re-sweep. We observe the throttle by - // confirming the call returns without crashing and the cache - // generation does NOT advance on the second call (only the first - // would invalidate sheets if there were anything to evict). - // Pins 169390b593 — the cache-sweep gating moved out of the - // render hot path. - TEST_CASE_METHOD(llfontfreetype_render_data, "LLFontFreetypeRender: two collectGarbage calls in a row return cleanly", "[LLFontFreetypeRender]") + // collectGarbage is throttled — a call within the interval of the + // last sweep (GC_INTERVAL_SEC, 5 s, in ALFontFace::collectGarbage) + // does not re-sweep, not even for a sheet idle past the threshold + // (IDLE_THRESHOLD_SEC, 15 min) that a sweep would release. The case + // moves the frame clock the sweep reads: the first call comes 899 s + // after the sheet was last used, inside the threshold, and starts + // the interval; the second comes 3 s later, past the threshold and + // inside the interval, so the sheet survives and the cache + // generation does NOT advance. Pins 169390b593 — the cache-sweep + // gating moved out of the render hot path. + TEST_CASE_METHOD(llfontfreetype_render_data, "LLFontFreetypeRender: collectGarbage again within its interval leaves an idle sheet alone", "[LLFontFreetypeRender]") { const std::string path = std::string(kFontDir) + "DejaVuSans.woff2"; if (!fileExists(path)) @@ -93,13 +112,23 @@ namespace // Force at least one glyph onto the atlas so collectGarbage // has something to consider. (void)ft->getGlyphInfo(L'A', EFontGlyphType::Grayscale); + const LLFontBitmapCache* cache = ft->getFontBitmapCache(); + REQUIRE(cache != nullptr); + CHECK(cache->getNumBitmaps(EFontGlyphType::Grayscale) == 1u); + const F64 used = cache->getSheetLastUsedTime(EFontGlyphType::Grayscale, 0); - // Two back-to-back calls: throttle gate keeps the second from - // doing real work. Both must complete without crashing. + FrameClockRestore restore_clock; + FrameClock::set(used + 899.0); ft->collectGarbage(); + const S32 gen = cache->getCacheGeneration(); + + FrameClock::set(used + 902.0); ft->collectGarbage(); - UNSCOPED_INFO("both calls returned"); - CHECK(true); + { + INFO("the throttled call released nothing"); + CHECK_FALSE(cache->isSheetReleased(EFontGlyphType::Grayscale, 0)); + CHECK(cache->getCacheGeneration() == gen); + } } // LLFontManager::collectGarbage drops face cache entries with @@ -122,7 +151,6 @@ namespace (void)ft->getGlyphInfo(L'A', EFontGlyphType::Grayscale); LLPointer face = gFontManagerp->getOrCreateFace(makeKey(path)); REQUIRE(face.notNull()); - CHECK(face->isValid()); const U32 glyph_a = face->getCharGlyphIndex(L'A'); UNSCOPED_INFO("the head rasterized into this face"); REQUIRE(face->findGlyphInfo(glyph_a, EFontGlyphType::Grayscale) != nullptr); @@ -140,7 +168,6 @@ namespace gFontManagerp->collectGarbage(); LLPointer reloaded = gFontManagerp->getOrCreateFace(makeKey(path)); REQUIRE(reloaded.notNull()); - CHECK(reloaded->isValid()); UNSCOPED_INFO("the reloaded face has no glyphs: the cached one was evicted"); CHECK(reloaded->findGlyphInfo(glyph_a, EFontGlyphType::Grayscale) == nullptr); } @@ -266,9 +293,10 @@ namespace // Cross-instance global generation contention: rasterizing on // face A bumps the global counter, then rasterizing on a // different face B bumps it again. Pins that the global counter - // ticks across faces (llfontbitmapcache.h:81-90) so that vertex - // buffers using face B's atlas can detect changes that fired on - // any face. + // ticks across faces (llfontbitmapcache.h:84-92): every atlas draws + // its generation from it, and LLFontGL::getCacheGeneration keys its + // memo on it, so a glyph rasterized on any face of a font's chain + // has to move it. TEST_CASE_METHOD(llfontfreetype_render_data, "LLFontFreetypeRender: rasterizing on each of two faces bumps the global generation", "[LLFontFreetypeRender]") { const std::string a_path = std::string(kFontDir) + "DejaVuSans.woff2"; @@ -360,6 +388,7 @@ namespace LLFontGlyphInfo* b_gi2 = head_b->getGlyphInfo(L'A', EFontGlyphType::Grayscale); REQUIRE(b_gi2 != nullptr); const auto post_entry = b_gi2->mPhaseSlots[0].mBitmapEntry; + CHECK(static_cast(post_entry.second) < cache->getNumBitmaps(post_entry.first)); CHECK_FALSE(cache->isSheetReleased(post_entry.first, static_cast(post_entry.second))); } @@ -409,6 +438,7 @@ namespace LLFontGlyphInfo* g2 = ft->getGlyphInfo(L'Z', EFontGlyphType::Grayscale); REQUIRE(g2 != nullptr); const auto post = g2->mPhaseSlots[0].mBitmapEntry; + CHECK(static_cast(post.second) < ft->getBitmapCache()->getNumBitmaps(post.first)); CHECK_FALSE(ft->getBitmapCache()->isSheetReleased( post.first, static_cast(post.second))); } diff --git a/indra/llrender/tests/llfontfreetype_test.cpp b/indra/llrender/tests/llfontfreetype_test.cpp index d5efb10982..115b0fe397 100644 --- a/indra/llrender/tests/llfontfreetype_test.cpp +++ b/indra/llrender/tests/llfontfreetype_test.cpp @@ -40,6 +40,10 @@ #include +#include +#include FT_FREETYPE_H +#include FT_COLOR_H + #include namespace @@ -103,8 +107,8 @@ namespace LLPointer nf = gFontManagerp->getOrCreateFace(makeKey(noto)); LLPointer df = gFontManagerp->getOrCreateFace(makeKey(dv)); - CHECK(nf->isValid()); - CHECK(df->isValid()); + REQUIRE(nf.notNull()); + REQUIRE(df.notNull()); CHECK(nf->hasColrV1()); CHECK_FALSE(df->hasColrV1()); } @@ -120,22 +124,29 @@ namespace LLPointer nf = gFontManagerp->getOrCreateFace(makeKey(noto)); LLPointer df = gFontManagerp->getOrCreateFace(makeKey(dv)); + REQUIRE(nf.notNull()); + REQUIRE(df.notNull()); CHECK(nf->hasColor()); CHECK_FALSE(df->hasColor()); } - // hasSvg: Noto-COLRv1 doesn't ship OT-SVG; DejaVuSans doesn't - // either. Both should report false. (TwemojiSVG would be the - // positive case but isn't checked into the fixture set.) - TEST_CASE_METHOD(alfontface_data, "ALFontFace: hasSvg is false for both Noto-COLRv1 and DejaVuSans", "[ALFontFace]") + // hasSvg: TwemojiSVG carries an OT-SVG table; Noto-COLRv1 (COLR + // and CPAL) and DejaVuSans (outlines only) do not. + TEST_CASE_METHOD(alfontface_data, "ALFontFace: hasSvg is true for TwemojiSVG and false for Noto-COLRv1 and DejaVuSans", "[ALFontFace]") { - const std::string noto = std::string(kFontDir) + "Noto-COLRv1.ttf"; - const std::string dv = std::string(kFontDir) + "DejaVuSans.woff2"; - if (!fileExists(noto) || !fileExists(dv)) - SKIP("Noto-COLRv1 + DejaVuSans required"); + const std::string twemoji = std::string(kFontDir) + "TwemojiSVG.woff2"; + const std::string noto = std::string(kFontDir) + "Noto-COLRv1.ttf"; + const std::string dv = std::string(kFontDir) + "DejaVuSans.woff2"; + if (!fileExists(twemoji) || !fileExists(noto) || !fileExists(dv)) + SKIP("TwemojiSVG + Noto-COLRv1 + DejaVuSans required"); + LLPointer tf = gFontManagerp->getOrCreateFace(makeKey(twemoji)); LLPointer nf = gFontManagerp->getOrCreateFace(makeKey(noto)); LLPointer df = gFontManagerp->getOrCreateFace(makeKey(dv)); + REQUIRE(tf.notNull()); + REQUIRE(nf.notNull()); + REQUIRE(df.notNull()); + CHECK(tf->hasSvg()); CHECK_FALSE(nf->hasSvg()); CHECK_FALSE(df->hasSvg()); } @@ -156,8 +167,8 @@ namespace LLPointer def = gFontManagerp->getOrCreateFace(k_default); LLPointer bld = gFontManagerp->getOrCreateFace(k_bold); - CHECK(def->isValid()); - CHECK(bld->isValid()); + REQUIRE(def.notNull()); + REQUIRE(bld.notNull()); CHECK_FALSE(def->wghtAxisSet()); CHECK(bld->wghtAxisSet()); } @@ -173,6 +184,8 @@ namespace SKIP("DejaVuSans + DejaVuSansMono required"); LLPointer m = gFontManagerp->getOrCreateFace(makeKey(mono)); LLPointer p = gFontManagerp->getOrCreateFace(makeKey(prop)); + REQUIRE(m.notNull()); + REQUIRE(p.notNull()); CHECK(m->isFixedWidth()); CHECK_FALSE(p->isFixedWidth()); } @@ -186,10 +199,12 @@ namespace if (!fileExists(path)) SKIP("DejaVuSans.woff2 not present"); LLPointer f = gFontManagerp->getOrCreateFace(makeKey(path)); + REQUIRE(f.notNull()); const U32 g_a = f->getCharGlyphIndex(L'A'); const U32 g_a_again = f->getCharGlyphIndex(L'A'); CHECK(g_a != 0u); + CHECK(g_a == FT_Get_Char_Index(f->face(), L'A')); UNSCOPED_INFO("a repeat lookup hits the cache"); CHECK(g_a == g_a_again); UNSCOPED_INFO("an absent codepoint"); @@ -199,10 +214,10 @@ namespace CHECK(f->getCharGlyphIndex(0xE000) == 0u); } - // Refcount sharing: two LLFontFreetype instances that point at - // the same ALFontFace via getOrCreateFace must keep the face - // alive while either lives. The face cache also holds a ref, - // so getNumRefs >= 3 (cache + a + b) when both shares exist. + // Refcount sharing: two getOrCreateFace lookups of one key share + // one ALFontFace, which stays alive while either holds it. The + // face cache also holds a ref, so getNumRefs >= 3 (cache + a + b) + // when both shares exist. TEST_CASE_METHOD(alfontface_data, "ALFontFace: two lookups share a face and the cache keeps it after both let go", "[ALFontFace]") { const std::string path = std::string(kFontDir) + "DejaVuSans.woff2"; @@ -211,10 +226,16 @@ namespace LLPointer a = gFontManagerp->getOrCreateFace(makeKey(path)); LLPointer b = gFontManagerp->getOrCreateFace(makeKey(path)); + REQUIRE(a.notNull()); REQUIRE(a.get() == b.get()); UNSCOPED_INFO("refs held by the cache and a and b"); CHECK(a->getNumRefs() >= 3); + // The atlas generation identifies this face: every face builds an + // atlas of its own, which draws a generation no atlas had before. + // Its address is no evidence -- the allocator may hand the same + // block straight back to a reload. + const S32 atlas_gen = a->getBitmapCache()->getCacheGeneration(); ALFontFace* raw = a.get(); b = nullptr; // drop b's ref UNSCOPED_INFO("face still alive after b dropped"); @@ -222,84 +243,94 @@ namespace a = nullptr; // drop a's ref // Cache still holds a ref, so the face is alive but has // refcount 1. Verify by re-fetching: should return the same - // pointer (cache hit), not a freshly-loaded face. + // face (cache hit), not a freshly-loaded one. LLPointer c = gFontManagerp->getOrCreateFace(makeKey(path)); + REQUIRE(c.notNull()); UNSCOPED_INFO("cache kept face alive after both refs dropped"); - CHECK(c.get() == raw); + CHECK(c->getBitmapCache()->getCacheGeneration() == atlas_gen); } - // resetBitmapCache clears atlas state without destroying the face. - // A subsequent getCharGlyphIndex still works (cmap survives). - TEST_CASE_METHOD(alfontface_data, "ALFontFace: resetBitmapCache keeps the face valid with its cmap and its atlas", "[ALFontFace]") + // resetBitmapCache drops the face's glyphs and resets its atlas + // without destroying the face. A subsequent getCharGlyphIndex still + // works (cmap survives). There is no GL here to rasterize a glyph + // with, so one goes into the face's glyph cache by hand, where the + // rasterizer would put it. + TEST_CASE_METHOD(alfontface_data, "ALFontFace: resetBitmapCache drops glyphs and the atlas and keeps face and cmap", "[ALFontFace]") { const std::string path = std::string(kFontDir) + "DejaVuSans.woff2"; if (!fileExists(path)) SKIP("DejaVuSans.woff2 not present"); LLPointer f = gFontManagerp->getOrCreateFace(makeKey(path)); - CHECK(f->isValid()); + REQUIRE(f.notNull()); + const U32 glyph_a = f->getCharGlyphIndex(L'A'); + f->insertGlyphInfo(glyph_a, new LLFontGlyphInfo(glyph_a, EFontGlyphType::Grayscale)); + const S32 atlas_gen = f->getBitmapCache()->getCacheGeneration(); + f->resetBitmapCache(); CHECK(f->isValid()); UNSCOPED_INFO("cmap survives reset"); CHECK(f->getCharGlyphIndex(L'A') != 0u); - CHECK(f->getBitmapCache() != nullptr); + UNSCOPED_INFO("the glyph is dropped"); + CHECK(f->findGlyphInfo(glyph_a, EFontGlyphType::Grayscale) == nullptr); + REQUIRE(f->getBitmapCache() != nullptr); + UNSCOPED_INFO("the atlas is reset: what was drawn from it rebuilds"); + CHECK(f->getBitmapCache()->getCacheGeneration() > atlas_gen); } // paletteIndex() is computed at load time from LLFontGL::sUseDarkEmojiPalette // — flipping the flag mid-process can't change an already-loaded face's - // index. Test by force-evicting between loads. If Noto-COLRv1 ships no - // FT_PALETTE_FOR_DARK_BACKGROUND palette the second load still returns 0 - // and we skip the equality check rather than fail. Pins b6ba746728's - // load-time read at alfontface.cpp:122-137. + // index. Test by force-evicting between loads. With the flag on, a load + // takes the first palette the font flags FT_PALETTE_FOR_DARK_BACKGROUND, + // and palette 0 where it flags none. Noto-COLRv1 flags none -- its + // CPAL table is version 0, which has no flags -- so with the package's + // font the case pins that fallback. Pins b6ba746728's load-time read + // at alfontface.cpp:122-137. TEST_CASE_METHOD(alfontface_data, "ALFontFace: paletteIndex follows sUseDarkEmojiPalette as it was at load", "[ALFontFace]") { const std::string noto = std::string(kFontDir) + "Noto-COLRv1.ttf"; if (!fileExists(noto)) SKIP("Noto-COLRv1.ttf not present"); - const bool saved_flag = LLFontGL::sUseDarkEmojiPalette; + // The flag is the process's: put back however the case ends. + struct DarkPaletteRestore + { + const bool mSaved = LLFontGL::sUseDarkEmojiPalette; + ~DarkPaletteRestore() { LLFontGL::sUseDarkEmojiPalette = mSaved; } + } restore; // Use an unusual point size so the cache key is fresh. auto k = makeKey(noto, 11.5f); LLFontGL::sUseDarkEmojiPalette = false; - U32 pi_default; { LLPointer f = gFontManagerp->getOrCreateFace(k); - { - INFO("face valid (dark=false)"); - REQUIRE(f.notNull()); - CHECK(f->isValid()); - } - pi_default = f->paletteIndex(); + REQUIRE(f.notNull()); UNSCOPED_INFO("loaded with dark=false"); - CHECK(pi_default == 0u); + CHECK(f->paletteIndex() == 0u); } // Drop our ref; collectGarbage evicts since cache holds the only ref. gFontManagerp->collectGarbage(); LLFontGL::sUseDarkEmojiPalette = true; - U32 pi_dark; + LLPointer f = gFontManagerp->getOrCreateFace(k); + REQUIRE(f.notNull()); + + // The palette a dark background gets, as FreeType reads the font. + U32 dark_palette = 0; + FT_Palette_Data palettes = {}; + if (FT_Palette_Data_Get(f->face(), &palettes) == 0 && palettes.palette_flags) { - LLPointer f = gFontManagerp->getOrCreateFace(k); + for (FT_UShort i = 0; i < palettes.num_palettes; ++i) { - INFO("face valid (dark=true)"); - REQUIRE(f.notNull()); - CHECK(f->isValid()); + if (palettes.palette_flags[i] & FT_PALETTE_FOR_DARK_BACKGROUND) + { + dark_palette = i; + break; + } } - pi_dark = f->paletteIndex(); } - - // Restore early so a failure below doesn't pollute later tests. - LLFontGL::sUseDarkEmojiPalette = saved_flag; - gFontManagerp->collectGarbage(); - - // Noto-COLRv1 may or may not ship a dark-flagged palette. If it - // does, pi_dark is non-zero (the index of that palette); if it - // doesn't, pi_dark stays 0 and we skip the assertion. - if (pi_dark == 0) - SKIP("font has no FT_PALETTE_FOR_DARK_BACKGROUND palette"); - - CHECK(pi_dark != 0u); + UNSCOPED_INFO("loaded with dark=true"); + CHECK(f->paletteIndex() == dark_palette); } // Variable axis on a non-variable font: requesting weight=600 on @@ -317,7 +348,6 @@ namespace k.var_axes.wght_set = true; LLPointer f = gFontManagerp->getOrCreateFace(k); REQUIRE(f.notNull()); - CHECK(f->isValid()); CHECK_FALSE(f->wghtAxisSet()); } @@ -343,8 +373,8 @@ namespace LLPointer f400 = gFontManagerp->getOrCreateFace(k_400); LLPointer f600 = gFontManagerp->getOrCreateFace(k_600); - CHECK(f400->isValid()); - CHECK(f600->isValid()); + REQUIRE(f400.notNull()); + REQUIRE(f600.notNull()); CHECK(f400.get() != f600.get()); } @@ -390,8 +420,8 @@ namespace // Cache identity round-trips through the face manager. LLPointer fd = gFontManagerp->getOrCreateFace(k_default); LLPointer fi = gFontManagerp->getOrCreateFace(k_ital); - CHECK(fd->isValid()); - CHECK(fi->isValid()); + REQUIRE(fd.notNull()); + REQUIRE(fi.notNull()); UNSCOPED_INFO("distinct ital settings are distinct cached faces"); CHECK(fd.get() != fi.get()); } @@ -417,7 +447,6 @@ namespace LLPointer f = gFontManagerp->getOrCreateFace(k); REQUIRE(f.notNull()); - CHECK(f->isValid()); CHECK_FALSE(f->italAxisSet()); CHECK_FALSE(f->wdthAxisSet()); CHECK_FALSE(f->slntAxisSet()); @@ -471,11 +500,8 @@ namespace LLPointer b = gFontManagerp->getOrCreateFace(makeKey(path, 14.f)); LLPointer c = gFontManagerp->getOrCreateFace(makeKey(path, 18.f)); REQUIRE(a.notNull()); - CHECK(a->isValid()); REQUIRE(b.notNull()); - CHECK(b->isValid()); REQUIRE(c.notNull()); - CHECK(c->isValid()); CHECK(a.get() == b.get()); CHECK(a.get() != c.get()); } @@ -499,10 +525,9 @@ namespace 14.f, 96.f, 96.f, true, 0, EFontHinting::DEFAULT, 0); CHECK_FALSE(ok); - // Don't deref ft beyond this — destructor must run cleanly. + // Don't deref ft beyond this — destructor must run cleanly, and + // the case passes by getting past it. ft = nullptr; - UNSCOPED_INFO("destruct after failed load did not crash"); - CHECK(true); } // getCharGlyphIndex returns non-zero for ASCII present in the cmap @@ -593,6 +618,8 @@ namespace LLPointer m = loadFt(mono); LLPointer p = loadFt(prop); + REQUIRE(m.notNull()); + REQUIRE(p.notNull()); CHECK(m->isFixedWidth()); CHECK_FALSE(p->isFixedWidth()); UNSCOPED_INFO("ligatures default off"); @@ -614,6 +641,8 @@ namespace LLPointer n = loadFt(normal); LLPointer b = loadFt(bold); + REQUIRE(n.notNull()); + REQUIRE(b.notNull()); CHECK((n->getStyle() & LLFontGL::BOLD) == 0); CHECK((b->getStyle() & LLFontGL::BOLD) != 0); } @@ -621,11 +650,13 @@ namespace // selectShapingFace walks the fallback chain by codepoint coverage: // ASCII routes to the head, CJK routes to a CJK fallback, emoji // routes to the emoji fallback. Pins multi-hop priority resolution - // at llfontfreetype.cpp:266-284. + // at llfontfreetype.cpp:266-284. The CJK face is the first of the + // package's Source Han Sans collection, which the CJK font in + // fonts.xml loads with the rest. TEST_CASE_METHOD(llfontfreetype_data, "LLFontFreetype: selectShapingFace sends ASCII to the head and CJK and emoji to fallbacks", "[LLFontFreetype]") { const std::string dejavu = std::string(kFontDir) + "DejaVuSans.woff2"; - const std::string han = std::string(kFontDir) + "SourceHanSans-Regular.woff2"; + const std::string han = std::string(kFontDir) + "SourceHanSans-VF.otf.ttc"; const std::string emoji = std::string(kFontDir) + "Noto-COLRv1.ttf"; if (!fileExists(dejavu) || !fileExists(han) || !fileExists(emoji)) SKIP("DejaVuSans + SourceHanSans + Noto-COLRv1 required"); @@ -645,7 +676,7 @@ namespace UNSCOPED_INFO("the head's glyph for 'a'"); CHECK(idx != 0u); - // U+4F60 你 lives in SourceHanSans, not in DejaVuSans → CJK fallback. + // U+4F60 你 lives in Source Han Sans, not in DejaVuSans → CJK fallback. idx = 0; CHECK(head->selectShapingFace(0x4F60, idx) == cjk.get()); @@ -711,15 +742,17 @@ namespace REQUIRE(head.notNull()); REQUIRE(emo.notNull()); - // Functor accepts only emoji-range codepoints; rejects ASCII. - // If TwemojiSVG happens to carry ASCII outlines, the functor - // forces the head to win 'a' regardless. + // Functor accepts only emoji-range codepoints. U+2600 (sun) is + // below that range and in both faces' cmaps, and a functor + // fallback is asked before the head, so only the functor keeps + // Twemoji from it. (TwemojiSVG has no ASCII: 'a' would go to the + // head whatever the functor said.) auto pictograph_only = [](llwchar c) -> bool { return c >= 0x1F000; }; head->addFallbackFont(emo, pictograph_only); U32 idx = 0; - UNSCOPED_INFO("the functor rejects 'a' for Twemoji"); - CHECK(head->selectShapingFace(L'a', idx) == head.get()); + UNSCOPED_INFO("the functor rejects U+2600 for Twemoji"); + CHECK(head->selectShapingFace(0x2600, idx) == head.get()); // Functor allows emoji range, so emoji codepoints route to Twemoji. idx = 0; UNSCOPED_INFO("the functor passes the emoji to Twemoji"); diff --git a/indra/llrender/tests/lluitransform_test.cpp b/indra/llrender/tests/lluitransform_test.cpp index 56c4604d77..09756a2050 100644 --- a/indra/llrender/tests/lluitransform_test.cpp +++ b/indra/llrender/tests/lluitransform_test.cpp @@ -183,8 +183,11 @@ namespace CHECK(ll_round(x) == 40); CHECK(ll_round(y) == 50); } - UNSCOPED_INFO("the scale is off with it"); - CHECK(LLFontGL::sCurScaleX == 1.f); + { + INFO("the scale is off with it"); + CHECK(LLFontGL::sCurScaleX == 1.f); + CHECK(LLFontGL::sCurScaleY == 1.f); + } LLRender2D::popMatrix(); } From fae1eb915873c4797af2278f1d7cc571e0c06050 Mon Sep 17 00:00:00 2001 From: Rye Date: Tue, 13 Oct 2026 08:34:53 -0400 Subject: [PATCH 5/5] llrender's GL object tests check what they say they check Old test bugs TM22's idiom pass found (bb3f752d28), fixed. - llglslshader_gl: - the uniform readers never checked a uniform's location, so a missing one read zeros, and every zero expected passed; each location is required, and so is a list of at most four values; - storage binding points and texture units were used unchecked, where a -1 would end the process in an llassert or leave a check passing for want of anything to read; they are required. - llimagegl: - the mip chain case checked mip 1's width, which glTexStorage2D gives every level whether or not mips were made; it reads mip 1 back and checks it holds the base's colour; - a binding was read just after the test bound the same name itself; the texture is bound through LLRender's slot now; - the texel reader indexed unchecked, sized from the image's size, not GL's; it reads GL's and fails on a texel outside it; - a readback was compared over the full size after checking only its width; its height and components are required too; - "destroyGLTexture leaves the image usable" never used it; it takes a new upload. - algpubuffer_gl: readAll ignored read()'s result, so a failed read showed only as 0xFFFFFFFF words. - alimageslot_gl: its fixture comment named the wrong targets and the wrong reason to skip. - alinfiniteprojection_gl: each occlusion query was deleted only at its case's end, so a failed REQUIRE leaked it on the shared context; the fixture owns it. - alwindowsdlheadless: the worker never checked that its shared context became current, so with none its GL calls did nothing and its error said nothing. It records what the platform reports (WGL, CGL, or EGL through SDL, as llsdl makes the context), makes the texture only then, and the test's thread requires it. A REQUIRE that guarded nothing is a CHECK. Each new or corrected check fails under a small change to the test's own input or expectation, at the check and not by crashing. Most of llglslshader's and all of alimageslot's and three of alinfiniteprojection's run only where the context has compute or clip control, so their changes were compiled here and not run. Co-Authored-By: Claude Opus 5.5 --- indra/llrender/tests/algpubuffer_gl_test.cpp | 7 +- indra/llrender/tests/alimageslot_gl_test.cpp | 4 +- .../tests/alinfiniteprojection_gl_test.cpp | 30 ++++----- .../tests/alwindowsdlheadless_test.cpp | 54 ++++++++++++--- indra/llrender/tests/llglslshader_gl_test.cpp | 36 ++++++++-- indra/llrender/tests/llimagegl_test.cpp | 67 ++++++++++++++----- 6 files changed, 148 insertions(+), 50 deletions(-) diff --git a/indra/llrender/tests/algpubuffer_gl_test.cpp b/indra/llrender/tests/algpubuffer_gl_test.cpp index 647cfda0c5..9d297f9550 100644 --- a/indra/llrender/tests/algpubuffer_gl_test.cpp +++ b/indra/llrender/tests/algpubuffer_gl_test.cpp @@ -66,10 +66,15 @@ namespace return words; } + // What the buffer holds. Called inside a check, so it asserts only on failure: an assertion that passed would + // take the check's UNSCOPED_INFO. std::vector readAll(const ALGPUBuffer& buffer) { std::vector words(buffer.size() / sizeof(U32), 0xFFFFFFFFu); - buffer.read(0, words.data(), words.size() * sizeof(U32)); + if (!buffer.read(0, words.data(), words.size() * sizeof(U32))) + { + FAIL_CHECK("the buffer's read failed"); + } return words; } diff --git a/indra/llrender/tests/alimageslot_gl_test.cpp b/indra/llrender/tests/alimageslot_gl_test.cpp index 3d5c1545eb..08b140206a 100644 --- a/indra/llrender/tests/alimageslot_gl_test.cpp +++ b/indra/llrender/tests/alimageslot_gl_test.cpp @@ -117,8 +117,8 @@ namespace } } - // Compute in use and two R32F targets with a mip level beyond the first, or the case skipped where the - // context has no images. + // Compute in use, an R32F target with a mip level beyond the first and an RGBA16F one without, or the case + // skipped where the context has no compute. void require_targets() { if (!ll_test::contextHasCompute()) diff --git a/indra/llrender/tests/alinfiniteprojection_gl_test.cpp b/indra/llrender/tests/alinfiniteprojection_gl_test.cpp index 5ed6e330eb..77592bf21b 100644 --- a/indra/llrender/tests/alinfiniteprojection_gl_test.cpp +++ b/indra/llrender/tests/alinfiniteprojection_gl_test.cpp @@ -104,6 +104,10 @@ namespace // program the tests change go back with mGL. ~alinfiniteprojection_data() { + if (mQuery) + { + glDeleteQueries(1, &mQuery); + } if (mFBO) { glDeleteFramebuffers(1, &mFBO); @@ -196,6 +200,8 @@ namespace GLuint mDepth = 0; GLuint mVAO = 0; GLuint mProgram = 0; + // A samples-passed query, for the cases that count what a pass shades. + GLuint mQuery = 0; }; @@ -332,19 +338,18 @@ namespace { setUp(); const LLMatrix4a proj = infinite(); - GLuint query = 0; - glGenQueries(1, &query); + glGenQueries(1, &mQuery); const auto sky_samples = [&]() { glDepthFunc(GL_GEQUAL); glDepthMask(GL_FALSE); - glBeginQuery(GL_SAMPLES_PASSED, query); + glBeginQuery(GL_SAMPLES_PASSED, mQuery); draw(proj, 1000.f, true); glEndQuery(GL_SAMPLES_PASSED); glDepthMask(GL_TRUE); glDepthFunc(GL_GREATER); GLuint samples = 0; - glGetQueryObjectuiv(query, GL_QUERY_RESULT, &samples); + glGetQueryObjectuiv(mQuery, GL_QUERY_RESULT, &samples); return samples; }; @@ -365,7 +370,6 @@ namespace REQUIRE(sky_samples() == (GLuint)0); } - glDeleteQueries(1, &query); CHECK((U32)glGetError() == (U32)GL_NO_ERROR); } @@ -377,19 +381,18 @@ namespace const LLMatrix4a proj = infinite(); UNSCOPED_INFO("in front of the far plane is LESS under reverse-Z"); CHECK((U32)LLGLDepthTest::remap(GL_GREATER) == (U32)GL_LESS); - GLuint query = 0; - glGenQueries(1, &query); + glGenQueries(1, &mQuery); const auto haze_samples = [&]() { glDepthFunc(LLGLDepthTest::remap(GL_GREATER)); glDepthMask(GL_FALSE); - glBeginQuery(GL_SAMPLES_PASSED, query); + glBeginQuery(GL_SAMPLES_PASSED, mQuery); draw(proj, 1000.f, true); glEndQuery(GL_SAMPLES_PASSED); glDepthMask(GL_TRUE); glDepthFunc(GL_GREATER); GLuint samples = 0; - glGetQueryObjectuiv(query, GL_QUERY_RESULT, &samples); + glGetQueryObjectuiv(mQuery, GL_QUERY_RESULT, &samples); return samples; }; @@ -414,7 +417,6 @@ namespace UNSCOPED_INFO("haze over the geometry's half alone"); CHECK(haze_samples() == (GLuint)(SIZE * SIZE / 2)); - glDeleteQueries(1, &query); CHECK((U32)glGetError() == (U32)GL_NO_ERROR); } @@ -425,19 +427,18 @@ namespace { setUp(); const LLMatrix4a proj = infinite(); - GLuint query = 0; - glGenQueries(1, &query); + glGenQueries(1, &mQuery); const auto samples = [&](GLenum func) { glDepthFunc(LLGLDepthTest::remap(func)); glDepthMask(GL_FALSE); - glBeginQuery(GL_SAMPLES_PASSED, query); + glBeginQuery(GL_SAMPLES_PASSED, mQuery); draw(proj, 1000.f, true); glEndQuery(GL_SAMPLES_PASSED); glDepthMask(GL_TRUE); glDepthFunc(GL_GREATER); GLuint passed = 0; - glGetQueryObjectuiv(query, GL_QUERY_RESULT, &passed); + glGetQueryObjectuiv(mQuery, GL_QUERY_RESULT, &passed); return passed; }; @@ -470,7 +471,6 @@ namespace UNSCOPED_INFO("every pixel shaded once"); CHECK(lit + sky == (GLuint)(SIZE * SIZE)); - glDeleteQueries(1, &query); CHECK((U32)glGetError() == (U32)GL_NO_ERROR); } } diff --git a/indra/llrender/tests/alwindowsdlheadless_test.cpp b/indra/llrender/tests/alwindowsdlheadless_test.cpp index 41a57d81c2..f6f5d53195 100644 --- a/indra/llrender/tests/alwindowsdlheadless_test.cpp +++ b/indra/llrender/tests/alwindowsdlheadless_test.cpp @@ -31,12 +31,35 @@ #include "llwindow.h" +#if LL_DARWIN +#include +#elif LL_LINUX +#include "SDL3/SDL_video.h" +#endif + #include #include namespace { + // Whether a GL context is current on the calling thread, as the platform + // reports it: a worker's shared context is made outside SDL, so SDL's own + // record of the current context never names it. + bool contextCurrentHere() + { +#if LL_WINDOWS + return wglGetCurrentContext() != nullptr; +#elif LL_DARWIN + return CGLGetCurrentContext() != nullptr; +#else + // EGL, reached through SDL as the shared contexts themselves are. + using GetCurrentContext = void* (*)(); + const auto get_current = reinterpret_cast(SDL_EGL_GetProcAddress("eglGetCurrentContext")); + return get_current && get_current() != nullptr; +#endif + } + struct alwindowsdlheadless_data { // The window is the shared context's, which has the image, render @@ -62,7 +85,7 @@ namespace // createWindow would have returned null on an invalid one; what is // left to see is the SDL window behind it. UNSCOPED_INFO("there is an SDL window behind it"); - REQUIRE(window->getPlatformWindow() != nullptr); + CHECK(window->getPlatformWindow() != nullptr); LLCoordWindow size; REQUIRE(window->getSize(&size)); @@ -81,26 +104,37 @@ namespace void* shared = window->createSharedContext(); REQUIRE(shared != nullptr); + // What the worker saw, checked on this thread once it has ended. + bool current_on_worker = false; GLuint name = 0; GLenum worker_error = GL_NO_ERROR; std::thread worker([&] { window->makeContextCurrent(shared); - glGenTextures(1, &name); - glBindTexture(GL_TEXTURE_2D, name); - const U8 texel[4] = { 0x12, 0x34, 0x56, 0x78 }; - glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA8, 1, 1, 0, GL_RGBA, GL_UNSIGNED_BYTE, texel); - glBindTexture(GL_TEXTURE_2D, 0); - // The object is complete and visible to the share group once the - // commands that made it have run. - glFinish(); - worker_error = glGetError(); + // With no context current, GL does nothing -- or, on macOS, + // crashes -- so the worker makes nothing unless the bind took. + current_on_worker = contextCurrentHere(); + if (current_on_worker) + { + glGenTextures(1, &name); + glBindTexture(GL_TEXTURE_2D, name); + const U8 texel[4] = { 0x12, 0x34, 0x56, 0x78 }; + glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA8, 1, 1, 0, GL_RGBA, GL_UNSIGNED_BYTE, texel); + glBindTexture(GL_TEXTURE_2D, 0); + // The object is complete and visible to the share group once + // the commands that made it have run. + glFinish(); + worker_error = glGetError(); + } window->destroySharedContext(shared); }); worker.join(); + // Unless the shared context was current there, the worker's error + // and texture say nothing. + REQUIRE(current_on_worker); CHECK(worker_error == (GLenum)GL_NO_ERROR); UNSCOPED_INFO("the worker made a texture"); CHECK(name != 0); diff --git a/indra/llrender/tests/llglslshader_gl_test.cpp b/indra/llrender/tests/llglslshader_gl_test.cpp index dea89e7cf8..f41c53625a 100644 --- a/indra/llrender/tests/llglslshader_gl_test.cpp +++ b/indra/llrender/tests/llglslshader_gl_test.cpp @@ -732,13 +732,25 @@ namespace return glGetUniformLocation(mShader.mProgramObject, name); } + // The location of an active uniform. At -1 the read back raises an error and fills nothing, so every + // expectation of 0 would be met. + GLint require_location(const char* name) const + { + const GLint loc = location(name); + CAPTURE(name); + REQUIRE(loc >= 0); + return loc; + } + // What the program holds at a uniform, as GL reads it back, against what a check expects of its first - // components. Floats are compared by their bits, so a check is exact. + // components, at most four. Floats are compared by their bits, so a check is exact. void require_ints(const std::string& what, const char* name, std::initializer_list expected) const { INFO(what); + const GLint loc = require_location(name); std::array held = {}; - glGetUniformiv(mShader.mProgramObject, location(name), held.data()); + REQUIRE(expected.size() <= held.size()); + glGetUniformiv(mShader.mProgramObject, loc, held.data()); size_t component = 0; for (const GLint value : expected) { @@ -751,8 +763,10 @@ namespace void require_uints(const std::string& what, const char* name, std::initializer_list expected) const { INFO(what); + const GLint loc = require_location(name); std::array held = {}; - glGetUniformuiv(mShader.mProgramObject, location(name), held.data()); + REQUIRE(expected.size() <= held.size()); + glGetUniformuiv(mShader.mProgramObject, loc, held.data()); size_t component = 0; for (const GLuint value : expected) { @@ -765,8 +779,10 @@ namespace void require_floats(const std::string& what, const char* name, std::initializer_list expected) const { INFO(what); + const GLint loc = require_location(name); std::array held = {}; - glGetUniformfv(mShader.mProgramObject, location(name), held.data()); + REQUIRE(expected.size() <= held.size()); + glGetUniformfv(mShader.mProgramObject, loc, held.data()); size_t component = 0; for (const F32 value : expected) { @@ -1309,6 +1325,7 @@ namespace { { "test/compactV.glsl", GL_VERTEX_SHADER }, { "test/compactF.glsl", GL_FRAGMENT_SHADER } }, 430)); const S32 point = mShaders.getStorageBlockBinding(TEST_DRAW_LIST); + REQUIRE(point >= 0); UNSCOPED_INFO("the kernel's list at the table's point"); CHECK(mCompute.getStorageBinding(TEST_DRAW_LIST) == point); UNSCOPED_INFO("and the draw's"); @@ -1390,6 +1407,7 @@ namespace setUpCompute(); REQUIRE(buildProgram(mCompute, "block remap (test)", { { "test/remapC.glsl", GL_COMPUTE_SHADER } })); const S32 first = mShaders.getStorageBlockBinding(TEST_BLOCK_FIRST); + REQUIRE(first >= 0); UNSCOPED_INFO("the first block at its point"); CHECK(mCompute.getStorageBinding(TEST_BLOCK_FIRST) == first); UNSCOPED_INFO("the second at the next"); @@ -1531,9 +1549,9 @@ namespace REQUIRE(buildProgram(mDraw3, "integer sample (test)", { { "test/fullscreenV.glsl", GL_VERTEX_SHADER }, { "test/intSampleF.glsl", GL_FRAGMENT_SHADER } })); UNSCOPED_INFO("the unsigned sampler has a unit"); - CHECK(mDraw3.getTextureChannel(TEST_UINT_MAP) >= 0); + REQUIRE(mDraw3.getTextureChannel(TEST_UINT_MAP) >= 0); UNSCOPED_INFO("the signed sampler has a unit"); - CHECK(mDraw3.getTextureChannel(TEST_INT_MAP) >= 0); + REQUIRE(mDraw3.getTextureChannel(TEST_INT_MAP) >= 0); LLGLDisable blend(GL_BLEND); for (U32 source = 0; source < 2; ++source) @@ -1733,6 +1751,8 @@ namespace REQUIRE(buildProgram(mCompute2, "first and counts (test)", { { "test/firstAndCountsC.glsl", GL_COMPUTE_SHADER } })); const S32 first = mShaders.getStorageBlockBinding(TEST_BLOCK_FIRST); const S32 counts = mShaders.getStorageBlockBinding(TEST_COUNTS); + REQUIRE(first >= 0); + REQUIRE(counts >= 0); UNSCOPED_INFO("the first program's block at the table's point"); CHECK(mCompute.getStorageBinding(TEST_BLOCK_FIRST) == first); UNSCOPED_INFO("the second's too"); @@ -1925,6 +1945,10 @@ namespace { { "test/fullscreenV.glsl", GL_VERTEX_SHADER }, { "test/intSampleF.glsl", GL_FRAGMENT_SHADER } })); REQUIRE(buildProgram(mDraw2, "depth read (test)", { { "test/fullscreenV.glsl", GL_VERTEX_SHADER }, { "test/sampleF.glsl", GL_FRAGMENT_SHADER } })); + // The units the reads below bind the texture at. At -1 the bind would go to LLRender's dummy slot and the mode + // to whatever texture the active unit holds. + REQUIRE(mDraw.getTextureChannel(TEST_UINT_MAP) >= 0); + REQUIRE(mDraw2.getTextureChannel(TEST_SOURCE_MAP) >= 0); GLuint texture = 0; glGenTextures(1, &texture); diff --git a/indra/llrender/tests/llimagegl_test.cpp b/indra/llrender/tests/llimagegl_test.cpp index 98f1771776..962064bb79 100644 --- a/indra/llrender/tests/llimagegl_test.cpp +++ b/indra/llrender/tests/llimagegl_test.cpp @@ -42,6 +42,7 @@ #include +#include #include #include #include @@ -83,13 +84,22 @@ namespace glGetTexImage(GL_TEXTURE_2D, level, format, type, out); } - // The texel at (x, y) of level 0 of an RGBA8 texture. + // The texel at (x, y) of level 0 of an RGBA8 texture, which must lie in the level. The level is read at the + // size GL holds it at, which need not be the size the image reports. Called inside a check, so it asserts + // only on failure: an assertion that passed would take the check's UNSCOPED_INFO. static U32 readTexelRGBA(const LLImageGL* img, S32 x, S32 y) { - const S32 w = img->getWidth(); - const S32 h = img->getHeight(); + bindForRead(img); + GLint w = 0; + GLint h = 0; + glGetTexLevelParameteriv(GL_TEXTURE_2D, 0, GL_TEXTURE_WIDTH, &w); + glGetTexLevelParameteriv(GL_TEXTURE_2D, 0, GL_TEXTURE_HEIGHT, &h); + if (x >= w || y >= h) + { + FAIL("texel (" << x << ", " << y << ") is outside the " << w << "x" << h << " level"); + } std::vector px(static_cast(w) * h * 4); - readTexture(img, GL_RGBA, GL_UNSIGNED_BYTE, px.data()); + glGetTexImage(GL_TEXTURE_2D, 0, GL_RGBA, GL_UNSIGNED_BYTE, px.data()); U32 texel = 0; std::memcpy(&texel, px.data() + (static_cast(y) * w + x) * 4, 4); return texel; @@ -112,8 +122,8 @@ namespace // destroyGLTexture must drop the GL name. Subsequent reads see // mTexName == 0 and getHasGLTexture() == false; the LLImageGL - // instance itself remains usable (could be re-uploaded). - TEST_CASE_METHOD(llimagegl_data, "LLImageGL: destroyGLTexture drops the GL name", "[LLImageGL]") + // instance itself remains usable: it takes a new upload. + TEST_CASE_METHOD(llimagegl_data, "LLImageGL: destroyGLTexture drops the GL name and the image takes a new upload", "[LLImageGL]") { LLPointer raw = makeRaw(16, 16, 4, /*fill=*/0x80); LLPointer img = new LLImageGL(/*usemipmaps=*/false); @@ -124,6 +134,11 @@ namespace CHECK((S32)img->getTexName() == 0); CHECK_FALSE(img->getHasGLTexture()); + + // Uploaded again, it holds a texture again. + CHECK(img->createGLTexture(0, raw.get())); + UNSCOPED_INFO("re-uploaded"); + CHECK(img->getHasGLTexture()); } // Regression for 8e85c68d69 — the partial-rect setSubImage path @@ -138,9 +153,12 @@ namespace REQUIRE(img->createGLTexture(0, raw.get())); // Bind the new texture to unit 0 the same way the renderer - // would, then snapshot what GL_TEXTURE_BINDING_2D points at. + // would, through its slot, then snapshot what + // GL_TEXTURE_BINDING_2D points at. createGLTexture ends by + // unbinding the slot, so the texture is there only if the + // slot's bind reached GL. + gGL.getTextureSlot(0)->bind(img.get()); glActiveTexture(GL_TEXTURE0); - glBindTexture(GL_TEXTURE_2D, img->getTexName()); GLint bound_before = 0; glGetIntegerv(GL_TEXTURE_BINDING_2D, &bound_before); @@ -268,11 +286,13 @@ namespace CHECK((S32)img4->getPrimaryFormat() == (S32)GL_RGBA); } - // usemipmaps=true must wire the auto-mipmap path through to GL — - // verify by reading the bound texture's mip 1 dimensions back. Mip - // 1 of an 8x8 base is 4x4; if mipmaps weren't generated the level - // would have width 0. - TEST_CASE_METHOD(llimagegl_data, "LLImageGL: a texture made with usemipmaps has a mip chain in GL", "[LLImageGL]") + // usemipmaps=true must wire the auto-mipmap path through to GL. The + // storage holds the chain: mip 1 of an 8x8 base is 4x4, where a + // texture made without mips has no level 1. That alone does not say + // the mips were generated, since glTexStorage2D allocates every + // level whether or not anything is written to it; but mip 1 of a + // flat base holds the base's value only once it has been. + TEST_CASE_METHOD(llimagegl_data, "LLImageGL: a texture made with usemipmaps has a generated mip chain in GL", "[LLImageGL]") { LLPointer raw = makeRaw(8, 8, 4, /*fill=*/0x80); LLPointer img = new LLImageGL(/*usemipmaps=*/true); @@ -283,8 +303,16 @@ namespace glBindTexture(GL_TEXTURE_2D, img->getTexName()); GLint mip1_w = 0; + GLint mip1_h = 0; glGetTexLevelParameteriv(GL_TEXTURE_2D, 1, GL_TEXTURE_WIDTH, &mip1_w); - CHECK((S32)mip1_w == 4); + glGetTexLevelParameteriv(GL_TEXTURE_2D, 1, GL_TEXTURE_HEIGHT, &mip1_h); + REQUIRE((S32)mip1_w == 4); + REQUIRE((S32)mip1_h == 4); + + std::vector mip1((size_t)4 * 4 * 4, 0); + readTexture(img, GL_RGBA, GL_UNSIGNED_BYTE, mip1.data(), /*level=*/1); + UNSCOPED_INFO("bytes of mip 1 that are not the base's"); + CHECK(std::count_if(mip1.begin(), mip1.end(), [](U8 byte) { return byte != 0x80; }) == 0); } // Regression for 2d4e2a60ea — GL_TEXTURE_SWIZZLE_RGBA must be set @@ -423,8 +451,11 @@ namespace bindForRead(img); GLint w = 0; + GLint h = 0; glGetTexLevelParameteriv(GL_TEXTURE_2D, 0, GL_TEXTURE_WIDTH, &w); + glGetTexLevelParameteriv(GL_TEXTURE_2D, 0, GL_TEXTURE_HEIGHT, &h); REQUIRE((S32)w == (S32)W / 2); + REQUIRE((S32)h == (S32)H / 2); GLint mask[4] = { 0, 0, 0, 0 }; glGetTexParameteriv(GL_TEXTURE_2D, GL_TEXTURE_SWIZZLE_RGBA, mask); @@ -499,8 +530,12 @@ namespace img->endUpload(); REQUIRE(read); - UNSCOPED_INFO("read at the size on screen"); - REQUIRE((S32)dst->getWidth() == (S32)W); + { + INFO("read at the size on screen"); + REQUIRE((S32)dst->getWidth() == (S32)W); + REQUIRE((S32)dst->getHeight() == (S32)H); + } + REQUIRE((S32)dst->getComponents() == 4); UNSCOPED_INFO("and reads what is on screen"); CHECK(std::memcmp(dst->getData(), sd, (size_t)W * H * 4) == 0); }