From 83ced124c4a077eea9d913de7b6b2c74b62be182 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Mon, 16 Oct 2023 15:38:58 +1000 Subject: [PATCH] Split Vp8LHistogram and clean up --- .../Webp/Lossless/BackwardReferenceEncoder.cs | 4 +- .../Formats/Webp/Lossless/CostModel.cs | 2 +- .../Formats/Webp/Lossless/HistogramEncoder.cs | 10 +- .../Formats/Webp/Lossless/Vp8LEncoder.cs | 2 +- .../Formats/Webp/Lossless/Vp8LHistogram.cs | 99 ++++++++++--------- .../Formats/Webp/Lossless/Vp8LHistogramSet.cs | 62 ++++++------ .../Formats/WebP/DominantCostRangeTests.cs | 4 +- .../Formats/WebP/Vp8LHistogramTests.cs | 6 +- 8 files changed, 98 insertions(+), 91 deletions(-) diff --git a/src/ImageSharp/Formats/Webp/Lossless/BackwardReferenceEncoder.cs b/src/ImageSharp/Formats/Webp/Lossless/BackwardReferenceEncoder.cs index 185ba1e34..211185dbb 100644 --- a/src/ImageSharp/Formats/Webp/Lossless/BackwardReferenceEncoder.cs +++ b/src/ImageSharp/Formats/Webp/Lossless/BackwardReferenceEncoder.cs @@ -85,7 +85,7 @@ internal static class BackwardReferenceEncoder } // Keep the best backward references. - using Vp8LHistogram histo = Vp8LHistogram.Create(memoryAllocator, worst, cacheBitsTmp); + using OwnedVp8LHistogram histo = OwnedVp8LHistogram.Create(memoryAllocator, worst, cacheBitsTmp); double bitCost = histo.EstimateBits(stats, bitsEntropy); if (lz77TypeBest == 0 || bitCost < bitCostBest) @@ -102,7 +102,7 @@ internal static class BackwardReferenceEncoder { Vp8LHashChain hashChainTmp = lz77TypeBest == (int)Vp8LLz77Type.Lz77Standard ? hashChain : hashChainBox!; BackwardReferencesTraceBackwards(width, height, memoryAllocator, bgra, cacheBits, hashChainTmp, best, worst); - using Vp8LHistogram histo = Vp8LHistogram.Create(memoryAllocator, worst, cacheBits); + using OwnedVp8LHistogram histo = OwnedVp8LHistogram.Create(memoryAllocator, worst, cacheBits); double bitCostTrace = histo.EstimateBits(stats, bitsEntropy); if (bitCostTrace < bitCostBest) { diff --git a/src/ImageSharp/Formats/Webp/Lossless/CostModel.cs b/src/ImageSharp/Formats/Webp/Lossless/CostModel.cs index 94d60b4ee..beebc48ab 100644 --- a/src/ImageSharp/Formats/Webp/Lossless/CostModel.cs +++ b/src/ImageSharp/Formats/Webp/Lossless/CostModel.cs @@ -37,7 +37,7 @@ internal class CostModel public void Build(int xSize, int cacheBits, Vp8LBackwardRefs backwardRefs) { - using Vp8LHistogram histogram = Vp8LHistogram.Create(this.memoryAllocator, cacheBits); + using OwnedVp8LHistogram histogram = OwnedVp8LHistogram.Create(this.memoryAllocator, cacheBits); // The following code is similar to HistogramCreate but converts the distance to plane code. for (int i = 0; i < backwardRefs.Refs.Count; i++) diff --git a/src/ImageSharp/Formats/Webp/Lossless/HistogramEncoder.cs b/src/ImageSharp/Formats/Webp/Lossless/HistogramEncoder.cs index cce035617..6c2d18a91 100644 --- a/src/ImageSharp/Formats/Webp/Lossless/HistogramEncoder.cs +++ b/src/ImageSharp/Formats/Webp/Lossless/HistogramEncoder.cs @@ -172,8 +172,8 @@ internal static class HistogramEncoder // Skip the histogram if it is completely empty, which can happen for tiles with no information (when they are skipped because of LZ77). if (!origHistogram.IsUsed(0) && !origHistogram.IsUsed(1) && !origHistogram.IsUsed(2) && !origHistogram.IsUsed(3) && !origHistogram.IsUsed(4)) { - origHistograms.DisposeAt(i); - histograms.DisposeAt(i); + origHistograms[i] = null; + histograms[i] = null; histogramSymbols[i] = InvalidHistogramSymbol; } else @@ -254,7 +254,7 @@ internal static class HistogramEncoder // Move the (better) merged histogram to its final slot. (histograms[first], curCombo) = (curCombo, histograms[first]); - histograms.DisposeAt(idx); + histograms[idx] = null; indicesToRemove.Add(idx); clusterMappings[clusters[idx]] = clusters[first]; } @@ -415,7 +415,7 @@ internal static class HistogramEncoder // Merge the histograms and remove bestIdx2 from the list. HistogramAdd(histograms[bestIdx2], histograms[bestIdx1], histograms[bestIdx1]); histograms[bestIdx1].BitCost = histoPriorityList[0].CostCombo; - histograms.DisposeAt(bestIdx2); + histograms[bestIdx2] = null; numUsed--; for (int j = 0; j < histoPriorityList.Count;) @@ -512,7 +512,7 @@ internal static class HistogramEncoder histograms[idx1].BitCost = histoPriorityList[0].CostCombo; // Remove merged histogram. - histograms.DisposeAt(idx2); + histograms[idx2] = null; // Remove pairs intersecting the just combined best pair. for (int i = 0; i < histoPriorityList.Count;) diff --git a/src/ImageSharp/Formats/Webp/Lossless/Vp8LEncoder.cs b/src/ImageSharp/Formats/Webp/Lossless/Vp8LEncoder.cs index b53180a4f..878d487a8 100644 --- a/src/ImageSharp/Formats/Webp/Lossless/Vp8LEncoder.cs +++ b/src/ImageSharp/Formats/Webp/Lossless/Vp8LEncoder.cs @@ -589,7 +589,7 @@ internal class Vp8LEncoder : IDisposable Vp8LBackwardRefs refsTmp = this.Refs[refsBest.Equals(this.Refs[0]) ? 1 : 0]; this.bitWriter.Reset(bwInit); - using Vp8LHistogram tmpHisto = Vp8LHistogram.Create(this.memoryAllocator, cacheBits); + using OwnedVp8LHistogram tmpHisto = OwnedVp8LHistogram.Create(this.memoryAllocator, cacheBits); using Vp8LHistogramSet histogramImage = new(this.memoryAllocator, histogramImageXySize, cacheBits); // Build histogram image and symbols from backward references. diff --git a/src/ImageSharp/Formats/Webp/Lossless/Vp8LHistogram.cs b/src/ImageSharp/Formats/Webp/Lossless/Vp8LHistogram.cs index 023f1c943..f47397790 100644 --- a/src/ImageSharp/Formats/Webp/Lossless/Vp8LHistogram.cs +++ b/src/ImageSharp/Formats/Webp/Lossless/Vp8LHistogram.cs @@ -10,13 +10,9 @@ using SixLabors.ImageSharp.Memory; namespace SixLabors.ImageSharp.Formats.Webp.Lossless; -internal sealed unsafe class Vp8LHistogram : IDisposable +internal abstract unsafe class Vp8LHistogram { private const uint NonTrivialSym = 0xffffffff; - private readonly IMemoryOwner? bufferOwner; - private readonly Memory buffer; - private readonly MemoryHandle bufferHandle; - private readonly uint* red; private readonly uint* blue; private readonly uint* alpha; @@ -35,32 +31,21 @@ internal sealed unsafe class Vp8LHistogram : IDisposable /// /// Initializes a new instance of the class. /// - /// - /// This constructor should be used when the histogram is a member of a . - /// - /// The backing buffer. + /// The base pointer to the backing memory. /// The backward references to initialize the histogram with. /// The palette code bits. - public Vp8LHistogram(Memory buffer, Vp8LBackwardRefs refs, int paletteCodeBits) - : this(buffer, paletteCodeBits) => this.StoreRefs(refs); + protected Vp8LHistogram(uint* basePointer, Vp8LBackwardRefs refs, int paletteCodeBits) + : this(basePointer, paletteCodeBits) => this.StoreRefs(refs); /// /// Initializes a new instance of the class. /// - /// - /// This constructor should be used when the histogram is a member of a . - /// - /// The backing buffer. + /// The base pointer to the backing memory. /// The palette code bits. - /// Optional buffer owner to dispose. - public Vp8LHistogram(Memory buffer, int paletteCodeBits, IMemoryOwner? bufferOwner = null) + protected Vp8LHistogram(uint* basePointer, int paletteCodeBits) { - this.bufferOwner = bufferOwner; - this.buffer = buffer; - this.bufferHandle = this.buffer.Pin(); this.PaletteCodeBits = paletteCodeBits; - - this.red = (uint*)this.bufferHandle.Pointer; + this.red = basePointer; this.blue = this.red + RedSize; this.alpha = this.blue + BlueSize; this.distance = this.alpha + AlphaSize; @@ -109,27 +94,6 @@ internal sealed unsafe class Vp8LHistogram : IDisposable private Span TotalSpan => new(this.red, BufferSize); - public bool IsDisposed { get; set; } - - /// - /// Creates an that is not a member of a . - /// - public static Vp8LHistogram Create(MemoryAllocator memoryAllocator, int paletteCodeBits) - { - IMemoryOwner bufferOwner = memoryAllocator.Allocate(BufferSize, AllocationOptions.Clean); - return new Vp8LHistogram(bufferOwner.Memory, paletteCodeBits, bufferOwner); - } - - /// - /// Creates an that is not a member of a . - /// - public static Vp8LHistogram Create(MemoryAllocator memoryAllocator, Vp8LBackwardRefs refs, int paletteCodeBits) - { - Vp8LHistogram histogram = Create(memoryAllocator, paletteCodeBits); - histogram.StoreRefs(refs); - return histogram; - } - [MethodImpl(MethodImplOptions.AggressiveInlining)] public bool IsUsed(int index) => this.IsUsedSpan[index] == 1u; @@ -621,14 +585,57 @@ internal sealed unsafe class Vp8LHistogram : IDisposable } } } +} + +internal sealed unsafe class OwnedVp8LHistogram : Vp8LHistogram, IDisposable +{ + private readonly IMemoryOwner bufferOwner; + private MemoryHandle bufferHandle; + private bool isDisposed; + + private OwnedVp8LHistogram( + IMemoryOwner bufferOwner, + ref MemoryHandle bufferHandle, + uint* basePointer, + int paletteCodeBits) + : base(basePointer, paletteCodeBits) + { + this.bufferOwner = bufferOwner; + this.bufferHandle = bufferHandle; + } + + /// + /// Creates an that is not a member of a . + /// + /// The memory allocator. + /// The palette code bits. + public static OwnedVp8LHistogram Create(MemoryAllocator memoryAllocator, int paletteCodeBits) + { + IMemoryOwner bufferOwner = memoryAllocator.Allocate(BufferSize, AllocationOptions.Clean); + MemoryHandle bufferHandle = bufferOwner.Memory.Pin(); + return new OwnedVp8LHistogram(bufferOwner, ref bufferHandle, (uint*)bufferHandle.Pointer, paletteCodeBits); + } + + /// + /// Creates an that is not a member of a . + /// + /// The memory allocator. + /// The backward references to initialize the histogram with. + /// The palette code bits. + public static OwnedVp8LHistogram Create(MemoryAllocator memoryAllocator, Vp8LBackwardRefs refs, int paletteCodeBits) + { + OwnedVp8LHistogram histogram = Create(memoryAllocator, paletteCodeBits); + histogram.StoreRefs(refs); + return histogram; + } public void Dispose() { - if (!this.IsDisposed) + if (!this.isDisposed) { this.bufferHandle.Dispose(); - this.bufferOwner?.Dispose(); - this.IsDisposed = true; + this.bufferOwner.Dispose(); + this.isDisposed = true; } } } diff --git a/src/ImageSharp/Formats/Webp/Lossless/Vp8LHistogramSet.cs b/src/ImageSharp/Formats/Webp/Lossless/Vp8LHistogramSet.cs index b7b884dfc..a46838ee6 100644 --- a/src/ImageSharp/Formats/Webp/Lossless/Vp8LHistogramSet.cs +++ b/src/ImageSharp/Formats/Webp/Lossless/Vp8LHistogramSet.cs @@ -13,30 +13,39 @@ namespace SixLabors.ImageSharp.Formats.Webp.Lossless; internal sealed class Vp8LHistogramSet : IEnumerable, IDisposable { private readonly IMemoryOwner buffer; + private MemoryHandle bufferHandle; private readonly List items; private bool isDisposed; public Vp8LHistogramSet(MemoryAllocator memoryAllocator, int capacity, int cacheBits) { this.buffer = memoryAllocator.Allocate(Vp8LHistogram.BufferSize * capacity, AllocationOptions.Clean); + this.bufferHandle = this.buffer.Memory.Pin(); - this.items = new List(capacity); - for (int i = 0; i < capacity; i++) + unsafe { - Memory subBuffer = this.buffer.Memory.Slice(Vp8LHistogram.BufferSize * i, Vp8LHistogram.BufferSize); - this.items.Add(new Vp8LHistogram(subBuffer, cacheBits)); + uint* basePointer = (uint*)this.bufferHandle.Pointer; + this.items = new List(capacity); + for (int i = 0; i < capacity; i++) + { + this.items.Add(new MemberVp8LHistogram(basePointer + (Vp8LHistogram.BufferSize * i), cacheBits)); + } } } public Vp8LHistogramSet(MemoryAllocator memoryAllocator, Vp8LBackwardRefs refs, int capacity, int cacheBits) { this.buffer = memoryAllocator.Allocate(Vp8LHistogram.BufferSize * capacity, AllocationOptions.Clean); + this.bufferHandle = this.buffer.Memory.Pin(); - this.items = new List(capacity); - for (int i = 0; i < capacity; i++) + unsafe { - Memory subBuffer = this.buffer.Memory.Slice(Vp8LHistogram.BufferSize * i, Vp8LHistogram.BufferSize); - this.items.Add(new Vp8LHistogram(subBuffer, refs, cacheBits)); + uint* basePointer = (uint*)this.bufferHandle.Pointer; + this.items = new List(capacity); + for (int i = 0; i < capacity; i++) + { + this.items.Add(new MemberVp8LHistogram(basePointer + (Vp8LHistogram.BufferSize * i), refs, cacheBits)); + } } } @@ -49,30 +58,13 @@ internal sealed class Vp8LHistogramSet : IEnumerable, IDisposable public Vp8LHistogram this[int index] { get => this.items[index]; - - // TODO: Should we check and throw for null? set => this.items[index] = value; } - public void DisposeAt(int index) - { - this.CheckDisposed(); - - Vp8LHistogram item = this.items[index]; - item?.Dispose(); - this.items[index] = null; - } - public void RemoveAt(int index) { this.CheckDisposed(); - - Vp8LHistogram item = this.items[index]; - item?.Dispose(); this.items.RemoveAt(index); -#pragma warning disable IDE0059 // Unnecessary assignment of a value - item = null; -#pragma warning restore IDE0059 // Unnecessary assignment of a value } public void Dispose() @@ -82,13 +74,8 @@ internal sealed class Vp8LHistogramSet : IEnumerable, IDisposable return; } - foreach (Vp8LHistogram item in this.items) - { - // First, make sure to unpin individual sub buffers. - item?.Dispose(); - } - this.buffer.Dispose(); + this.bufferHandle.Dispose(); this.items.Clear(); this.isDisposed = true; } @@ -107,4 +94,17 @@ internal sealed class Vp8LHistogramSet : IEnumerable, IDisposable } private static void ThrowDisposed() => throw new ObjectDisposedException(nameof(Vp8LHistogramSet)); + + private sealed unsafe class MemberVp8LHistogram : Vp8LHistogram + { + public MemberVp8LHistogram(uint* basePointer, int paletteCodeBits) + : base(basePointer, paletteCodeBits) + { + } + + public MemberVp8LHistogram(uint* basePointer, Vp8LBackwardRefs refs, int paletteCodeBits) + : base(basePointer, refs, paletteCodeBits) + { + } + } } diff --git a/tests/ImageSharp.Tests/Formats/WebP/DominantCostRangeTests.cs b/tests/ImageSharp.Tests/Formats/WebP/DominantCostRangeTests.cs index 5e3f6d0c9..9c48e6182 100644 --- a/tests/ImageSharp.Tests/Formats/WebP/DominantCostRangeTests.cs +++ b/tests/ImageSharp.Tests/Formats/WebP/DominantCostRangeTests.cs @@ -25,7 +25,7 @@ public class DominantCostRangeTests { // arrange DominantCostRange dominantCostRange = new(); - using Vp8LHistogram histogram = Vp8LHistogram.Create(Configuration.Default.MemoryAllocator, 10); + using OwnedVp8LHistogram histogram = OwnedVp8LHistogram.Create(Configuration.Default.MemoryAllocator, 10); histogram.LiteralCost = 1.0d; histogram.RedCost = 2.0d; histogram.BlueCost = 3.0d; @@ -57,7 +57,7 @@ public class DominantCostRangeTests RedMax = 191.0, RedMin = 109.0 }; - using Vp8LHistogram histogram = Vp8LHistogram.Create(Configuration.Default.MemoryAllocator, 6); + using OwnedVp8LHistogram histogram = OwnedVp8LHistogram.Create(Configuration.Default.MemoryAllocator, 6); histogram.LiteralCost = 247.0d; histogram.RedCost = 112.0d; histogram.BlueCost = 202.0d; diff --git a/tests/ImageSharp.Tests/Formats/WebP/Vp8LHistogramTests.cs b/tests/ImageSharp.Tests/Formats/WebP/Vp8LHistogramTests.cs index c27d30eea..cfe79e49e 100644 --- a/tests/ImageSharp.Tests/Formats/WebP/Vp8LHistogramTests.cs +++ b/tests/ImageSharp.Tests/Formats/WebP/Vp8LHistogramTests.cs @@ -78,15 +78,15 @@ public class Vp8LHistogramTests } MemoryAllocator memoryAllocator = Configuration.Default.MemoryAllocator; - using Vp8LHistogram histogram0 = Vp8LHistogram.Create(memoryAllocator, backwardRefs, 3); - using Vp8LHistogram histogram1 = Vp8LHistogram.Create(memoryAllocator, backwardRefs, 3); + using OwnedVp8LHistogram histogram0 = OwnedVp8LHistogram.Create(memoryAllocator, backwardRefs, 3); + using OwnedVp8LHistogram histogram1 = OwnedVp8LHistogram.Create(memoryAllocator, backwardRefs, 3); for (int i = 0; i < 5; i++) { histogram0.IsUsed(i, true); histogram1.IsUsed(i, true); } - using Vp8LHistogram output = Vp8LHistogram.Create(memoryAllocator, 3); + using OwnedVp8LHistogram output = OwnedVp8LHistogram.Create(memoryAllocator, 3); // act histogram0.Add(histogram1, output);