Add SSE2 version of Vp8Sse4X4 #1817

brianpopow · 2021-11-09T17:18:49Z

Prerequisites

I have written a descriptive pull-request title
I have verified that there are no overlapping pull-requests open
I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
I have provided test coverage for my change (where applicable)

Description

This adds a SSE2 version of the method Vp8Sse4X4, which is used during lossy encoding of webp images.
Related to #1786

Profiling results:

Master Branch:

PR:

P.S. im not sure again why the call count is different, it was the same image

# Conflicts: # src/ImageSharp/Formats/Webp/Lossy/LossyUtils.cs

codecov · 2021-11-09T17:38:18Z

Codecov Report

Merging #1817 (1997d59) into master (255226b) will decrease coverage by 0.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1817      +/-   ##
==========================================
- Coverage   87.34%   87.19%   -0.15%     
==========================================
  Files         936      936              
  Lines       48154    48164      +10     
  Branches     6038     6037       -1     
==========================================
- Hits        42058    41998      -60     
- Misses       5095     5162      +67     
- Partials     1001     1004       +3

Flag	Coverage Δ
unittests	`87.19% <100.00%> (-0.15%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/ImageSharp/Formats/Webp/Lossy/Vp8Encoding.cs	`100.00% <ø> (ø)`
src/ImageSharp/Formats/Webp/Lossy/Vp8Histogram.cs	`100.00% <ø> (ø)`
src/ImageSharp/Formats/Webp/Lossy/LossyUtils.cs	`100.00% <100.00%> (ø)`
src/ImageSharp/Formats/Webp/Lossy/QuantEnc.cs	`96.92% <100.00%> (-0.04%)`	⬇️
src/ImageSharp/Formats/Webp/Lossy/Vp8ModeScore.cs	`100.00% <100.00%> (ø)`
.../ImageSharp/Formats/Webp/Lossless/LosslessUtils.cs	`90.98% <0.00%> (-6.56%)`	⬇️
...ageSharp/Formats/Webp/Lossless/PredictorEncoder.cs	`92.81% <0.00%> (-5.49%)`	⬇️
...ImageSharp/Formats/Webp/Lossless/Vp8LBitEntropy.cs	`98.80% <0.00%> (-1.20%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 255226b...1997d59. Read the comment docs.

antonfirsov · 2021-11-09T18:24:53Z

src/ImageSharp/Formats/Webp/Lossy/LossyUtils.cs

+                Vector128<byte> a0 = Unsafe.As<byte, Vector128<byte>>(ref MemoryMarshal.GetReference(a));
+                Vector128<byte> a1 = Unsafe.As<byte, Vector128<byte>>(ref MemoryMarshal.GetReference(a.Slice(WebpConstants.Bps, 8)));
+                Vector128<byte> a2 = Unsafe.As<byte, Vector128<byte>>(ref MemoryMarshal.GetReference(a.Slice(WebpConstants.Bps * 2, 8)));
+                Vector128<byte> a3 = Unsafe.As<byte, Vector128<byte>>(ref MemoryMarshal.GetReference(a.Slice(WebpConstants.Bps * 3, 8)));


Slicing has some unnecessary extra costs, if input is safe we can do everything with ~~pointer~~ Unsafe arithmetics:

Suggested change

Vector128<byte> a0 = Unsafe.As<byte, Vector128<byte>>(ref MemoryMarshal.GetReference(a));

Vector128<byte> a1 = Unsafe.As<byte, Vector128<byte>>(ref MemoryMarshal.GetReference(a.Slice(WebpConstants.Bps, 8)));

Vector128<byte> a2 = Unsafe.As<byte, Vector128<byte>>(ref MemoryMarshal.GetReference(a.Slice(WebpConstants.Bps * 2, 8)));

Vector128<byte> a3 = Unsafe.As<byte, Vector128<byte>>(ref MemoryMarshal.GetReference(a.Slice(WebpConstants.Bps * 3, 8)));

ref byte aRef = ref MemoryMarshal.GetReference(a);

Vector128<byte> a0 = Unsafe.As<byte, Vector128<byte>>(ref aRef);

Vector128<byte> a1 = Unsafe.As<byte, Vector128<byte>>(ref Unsafe.Add(ref aRef, WebpConstants.Bps));

Vector128<byte> a2 = Unsafe.As<byte, Vector128<byte>>(ref Unsafe.Add(ref aRef, WebpConstants.Bps * 2));

Vector128<byte> a3 = Unsafe.As<byte, Vector128<byte>>(ref Unsafe.Add(ref aRef, WebpConstants.Bps * 3));

Same for b.

# Conflicts: # tests/ImageSharp.Tests/Formats/WebP/LossyUtilsTests.cs

JimBobSquarePants · 2021-11-09T23:41:23Z

src/ImageSharp/Formats/Webp/Lossy/LossyUtils.cs

+            {
+                return GetSse(a, b, 4, 4);
+            }
+        }

        [MethodImpl(InliningOptions.ShortMethod)]
        public static int GetSse(Span<byte> a, Span<byte> b, int w, int h)


This method name confuses me shouldn't it be Vp84X4?

Vp84X4 is confusing because of 84. Parent method can be Vp8_4x4 while this one can be Vp8_4X4Scalar. Or maybe Vp8_4x4Scalar could be inlined if it's not used anywhere else?

I agree that the name is not ideal, but its called VP8SSE4x4 in the original libwebp source and I wanted to keep the method names to be as close to the original as possible to make it easier to follow/commpare the original code.
A better name might have been CalculateDistortion4x4, but again I am still in favor of keeping the names similar to the original.

This method name confuses me shouldn't it be Vp84X4?

GetSse is not called Vp84X4 because of the variable parameters w and h.

I tried to rename the methods: 7e20c5d
Is that better (or worse)?

Much better yes 👍

antonfirsov · 2021-11-10T09:32:44Z

src/ImageSharp/Formats/Webp/Lossy/LossyUtils.cs

+            {
+                return GetSse(a, b, 4, 4);
+            }
+        }

        [MethodImpl(InliningOptions.ShortMethod)]
        public static int GetSse(Span<byte> a, Span<byte> b, int w, int h)


Vp84X4 is confusing because of 84. Parent method can be Vp8_4x4 while this one can be Vp8_4X4Scalar. Or maybe Vp8_4x4Scalar could be inlined if it's not used anywhere else?

antonfirsov · 2021-11-10T09:41:28Z

src/ImageSharp/Formats/Webp/Lossy/Vp8ModeScore.cs

+            Array.Clear(this.YDcLevels, 0, this.YDcLevels.Length);
+            Array.Clear(this.YAcLevels, 0, this.YAcLevels.Length);
+            Array.Clear(this.UvLevels, 0, this.UvLevels.Length);
+            Array.Clear(this.ModesI4, 0, this.ModesI4.Length);
+            Array.Clear(this.Derr, 0, this.Derr.Length);


Unrelated to the main thing in the PR, but Vp8ModeScore is also a good candidate to be made a value type with fixed buffers.

yes seems reasonable, Vp8ModeScore makes up the most of the allocations during encoding, but I might try that in a different PR.

Definitely worth a look after.

JimBobSquarePants

LGTM so far.

brianpopow added 6 commits November 8, 2021 10:41

Add sse2 version of Vp8Sse4X4

8d19c28

Avoid pinning

99a3510

Merge remote-tracking branch 'origin/master' into bp/sse4X4

9756adc

# Conflicts: # src/ImageSharp/Formats/Webp/Lossy/LossyUtils.cs

Use Numerics.ReduceSum(sum)

de3140b

Remove not needed clear

80a826f

Add Vp8Sse4X4 sse tests

5abd774

brianpopow added area:performance formats:webp labels Nov 9, 2021

Use Array.Clear to reset the arrays

5ead844

antonfirsov reviewed Nov 9, 2021

View reviewed changes

brianpopow added 4 commits November 9, 2021 19:43

Dont use slice

7312b1a

Remove unnecessary Clear() and scratch buffer

3dd7c8e

Remove more unnecessary Clear's

5630b25

Merge remote-tracking branch 'origin/master' into bp/sse4X4

dcca236

# Conflicts: # tests/ImageSharp.Tests/Formats/WebP/LossyUtilsTests.cs

JimBobSquarePants reviewed Nov 9, 2021

View reviewed changes

antonfirsov reviewed Nov 10, 2021

View reviewed changes

brianpopow added 2 commits November 10, 2021 12:10

Rename Vp8Sse methods

7e20c5d

Fix build error due to renaming

1997d59

JimBobSquarePants approved these changes Nov 10, 2021

View reviewed changes

JimBobSquarePants merged commit 7d74c4c into master Nov 10, 2021

JimBobSquarePants deleted the bp/sse4X4 branch November 10, 2021 13:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SSE2 version of Vp8Sse4X4 #1817

Add SSE2 version of Vp8Sse4X4 #1817

brianpopow commented Nov 9, 2021

codecov bot commented Nov 9, 2021 •

edited

Loading

antonfirsov Nov 9, 2021 •

edited

Loading

JimBobSquarePants Nov 9, 2021

antonfirsov Nov 10, 2021 •

edited

Loading

brianpopow Nov 10, 2021

brianpopow Nov 10, 2021

JimBobSquarePants Nov 10, 2021

antonfirsov Nov 10, 2021 •

edited

Loading

antonfirsov Nov 10, 2021

brianpopow Nov 10, 2021

JimBobSquarePants Nov 10, 2021

JimBobSquarePants left a comment

-                Vector128<byte> a0 = Unsafe.As<byte, Vector128<byte>>(ref MemoryMarshal.GetReference(a));
-                Vector128<byte> a1 = Unsafe.As<byte, Vector128<byte>>(ref MemoryMarshal.GetReference(a.Slice(WebpConstants.Bps, 8)));
-                Vector128<byte> a2 = Unsafe.As<byte, Vector128<byte>>(ref MemoryMarshal.GetReference(a.Slice(WebpConstants.Bps * 2, 8)));
-                Vector128<byte> a3 = Unsafe.As<byte, Vector128<byte>>(ref MemoryMarshal.GetReference(a.Slice(WebpConstants.Bps * 3, 8)));
+                ref byte aRef = ref MemoryMarshal.GetReference(a);
+                Vector128<byte> a0 = Unsafe.As<byte, Vector128<byte>>(ref aRef);
+                Vector128<byte> a1 = Unsafe.As<byte, Vector128<byte>>(ref Unsafe.Add(ref aRef, WebpConstants.Bps));
+                Vector128<byte> a2 = Unsafe.As<byte, Vector128<byte>>(ref Unsafe.Add(ref aRef, WebpConstants.Bps * 2));
+                Vector128<byte> a3 = Unsafe.As<byte, Vector128<byte>>(ref Unsafe.Add(ref aRef, WebpConstants.Bps * 3));

Add SSE2 version of Vp8Sse4X4 #1817

Add SSE2 version of Vp8Sse4X4 #1817

Conversation

brianpopow commented Nov 9, 2021

Prerequisites

Description

Profiling results:

Master Branch:

PR:

codecov bot commented Nov 9, 2021 • edited Loading

Codecov Report

antonfirsov Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

JimBobSquarePants Nov 9, 2021

Choose a reason for hiding this comment

antonfirsov Nov 10, 2021 • edited Loading

Choose a reason for hiding this comment

brianpopow Nov 10, 2021

Choose a reason for hiding this comment

brianpopow Nov 10, 2021

Choose a reason for hiding this comment

JimBobSquarePants Nov 10, 2021

Choose a reason for hiding this comment

antonfirsov Nov 10, 2021 • edited Loading

Choose a reason for hiding this comment

antonfirsov Nov 10, 2021

Choose a reason for hiding this comment

brianpopow Nov 10, 2021

Choose a reason for hiding this comment

JimBobSquarePants Nov 10, 2021

Choose a reason for hiding this comment

JimBobSquarePants left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 9, 2021 •

edited

Loading

antonfirsov Nov 9, 2021 •

edited

Loading

antonfirsov Nov 10, 2021 •

edited

Loading

antonfirsov Nov 10, 2021 •

edited

Loading