Skip to content

Commit bf66f24

Browse files
JimBobSquarePantsJimBobSquarePants
committed
Respond to feedback
1 parent 6430b8e commit bf66f24

File tree

20 files changed

+156
-23
lines changed

20 files changed

+156
-23
lines changed

‎src/ImageSharp/Formats/Webp/Lossy/Vp8Decoder.cs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -67,13 +67,13 @@ public Vp8Decoder(Vp8FrameHeader frameHeader, Vp8PictureHeader pictureHeader, Vp
6767
int extraY = extraRows * this.CacheYStride;
6868
int extraUv = extraRows / 2 * this.CacheUvStride;
6969
this.YuvBuffer = memoryAllocator.Allocate<byte>((WebpConstants.Bps * 17) + (WebpConstants.Bps * 9) + extraY);
70-
this.CacheY = memoryAllocator.Allocate<byte>((16 * this.CacheYStride) + extraY, AllocationOptions.Clean);
70+
this.CacheY = memoryAllocator.Allocate<byte>((16 * this.CacheYStride) + extraY);
7171
int cacheUvSize = (16 * this.CacheUvStride) + extraUv;
72-
this.CacheU = memoryAllocator.Allocate<byte>(cacheUvSize, AllocationOptions.Clean);
73-
this.CacheV = memoryAllocator.Allocate<byte>(cacheUvSize, AllocationOptions.Clean);
74-
this.TmpYBuffer = memoryAllocator.Allocate<byte>((int)width, AllocationOptions.Clean);
75-
this.TmpUBuffer = memoryAllocator.Allocate<byte>((int)width, AllocationOptions.Clean);
76-
this.TmpVBuffer = memoryAllocator.Allocate<byte>((int)width, AllocationOptions.Clean);
72+
this.CacheU = memoryAllocator.Allocate<byte>(cacheUvSize);
73+
this.CacheV = memoryAllocator.Allocate<byte>(cacheUvSize);
74+
this.TmpYBuffer = memoryAllocator.Allocate<byte>((int)width);
75+
this.TmpUBuffer = memoryAllocator.Allocate<byte>((int)width);
76+
this.TmpVBuffer = memoryAllocator.Allocate<byte>((int)width);
7777
this.Pixels = memoryAllocator.Allocate<byte>((int)(width * height * 4), AllocationOptions.Clean);
7878

7979
#if DEBUG

‎src/ImageSharp/Processing/Processors/Quantization/EuclideanPixelMap{TPixel}.cs

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// Licensed under the Six Labors Split License.
33

44
using System.Buffers;
5+
using System.Numerics;
56
using System.Runtime.CompilerServices;
67
using System.Runtime.InteropServices;
78
using SixLabors.ImageSharp.Memory;
@@ -126,7 +127,7 @@ private int GetClosestColorSlow(Rgba32 rgba, ref TPixel paletteRef, out TPixel m
126127
{
127128
// We have explicit instructions. No need to search.
128129
index = this.transparentIndex;
129-
this.cache.Add(rgba, (byte)index);
130+
this.cache.Add(rgba, (short)index);
130131
match = this.transparentMatch;
131132
return index;
132133
}
@@ -153,7 +154,7 @@ private int GetClosestColorSlow(Rgba32 rgba, ref TPixel paletteRef, out TPixel m
153154
}
154155

155156
// Now I have the index, pop it into the cache for next time
156-
this.cache.Add(rgba, (byte)index);
157+
this.cache.Add(rgba, (short)index);
157158
match = Unsafe.Add(ref paletteRef, (uint)index);
158159

159160
return index;
@@ -168,11 +169,9 @@ private int GetClosestColorSlow(Rgba32 rgba, ref TPixel paletteRef, out TPixel m
168169
[MethodImpl(InliningOptions.ShortMethod)]
169170
private static float DistanceSquared(Rgba32 a, Rgba32 b)
170171
{
171-
float deltaR = a.R - b.R;
172-
float deltaG = a.G - b.G;
173-
float deltaB = a.B - b.B;
174-
float deltaA = a.A - b.A;
175-
return (deltaR * deltaR) + (deltaG * deltaG) + (deltaB * deltaB) + (deltaA * deltaA);
172+
Vector4 va = new(a.R, a.G, a.B, a.A);
173+
Vector4 vb = new(b.R, b.G, b.B, b.A);
174+
return Vector4.DistanceSquared(va, vb);
176175
}
177176

178177
public void Dispose() => this.cache.Dispose();
@@ -221,7 +220,7 @@ public HybridColorDistanceCache(MemoryAllocator allocator)
221220
this.exactCache = new ExactCache(allocator);
222221
}
223222

224-
[MethodImpl(MethodImplOptions.AggressiveInlining)]
223+
[MethodImpl(InliningOptions.ShortMethod)]
225224
public readonly void Add(Rgba32 color, short index)
226225
{
227226
if (this.exactCache.TryAdd(color.PackedValue, index))
@@ -232,7 +231,7 @@ public readonly void Add(Rgba32 color, short index)
232231
this.fallbackPointer[GetCoarseIndex(color)] = index;
233232
}
234233

235-
[MethodImpl(MethodImplOptions.AggressiveInlining)]
234+
[MethodImpl(InliningOptions.ShortMethod)]
236235
public readonly bool TryGetValue(Rgba32 color, out short match)
237236
{
238237
if (this.exactCache.TryGetValue(color.PackedValue, out match))
@@ -244,7 +243,7 @@ public readonly bool TryGetValue(Rgba32 color, out short match)
244243
return match > -1; // Coarse match found
245244
}
246245

247-
[MethodImpl(MethodImplOptions.AggressiveInlining)]
246+
[MethodImpl(InliningOptions.ShortMethod)]
248247
private static int GetCoarseIndex(Rgba32 color)
249248
{
250249
int rIndex = color.R >> (8 - IndexRBits);
@@ -325,8 +324,6 @@ public ExactCache(MemoryAllocator allocator)
325324
/// </summary>
326325
/// <param name="key">The key to add.</param>
327326
/// <param name="value">The value to add.</param>
328-
/// <returns><see langword="true"/> if the key was added; otherwise, <see langword="false"/>.</returns>
329-
[MethodImpl(MethodImplOptions.AggressiveInlining)]
330327
public bool TryAdd(uint key, short value)
331328
{
332329
if (this.Count == Capacity)
@@ -380,7 +377,6 @@ public bool TryAdd(uint key, short value)
380377
/// <param name="key">The key to search for.</param>
381378
/// <param name="value">The value associated with the key, if found.</param>
382379
/// <returns><see langword="true"/> if the key is found; otherwise, <see langword="false"/>.</returns>
383-
[MethodImpl(MethodImplOptions.AggressiveInlining)]
384380
public bool TryGetValue(uint key, out short value)
385381
{
386382
int bucket = (int)(((key >> 16) ^ (key >> 8) ^ key) & 0x1FF);

‎tests/ImageSharp.Tests/Formats/Gif/GifEncoderTests.cs

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
using SixLabors.ImageSharp.PixelFormats;
99
using SixLabors.ImageSharp.Processing.Processors.Quantization;
1010
using SixLabors.ImageSharp.Tests.TestUtilities.ImageComparison;
11-
using System.Linq;
1211

1312
// ReSharper disable InconsistentNaming
1413
namespace SixLabors.ImageSharp.Tests.Formats.Gif;
@@ -386,12 +385,18 @@ public void Encode_Animated_VisualTest<TPixel>(TestImageProvider<TPixel> provide
386385

387386
[Theory]
388387
[WithFile(TestImages.Gif.Issues.Issue2866, PixelTypes.Rgba32)]
389-
public void GifEncoder_CanDecode_Issue2866<TPixel>(TestImageProvider<TPixel> provider)
388+
public void GifEncoder_CanDecode_AndEncode_Issue2866<TPixel>(TestImageProvider<TPixel> provider)
390389
where TPixel : unmanaged, IPixel<TPixel>
391390
{
392391
using Image<TPixel> image = provider.GetImage();
393392

394-
// image.DebugSaveMultiFrame(provider);
393+
// Save the image for visual inspection.
395394
provider.Utility.SaveTestOutputFile(image, "gif", new GifEncoder(), "animated");
395+
396+
// Now compare the debug output with the reference output.
397+
// We do this because the gif encoding is lossy and encoding will lead to differences in the 10s of percent.
398+
// From the unencoded image, we can see that the image is visually the same.
399+
static bool Predicate(int i, int _) => i % 8 == 0; // Image has many frames, only compare a selection of them.
400+
image.CompareDebugOutputToReferenceOutputMultiFrame(provider, ImageComparer.Exact, extension: "gif", predicate: Predicate);
396401
}
397402
}

‎tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -489,7 +489,7 @@ public void Encode_APng<TPixel>(TestImageProvider<TPixel> provider)
489489
public void Encode_AnimatedFormatTransform_FromGif<TPixel>(TestImageProvider<TPixel> provider, float percentage)
490490
where TPixel : unmanaged, IPixel<TPixel>
491491
{
492-
if (TestEnvironment.RunsOnCI)
492+
if (TestEnvironment.RunsOnCI && !TestEnvironment.IsWindows)
493493
{
494494
return;
495495
}

‎tests/ImageSharp.Tests/TestUtilities/TestImageExtensions.cs

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -277,6 +277,45 @@ public static Image<TPixel> CompareFirstFrameToReferenceOutput<TPixel>(
277277
return image;
278278
}
279279

280+
public static Image<TPixel> CompareDebugOutputToReferenceOutputMultiFrame<TPixel>(
281+
this Image<TPixel> image,
282+
ITestImageProvider provider,
283+
ImageComparer comparer,
284+
object testOutputDetails = null,
285+
string extension = "png",
286+
bool appendPixelTypeToFileName = true,
287+
Func<int, int, bool> predicate = null)
288+
where TPixel : unmanaged, IPixel<TPixel>
289+
{
290+
image.DebugSaveMultiFrame(
291+
provider,
292+
testOutputDetails,
293+
extension,
294+
appendPixelTypeToFileName,
295+
predicate: predicate);
296+
297+
using (Image<TPixel> debugImage = GetDebugOutputImageMultiFrame<TPixel>(
298+
provider,
299+
image.Frames.Count,
300+
testOutputDetails,
301+
extension,
302+
appendPixelTypeToFileName,
303+
predicate: predicate))
304+
305+
using (Image<TPixel> referenceImage = GetReferenceOutputImageMultiFrame<TPixel>(
306+
provider,
307+
image.Frames.Count,
308+
testOutputDetails,
309+
extension,
310+
appendPixelTypeToFileName,
311+
predicate: predicate))
312+
{
313+
comparer.VerifySimilarity(referenceImage, debugImage);
314+
}
315+
316+
return image;
317+
}
318+
280319
public static Image<TPixel> CompareToReferenceOutputMultiFrame<TPixel>(
281320
this Image<TPixel> image,
282321
ITestImageProvider provider,
@@ -375,6 +414,54 @@ public static Image<TPixel> GetReferenceOutputImageMultiFrame<TPixel>(
375414
return result;
376415
}
377416

417+
public static Image<TPixel> GetDebugOutputImageMultiFrame<TPixel>(
418+
this ITestImageProvider provider,
419+
int frameCount,
420+
object testOutputDetails = null,
421+
string extension = "png",
422+
bool appendPixelTypeToFileName = true,
423+
Func<int, int, bool> predicate = null)
424+
where TPixel : unmanaged, IPixel<TPixel>
425+
{
426+
(int Index, string FileName)[] frameFiles = provider.Utility.GetTestOutputFileNamesMultiFrame(
427+
frameCount,
428+
extension,
429+
testOutputDetails,
430+
appendPixelTypeToFileName,
431+
predicate: predicate).ToArray();
432+
433+
List<Image<TPixel>> temporaryFrameImages = new();
434+
435+
IImageDecoder decoder = TestEnvironment.GetReferenceDecoder(frameFiles[0].FileName);
436+
437+
for (int i = 0; i < frameFiles.Length; i++)
438+
{
439+
string path = frameFiles[i].FileName;
440+
if (!File.Exists(path))
441+
{
442+
throw new FileNotFoundException("Reference output file missing: " + path);
443+
}
444+
445+
using FileStream stream = File.OpenRead(path);
446+
Image<TPixel> tempImage = decoder.Decode<TPixel>(DecoderOptions.Default, stream);
447+
temporaryFrameImages.Add(tempImage);
448+
}
449+
450+
Image<TPixel> firstTemp = temporaryFrameImages[0];
451+
452+
Image<TPixel> result = new(firstTemp.Width, firstTemp.Height);
453+
454+
foreach (Image<TPixel> fi in temporaryFrameImages)
455+
{
456+
result.Frames.AddFrame(fi.Frames.RootFrame);
457+
fi.Dispose();
458+
}
459+
460+
// Remove the initial empty frame:
461+
result.Frames.RemoveFrame(0);
462+
return result;
463+
}
464+
378465
public static IEnumerable<ImageSimilarityReport> GetReferenceOutputSimilarityReports<TPixel>(
379466
this Image<TPixel> image,
380467
ITestImageProvider provider,

‎tests/Images/External/ReferenceOutput/GifEncoderTests/GifEncoder_CanDecode_AndEncode_Issue2866_Rgba32_issue_2866.gif/00.gif

Lines changed: 3 additions & 0 deletions
Loading

‎tests/Images/External/ReferenceOutput/GifEncoderTests/GifEncoder_CanDecode_AndEncode_Issue2866_Rgba32_issue_2866.gif/08.gif

Lines changed: 3 additions & 0 deletions
Loading

‎tests/Images/External/ReferenceOutput/GifEncoderTests/GifEncoder_CanDecode_AndEncode_Issue2866_Rgba32_issue_2866.gif/104.gif

Lines changed: 3 additions & 0 deletions
Loading

‎tests/Images/External/ReferenceOutput/GifEncoderTests/GifEncoder_CanDecode_AndEncode_Issue2866_Rgba32_issue_2866.gif/112.gif

Lines changed: 3 additions & 0 deletions
Loading

‎tests/Images/External/ReferenceOutput/GifEncoderTests/GifEncoder_CanDecode_AndEncode_Issue2866_Rgba32_issue_2866.gif/16.gif

Lines changed: 3 additions & 0 deletions
Loading

0 commit comments

Comments
 (0)