Skip to content

Commit 83ced12

Browse files
JimBobSquarePantsJimBobSquarePants
committed
Split Vp8LHistogram and clean up
1 parent 69e0b8d commit 83ced12

8 files changed

Lines changed: 98 additions & 91 deletions

File tree

‎src/ImageSharp/Formats/Webp/Lossless/BackwardReferenceEncoder.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ public static Vp8LBackwardRefs GetBackwardReferences(
8585
}
8686

8787
// Keep the best backward references.
88-
using Vp8LHistogram histo = Vp8LHistogram.Create(memoryAllocator, worst, cacheBitsTmp);
88+
using OwnedVp8LHistogram histo = OwnedVp8LHistogram.Create(memoryAllocator, worst, cacheBitsTmp);
8989
double bitCost = histo.EstimateBits(stats, bitsEntropy);
9090

9191
if (lz77TypeBest == 0 || bitCost < bitCostBest)
@@ -102,7 +102,7 @@ public static Vp8LBackwardRefs GetBackwardReferences(
102102
{
103103
Vp8LHashChain hashChainTmp = lz77TypeBest == (int)Vp8LLz77Type.Lz77Standard ? hashChain : hashChainBox!;
104104
BackwardReferencesTraceBackwards(width, height, memoryAllocator, bgra, cacheBits, hashChainTmp, best, worst);
105-
using Vp8LHistogram histo = Vp8LHistogram.Create(memoryAllocator, worst, cacheBits);
105+
using OwnedVp8LHistogram histo = OwnedVp8LHistogram.Create(memoryAllocator, worst, cacheBits);
106106
double bitCostTrace = histo.EstimateBits(stats, bitsEntropy);
107107
if (bitCostTrace < bitCostBest)
108108
{

‎src/ImageSharp/Formats/Webp/Lossless/CostModel.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ public CostModel(MemoryAllocator memoryAllocator, int literalArraySize)
3737

3838
public void Build(int xSize, int cacheBits, Vp8LBackwardRefs backwardRefs)
3939
{
40-
using Vp8LHistogram histogram = Vp8LHistogram.Create(this.memoryAllocator, cacheBits);
40+
using OwnedVp8LHistogram histogram = OwnedVp8LHistogram.Create(this.memoryAllocator, cacheBits);
4141

4242
// The following code is similar to HistogramCreate but converts the distance to plane code.
4343
for (int i = 0; i < backwardRefs.Refs.Count; i++)

‎src/ImageSharp/Formats/Webp/Lossless/HistogramEncoder.cs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -172,8 +172,8 @@ private static int HistogramCopyAndAnalyze(
172172
// Skip the histogram if it is completely empty, which can happen for tiles with no information (when they are skipped because of LZ77).
173173
if (!origHistogram.IsUsed(0) && !origHistogram.IsUsed(1) && !origHistogram.IsUsed(2) && !origHistogram.IsUsed(3) && !origHistogram.IsUsed(4))
174174
{
175-
origHistograms.DisposeAt(i);
176-
histograms.DisposeAt(i);
175+
origHistograms[i] = null;
176+
histograms[i] = null;
177177
histogramSymbols[i] = InvalidHistogramSymbol;
178178
}
179179
else
@@ -254,7 +254,7 @@ private static void HistogramCombineEntropyBin(
254254
// Move the (better) merged histogram to its final slot.
255255
(histograms[first], curCombo) = (curCombo, histograms[first]);
256256

257-
histograms.DisposeAt(idx);
257+
histograms[idx] = null;
258258
indicesToRemove.Add(idx);
259259
clusterMappings[clusters[idx]] = clusters[first];
260260
}
@@ -415,7 +415,7 @@ private static bool HistogramCombineStochastic(Vp8LHistogramSet histograms, int
415415
// Merge the histograms and remove bestIdx2 from the list.
416416
HistogramAdd(histograms[bestIdx2], histograms[bestIdx1], histograms[bestIdx1]);
417417
histograms[bestIdx1].BitCost = histoPriorityList[0].CostCombo;
418-
histograms.DisposeAt(bestIdx2);
418+
histograms[bestIdx2] = null;
419419
numUsed--;
420420

421421
for (int j = 0; j < histoPriorityList.Count;)
@@ -512,7 +512,7 @@ private static void HistogramCombineGreedy(Vp8LHistogramSet histograms)
512512
histograms[idx1].BitCost = histoPriorityList[0].CostCombo;
513513

514514
// Remove merged histogram.
515-
histograms.DisposeAt(idx2);
515+
histograms[idx2] = null;
516516

517517
// Remove pairs intersecting the just combined best pair.
518518
for (int i = 0; i < histoPriorityList.Count;)

‎src/ImageSharp/Formats/Webp/Lossless/Vp8LEncoder.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -589,7 +589,7 @@ private void EncodeImage(int width, int height, bool useCache, CrunchConfig conf
589589
Vp8LBackwardRefs refsTmp = this.Refs[refsBest.Equals(this.Refs[0]) ? 1 : 0];
590590

591591
this.bitWriter.Reset(bwInit);
592-
using Vp8LHistogram tmpHisto = Vp8LHistogram.Create(this.memoryAllocator, cacheBits);
592+
using OwnedVp8LHistogram tmpHisto = OwnedVp8LHistogram.Create(this.memoryAllocator, cacheBits);
593593
using Vp8LHistogramSet histogramImage = new(this.memoryAllocator, histogramImageXySize, cacheBits);
594594

595595
// Build histogram image and symbols from backward references.

‎src/ImageSharp/Formats/Webp/Lossless/Vp8LHistogram.cs‎

Lines changed: 53 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,9 @@
1010

1111
namespace SixLabors.ImageSharp.Formats.Webp.Lossless;
1212

13-
internal sealed unsafe class Vp8LHistogram : IDisposable
13+
internal abstract unsafe class Vp8LHistogram
1414
{
1515
private const uint NonTrivialSym = 0xffffffff;
16-
private readonly IMemoryOwner<uint>? bufferOwner;
17-
private readonly Memory<uint> buffer;
18-
private readonly MemoryHandle bufferHandle;
19-
2016
private readonly uint* red;
2117
private readonly uint* blue;
2218
private readonly uint* alpha;
@@ -35,32 +31,21 @@ internal sealed unsafe class Vp8LHistogram : IDisposable
3531
/// <summary>
3632
/// Initializes a new instance of the <see cref="Vp8LHistogram"/> class.
3733
/// </summary>
38-
/// <remarks>
39-
/// This constructor should be used when the histogram is a member of a <see cref="Vp8LHistogramSet"/>.
40-
/// </remarks>
41-
/// <param name="buffer">The backing buffer.</param>
34+
/// <param name="basePointer">The base pointer to the backing memory.</param>
4235
/// <param name="refs">The backward references to initialize the histogram with.</param>
4336
/// <param name="paletteCodeBits">The palette code bits.</param>
44-
public Vp8LHistogram(Memory<uint> buffer, Vp8LBackwardRefs refs, int paletteCodeBits)
45-
: this(buffer, paletteCodeBits) => this.StoreRefs(refs);
37+
protected Vp8LHistogram(uint* basePointer, Vp8LBackwardRefs refs, int paletteCodeBits)
38+
: this(basePointer, paletteCodeBits) => this.StoreRefs(refs);
4639

4740
/// <summary>
4841
/// Initializes a new instance of the <see cref="Vp8LHistogram"/> class.
4942
/// </summary>
50-
/// <remarks>
51-
/// This constructor should be used when the histogram is a member of a <see cref="Vp8LHistogramSet"/>.
52-
/// </remarks>
53-
/// <param name="buffer">The backing buffer.</param>
43+
/// <param name="basePointer">The base pointer to the backing memory.</param>
5444
/// <param name="paletteCodeBits">The palette code bits.</param>
55-
/// <param name="bufferOwner">Optional buffer owner to dispose.</param>
56-
public Vp8LHistogram(Memory<uint> buffer, int paletteCodeBits, IMemoryOwner<uint>? bufferOwner = null)
45+
protected Vp8LHistogram(uint* basePointer, int paletteCodeBits)
5746
{
58-
this.bufferOwner = bufferOwner;
59-
this.buffer = buffer;
60-
this.bufferHandle = this.buffer.Pin();
6147
this.PaletteCodeBits = paletteCodeBits;
62-
63-
this.red = (uint*)this.bufferHandle.Pointer;
48+
this.red = basePointer;
6449
this.blue = this.red + RedSize;
6550
this.alpha = this.blue + BlueSize;
6651
this.distance = this.alpha + AlphaSize;
@@ -109,27 +94,6 @@ public Vp8LHistogram(Memory<uint> buffer, int paletteCodeBits, IMemoryOwner<uint
10994

11095
private Span<uint> TotalSpan => new(this.red, BufferSize);
11196

112-
public bool IsDisposed { get; set; }
113-
114-
/// <summary>
115-
/// Creates an <see cref="Vp8LHistogram"/> that is not a member of a <see cref="Vp8LHistogramSet"/>.
116-
/// </summary>
117-
public static Vp8LHistogram Create(MemoryAllocator memoryAllocator, int paletteCodeBits)
118-
{
119-
IMemoryOwner<uint> bufferOwner = memoryAllocator.Allocate<uint>(BufferSize, AllocationOptions.Clean);
120-
return new Vp8LHistogram(bufferOwner.Memory, paletteCodeBits, bufferOwner);
121-
}
122-
123-
/// <summary>
124-
/// Creates an <see cref="Vp8LHistogram"/> that is not a member of a <see cref="Vp8LHistogramSet"/>.
125-
/// </summary>
126-
public static Vp8LHistogram Create(MemoryAllocator memoryAllocator, Vp8LBackwardRefs refs, int paletteCodeBits)
127-
{
128-
Vp8LHistogram histogram = Create(memoryAllocator, paletteCodeBits);
129-
histogram.StoreRefs(refs);
130-
return histogram;
131-
}
132-
13397
[MethodImpl(MethodImplOptions.AggressiveInlining)]
13498
public bool IsUsed(int index) => this.IsUsedSpan[index] == 1u;
13599

@@ -621,14 +585,57 @@ private static void AddVector(Span<uint> a, Span<uint> b, Span<uint> output, int
621585
}
622586
}
623587
}
588+
}
589+
590+
internal sealed unsafe class OwnedVp8LHistogram : Vp8LHistogram, IDisposable
591+
{
592+
private readonly IMemoryOwner<uint> bufferOwner;
593+
private MemoryHandle bufferHandle;
594+
private bool isDisposed;
595+
596+
private OwnedVp8LHistogram(
597+
IMemoryOwner<uint> bufferOwner,
598+
ref MemoryHandle bufferHandle,
599+
uint* basePointer,
600+
int paletteCodeBits)
601+
: base(basePointer, paletteCodeBits)
602+
{
603+
this.bufferOwner = bufferOwner;
604+
this.bufferHandle = bufferHandle;
605+
}
606+
607+
/// <summary>
608+
/// Creates an <see cref="OwnedVp8LHistogram"/> that is not a member of a <see cref="Vp8LHistogramSet"/>.
609+
/// </summary>
610+
/// <param name="memoryAllocator">The memory allocator.</param>
611+
/// <param name="paletteCodeBits">The palette code bits.</param>
612+
public static OwnedVp8LHistogram Create(MemoryAllocator memoryAllocator, int paletteCodeBits)
613+
{
614+
IMemoryOwner<uint> bufferOwner = memoryAllocator.Allocate<uint>(BufferSize, AllocationOptions.Clean);
615+
MemoryHandle bufferHandle = bufferOwner.Memory.Pin();
616+
return new OwnedVp8LHistogram(bufferOwner, ref bufferHandle, (uint*)bufferHandle.Pointer, paletteCodeBits);
617+
}
618+
619+
/// <summary>
620+
/// Creates an <see cref="OwnedVp8LHistogram"/> that is not a member of a <see cref="Vp8LHistogramSet"/>.
621+
/// </summary>
622+
/// <param name="memoryAllocator">The memory allocator.</param>
623+
/// <param name="refs">The backward references to initialize the histogram with.</param>
624+
/// <param name="paletteCodeBits">The palette code bits.</param>
625+
public static OwnedVp8LHistogram Create(MemoryAllocator memoryAllocator, Vp8LBackwardRefs refs, int paletteCodeBits)
626+
{
627+
OwnedVp8LHistogram histogram = Create(memoryAllocator, paletteCodeBits);
628+
histogram.StoreRefs(refs);
629+
return histogram;
630+
}
624631

625632
public void Dispose()
626633
{
627-
if (!this.IsDisposed)
634+
if (!this.isDisposed)
628635
{
629636
this.bufferHandle.Dispose();
630-
this.bufferOwner?.Dispose();
631-
this.IsDisposed = true;
637+
this.bufferOwner.Dispose();
638+
this.isDisposed = true;
632639
}
633640
}
634641
}

‎src/ImageSharp/Formats/Webp/Lossless/Vp8LHistogramSet.cs‎

Lines changed: 31 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -13,30 +13,39 @@ namespace SixLabors.ImageSharp.Formats.Webp.Lossless;
1313
internal sealed class Vp8LHistogramSet : IEnumerable<Vp8LHistogram>, IDisposable
1414
{
1515
private readonly IMemoryOwner<uint> buffer;
16+
private MemoryHandle bufferHandle;
1617
private readonly List<Vp8LHistogram> items;
1718
private bool isDisposed;
1819

1920
public Vp8LHistogramSet(MemoryAllocator memoryAllocator, int capacity, int cacheBits)
2021
{
2122
this.buffer = memoryAllocator.Allocate<uint>(Vp8LHistogram.BufferSize * capacity, AllocationOptions.Clean);
23+
this.bufferHandle = this.buffer.Memory.Pin();
2224

23-
this.items = new List<Vp8LHistogram>(capacity);
24-
for (int i = 0; i < capacity; i++)
25+
unsafe
2526
{
26-
Memory<uint> subBuffer = this.buffer.Memory.Slice(Vp8LHistogram.BufferSize * i, Vp8LHistogram.BufferSize);
27-
this.items.Add(new Vp8LHistogram(subBuffer, cacheBits));
27+
uint* basePointer = (uint*)this.bufferHandle.Pointer;
28+
this.items = new List<Vp8LHistogram>(capacity);
29+
for (int i = 0; i < capacity; i++)
30+
{
31+
this.items.Add(new MemberVp8LHistogram(basePointer + (Vp8LHistogram.BufferSize * i), cacheBits));
32+
}
2833
}
2934
}
3035

3136
public Vp8LHistogramSet(MemoryAllocator memoryAllocator, Vp8LBackwardRefs refs, int capacity, int cacheBits)
3237
{
3338
this.buffer = memoryAllocator.Allocate<uint>(Vp8LHistogram.BufferSize * capacity, AllocationOptions.Clean);
39+
this.bufferHandle = this.buffer.Memory.Pin();
3440

35-
this.items = new List<Vp8LHistogram>(capacity);
36-
for (int i = 0; i < capacity; i++)
41+
unsafe
3742
{
38-
Memory<uint> subBuffer = this.buffer.Memory.Slice(Vp8LHistogram.BufferSize * i, Vp8LHistogram.BufferSize);
39-
this.items.Add(new Vp8LHistogram(subBuffer, refs, cacheBits));
43+
uint* basePointer = (uint*)this.bufferHandle.Pointer;
44+
this.items = new List<Vp8LHistogram>(capacity);
45+
for (int i = 0; i < capacity; i++)
46+
{
47+
this.items.Add(new MemberVp8LHistogram(basePointer + (Vp8LHistogram.BufferSize * i), refs, cacheBits));
48+
}
4049
}
4150
}
4251

@@ -49,30 +58,13 @@ public Vp8LHistogramSet(MemoryAllocator memoryAllocator, Vp8LBackwardRefs refs,
4958
public Vp8LHistogram this[int index]
5059
{
5160
get => this.items[index];
52-
53-
// TODO: Should we check and throw for null?
5461
set => this.items[index] = value;
5562
}
5663

57-
public void DisposeAt(int index)
58-
{
59-
this.CheckDisposed();
60-
61-
Vp8LHistogram item = this.items[index];
62-
item?.Dispose();
63-
this.items[index] = null;
64-
}
65-
6664
public void RemoveAt(int index)
6765
{
6866
this.CheckDisposed();
69-
70-
Vp8LHistogram item = this.items[index];
71-
item?.Dispose();
7267
this.items.RemoveAt(index);
73-
#pragma warning disable IDE0059 // Unnecessary assignment of a value
74-
item = null;
75-
#pragma warning restore IDE0059 // Unnecessary assignment of a value
7668
}
7769

7870
public void Dispose()
@@ -82,13 +74,8 @@ public void Dispose()
8274
return;
8375
}
8476

85-
foreach (Vp8LHistogram item in this.items)
86-
{
87-
// First, make sure to unpin individual sub buffers.
88-
item?.Dispose();
89-
}
90-
9177
this.buffer.Dispose();
78+
this.bufferHandle.Dispose();
9279
this.items.Clear();
9380
this.isDisposed = true;
9481
}
@@ -107,4 +94,17 @@ private void CheckDisposed()
10794
}
10895

10996
private static void ThrowDisposed() => throw new ObjectDisposedException(nameof(Vp8LHistogramSet));
97+
98+
private sealed unsafe class MemberVp8LHistogram : Vp8LHistogram
99+
{
100+
public MemberVp8LHistogram(uint* basePointer, int paletteCodeBits)
101+
: base(basePointer, paletteCodeBits)
102+
{
103+
}
104+
105+
public MemberVp8LHistogram(uint* basePointer, Vp8LBackwardRefs refs, int paletteCodeBits)
106+
: base(basePointer, refs, paletteCodeBits)
107+
{
108+
}
109+
}
110110
}

‎tests/ImageSharp.Tests/Formats/WebP/DominantCostRangeTests.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ public void UpdateDominantCostRange_Works()
2525
{
2626
// arrange
2727
DominantCostRange dominantCostRange = new();
28-
using Vp8LHistogram histogram = Vp8LHistogram.Create(Configuration.Default.MemoryAllocator, 10);
28+
using OwnedVp8LHistogram histogram = OwnedVp8LHistogram.Create(Configuration.Default.MemoryAllocator, 10);
2929
histogram.LiteralCost = 1.0d;
3030
histogram.RedCost = 2.0d;
3131
histogram.BlueCost = 3.0d;
@@ -57,7 +57,7 @@ public void GetHistoBinIndex_Works(int partitions, int expectedIndex)
5757
RedMax = 191.0,
5858
RedMin = 109.0
5959
};
60-
using Vp8LHistogram histogram = Vp8LHistogram.Create(Configuration.Default.MemoryAllocator, 6);
60+
using OwnedVp8LHistogram histogram = OwnedVp8LHistogram.Create(Configuration.Default.MemoryAllocator, 6);
6161
histogram.LiteralCost = 247.0d;
6262
histogram.RedCost = 112.0d;
6363
histogram.BlueCost = 202.0d;

‎tests/ImageSharp.Tests/Formats/WebP/Vp8LHistogramTests.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -78,15 +78,15 @@ private static void RunAddVectorTest()
7878
}
7979

8080
MemoryAllocator memoryAllocator = Configuration.Default.MemoryAllocator;
81-
using Vp8LHistogram histogram0 = Vp8LHistogram.Create(memoryAllocator, backwardRefs, 3);
82-
using Vp8LHistogram histogram1 = Vp8LHistogram.Create(memoryAllocator, backwardRefs, 3);
81+
using OwnedVp8LHistogram histogram0 = OwnedVp8LHistogram.Create(memoryAllocator, backwardRefs, 3);
82+
using OwnedVp8LHistogram histogram1 = OwnedVp8LHistogram.Create(memoryAllocator, backwardRefs, 3);
8383
for (int i = 0; i < 5; i++)
8484
{
8585
histogram0.IsUsed(i, true);
8686
histogram1.IsUsed(i, true);
8787
}
8888

89-
using Vp8LHistogram output = Vp8LHistogram.Create(memoryAllocator, 3);
89+
using OwnedVp8LHistogram output = OwnedVp8LHistogram.Create(memoryAllocator, 3);
9090

9191
// act
9292
histogram0.Add(histogram1, output);

0 commit comments

Comments
 (0)