Fix WebGL texture atlas background cache churn on per-cell background colors

addons/addon-webgl/src/CharAtlasUtils.test.ts

@@ -4,8 +4,9 @@
  */
 
 import { assert } from 'chai';
-import { configEquals } from './CharAtlasUtils';
+import { cacheKeyBg, configEquals } from './CharAtlasUtils';
 import { ICharAtlasConfig } from './Types';
+import { Attributes, BgFlags, FgFlags } from 'common/buffer/Constants';
 import { NULL_COLOR } from 'common/Color';
 import { IColor } from 'common/Types';
 
@@ -85,3 +86,88 @@ describe('CharAtlasUtils', () => {
     });
   });
 });
+
+describe('cacheKeyBg', () => {
+  const RGB_RED = Attributes.CM_RGB | 0xFF0000;
+  const RGB_BLUE = Attributes.CM_RGB | 0x0000FF;
+  const TRANSPARENT = { allowTransparency: true };
+
+  it('should keep the background colour in the key by default', () => {
+    // With an opaque atlas the tile is filled with the background and the
+    // antialiased fringe is blended against it, so two backgrounds cannot
+    // share a glyph.
+    const config = createTestConfig();
+    assert.notStrictEqual(
+      cacheKeyBg(RGB_RED, 0, config),
+      cacheKeyBg(RGB_BLUE, 0, config)
+    );
+    assert.strictEqual(cacheKeyBg(RGB_RED, 0, config), RGB_RED);
+  });
+
+  it('should collapse background colours when the tile cannot depend on them', () => {
+    const config = createTestConfig(TRANSPARENT);
+    assert.strictEqual(
+      cacheKeyBg(RGB_RED, 0, config),
+      cacheKeyBg(RGB_BLUE, 0, config)
+    );
+  });
+
+  it('should collapse every colour mode onto the same key', () => {
+    const config = createTestConfig(TRANSPARENT);
+    const expected = cacheKeyBg(Attributes.CM_DEFAULT, 0, config);
+    assert.strictEqual(cacheKeyBg(Attributes.CM_P16 | 3, 0, config), expected);
+    assert.strictEqual(cacheKeyBg(Attributes.CM_P256 | 200, 0, config), expected);
+    assert.strictEqual(cacheKeyBg(RGB_RED, 0, config), expected);
+  });
+
+  it('should keep the background flags that change what is rasterised', () => {
+    // ITALIC, DIM and OVERLINE live in bg and each alters the tile, so they
+    // must survive even when the colour is dropped.
+    const config = createTestConfig(TRANSPARENT);
+    for (const flag of [BgFlags.ITALIC, BgFlags.DIM, BgFlags.OVERLINE]) {
+      assert.notStrictEqual(
+        cacheKeyBg(RGB_RED | flag, 0, config),
+        cacheKeyBg(RGB_RED, 0, config),
+        `flag ${flag.toString(16)} was dropped from the key`
+      );
+      // The flag survives; only the colour is masked away.
+      assert.strictEqual(cacheKeyBg(RGB_RED | flag, 0, config) & flag, flag);
+      // ...and it still collapses across colours.
+      assert.strictEqual(
+        cacheKeyBg(RGB_RED | flag, 0, config),
+        cacheKeyBg(RGB_BLUE | flag, 0, config)
+      );
+    }
+  });
+
+  it('should keep the background colour for inverse cells', () => {
+    // Inverse promotes the background colour to the foreground, so the tile
+    // depends on it after all. The flag is read from fg.
+    const config = createTestConfig(TRANSPARENT);
+    assert.notStrictEqual(
+      cacheKeyBg(RGB_RED, FgFlags.INVERSE, config),
+      cacheKeyBg(RGB_BLUE, FgFlags.INVERSE, config)
+    );
+    assert.strictEqual(cacheKeyBg(RGB_RED, FgFlags.INVERSE, config), RGB_RED);
+  });
+
+  it('should keep the background colour when a minimum contrast ratio is set', () => {
+    // The foreground is then adjusted against the background.
+    const config = createTestConfig({ ...TRANSPARENT, minimumContrastRatio: 4.5 });
+    assert.notStrictEqual(
+      cacheKeyBg(RGB_RED, 0, config),
+      cacheKeyBg(RGB_BLUE, 0, config)
+    );
+  });
+
+  it('should bound the number of keys over a background that changes per cell', () => {
+    // The workload this exists for: an animated background walks the truecolor
+    // space, and every glyph over it used to take its own atlas entry.
+    const config = createTestConfig(TRANSPARENT);
+    const keys = new Set<number>();
+    for (let i = 0; i < 4096; i++) {
+      keys.add(cacheKeyBg(Attributes.CM_RGB | i, 0, config));
+    }
+    assert.strictEqual(keys.size, 1);
+  });
+});

addons/addon-webgl/src/CharAtlasUtils.ts

@@ -4,7 +4,7 @@
  */
 
 import { ICharAtlasConfig } from './Types';
-import { Attributes } from 'common/buffer/Constants';
+import { Attributes, FgFlags } from 'common/buffer/Constants';
 import { ITerminalOptions } from '@xterm/xterm';
 import { IColorSet, ReadonlyColorSet } from 'browser/Types';
 import { NULL_COLOR } from 'common/Color';
@@ -81,3 +81,41 @@ export function configEquals(a: ICharAtlasConfig, b: ICharAtlasConfig): boolean
 export function is256Color(colorCode: number): boolean {
   return (colorCode & Attributes.CM_MASK) === Attributes.CM_P16 || (colorCode & Attributes.CM_MASK) === Attributes.CM_P256;
 }
+
+/**
+ * The `bg` component of a glyph's atlas cache key.
+ *
+ * `TextureAtlas._drawToCache` reads the *colour* held in `bg` in exactly three
+ * places:
+ *
+ * - `_getBackgroundColor`, which fills the tile. It returns `NULL_COLOR` for
+ *   every background when `allowTransparency` is set, because a translucent
+ *   background must not be baked into the glyph.
+ * - `_getMinimumContrastColor`, which adjusts the foreground against the
+ *   background. It returns early when `minimumContrastRatio` is 1.
+ * - the inverse swap, which promotes the background colour to the foreground.
+ *
+ * When none of the three apply, every background produces a pixel-identical
+ * tile, and keying the cache on the background colour only multiplies the
+ * atlas: one entry per glyph per colour it has ever been drawn over. Content
+ * that varies the background per cell — an animated background, a heatmap, a
+ * diff with highlighted regions, ANSI art — then misses on every cell of every
+ * frame and grows the atlas without bound, since glyphs are never evicted.
+ *
+ * Only the colour bits are dropped. `bg` also carries `BgFlags`, and `ITALIC`,
+ * `DIM` and `OVERLINE` each change what is rasterised, so they must stay part
+ * of the key.
+ *
+ * `INVERSE` is read from `fg`: the flag lives there even though the swap it
+ * describes is what makes the background colour matter.
+ */
+export function cacheKeyBg(bg: number, fg: number, config: ICharAtlasConfig): number {
+  if (
+    !config.allowTransparency ||
+    config.minimumContrastRatio !== 1 ||
+    (fg & FgFlags.INVERSE) !== 0
+  ) {
+    return bg;
+  }
+  return bg & ~(Attributes.CM_MASK | Attributes.RGB_MASK);
+}

addons/addon-webgl/src/TextureAtlas.ts

@@ -4,6 +4,7 @@
  */
 
 import { IColorContrastCache } from 'browser/Types';
+import { cacheKeyBg } from './CharAtlasUtils';
 import { DIM_OPACITY, TEXT_BASELINE } from './Constants';
 import { tryDrawCustomGlyph } from './customGlyphs/CustomGlyphRasterizer';
 import { computeNextVariantOffset, treatGlyphAsBackgroundColor, isPowerlineGlyph, isRestrictedPowerlineGlyph, throwIfFalsy } from 'browser/renderer/shared/RendererUtils';
@@ -298,10 +299,17 @@ export class TextureAtlas implements ITextureAtlas {
     restrictToCellHeight: boolean,
     domContainer: HTMLElement | undefined
   ): IRasterizedGlyph {
-    $glyph = cacheMap.get(key, bg, fg, ext);
+    // Glyphs that cannot depend on the background colour share a single entry
+    // across every background they are drawn over — see `cacheKeyBg`. The real
+    // `bg` is still handed to `_drawToCache`: by that same invariant every
+    // background rasterises an identical tile, so whichever arrives first is
+    // as good as any, and passing the caller's own value keeps this purely a
+    // change to how entries are keyed.
+    const keyBg = cacheKeyBg(bg, fg, this._config);
+    $glyph = cacheMap.get(key, keyBg, fg, ext);
     if (!$glyph) {
       $glyph = this._drawToCache(key, bg, fg, ext, restrictToCellHeight, domContainer);
-      cacheMap.set(key, bg, fg, ext, $glyph);
+      cacheMap.set(key, keyBg, fg, ext, $glyph);
     }
     return $glyph;
   }