From 1747da065c713745a437f5231b0ea94f3e7b06bc Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Mon, 14 Sep 2026 21:05:03 +1000 Subject: [PATCH 1/2] Make DrawingTextCache thread-safe Protect cache metadata, publish complete glyph entries, and lease reusable working buffers to each drawing operation. Keep outline construction and rasterization unchanged. Add tests for concurrent drawing, outline reuse, complete publication, eviction, clearing, and buffer ownership. Update Fonts to 3.1.2 and ImageSharp to 4.1.2. Validation: Full Release suite passed on .NET 8 and .NET 10, with 2,754 passed and 9 skipped per target. --- .../ImageSharp.Drawing.csproj | 4 +- .../Processing/DrawingCanvas{TPixel}.cs | 72 ++-- .../Processing/DrawingTextCache.cs | 249 ++++++++----- .../Processing/RichTextGlyphRenderer.cs | 68 +++- .../Processing/DrawingTextCacheTests.cs | 330 ++++++++++++++++++ .../Processing/RichTextGlyphRendererTests.cs | 62 +++- 6 files changed, 622 insertions(+), 163 deletions(-) create mode 100644 tests/ImageSharp.Drawing.Tests/Processing/DrawingTextCacheTests.cs diff --git a/src/ImageSharp.Drawing/ImageSharp.Drawing.csproj b/src/ImageSharp.Drawing/ImageSharp.Drawing.csproj index 16b60759..c1795038 100644 --- a/src/ImageSharp.Drawing/ImageSharp.Drawing.csproj +++ b/src/ImageSharp.Drawing/ImageSharp.Drawing.csproj @@ -44,8 +44,8 @@ - - + + diff --git a/src/ImageSharp.Drawing/Processing/DrawingCanvas{TPixel}.cs b/src/ImageSharp.Drawing/Processing/DrawingCanvas{TPixel}.cs index 4fee64a5..81e40129 100644 --- a/src/ImageSharp.Drawing/Processing/DrawingCanvas{TPixel}.cs +++ b/src/ImageSharp.Drawing/Processing/DrawingCanvas{TPixel}.cs @@ -72,26 +72,6 @@ public sealed class DrawingCanvas : DrawingCanvas /// private readonly DrawingTextCache textCache; - /// - /// Reusable operation list handed to each text renderer. Hosted by the text cache because - /// canvases are per-frame objects; sharing the cache-owned list keeps its capacity across - /// frames instead of regrowing a fresh list of large operation structs per draw. - /// - private readonly List textOperations; - - /// - /// Reusable sort buffer for , hosted by the text cache for - /// the same reason as . Only pass and index pairs are sorted; - /// the operations themselves stay in place so the per-draw sort moves eight bytes per - /// entry instead of the full operation struct. - /// - private readonly List<(byte RenderPass, int Sequence)> textOperationSortBuffer; - - /// - /// Reusable stack pairing the begin and end commands for nested text composite layers. - /// - private readonly List textCompositeLayerStack; - /// /// Initializes a new instance of the class. /// @@ -303,9 +283,6 @@ private DrawingCanvas( this.targetFrame = targetFrame; this.batcher = batcher; this.textCache = textCache; - this.textOperations = textCache.OperationScratch; - this.textOperationSortBuffer = textCache.OperationSortScratch; - this.textCompositeLayerStack = textCache.CompositeLayerScratch; this.ownsBatcher = ownsBatcher; this.ownsTextCache = ownsTextCache; this.pendingImageResources = pendingImageResources; @@ -869,11 +846,11 @@ configuredOptions.VisibleBounds is null && }; } - using RichTextGlyphRenderer glyphRenderer = new(effectiveOptions, configuredPath, pen, brush, this.textCache, this.textOperations); + using RichTextGlyphRenderer glyphRenderer = new(effectiveOptions, configuredPath, pen, brush, this.textCache); TextRenderer renderer = new(glyphRenderer); renderer.Render(text, configuredOptions); - this.DrawTextOperations(glyphRenderer.DrawingOperations, effectiveOptions); + this.DrawTextOperations(glyphRenderer.DrawingOperations, glyphRenderer.Scratch, effectiveOptions); } /// @@ -900,7 +877,7 @@ public override void DrawText( Matrix4x4.CreateTranslation(location.X, location.Y, 0) * effectiveOptions.Transform, effectiveOptions.TextContrast); - using RichTextGlyphRenderer glyphRenderer = new(placedOptions, path: null, pen, brush, this.textCache, this.textOperations); + using RichTextGlyphRenderer glyphRenderer = new(placedOptions, path: null, pen, brush, this.textCache); if (TryGetVisibleTextBounds(state, placedOptions.Transform, out FontRectangle visibleBounds)) { textBlock.RenderTo(glyphRenderer, wrappingLength, visibleBounds); @@ -910,7 +887,7 @@ public override void DrawText( textBlock.RenderTo(glyphRenderer, wrappingLength); } - this.DrawTextOperations(glyphRenderer.DrawingOperations, placedOptions); + this.DrawTextOperations(glyphRenderer.DrawingOperations, glyphRenderer.Scratch, placedOptions); } /// @@ -929,10 +906,10 @@ public override void DrawText( DrawingCanvasState state = this.ResolveState(); DrawingOptions effectiveOptions = state.Options; - using RichTextGlyphRenderer glyphRenderer = new(effectiveOptions, path, pen, brush, this.textCache, this.textOperations); + using RichTextGlyphRenderer glyphRenderer = new(effectiveOptions, path, pen, brush, this.textCache); textBlock.RenderTo(glyphRenderer, wrappingLength); - this.DrawTextOperations(glyphRenderer.DrawingOperations, effectiveOptions); + this.DrawTextOperations(glyphRenderer.DrawingOperations, glyphRenderer.Scratch, effectiveOptions); } /// @@ -958,10 +935,10 @@ public override void DrawText( Matrix4x4.CreateTranslation(location.X, location.Y, 0) * effectiveOptions.Transform, effectiveOptions.TextContrast); - using RichTextGlyphRenderer glyphRenderer = new(placedOptions, path: null, pen, brush, this.textCache, this.textOperations); + using RichTextGlyphRenderer glyphRenderer = new(placedOptions, path: null, pen, brush, this.textCache); lineLayout.RenderTo(glyphRenderer); - this.DrawTextOperations(glyphRenderer.DrawingOperations, placedOptions); + this.DrawTextOperations(glyphRenderer.DrawingOperations, glyphRenderer.Scratch, placedOptions); } /// @@ -979,10 +956,10 @@ public override void DrawText( DrawingCanvasState state = this.ResolveState(); DrawingOptions effectiveOptions = state.Options; - using RichTextGlyphRenderer glyphRenderer = new(effectiveOptions, path, pen, brush, this.textCache, this.textOperations); + using RichTextGlyphRenderer glyphRenderer = new(effectiveOptions, path, pen, brush, this.textCache); lineLayout.RenderTo(glyphRenderer); - this.DrawTextOperations(glyphRenderer.DrawingOperations, effectiveOptions); + this.DrawTextOperations(glyphRenderer.DrawingOperations, glyphRenderer.Scratch, effectiveOptions); } /// @@ -999,11 +976,11 @@ public override void DrawText( DrawingCanvasState state = this.ResolveState(); DrawingOptions effectiveOptions = state.Options; - using RichTextGlyphRenderer glyphRenderer = new(effectiveOptions, path: null, pen, brush, this.textCache, this.textOperations); + using RichTextGlyphRenderer glyphRenderer = new(effectiveOptions, path: null, pen, brush, this.textCache); TextRenderer renderer = new(glyphRenderer); renderer.Render(glyphId, options); - this.DrawTextOperations(glyphRenderer.DrawingOperations, effectiveOptions); + this.DrawTextOperations(glyphRenderer.DrawingOperations, glyphRenderer.Scratch, effectiveOptions); } /// @@ -1023,11 +1000,11 @@ public override void DrawText(ReadOnlySpan glyphIds, ReadOnlySpan @@ -1801,17 +1778,20 @@ private static IPath GetPositionedGlyphPath(DrawingTextCache.RunPathCacheEntry e /// Converts rendered text operations to composition commands and submits them to the batcher. /// /// Text drawing operations produced by glyph layout/rendering. + /// The working buffers leased by this draw's renderer. /// Drawing options applied to each operation. - private void DrawTextOperations(List operations, DrawingOptions drawingOptions) + private void DrawTextOperations( + List operations, + DrawingTextCache.DrawingScratch scratch, + DrawingOptions drawingOptions) { // Enforce render-pass ordering while preserving original emission order inside each // pass. This preserves overlapping color-font layer compositing semantics (for // example emoji mouth/teeth layers) and keeps the composite group markers paired // with the fill operations they contain. - // The cache-owned buffer keeps its capacity across draws; draw calls never overlap on - // one canvas. - List<(byte RenderPass, int Sequence)> entries = this.textOperationSortBuffer; - entries.Clear(); + // The renderer leases these buffers until command submission completes. Other canvases + // sharing the cache rent different buffers, while sequential draws retain capacity. + List<(byte RenderPass, int Sequence)> entries = scratch.SortBuffer; // Queued glyph commands never carry the canvas transform: glyph geometry arrives with // it already applied, and the sub-pixel remainder rides the command itself. One shared @@ -1832,8 +1812,7 @@ private void DrawTextOperations(List operations, DrawingOption return cmp != 0 ? cmp : a.Sequence.CompareTo(b.Sequence); }); - List compositeLayers = this.textCompositeLayerStack; - compositeLayers.Clear(); + List compositeLayers = scratch.CompositeLayers; DrawingCanvasState state = this.ResolveState(); for (int i = 0; i < entries.Count; i++) @@ -1879,11 +1858,6 @@ private void DrawTextOperations(List operations, DrawingOption this.batcher.AddStrokePath(((StrokePathCompositionSceneCommand)command).Command); } } - - // The buffers outlive the canvas (the text cache hosts them), so drop the layer - // references now rather than rooting the final draw's state until the next draw. - entries.Clear(); - compositeLayers.Clear(); } /// diff --git a/src/ImageSharp.Drawing/Processing/DrawingTextCache.cs b/src/ImageSharp.Drawing/Processing/DrawingTextCache.cs index fbb42304..e8d025e3 100644 --- a/src/ImageSharp.Drawing/Processing/DrawingTextCache.cs +++ b/src/ImageSharp.Drawing/Processing/DrawingTextCache.cs @@ -11,15 +11,7 @@ namespace SixLabors.ImageSharp.Drawing.Processing; /// Stores reusable text drawing data shared by one or more drawing canvases. /// /// -/// Two tiers are cached. The glyph cache holds one flattened outline per glyph, keyed by glyph id, -/// size and pen; it is the base layer that avoids re-flattening the same glyph and is shared by -/// every run. The run-path cache is derived from it: the glyphs of a whole uniform run merged into a -/// single positioned path, so that redrawing the run collapses to one composition command instead of -/// one per glyph. The run path is keyed in run-local space, so the same run content drawn at any -/// position, including a fractionally scrolled one, is a hit. The cost is memory, because the merged -/// path holds a copy of each glyph's geometry; far fewer run paths than glyph entries are kept, only -/// whole-run repeats benefit, and a run that differs by a single glyph misses and falls back to the -/// per-glyph commands. +/// This class is thread-safe. Concurrent drawing operations must use separate canvas instances. /// public sealed class DrawingTextCache { @@ -35,6 +27,31 @@ public sealed class DrawingTextCache /// private const int RunPathCapacityDivisor = 4; + /// + /// Protects cache metadata and the scratch pool, never glyph construction or drawing. + /// + private readonly object sync = new(); + + /// + /// Idle draw buffers. Renting transfers exclusive ownership until the renderer is disposed. + /// + private readonly Stack scratchPool = new(); + + // Two tiers are cached. The glyph cache holds reusable outline data keyed by glyph identity, + // size and pen. It avoids rebuilding glyph outlines and supplies the geometry used by every + // run. Layered glyphs retain their complete layer and composite-group sequence. + // + // The run-path cache is derived from the glyph cache: the glyphs of a whole uniform run are + // merged into a single positioned path, so redrawing the run requires one composition command + // instead of one per glyph. The key uses run-local positions, allowing the same run to be + // reused at different locations, including during fractional scrolling. Only whole-run + // repeats benefit; a run that differs by one glyph misses and reuses the individual glyph + // entries to construct its own combined path. + // + // Combined paths retain additional geometry, so the run-path cache has a smaller capacity + // than the glyph cache. This limits memory retention while preserving reuse of individual + // glyphs across different runs. + // // Both caches are LRU: the dictionary provides O(1) lookup while the linked list // tracks usage order (most recently used at the head, eviction from the tail). @@ -96,42 +113,76 @@ public DrawingTextCache(int capacity) /// /// Gets the number of glyph cache entries. Run-path entries are not included. /// - public int Count => this.entries.Count; + public int Count + { + get + { + lock (this.sync) + { + return this.entries.Count; + } + } + } /// - /// Gets the reusable drawing-operation scratch list handed to text renderers. Canvases are - /// per-frame objects while this cache survives across frames, so hosting the scratch here - /// keeps its capacity instead of regrowing a list of large operation structs every draw. - /// The list is cleared at the start of each text draw; like the caches on this type it - /// assumes single-threaded use. + /// Rents exclusive working buffers for one text draw, retaining capacity across frames. /// - internal List OperationScratch { get; } = []; + /// The working buffers owned by the renderer until it is disposed. + internal DrawingScratch RentScratch() + { + lock (this.sync) + { + if (this.scratchPool.Count > 0) + { + return this.scratchPool.Pop(); + } + } - /// - /// Gets the reusable render-pass sort buffer used before text operations are lowered to - /// composition commands, hosted here for the same lifetime reason as - /// . Entries index into the operation list so the sort - /// moves pass and index pairs rather than whole operation structs. - /// - internal List<(byte RenderPass, int Sequence)> OperationSortScratch { get; } = []; + return new DrawingScratch(); + } /// - /// Gets the reusable stack used to pair nested text composite layer commands. + /// Returns working buffers after all operations have been consumed or the draw has failed. /// - internal List CompositeLayerScratch { get; } = []; + /// The exclusively owned working buffers to return. + internal void ReturnScratch(DrawingScratch scratch) + { + // Drop per-draw references outside the lock while retaining the lists' capacity. + scratch.Operations.Clear(); + scratch.SortBuffer.Clear(); + scratch.CompositeLayers.Clear(); + + lock (this.sync) + { + // Match the backend worker pool's bound so a concurrency spike does not retain + // arbitrarily many large operation buffers for the lifetime of this cache. + if (this.scratchPool.Count < Environment.ProcessorCount) + { + this.scratchPool.Push(scratch); + } + } + } /// /// Removes all cached text drawing data. /// + /// + /// Draws already in progress can continue using previously cached data and can populate + /// the cache again after this method returns. + /// public void Clear() { - this.entries.Clear(); - this.usage.Clear(); - this.runPathEntries.Clear(); - this.runPathUsage.Clear(); - this.OperationScratch.Clear(); - this.OperationSortScratch.Clear(); - this.CompositeLayerScratch.Clear(); + lock (this.sync) + { + this.entries.Clear(); + this.usage.Clear(); + this.runPathEntries.Clear(); + this.runPathUsage.Clear(); + + // Active renderers own their buffers and cached values independently of these + // indexes. Clearing must not mutate either while a draw is consuming them. + this.scratchPool.Clear(); + } } /// @@ -144,47 +195,51 @@ public void Clear() /// internal bool TryGetValue(RichTextGlyphRenderer.CacheKey key, [NotNullWhen(true)] out List? value) { - if (!this.entries.TryGetValue(key, out LinkedListNode? node)) + lock (this.sync) { - value = null; - return false; - } + if (!this.entries.TryGetValue(key, out LinkedListNode? node)) + { + value = null; + return false; + } - // Move the hit to the head so the least recently used entry stays at the tail. - this.usage.Remove(node); - this.usage.AddFirst(node); - value = node.Value.Value; - return true; + // Move the hit to the head so the least recently used entry stays at the tail. + this.usage.Remove(node); + this.usage.AddFirst(node); + value = node.Value.Value; + return true; + } } /// - /// Gets existing glyph drawing data for the specified key, or creates a new cache entry. + /// Publishes a complete glyph. Ownership of the list transfers to the cache; it must not + /// be modified after this call, including when another draw has already populated the key. /// /// The glyph cache key. - /// - /// The glyph drawing data associated with . - /// - internal List GetOrAdd(RichTextGlyphRenderer.CacheKey key) + /// The complete glyph entries in callback order. + internal void Add(RichTextGlyphRenderer.CacheKey key, List value) { - if (this.TryGetValue(key, out List? value)) + lock (this.sync) { - return value; - } + // Misses build outside the lock. Keep the first complete result if two draws + // built the same key, rather than combining their layer sequences. + if (this.entries.ContainsKey(key)) + { + return; + } - value = []; - LinkedListNode node = new(new Entry(key, value)); - this.usage.AddFirst(node); - this.entries.Add(key, node); + LinkedListNode node = new(new Entry(key, value)); + this.usage.AddFirst(node); + this.entries.Add(key, node); - // Evict the least recently used entry once over capacity. - if (this.entries.Count > this.Capacity) - { - LinkedListNode last = this.usage.Last!; - this.usage.RemoveLast(); - _ = this.entries.Remove(last.Value.Key); + // Evict the least recently used entry once over capacity. + if (this.entries.Count > this.Capacity) + { + LinkedListNode last = this.usage.Last!; + this.usage.RemoveLast(); + _ = this.entries.Remove(last.Value.Key); + } } - - return value; } /// @@ -197,17 +252,20 @@ internal bool TryGetValue(RichTextGlyphRenderer.CacheKey key, [NotNullWhen(true) /// internal bool TryGetRunPath(RunPathCacheKey key, [NotNullWhen(true)] out IPath? path) { - if (!this.runPathEntries.TryGetValue(key, out LinkedListNode? node)) + lock (this.sync) { - path = null; - return false; - } + if (!this.runPathEntries.TryGetValue(key, out LinkedListNode? node)) + { + path = null; + return false; + } - // Move the hit to the head so the least recently used entry stays at the tail. - this.runPathUsage.Remove(node); - this.runPathUsage.AddFirst(node); - path = node.Value.Path; - return true; + // Move the hit to the head so the least recently used entry stays at the tail. + this.runPathUsage.Remove(node); + this.runPathUsage.AddFirst(node); + path = node.Value.Path; + return true; + } } /// @@ -217,16 +275,30 @@ internal bool TryGetRunPath(RunPathCacheKey key, [NotNullWhen(true)] out IPath? /// The positioned path to cache. internal void AddRunPath(RunPathCacheKey key, IPath path) { - LinkedListNode node = new(new RunPathEntry(key, path)); - this.runPathUsage.AddFirst(node); - this.runPathEntries.Add(key, node); + // Bounds are lazily stored as a nullable struct by paths. Initialize them before + // sharing the path so concurrent command creation never races that first write. + _ = path.Bounds; - // Evict the least recently used entry once over capacity. - if (this.runPathEntries.Count > this.runPathCapacity) + lock (this.sync) { - LinkedListNode last = this.runPathUsage.Last!; - this.runPathUsage.RemoveLast(); - _ = this.runPathEntries.Remove(last.Value.Key); + // Positioned paths are built outside the lock, so concurrent misses may publish + // the same key. Keep the first completed path in the cache. + if (this.runPathEntries.ContainsKey(key)) + { + return; + } + + LinkedListNode node = new(new RunPathEntry(key, path)); + this.runPathUsage.AddFirst(node); + this.runPathEntries.Add(key, node); + + // Evict the least recently used entry once over capacity. + if (this.runPathEntries.Count > this.runPathCapacity) + { + LinkedListNode last = this.runPathUsage.Last!; + this.runPathUsage.RemoveLast(); + _ = this.runPathEntries.Remove(last.Value.Key); + } } } @@ -493,4 +565,25 @@ public RunPathEntry(RunPathCacheKey key, IPath path) /// public IPath Path { get; } } + + /// + /// Working buffers leased to one renderer through operation submission and disposal. + /// + internal sealed class DrawingScratch + { + /// + /// Gets the drawing operations emitted by this renderer. + /// + public List Operations { get; } = []; + + /// + /// Gets the render-pass index buffer, avoiding sorting full operation structs. + /// + public List<(byte RenderPass, int Sequence)> SortBuffer { get; } = []; + + /// + /// Gets the stack pairing nested text composite layer commands. + /// + public List CompositeLayers { get; } = []; + } } diff --git a/src/ImageSharp.Drawing/Processing/RichTextGlyphRenderer.cs b/src/ImageSharp.Drawing/Processing/RichTextGlyphRenderer.cs index 3073a590..c7b17e7a 100644 --- a/src/ImageSharp.Drawing/Processing/RichTextGlyphRenderer.cs +++ b/src/ImageSharp.Drawing/Processing/RichTextGlyphRenderer.cs @@ -188,12 +188,17 @@ internal sealed partial class RichTextGlyphRenderer : BaseGlyphBuilder private CacheKey currentCacheKey; /// - /// The cache entries for the current glyph key. Assigned on a cache hit and only read on - /// hit paths; the miss path appends through the cache-owned list instead, so no per-glyph - /// list is allocated here. + /// Completed cache entries for the current glyph. Published lists are never modified, + /// so a renderer can consume them outside the cache lock, even after eviction or Clear. /// private List? currentCacheEntries; + /// + /// Entries being built for a cache miss. The list transfers to the cache only after the + /// entire glyph has completed, so other draws cannot see an incomplete layer sequence. + /// + private List? pendingCacheEntries; + /// /// The transformed (post-) bounding-box location /// of the current glyph. Stored so can compute @@ -209,25 +214,18 @@ internal sealed partial class RichTextGlyphRenderer : BaseGlyphBuilder /// Default pen for outlined text, or for fill-only. /// Default brush for filled text, or for outline-only. /// Caller-owned glyph cache shared across renderer instances. - /// - /// The caller-owned operation list this renderer emits into. Passing the canvas's reusable - /// list keeps its capacity across draws instead of regrowing a fresh list per call; it is - /// cleared in and must not be shared by concurrently live renderers. - /// public RichTextGlyphRenderer( DrawingOptions drawingOptions, IPath? path, Pen? pen, Brush? brush, - DrawingTextCache glyphCache, - List operations) + DrawingTextCache glyphCache) : base(drawingOptions.Transform) { this.drawingOptions = drawingOptions; this.defaultPen = pen; this.defaultBrush = brush; this.glyphCache = glyphCache; - this.DrawingOperations = operations; this.currentCompositionMode = drawingOptions.GraphicsOptions.AlphaCompositionMode; this.currentBlendingMode = drawingOptions.GraphicsOptions.ColorBlendingMode; @@ -254,6 +252,8 @@ public RichTextGlyphRenderer( this.rasterizationRequired = true; this.noCache = true; } + + this.Scratch = glyphCache.RentScratch(); } /// @@ -261,7 +261,12 @@ public RichTextGlyphRenderer( /// After RenderText completes, this list is consumed by /// to build composition commands. /// - public List DrawingOperations { get; } + public List DrawingOperations => this.Scratch.Operations; + + /// + /// Gets the exclusive working buffers retained until this renderer is disposed. + /// + public DrawingTextCache.DrawingScratch Scratch { get; } /// /// Gets a value indicating whether per-grapheme glyph collections are aggregated. @@ -660,6 +665,7 @@ protected override void EndGlyph() { // The layer has already been rendered. this.hasLayer = false; + this.PublishGlyph(); return; } @@ -713,6 +719,7 @@ protected override void EndGlyph() this.UpdateCache(renderData); } + this.PublishGlyph(); return; } @@ -792,6 +799,8 @@ protected override void EndGlyph() GlyphClip = this.currentGlyphClip }); } + + this.PublishGlyph(); } /// @@ -1029,8 +1038,8 @@ private void RecordMarker(GlyphRenderData entry) } /// - /// Stores a entry in the glyph cache under the - /// current key. Creates the cache list on first insertion for a given key. Every entry + /// Appends a entry to the private pending glyph list. + /// Creates the list on the first callback that produces cacheable data. Every entry /// is stamped with the glyph's build-time transformed metric origin so layered replays /// can derive the positional delta for paint brushes. /// @@ -1038,7 +1047,30 @@ private void RecordMarker(GlyphRenderData entry) private void UpdateCache(GlyphRenderData renderData) { renderData.SourceOrigin = this.currentTransformedBoundsLocation; - this.glyphCache.GetOrAdd(this.currentCacheKey).Add(renderData); + + // Path bounds use a lazy nullable-struct field. Materialize it while the translated + // path is still private; later canvases may read these bounds concurrently. + if (renderData.FillPath is not null) + { + _ = renderData.FillPath.Bounds; + } + + this.pendingCacheEntries ??= []; + this.pendingCacheEntries.Add(renderData); + } + + /// + /// Publishes a successfully completed glyph without copying its entries. + /// + private void PublishGlyph() + { + // Cache hits and non-cacheable glyphs have no pending list. On a miss, transfer + // ownership once, after all layers and group markers have been recorded. + if (this.pendingCacheEntries is not null) + { + this.glyphCache.Add(this.currentCacheKey, this.pendingCacheEntries); + this.pendingCacheEntries = null; + } } /// @@ -1058,8 +1090,10 @@ protected override void Dispose(bool disposing) this.isDisposed = true; if (disposing) { - // The glyph cache is owned outside this renderer and outlives this draw call. - this.DrawingOperations.Clear(); + // Return all buffers even if layout or command submission threw. A partial glyph + // remains private and is discarded rather than being published by cleanup. + this.pendingCacheEntries = null; + this.glyphCache.ReturnScratch(this.Scratch); } base.Dispose(disposing); diff --git a/tests/ImageSharp.Drawing.Tests/Processing/DrawingTextCacheTests.cs b/tests/ImageSharp.Drawing.Tests/Processing/DrawingTextCacheTests.cs new file mode 100644 index 00000000..b840f191 --- /dev/null +++ b/tests/ImageSharp.Drawing.Tests/Processing/DrawingTextCacheTests.cs @@ -0,0 +1,330 @@ +// Copyright (c) Six Labors. +// Licensed under the Six Labors Split License. + +using System.Numerics; +using Moq; +using SixLabors.Fonts; +using SixLabors.Fonts.Rendering; +using SixLabors.Fonts.Unicode; +using SixLabors.ImageSharp.Drawing.Processing; +using SixLabors.ImageSharp.Drawing.Processing.Processors.Text; +using SixLabors.ImageSharp.Drawing.Tests.TestUtilities.ImageComparison; +using SixLabors.ImageSharp.PixelFormats; + +namespace SixLabors.ImageSharp.Drawing.Tests.Processing; + +public class DrawingTextCacheTests +{ + /// + /// Verifies exclusive buffer ownership across overlapping draws, clearing, and disposal. + /// + [Fact] + public void OverlappingRenderers_ClearAndDisposePreserveOtherDraw() + { + Font font = TestFontUtilities.GetFont(TestFonts.OpenSans, 24); + DrawingTextCache cache = new(); + using RichTextGlyphRenderer first = new(new DrawingOptions(), null, null, Brushes.Solid(Color.Red), cache); + TextRenderer.RenderTo(first, "Hello", new RichTextOptions(font)); + DrawingOperation[] expected = first.DrawingOperations.ToArray(); + + using (RichTextGlyphRenderer second = new(new DrawingOptions(), null, null, Brushes.Solid(Color.Blue), cache)) + { + // Hold both renderers live to test exclusive leases deterministically, without + // depending on the scheduler to overlap the two operation lists. + TextRenderer.RenderTo(second, "World", new RichTextOptions(font)); + cache.Clear(); + Assert.Equal(0, cache.Count); + Assert.NotEmpty(second.DrawingOperations); + Assert.NotSame(first.Scratch, second.Scratch); + } + + Assert.NotEmpty(expected); + Assert.Equal(expected, first.DrawingOperations); + + DrawingTextCache.DrawingScratch returned; + using (RichTextGlyphRenderer next = new(new DrawingOptions(), null, null, Brushes.Solid(Color.Black), cache)) + { + returned = next.Scratch; + Assert.Empty(next.DrawingOperations); + Assert.NotSame(first.Scratch, returned); + TextRenderer.RenderTo(next, "H", new RichTextOptions(font)); + } + + using RichTextGlyphRenderer reused = new(new DrawingOptions(), null, null, Brushes.Solid(Color.Black), cache); + Assert.Same(returned, reused.Scratch); + Assert.Empty(reused.DrawingOperations); + Assert.Equal(expected, first.DrawingOperations); + } + + /// + /// Verifies complete publication and abandonment of layered glyphs. + /// + /// Whether to finish the glyph before disposing the renderer. + [Theory] + [InlineData(false)] + [InlineData(true)] + public void LayeredGlyph_PublishesOnlyAfterCompletion(bool completeGlyph) + { + Font font = TestFontUtilities.GetFont(TestFonts.OpenSans, 24); + RichTextOptions options = new(font); + FontRectangle bounds = default; + GlyphRendererParameters parameters = default; + Mock capture = new(); + capture.Setup(x => x.BeginGlyph(in It.Ref.IsAny, in It.Ref.IsAny)) + .Callback(new InvocationAction(invocation => + { + bounds = (FontRectangle)invocation.Arguments[0]; + parameters = (GlyphRendererParameters)invocation.Arguments[1]; + })) + .Returns(false); + + // Obtain valid callback parameters through the font renderer, then stop between + // layers deterministically rather than relying on a scheduler to expose the race. + TextRenderer.RenderTo(capture.Object, "H", options); + capture.Verify(x => x.BeginGlyph(in It.Ref.IsAny, in It.Ref.IsAny), Times.Once); + DrawingTextCache cache = new(); + using (RichTextGlyphRenderer builder = new(new DrawingOptions(), null, null, Brushes.Solid(Color.Red), cache)) + { + IGlyphRenderer callbacks = builder; + callbacks.BeginText(bounds); + Assert.True(callbacks.BeginGlyph(bounds, parameters)); + for (int layer = 0; layer < 2; layer++) + { + callbacks.BeginLayer(null, FillRule.NonZero); + callbacks.BeginFigure(); + callbacks.MoveTo(new Vector2(layer * 20, 0)); + callbacks.LineTo(new Vector2((layer * 20) + 10, 0)); + callbacks.LineTo(new Vector2((layer * 20) + 10, 10)); + callbacks.LineTo(new Vector2(layer * 20, 10)); + callbacks.EndFigure(); + callbacks.EndLayer(); + Assert.Equal(0, cache.Count); + } + + Assert.Equal(2, builder.DrawingOperations.Count); + if (completeGlyph) + { + callbacks.EndGlyph(); + callbacks.EndText(); + Assert.Equal(1, cache.Count); + using RichTextGlyphRenderer reader = new(new DrawingOptions(), null, null, Brushes.Solid(Color.Blue), cache); + TextRenderer.RenderTo(reader, "H", options); + Assert.Equal(2, reader.DrawingOperations.Count); + for (int layer = 0; layer < 2; layer++) + { + Assert.Same(builder.DrawingOperations[layer].Path, reader.DrawingOperations[layer].Path); + } + } + } + + // Abandoning a draw must discard its incomplete glyph, not publish it from Dispose. + Assert.Equal(completeGlyph ? 1 : 0, cache.Count); + } + + /// + /// Verifies duplicate publication preserves the first complete entry and eviction preserves acquired data. + /// + [Fact] + public void Add_DuplicateCompletedGlyphPreservesFirstEntry() + { + DrawingTextCache cache = new(1); + RichTextGlyphRenderer.CacheKey key = new() { Font = "test", GlyphId = 1 }; + List first = [new() { FillPath = new RectanglePolygon(0, 0, 10, 10) }]; + List second = [new() { FillPath = new RectanglePolygon(0, 0, 20, 20) }]; + + // Two misses can finish the same glyph independently. Publishing the second must + // neither append layers to the first nor replace the result another draw is reading. + cache.Add(key, first); + cache.Add(key, second); + Assert.True(cache.TryGetValue(key, out List actual)); + Assert.Same(first, actual); + Assert.Single(actual); + Assert.Equal(1, cache.Count); + + cache.Add(new RichTextGlyphRenderer.CacheKey { Font = "test", GlyphId = 2 }, second); + Assert.False(cache.TryGetValue(key, out _)); + Assert.Equal(1, cache.Count); + Assert.Single(actual); + } + + /// + /// Verifies concurrent drawing matches serial reuse of the same outlines at varied positions. + /// + [Fact] + public async Task ConcurrentCanvases_MatchSerialOutputWithSameCachedOutlines() + { + const int workers = 4; + Font font = TestFontUtilities.GetFont(TestFonts.OpenSans, 24); + Font emojiFont = TestFontUtilities.GetFont(TestFonts.NotoColorEmojiRegular, 48); + Assert.True(font.FontMetrics.TryGetGlyphMetrics( + new CodePoint('H'), + TextAttributes.None, + TextDecorations.None, + LayoutMode.HorizontalTopBottom, + ColorFontSupport.None, + null, + out FontGlyphMetrics metrics)); + + DrawingTextCache shared = new(); + DrawingTextCache serial = new(); + + // Both caches must contain outlines built at the same positions. The assertion + // compares concurrent and serial reuse, not reuse against newly built geometry. + for (int index = 0; index < workers; index++) + { + using Image warmShared = new(320, 160); + using Image warmSerial = new(320, 160); + DrawSample(warmShared, shared, font, emojiFont, metrics.GlyphId, index); + DrawSample(warmSerial, serial, font, emojiFont, metrics.GlyphId, index); + } + + using Barrier start = new(workers); + Task[] tasks = new Task[workers]; + for (int worker = 0; worker < workers; worker++) + { + int index = worker; + tasks[worker] = Task.Run(() => + { + using Image expected = new(320, 160); + lock (serial) + { + DrawSample(expected, serial, font, emojiFont, metrics.GlyphId, index); + } + + Assert.True(start.SignalAndWait(TimeSpan.FromSeconds(30))); + + for (int iteration = 0; iteration < 12; iteration++) + { + using Image actual = new(320, 160); + DrawSample(actual, shared, font, emojiFont, metrics.GlyphId, index); + ImageComparer.Exact.VerifySimilarity(expected, actual); + Assert.Equal(serial.Count, shared.Count); + } + }); + } + + await Task.WhenAll(tasks); + } + + /// + /// Verifies competing publications and cache removal preserve acquired glyph and run data. + /// + /// The glyph cache capacity. + /// Whether competing writers also clear the cache. + [Theory] + [InlineData(2, false)] + [InlineData(DrawingTextCache.DefaultCapacity, false)] + [InlineData(DrawingTextCache.DefaultCapacity, true)] + public async Task ConcurrentPublication_EvictionAndClearPreserveAcquiredEntries(int capacity, bool clearDuringDrawing) + { + const int workers = 4; + DrawingTextCache cache = new(capacity); + using Barrier phase = new(workers); + Task[] tasks = new Task[workers]; + for (int worker = 0; worker < workers; worker++) + { + int index = worker; + tasks[worker] = Task.Run(() => + { + IPath first = new RectanglePolygon(index, 0, 10, 10); + IPath second = new RectanglePolygon(index, 10, 10, 10); + List entries = + [ + new() { FillPath = first }, + new() { FillPath = second } + ]; + + for (int iteration = 0; iteration < 32; iteration++) + { + RichTextGlyphRenderer.CacheKey key = new() { Font = "test", GlyphId = (ushort)iteration }; + DrawingTextCache.RunPathCacheKey runKey = new( + [new DrawingTextCache.RunPathCacheEntry(first, Vector2.Zero, key, true)], 1); + + // Race complete publications of the same key. No reader may see a + // mixture of entries from different producers or an incomplete list. + Assert.True(phase.SignalAndWait(TimeSpan.FromSeconds(30))); + cache.Add(key, entries); + cache.AddRunPath(runKey, first); + Assert.True(phase.SignalAndWait(TimeSpan.FromSeconds(30))); + Assert.True(cache.TryGetValue(key, out List acquired)); + Assert.True(cache.TryGetRunPath(runKey, out IPath acquiredRun)); + Assert.Equal(2, acquired.Count); + IPath acquiredFirst = acquired[0].FillPath; + IPath acquiredSecond = acquired[1].FillPath; + Assert.Equal(new RectangleF(acquiredFirst.Bounds.X, 0, 10, 10), acquiredFirst.Bounds); + Assert.Equal(new RectangleF(acquiredFirst.Bounds.X, 10, 10, 10), acquiredSecond.Bounds); + RectangleF runBounds = acquiredRun.Bounds; + Assert.Equal(new SizeF(10, 10), runBounds.Size); + Assert.True(phase.SignalAndWait(TimeSpan.FromSeconds(30))); + + // Every reader now owns a result. Competing insertions force eviction + // at small capacity; Clear additionally removes both indexes at once. + RichTextGlyphRenderer.CacheKey competingKey = new() + { + Font = "test", + GlyphId = (ushort)(100 + (iteration * workers) + index) + }; + + cache.Add(competingKey, entries); + cache.AddRunPath(new DrawingTextCache.RunPathCacheKey( + [new DrawingTextCache.RunPathCacheEntry(first, Vector2.Zero, competingKey, true)], 1), first); + + if (clearDuringDrawing) + { + cache.Clear(); + } + + Assert.InRange(cache.Count, 0, capacity); + Assert.True(phase.SignalAndWait(TimeSpan.FromSeconds(30))); + Assert.Equal(2, acquired.Count); + Assert.Same(acquiredFirst, acquired[0].FillPath); + Assert.Same(acquiredSecond, acquired[1].FillPath); + Assert.Equal(runBounds, acquiredRun.Bounds); + } + }); + } + + await Task.WhenAll(tasks); + } + + /// + /// Draws ordinary and decorated text, a positioned glyph run, and nested color layers. + /// + /// The independent target for this draw. + /// The cache shared by concurrent canvases or used for the serial reference. + /// The ordinary text font. + /// The layered color font. + /// The ordinary font's H glyph. + /// The worker index, varying placement and paint between canvases. + private static void DrawSample(Image image, DrawingTextCache cache, Font font, Font emojiFont, ushort glyphId, int index) + { + using DrawingCanvas canvas = image.Frames.RootFrame.CreateCanvas(image.Configuration, new DrawingOptions(), cache); + Brush brush = Brushes.Solid(index % 2 == 0 ? Color.Red : Color.Blue); + + RichTextOptions textOptions = new(font) + { + Origin = new Vector2(8 + index, 8), + TextRuns = [new RichTextRun { Start = 0, End = 5, TextDecorations = TextDecorations.Underline }] + }; + + canvas.DrawText(textOptions, "Hello World", brush, null); + RichGlyphOptions glyphOptions = new() { Font = font }; + canvas.DrawText( + [glyphId, glyphId, glyphId], + [new Vector2(8 + index, 72), new Vector2(32 + index, 72), new Vector2(56 + index, 72)], + glyphOptions, + brush, + null); + + RichGlyphOptions emojiOptions = new() + { + Font = emojiFont, + Origin = new Vector2(100 + index, 80), + ColorFontSupport = ColorFontSupport.ColrV1 + }; + + // This glyph contains nested SoftLight/SrcIn composite groups, exercising layer-stack + // isolation and complete publication of the cached layer and group-marker sequence. + canvas.DrawText(2629, emojiOptions, brush, null); + } +} diff --git a/tests/ImageSharp.Drawing.Tests/Processing/RichTextGlyphRendererTests.cs b/tests/ImageSharp.Drawing.Tests/Processing/RichTextGlyphRendererTests.cs index 86a137eb..54f7554b 100644 --- a/tests/ImageSharp.Drawing.Tests/Processing/RichTextGlyphRendererTests.cs +++ b/tests/ImageSharp.Drawing.Tests/Processing/RichTextGlyphRendererTests.cs @@ -14,6 +14,34 @@ namespace SixLabors.ImageSharp.Drawing.Tests.Processing; public class RichTextGlyphRendererTests { + /// + /// Verifies moved text retains cached outline identity and updates its destination. + /// + [Fact] + public void MovedText_CacheHitReusesOutline() + { + Font font = TestFontUtilities.GetFont(TestFonts.OpenSans, 24); + DrawingTextCache cache = new(); + using RichTextGlyphRenderer first = new(new DrawingOptions(), null, null, Brushes.Solid(Color.Red), cache); + TextRenderer.RenderTo(first, "Hello World", new RichTextOptions(font) { Origin = new Vector2(11, 8) }); + using RichTextGlyphRenderer cached = new(new DrawingOptions(), null, null, Brushes.Solid(Color.Red), cache); + TextRenderer.RenderTo(cached, "Hello World", new RichTextOptions(font) { Origin = new Vector2(8, 8) }); + + Assert.NotEmpty(first.DrawingOperations); + Assert.Equal(first.DrawingOperations.Count, cached.DrawingOperations.Count); + for (int i = 0; i < first.DrawingOperations.Count; i++) + { + DrawingOperation expected = first.DrawingOperations[i]; + DrawingOperation actual = cached.DrawingOperations[i]; + + // Moving text must reuse the vector outline while moving its destination. + // Rebuilding an outline at the new origin is not the reference for a cache hit. + Assert.Same(expected.Path, actual.Path); + Assert.Equal(expected.RenderLocation.X - 3, actual.RenderLocation.X); + Assert.Equal(expected.RenderLocation.Y, actual.RenderLocation.Y); + } + } + [Fact] public void SetDecoration_ContiguousRun_EmitsSingleDecorationOperation() { @@ -86,14 +114,14 @@ public void RenderGlyph_NestedColrV1Composite_EmitsIsolatedGroups() ColorFontSupport = ColorFontSupport.ColrV1 }; - List operations = []; using RichTextGlyphRenderer renderer = new( new DrawingOptions(), path: null, pen: null, brush: Brushes.Solid(Color.Black), - new DrawingTextCache(), - operations); + new DrawingTextCache()); + + List operations = renderer.DrawingOperations; // This glyph is a SoftLight composite whose source is a nested SrcIn composite, and // the inner source is a linear gradient with no outline. Each composite lowers to one @@ -192,24 +220,24 @@ public void RenderGlyph_LayeredColrV1CacheHit_ReplaysFreshOperationSequence() }; DrawingTextCache cache = new(); - List fresh = []; using RichTextGlyphRenderer freshRenderer = new( new DrawingOptions(), path: null, pen: null, brush: Brushes.Solid(Color.Black), - cache, - fresh); + cache); + + List fresh = freshRenderer.DrawingOperations; TextRenderer.RenderTo(freshRenderer, 2629, glyphOptions); - List cached = []; using RichTextGlyphRenderer cachedRenderer = new( new DrawingOptions(), path: null, pen: null, brush: Brushes.Solid(Color.Black), - cache, - cached); + cache); + + List cached = cachedRenderer.DrawingOperations; TextRenderer.RenderTo(cachedRenderer, 2629, glyphOptions); Assert.NotEmpty(fresh); @@ -287,18 +315,18 @@ private static int CountOperations(Font font, string text, List? ru options.TextRuns = [.. runs]; } - List operations = []; using RichTextGlyphRenderer renderer = new( new DrawingOptions(), path: null, pen: null, brush: Brushes.Solid(Color.Black), - new DrawingTextCache(), - operations); + new DrawingTextCache()); + + List operations = renderer.DrawingOperations; TextRenderer.RenderTo(renderer, text, options); - // Dispose clears the caller-owned operation list, so count before leaving scope. + // Dispose clears the leased operation list, so count before leaving scope. return operations.Count; } @@ -310,18 +338,18 @@ private static DrawingOperation RenderSingleGlyph(Font font, PointF origin, Draw Origin = origin, }; - List operations = []; using RichTextGlyphRenderer renderer = new( new DrawingOptions(), path: null, pen: null, brush: Brushes.Solid(Color.Black), - cache, - operations); + cache); + + List operations = renderer.DrawingOperations; TextRenderer.RenderTo(renderer, "H", options); - // Return the value copy before disposing the renderer, which clears the caller-owned list. + // Return the value copy before disposing the renderer, which clears the leased list. return Assert.Single(operations); } } From 70d94cd668542f5836b6af5b38e1b8014f351ac0 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Mon, 14 Sep 2026 21:18:37 +1000 Subject: [PATCH 2/2] Stabilize concurrent cache tests Use `Task.Factory.StartNew` with `TaskCreationOptions.LongRunning` in the concurrent drawing text cache tests so each barrier participant gets a dedicated thread. This avoids thread-pool starvation and intermittent failures on small CI runners when other tests are also scheduling work. --- .../Processing/DrawingTextCacheTests.cs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/ImageSharp.Drawing.Tests/Processing/DrawingTextCacheTests.cs b/tests/ImageSharp.Drawing.Tests/Processing/DrawingTextCacheTests.cs index b840f191..8cc12e8a 100644 --- a/tests/ImageSharp.Drawing.Tests/Processing/DrawingTextCacheTests.cs +++ b/tests/ImageSharp.Drawing.Tests/Processing/DrawingTextCacheTests.cs @@ -183,7 +183,9 @@ public async Task ConcurrentCanvases_MatchSerialOutputWithSameCachedOutlines() for (int worker = 0; worker < workers; worker++) { int index = worker; - tasks[worker] = Task.Run(() => + // Barrier participants need dedicated threads so waiting for their peers does + // not starve thread-pool work scheduled by other tests on small CI runners. + tasks[worker] = Task.Factory.StartNew(() => { using Image expected = new(320, 160); lock (serial) @@ -200,7 +202,7 @@ public async Task ConcurrentCanvases_MatchSerialOutputWithSameCachedOutlines() ImageComparer.Exact.VerifySimilarity(expected, actual); Assert.Equal(serial.Count, shared.Count); } - }); + }, CancellationToken.None, TaskCreationOptions.LongRunning, TaskScheduler.Default); } await Task.WhenAll(tasks); @@ -224,7 +226,9 @@ public async Task ConcurrentPublication_EvictionAndClearPreserveAcquiredEntries( for (int worker = 0; worker < workers; worker++) { int index = worker; - tasks[worker] = Task.Run(() => + // Barrier participants need dedicated threads so waiting for their peers does + // not starve thread-pool work scheduled by other tests on small CI runners. + tasks[worker] = Task.Factory.StartNew(() => { IPath first = new RectanglePolygon(index, 0, 10, 10); IPath second = new RectanglePolygon(index, 10, 10, 10); @@ -281,7 +285,7 @@ public async Task ConcurrentPublication_EvictionAndClearPreserveAcquiredEntries( Assert.Same(acquiredSecond, acquired[1].FillPath); Assert.Equal(runBounds, acquiredRun.Bounds); } - }); + }, CancellationToken.None, TaskCreationOptions.LongRunning, TaskScheduler.Default); } await Task.WhenAll(tasks);