Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Vp8Residual costs calculation #2432

Merged
merged 16 commits into from
Apr 16, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/ImageSharp/Formats/Webp/Lossy/Vp8BandProbas.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.

using System.Text.Json.Serialization;

namespace SixLabors.ImageSharp.Formats.Webp.Lossy;

/// <summary>
Expand All @@ -20,6 +22,13 @@ public Vp8BandProbas()
}
}

/// <summary>
/// Initializes a new instance of the <see cref="Vp8BandProbas"/> class.
/// Only used for unit tests.
/// </summary>
[JsonConstructor]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably use a custom JSON converter in the tests rather than adding attributes to the source.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @brianpopow I've given you the runaround here. I didn't realize you'd introduced the constructors.

Let's go with your original plan but wrap the namespace imports and constructor code in the following conditional.

#if SIXLABORS_TESTING || SIXLABORS_TESTING_PREVIEW

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go with your original plan but wrap the namespace imports and constructor code in the following conditional.
#if SIXLABORS_TESTING || SIXLABORS_TESTING_PREVIEW

I dont think that's a good idea. If someone does a fresh clone of the repo and tries to run the tests, those webp tests will fail without any obvious reason. I dont think test should behave different when a environment is set. Unfortunately I dont have a better idea so far.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid the json and populate the test instance with equivalent data generated in a class using string builder or similar?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid the json and populate the test instance with equivalent data generated in a class using string builder or similar?

Usually I would create a test instance in the unittest and populate it with data, but for Vp8Residual that does not seem feasible. There are so many nested arrays, that I dont know how this can be populated manually. Thats why I have chosen to go for json serialization.

Also I dont understand your suggestion with the string builder.

I now tried to use the binary serialization and this was looking promising. We would not need the additional constructors.
But now I noticed its marked as obsolete 😢

See: https://learn.microsoft.com/de-de/dotnet/standard/serialization/binaryformatter-security-guide

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed serialization now to use BinaryWriter/BinaryReader, according to https://learn.microsoft.com/de-de/dotnet/standard/serialization/binaryformatter-security-guide this should be fine.

No additional constructors required now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah that’s fantastic! We’ll done finding a solution!

public Vp8BandProbas(Vp8ProbaArray[] probabilities) => this.Probabilities = probabilities;

/// <summary>
/// Gets the Probabilities.
/// </summary>
Expand Down
9 changes: 9 additions & 0 deletions src/ImageSharp/Formats/Webp/Lossy/Vp8CostArray.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.

using System.Text.Json.Serialization;

namespace SixLabors.ImageSharp.Formats.Webp.Lossy;

internal class Vp8CostArray
Expand All @@ -10,5 +12,12 @@ internal class Vp8CostArray
/// </summary>
public Vp8CostArray() => this.Costs = new ushort[67 + 1];

/// <summary>
/// Initializes a new instance of the <see cref="Vp8CostArray"/> class.
/// Only used for unit tests.
/// </summary>
[JsonConstructor]
public Vp8CostArray(ushort[] costs) => this.Costs = costs;

public ushort[] Costs { get; }
}
9 changes: 9 additions & 0 deletions src/ImageSharp/Formats/Webp/Lossy/Vp8Costs.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.

using System.Text.Json.Serialization;

namespace SixLabors.ImageSharp.Formats.Webp.Lossy;

internal class Vp8Costs
Expand All @@ -17,6 +19,13 @@ public Vp8Costs()
}
}

/// <summary>
/// Initializes a new instance of the <see cref="Vp8Costs"/> class.
/// Only used for unit tests.
/// </summary>
[JsonConstructor]
public Vp8Costs(Vp8CostArray[] costs) => this.Costs = costs;

/// <summary>
/// Gets the Costs.
/// </summary>
Expand Down
9 changes: 9 additions & 0 deletions src/ImageSharp/Formats/Webp/Lossy/Vp8ProbaArray.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.

using System.Text.Json.Serialization;

namespace SixLabors.ImageSharp.Formats.Webp.Lossy;

/// <summary>
Expand All @@ -13,6 +15,13 @@ internal class Vp8ProbaArray
/// </summary>
public Vp8ProbaArray() => this.Probabilities = new byte[WebpConstants.NumProbas];

/// <summary>
/// Initializes a new instance of the <see cref="Vp8ProbaArray"/> class.
/// Only used for unit tests.
/// </summary>
[JsonConstructor]
public Vp8ProbaArray(byte[] probabilities) => this.Probabilities = probabilities;

/// <summary>
/// Gets the probabilities.
/// </summary>
Expand Down
41 changes: 31 additions & 10 deletions src/ImageSharp/Formats/Webp/Lossy/Vp8Residual.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Runtime.InteropServices;
using System.Runtime.Intrinsics;
using System.Runtime.Intrinsics.X86;
using System.Text.Json.Serialization;

namespace SixLabors.ImageSharp.Formats.Webp.Lossy;

Expand All @@ -15,6 +16,22 @@ namespace SixLabors.ImageSharp.Formats.Webp.Lossy;
/// </summary>
internal class Vp8Residual
{
public Vp8Residual()
{
}

[JsonConstructor]
public Vp8Residual(int first, int last, int coeffType, short[] coeffs, Vp8BandProbas[] prob, Vp8Stats[] stats, Vp8Costs[] costs)
{
this.First = first;
this.Last = last;
this.CoeffType = coeffType;
this.Coeffs = coeffs;
this.Prob = prob;
this.Stats = stats;
this.Costs = costs;
}

public int First { get; set; }

public int Last { get; set; }
Expand Down Expand Up @@ -156,7 +173,7 @@ public int GetResidualCost(int ctx0)
return LossyUtils.Vp8BitCost(0, (byte)p0);
}

if (Avx2.IsSupported)
if (Sse2.IsSupported)
{
Span<byte> scratch = stackalloc byte[32];
Span<byte> ctxs = scratch.Slice(0, 16);
Expand All @@ -165,19 +182,23 @@ public int GetResidualCost(int ctx0)

// Precompute clamped levels and contexts, packed to 8b.
ref short outputRef = ref MemoryMarshal.GetReference<short>(this.Coeffs);
Vector256<short> c0 = Unsafe.As<short, Vector256<byte>>(ref outputRef).AsInt16();
Vector256<short> d0 = Avx2.Subtract(Vector256<short>.Zero, c0);
Vector256<short> e0 = Avx2.Max(c0, d0); // abs(v), 16b
Vector256<sbyte> f = Avx2.PackSignedSaturate(e0, e0);
Vector256<byte> g = Avx2.Min(f.AsByte(), Vector256.Create((byte)2));
Vector256<byte> h = Avx2.Min(f.AsByte(), Vector256.Create((byte)67)); // clampLevel in [0..67]
Vector128<short> c0 = Unsafe.As<short, Vector128<byte>>(ref outputRef).AsInt16();
Vector128<short> c1 = Unsafe.As<short, Vector128<byte>>(ref Unsafe.Add(ref outputRef, 8)).AsInt16();
Vector128<short> d0 = Sse2.Subtract(Vector128<short>.Zero, c0);
Vector128<short> d1 = Sse2.Subtract(Vector128<short>.Zero, c1);
Vector128<short> e0 = Sse2.Max(c0, d0); // abs(v), 16b
Vector128<short> e1 = Sse2.Max(c1, d1);
Vector128<sbyte> f = Sse2.PackSignedSaturate(e0, e1);
Vector128<byte> g = Sse2.Min(f.AsByte(), Vector128.Create((byte)2)); // context = 0, 1, 2
Vector128<byte> h = Sse2.Min(f.AsByte(), Vector128.Create((byte)67)); // clampLevel in [0..67]

ref byte ctxsRef = ref MemoryMarshal.GetReference(ctxs);
ref byte levelsRef = ref MemoryMarshal.GetReference(levels);
ref ushort absLevelsRef = ref MemoryMarshal.GetReference(absLevels);
Unsafe.As<byte, Vector128<byte>>(ref ctxsRef) = g.GetLower();
Unsafe.As<byte, Vector128<byte>>(ref levelsRef) = h.GetLower();
Unsafe.As<ushort, Vector256<ushort>>(ref absLevelsRef) = e0.AsUInt16();
Unsafe.As<byte, Vector128<byte>>(ref ctxsRef) = g;
Unsafe.As<byte, Vector128<byte>>(ref levelsRef) = h;
Unsafe.As<ushort, Vector128<ushort>>(ref absLevelsRef) = e0.AsUInt16();
Unsafe.As<ushort, Vector128<ushort>>(ref Unsafe.Add(ref absLevelsRef, 8)) = e1.AsUInt16();

int level;
int flevel;
Expand Down
9 changes: 9 additions & 0 deletions src/ImageSharp/Formats/Webp/Lossy/Vp8Stats.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.

using System.Text.Json.Serialization;

namespace SixLabors.ImageSharp.Formats.Webp.Lossy;

internal class Vp8Stats
Expand All @@ -17,5 +19,12 @@ public Vp8Stats()
}
}

/// <summary>
/// Initializes a new instance of the <see cref="Vp8Stats"/> class.
/// Only used for unit tests.
/// </summary>
[JsonConstructor]
public Vp8Stats(Vp8StatsArray[] stats) => this.Stats = stats;

public Vp8StatsArray[] Stats { get; }
}
9 changes: 9 additions & 0 deletions src/ImageSharp/Formats/Webp/Lossy/Vp8StatsArray.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.

using System.Text.Json.Serialization;

namespace SixLabors.ImageSharp.Formats.Webp.Lossy;

internal class Vp8StatsArray
Expand All @@ -10,5 +12,12 @@ internal class Vp8StatsArray
/// </summary>
public Vp8StatsArray() => this.Stats = new uint[WebpConstants.NumProbas];

/// <summary>
/// Initializes a new instance of the <see cref="Vp8StatsArray"/> class.
/// Only used for unit tests.
/// </summary>
[JsonConstructor]
public Vp8StatsArray(uint[] stats) => this.Stats = stats;

public uint[] Stats { get; }
}
136 changes: 130 additions & 6 deletions tests/ImageSharp.Tests/Formats/WebP/Vp8ResidualTests.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.

using System.Runtime.Intrinsics.X86;
using System.Text.Json;
using SixLabors.ImageSharp.Formats.Webp;
using SixLabors.ImageSharp.Formats.Webp.Lossy;
using SixLabors.ImageSharp.Tests.TestUtilities;

Expand All @@ -9,10 +12,134 @@ namespace SixLabors.ImageSharp.Tests.Formats.Webp;
[Trait("Format", "Webp")]
public class Vp8ResidualTests
{
[Fact]
public void GetResidualCost_Works()
{
if (!Sse2.IsSupported)
{
// JsonSerializer without SSE2 does not seem to work, skip test then.
return;
}

// arrange
int ctx0 = 0;
int expected = 20911;
string jsonString = File.ReadAllText(Path.Combine("TestDataWebp", "Vp8Residual.json"));
Vp8Residual residual = JsonSerializer.Deserialize<Vp8Residual>(jsonString);

// act
int actual = residual.GetResidualCost(ctx0);

// assert
Assert.Equal(expected, actual);
}

[Fact]
public void Serialization_Works()
{
if (!Sse2.IsSupported)
{
// JsonSerializer without SSE2 does not seem to work, skip test then.
return;
}

// arrange
Vp8Residual expected = new();
Vp8EncProba encProb = new();
Random rand = new(439);
CreateRandomProbas(encProb, rand);
CreateCosts(encProb, rand);
expected.Init(1, 0, encProb);
for (int i = 0; i < expected.Coeffs.Length; i++)
{
expected.Coeffs[i] = (byte)rand.Next(255);
}

// act
string jsonString = JsonSerializer.Serialize(expected);
Vp8Residual actual = JsonSerializer.Deserialize<Vp8Residual>(jsonString);

// assert
Assert.Equal(expected.CoeffType, actual.CoeffType);
Assert.Equal(expected.Coeffs, actual.Coeffs);
Assert.Equal(expected.Costs.Length, actual.Costs.Length);
Assert.Equal(expected.First, actual.First);
Assert.Equal(expected.Last, actual.Last);
Assert.Equal(expected.Stats.Length, actual.Stats.Length);
for (int i = 0; i < expected.Stats.Length; i++)
{
Vp8StatsArray[] expectedStats = expected.Stats[i].Stats;
Vp8StatsArray[] actualStats = actual.Stats[i].Stats;
Assert.Equal(expectedStats.Length, actualStats.Length);
for (int j = 0; j < expectedStats.Length; j++)
{
Assert.Equal(expectedStats[j].Stats, actualStats[j].Stats);
}
}

Assert.Equal(expected.Prob.Length, actual.Prob.Length);
for (int i = 0; i < expected.Prob.Length; i++)
{
Vp8ProbaArray[] expectedProbabilities = expected.Prob[i].Probabilities;
Vp8ProbaArray[] actualProbabilities = actual.Prob[i].Probabilities;
Assert.Equal(expectedProbabilities.Length, actualProbabilities.Length);
for (int j = 0; j < expectedProbabilities.Length; j++)
{
Assert.Equal(expectedProbabilities[j].Probabilities, actualProbabilities[j].Probabilities);
}
}

for (int i = 0; i < expected.Costs.Length; i++)
{
Vp8CostArray[] expectedCosts = expected.Costs[i].Costs;
Vp8CostArray[] actualCosts = actual.Costs[i].Costs;
Assert.Equal(expectedCosts.Length, actualCosts.Length);
for (int j = 0; j < expectedCosts.Length; j++)
{
Assert.Equal(expectedCosts[j].Costs, actualCosts[j].Costs);
}
}
}

private static void CreateRandomProbas(Vp8EncProba probas, Random rand)
{
for (int t = 0; t < WebpConstants.NumTypes; ++t)
{
for (int b = 0; b < WebpConstants.NumBands; ++b)
{
for (int c = 0; c < WebpConstants.NumCtx; ++c)
{
for (int p = 0; p < WebpConstants.NumProbas; ++p)
{
probas.Coeffs[t][b].Probabilities[c].Probabilities[p] = (byte)rand.Next(255);
}
}
}
}
}

private static void CreateCosts(Vp8EncProba probas, Random rand)
{
for (int i = 0; i < probas.RemappedCosts.Length; i++)
{
for (int j = 0; j < probas.RemappedCosts[i].Length; j++)
{
for (int k = 0; k < probas.RemappedCosts[i][j].Costs.Length; k++)
{
ushort[] costs = probas.RemappedCosts[i][j].Costs[k].Costs;
for (int m = 0; m < costs.Length; m++)
{
costs[m] = (byte)rand.Next(255);
}
}
}
}
}

private static void RunSetCoeffsTest()
{
// arrange
var residual = new Vp8Residual();
Vp8Residual residual = new();
short[] coeffs = { 110, 0, -2, 0, 0, 0, 0, 0, 0, -1, 0, 0, 0, 0, 0, 0 };

// act
Expand All @@ -23,11 +150,8 @@ private static void RunSetCoeffsTest()
}

[Fact]
public void RunSetCoeffsTest_Works() => RunSetCoeffsTest();

[Fact]
public void RunSetCoeffsTest_WithHardwareIntrinsics_Works() => FeatureTestRunner.RunWithHwIntrinsicsFeature(RunSetCoeffsTest, HwIntrinsics.AllowAll);
public void SetCoeffsTest_Works() => FeatureTestRunner.RunWithHwIntrinsicsFeature(RunSetCoeffsTest, HwIntrinsics.AllowAll);

[Fact]
public void RunSetCoeffsTest_WithoutHardwareIntrinsics_Works() => FeatureTestRunner.RunWithHwIntrinsicsFeature(RunSetCoeffsTest, HwIntrinsics.DisableHWIntrinsic);
public void SetCoeffsTest_WithoutSSE2_Works() => FeatureTestRunner.RunWithHwIntrinsicsFeature(RunSetCoeffsTest, HwIntrinsics.DisableSSE2);
}
3 changes: 3 additions & 0 deletions tests/ImageSharp.Tests/ImageSharp.Tests.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@
<None Update="TestFonts\SixLaborsSampleAB.woff">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</None>
<None Update="TestDataWebp\Vp8Residual.json">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</None>
<None Update="xunit.runner.json">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</None>
Expand Down
1 change: 1 addition & 0 deletions tests/ImageSharp.Tests/TestDataWebp/Vp8Residual.json

Large diffs are not rendered by default.