From ded5b162d903db4c6332ef2f885a47e458ec033f Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Tue, 15 Jun 2021 04:18:52 +0300 Subject: [PATCH 01/12] Implemented log2 method --- src/ImageSharp/Common/Helpers/Numerics.cs | 58 +++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/src/ImageSharp/Common/Helpers/Numerics.cs b/src/ImageSharp/Common/Helpers/Numerics.cs index ef457f7ceb..a0ce62f683 100644 --- a/src/ImageSharp/Common/Helpers/Numerics.cs +++ b/src/ImageSharp/Common/Helpers/Numerics.cs @@ -23,6 +23,7 @@ namespace SixLabors.ImageSharp private const int ShuffleAlphaControl = 0b_11_11_11_11; #endif + // TODO: Obsolete - remove #if !SUPPORTS_BITOPERATIONS /// /// Gets the counts the number of bits needed to hold an integer. @@ -45,6 +46,16 @@ namespace SixLabors.ImageSharp }; #endif +#if !SUPPORTS_BITOPERATIONS + private static ReadOnlySpan Log2DeBruijn => new byte[32] + { + 00, 09, 01, 10, 13, 21, 02, 29, + 11, 14, 16, 18, 22, 25, 03, 30, + 08, 12, 20, 28, 15, 17, 24, 07, + 19, 27, 23, 06, 26, 05, 04, 31 + }; +#endif + /// /// Determine the Greatest CommonDivisor (GCD) of two numbers. /// @@ -868,5 +879,52 @@ namespace SixLabors.ImageSharp return bitInUnsignedInteger - BitOperations.LeadingZeroCount(number); #endif } + + /// + /// Calculates floored log of the specified value, base 2. + /// Note that by convention, input value 0 returns 0 since Log(0) is undefined. + /// + /// The value. + public static int Log2(uint value) + { +#if SUPPORTS_BITOPERATIONS + return BitOperations.Log2(value); +#else + return Log2SoftwareFallback(value); +#endif + } + +#if !SUPPORTS_BITOPERATIONS + /// + /// Calculates floored log of the specified value, base 2. + /// Note that by convention, input value 0 returns 0 since Log(0) is undefined. + /// Bit hacking with deBruijn sequence, extremely fast yet does not use any intrinsics so should work on every platform. + /// + /// + /// Description of this bit hacking can be found here: + /// https://cstheory.stackexchange.com/questions/19524/using-the-de-bruijn-sequence-to-find-the-lceil-log-2-v-rceil-of-an-integer + /// + /// The value. + private static int Log2SoftwareFallback(uint value) + { + // No AggressiveInlining due to large method size + // Has conventional contract 0->0 (Log(0) is undefined) by default, no need for if checking + + + // Fill trailing zeros with ones, eg 00010010 becomes 00011111 + value |= value >> 01; + value |= value >> 02; + value |= value >> 04; + value |= value >> 08; + value |= value >> 16; + + // uint.MaxValue >> 27 is always in range [0 - 31] so we use Unsafe.AddByteOffset to avoid bounds check + return Unsafe.AddByteOffset( + ref MemoryMarshal.GetReference(Log2DeBruijn), + + // uint|long -> IntPtr cast on 32-bit platforms does expensive overflow checks not needed here + (IntPtr)(int)((value * 0x07C4ACDDu) >> 27)); + } +#endif } } From 83643166bab1c141e93983f34c91410cef1e72ef Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Tue, 15 Jun 2021 05:31:09 +0300 Subject: [PATCH 02/12] Renamed MinimumBitsToStore16 metho to something more specific, added comments, added more peformant fallback implementation --- src/ImageSharp/Common/Helpers/Numerics.cs | 32 ++++++++++++------- .../Components/Encoder/HuffmanScanEncoder.cs | 2 +- 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/src/ImageSharp/Common/Helpers/Numerics.cs b/src/ImageSharp/Common/Helpers/Numerics.cs index a0ce62f683..28c9d6705b 100644 --- a/src/ImageSharp/Common/Helpers/Numerics.cs +++ b/src/ImageSharp/Common/Helpers/Numerics.cs @@ -860,23 +860,31 @@ namespace SixLabors.ImageSharp #endif /// - /// Calculates how many minimum bits needed to store given value. + /// Calculates how many minimum bits needed to store given value for Huffman jpeg encoding. + /// This method does not follow the standard convention - it does not support input value of zero. /// - /// Unsigned integer to store - /// Minimum number of bits needed to store given value + /// + /// Passing zero as input value would result in an undefined behaviour. + /// This is done for performance reasons as Huffman encoding code checks for zero value, second identical check could degrade the performance in the hot path. + /// If this method is needed somewhere else apart from jpeg encoding - use explicit if check for zero value case. + /// + /// The value. [MethodImpl(MethodImplOptions.AggressiveInlining)] - public static int MinimumBitsToStore16(uint number) + internal static int GetHuffmanEncodingLegth(uint value) { -#if !SUPPORTS_BITOPERATIONS - if (number < 0x100) - { - return BitCountLut[(int)number]; - } + DebugGuard.IsFalse(value == 0, nameof(value), "Huffman encoding does not encode zero values"); +#if SUPPORTS_BITOPERATIONS + // This should have been implemented as (BitOperations.Log2(value) + 1) as in non-intrinsic implementation + // But internal log2 is implementated like this: (31 - (int)Lzcnt.LeadingZeroCount(value)) - return 8 + BitCountLut[(int)number >> 8]; + // BitOperations.Log2 implementation also checks if input value is zero for the convention + // As this is a very specific method for a specific Huffman encoding code + // We can omit zero check as this is guranteed not to be invoked with value == 0 and guarded in debug builds + return 32 - BitOperations.LeadingZeroCount(value); #else - const int bitInUnsignedInteger = sizeof(uint) * 8; - return bitInUnsignedInteger - BitOperations.LeadingZeroCount(number); + // On the contrary to BitOperations implementation this does follow the convention and supports value == 0 case + // Although it's still won't be called with value == 0 + return Log2SoftwareFallback(value) + 1; #endif } diff --git a/src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs b/src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs index ca352397b8..2a21ae75f8 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs @@ -360,7 +360,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Encoder b = value - 1; } - int bt = Numerics.MinimumBitsToStore16((uint)a); + int bt = Numerics.GetHuffmanEncodingLegth((uint)a); this.EmitHuff(index, (runLength << 4) | bt); if (bt > 0) From e696b1971fc567e813a4c284a3b146c7ea65615a Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Tue, 15 Jun 2021 05:32:22 +0300 Subject: [PATCH 03/12] Removed obsolete table --- src/ImageSharp/Common/Helpers/Numerics.cs | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/src/ImageSharp/Common/Helpers/Numerics.cs b/src/ImageSharp/Common/Helpers/Numerics.cs index 28c9d6705b..f7d8c8014d 100644 --- a/src/ImageSharp/Common/Helpers/Numerics.cs +++ b/src/ImageSharp/Common/Helpers/Numerics.cs @@ -23,29 +23,6 @@ namespace SixLabors.ImageSharp private const int ShuffleAlphaControl = 0b_11_11_11_11; #endif - // TODO: Obsolete - remove -#if !SUPPORTS_BITOPERATIONS - /// - /// Gets the counts the number of bits needed to hold an integer. - /// - private static ReadOnlySpan BitCountLut => new byte[] - { - 0, 1, 2, 2, 3, 3, 3, 3, 4, 4, 4, 4, 4, 4, 4, 4, 5, 5, 5, 5, 5, 5, - 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, - 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, - 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, - 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, - 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, - 7, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, - 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, - 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, - 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, - 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, - 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, - 8, 8, 8, - }; -#endif - #if !SUPPORTS_BITOPERATIONS private static ReadOnlySpan Log2DeBruijn => new byte[32] { From bf61a7dc13314070317e9cd5ce2bcab05fcc6bec Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Tue, 15 Jun 2021 05:40:17 +0300 Subject: [PATCH 04/12] Fixed comments --- src/ImageSharp/Common/Helpers/Numerics.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/ImageSharp/Common/Helpers/Numerics.cs b/src/ImageSharp/Common/Helpers/Numerics.cs index f7d8c8014d..83f2a1f7c6 100644 --- a/src/ImageSharp/Common/Helpers/Numerics.cs +++ b/src/ImageSharp/Common/Helpers/Numerics.cs @@ -855,8 +855,9 @@ namespace SixLabors.ImageSharp // But internal log2 is implementated like this: (31 - (int)Lzcnt.LeadingZeroCount(value)) // BitOperations.Log2 implementation also checks if input value is zero for the convention - // As this is a very specific method for a specific Huffman encoding code - // We can omit zero check as this is guranteed not to be invoked with value == 0 and guarded in debug builds + // As this is a very specific method for a very specific Huffman encoding code + // We can omit zero check as this should not be invoked with value == 0 and guarded in debug builds + // This is also marked as internal so every use of this would be tracable & testable in tests return 32 - BitOperations.LeadingZeroCount(value); #else // On the contrary to BitOperations implementation this does follow the convention and supports value == 0 case From 3c70300a41526201213bcef6549b333f62d1bd62 Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Tue, 15 Jun 2021 06:40:54 +0300 Subject: [PATCH 05/12] Added Log2 tests --- .../ImageSharp.Tests/Common/NumericsTests.cs | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100644 tests/ImageSharp.Tests/Common/NumericsTests.cs diff --git a/tests/ImageSharp.Tests/Common/NumericsTests.cs b/tests/ImageSharp.Tests/Common/NumericsTests.cs new file mode 100644 index 0000000000..29eae6d488 --- /dev/null +++ b/tests/ImageSharp.Tests/Common/NumericsTests.cs @@ -0,0 +1,73 @@ +// Copyright (c) Six Labors. +// Licensed under the Apache License, Version 2.0. + +using System; +using Xunit; +using Xunit.Abstractions; + +namespace SixLabors.ImageSharp.Tests.Common +{ + public class NumericsTests + { + private ITestOutputHelper Output { get; } + + public NumericsTests(ITestOutputHelper output) + { + this.Output = output; + } + + private static int Log2_ReferenceImplementation(uint value) + { + int n = 0; + while ((value >>= 1) != 0) + { + ++n; + } + + return n; + } + + [Fact] + public void Log2_ZeroConvention() + { + uint value = 0; + int expected = 0; + int actual = Numerics.Log2(value); + + Assert.True(expected == actual, $"Expected: {expected}, Actual: {actual}"); + } + + [Fact] + public void Log2_PowersOfTwo() + { + for (int i = 0; i < sizeof(int) * 8; i++) + { + // from 2^0 to 2^32 + uint value = (uint)(1 << i); + int expected = i; + int actual = Numerics.Log2(value); + + Assert.True(expected == actual, $"Expected: {expected}, Actual: {actual}"); + } + } + + [Theory] + [InlineData(1, 100)] + [InlineData(2, 100)] + public void Log2_RandomValues(int seed, int count) + { + var rng = new Random(seed); + byte[] bytes = new byte[4]; + + for (int i = 0; i < count; i++) + { + rng.NextBytes(bytes); + uint value = BitConverter.ToUInt32(bytes, 0); + int expected = Log2_ReferenceImplementation(value); + int actual = Numerics.Log2(value); + + Assert.True(expected == actual, $"Expected: {expected}, Actual: {actual}"); + } + } + } +} From ab2a97a9653a6f3de3705b1892b1fa5b0cc85abb Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Tue, 15 Jun 2021 06:46:11 +0300 Subject: [PATCH 06/12] Moved jpeg specific code from Numerics.cs to the jpeg related code --- src/ImageSharp/Common/Helpers/Numerics.cs | 32 +------------------ .../Components/Encoder/HuffmanScanEncoder.cs | 29 +++++++++++++++++ 2 files changed, 30 insertions(+), 31 deletions(-) diff --git a/src/ImageSharp/Common/Helpers/Numerics.cs b/src/ImageSharp/Common/Helpers/Numerics.cs index 83f2a1f7c6..eff1372c17 100644 --- a/src/ImageSharp/Common/Helpers/Numerics.cs +++ b/src/ImageSharp/Common/Helpers/Numerics.cs @@ -836,36 +836,6 @@ namespace SixLabors.ImageSharp } #endif - /// - /// Calculates how many minimum bits needed to store given value for Huffman jpeg encoding. - /// This method does not follow the standard convention - it does not support input value of zero. - /// - /// - /// Passing zero as input value would result in an undefined behaviour. - /// This is done for performance reasons as Huffman encoding code checks for zero value, second identical check could degrade the performance in the hot path. - /// If this method is needed somewhere else apart from jpeg encoding - use explicit if check for zero value case. - /// - /// The value. - [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal static int GetHuffmanEncodingLegth(uint value) - { - DebugGuard.IsFalse(value == 0, nameof(value), "Huffman encoding does not encode zero values"); -#if SUPPORTS_BITOPERATIONS - // This should have been implemented as (BitOperations.Log2(value) + 1) as in non-intrinsic implementation - // But internal log2 is implementated like this: (31 - (int)Lzcnt.LeadingZeroCount(value)) - - // BitOperations.Log2 implementation also checks if input value is zero for the convention - // As this is a very specific method for a very specific Huffman encoding code - // We can omit zero check as this should not be invoked with value == 0 and guarded in debug builds - // This is also marked as internal so every use of this would be tracable & testable in tests - return 32 - BitOperations.LeadingZeroCount(value); -#else - // On the contrary to BitOperations implementation this does follow the convention and supports value == 0 case - // Although it's still won't be called with value == 0 - return Log2SoftwareFallback(value) + 1; -#endif - } - /// /// Calculates floored log of the specified value, base 2. /// Note that by convention, input value 0 returns 0 since Log(0) is undefined. @@ -891,7 +861,7 @@ namespace SixLabors.ImageSharp /// https://cstheory.stackexchange.com/questions/19524/using-the-de-bruijn-sequence-to-find-the-lceil-log-2-v-rceil-of-an-integer /// /// The value. - private static int Log2SoftwareFallback(uint value) + internal static int Log2SoftwareFallback(uint value) { // No AggressiveInlining due to large method size // Has conventional contract 0->0 (Log(0) is undefined) by default, no need for if checking diff --git a/src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs b/src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs index 2a21ae75f8..a8382df2bb 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs @@ -388,5 +388,34 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Encoder this.target.Write(this.emitBuffer, 0, this.emitLen); } } + + /// + /// Calculates how many minimum bits needed to store given value for Huffman jpeg encoding. + /// This method does not follow the standard convention - it does not support input value of zero. + /// + /// + /// Passing zero as input value would result in an undefined behaviour. + /// This is done for performance reasons as Huffman encoding code checks for zero value, second identical check would degrade the performance in the hot path. + /// + /// The value. + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private static int GetHuffmanEncodingLegth(uint value) + { + DebugGuard.IsFalse(value == 0, nameof(value), "Huffman encoding does not encode zero values"); +#if SUPPORTS_BITOPERATIONS + // This should have been implemented as (BitOperations.Log2(value) + 1) as in non-intrinsic implementation + // But internal log2 is implementated like this: (31 - (int)Lzcnt.LeadingZeroCount(value)) + + // BitOperations.Log2 implementation also checks if input value is zero for the convention + // As this is a very specific method for a very specific Huffman encoding code + // We can omit zero check as this should not be invoked with value == 0 and guarded in debug builds + // This is also marked as internal so every use of this would be tracable & testable in tests + return 32 - System.Numerics.BitOperations.LeadingZeroCount(value); +#else + // On the contrary to BitOperations implementation this does follow the convention and supports value == 0 case + // Although it's still won't be called with value == 0 + return Numerics.Log2SoftwareFallback(value) + 1; +#endif + } } } From a4475fa3b6e194e60e476ab78c985d19d93f0f1e Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Tue, 15 Jun 2021 06:57:10 +0300 Subject: [PATCH 07/12] Small docs fixes --- src/ImageSharp/Common/Helpers/Numerics.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/ImageSharp/Common/Helpers/Numerics.cs b/src/ImageSharp/Common/Helpers/Numerics.cs index eff1372c17..9faf1cda08 100644 --- a/src/ImageSharp/Common/Helpers/Numerics.cs +++ b/src/ImageSharp/Common/Helpers/Numerics.cs @@ -854,7 +854,7 @@ namespace SixLabors.ImageSharp /// /// Calculates floored log of the specified value, base 2. /// Note that by convention, input value 0 returns 0 since Log(0) is undefined. - /// Bit hacking with deBruijn sequence, extremely fast yet does not use any intrinsics so should work on every platform. + /// Bit hacking with deBruijn sequence, extremely fast yet does not use any intrinsics so will work on every platform/runtime. /// /// /// Description of this bit hacking can be found here: @@ -866,7 +866,6 @@ namespace SixLabors.ImageSharp // No AggressiveInlining due to large method size // Has conventional contract 0->0 (Log(0) is undefined) by default, no need for if checking - // Fill trailing zeros with ones, eg 00010010 becomes 00011111 value |= value >> 01; value |= value >> 02; From ab8f727f97158922cbe74eeacdcbd8ca345a4a75 Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Tue, 15 Jun 2021 06:59:48 +0300 Subject: [PATCH 08/12] Yet another docs fixes --- .../Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs b/src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs index a8382df2bb..d46b8c62c8 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs @@ -409,11 +409,11 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Encoder // BitOperations.Log2 implementation also checks if input value is zero for the convention // As this is a very specific method for a very specific Huffman encoding code // We can omit zero check as this should not be invoked with value == 0 and guarded in debug builds - // This is also marked as internal so every use of this would be tracable & testable in tests return 32 - System.Numerics.BitOperations.LeadingZeroCount(value); #else // On the contrary to BitOperations implementation this does follow the convention and supports value == 0 case // Although it's still won't be called with value == 0 + // As these implementations behave differently for the value == 0 case it's documented as undefined behaviour return Numerics.Log2SoftwareFallback(value) + 1; #endif } From 2a48032ab6298fa4392a825ba92c5e7aa52934fb Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Tue, 15 Jun 2021 07:04:10 +0300 Subject: [PATCH 09/12] Fixed compilation error --- .../Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs b/src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs index d46b8c62c8..174ae232c9 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs @@ -360,7 +360,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Encoder b = value - 1; } - int bt = Numerics.GetHuffmanEncodingLegth((uint)a); + int bt = GetHuffmanEncodingLegth((uint)a); this.EmitHuff(index, (runLength << 4) | bt); if (bt > 0) From 83f0a01d37905fca7c2cfc6f2563bc6b75495b53 Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Tue, 15 Jun 2021 08:35:21 +0300 Subject: [PATCH 10/12] Fixed typo, fixed GetHuffmanEncodingLength invalid fallback code --- .../Components/Encoder/HuffmanScanEncoder.cs | 32 +++++++++++-------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs b/src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs index 174ae232c9..df1b4e4686 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs @@ -360,7 +360,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Encoder b = value - 1; } - int bt = GetHuffmanEncodingLegth((uint)a); + int bt = GetHuffmanEncodingLength((uint)a); this.EmitHuff(index, (runLength << 4) | bt); if (bt > 0) @@ -391,30 +391,36 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Encoder /// /// Calculates how many minimum bits needed to store given value for Huffman jpeg encoding. - /// This method does not follow the standard convention - it does not support input value of zero. /// /// - /// Passing zero as input value would result in an undefined behaviour. - /// This is done for performance reasons as Huffman encoding code checks for zero value, second identical check would degrade the performance in the hot path. + /// This method returns 0 for input value 0. This is done specificaly for huffman encoding /// /// The value. [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static int GetHuffmanEncodingLegth(uint value) + private static int GetHuffmanEncodingLength(uint value) { - DebugGuard.IsFalse(value == 0, nameof(value), "Huffman encoding does not encode zero values"); + DebugGuard.IsTrue(value <= (1 << 16), "Huffman encoder is supposed to encode a value of 16bit size max"); #if SUPPORTS_BITOPERATIONS // This should have been implemented as (BitOperations.Log2(value) + 1) as in non-intrinsic implementation // But internal log2 is implementated like this: (31 - (int)Lzcnt.LeadingZeroCount(value)) - // BitOperations.Log2 implementation also checks if input value is zero for the convention - // As this is a very specific method for a very specific Huffman encoding code - // We can omit zero check as this should not be invoked with value == 0 and guarded in debug builds + // BitOperations.Log2 implementation also checks if input value is zero for the convention 0->0 + // Lzcnt would return 32 for input value of 0 - no need to check that with branching + // Fallback code if Lzcnt is not supported still use if-check + // But most modern CPUs support this instruction so this should not be a problem return 32 - System.Numerics.BitOperations.LeadingZeroCount(value); #else - // On the contrary to BitOperations implementation this does follow the convention and supports value == 0 case - // Although it's still won't be called with value == 0 - // As these implementations behave differently for the value == 0 case it's documented as undefined behaviour - return Numerics.Log2SoftwareFallback(value) + 1; + // Ideally: + // if 0 - return 0 in this case + // else - return log2(value) + 1 + // + // Hack based on input value constaint: + // We know that input values are guaranteed to be maximum 16 bit large for huffman encoding + // We can safely shift input value for one bit -> log2(value << 1) + // Because of the 16 bit value constraint it won't overflow + // With that input value change we no longer need to add 1 before returning + // And this eliminates need to check if input value is zero - it is a standard convention which Log2SoftwareFallback adheres to + return Numerics.Log2SoftwareFallback(value << 1); #endif } } From 5e5e48c5371b47b2b1667680650a00377d344f0d Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Tue, 15 Jun 2021 08:40:17 +0300 Subject: [PATCH 11/12] Style fix --- src/ImageSharp/Common/Helpers/Numerics.cs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/ImageSharp/Common/Helpers/Numerics.cs b/src/ImageSharp/Common/Helpers/Numerics.cs index 9faf1cda08..9d60ac35cd 100644 --- a/src/ImageSharp/Common/Helpers/Numerics.cs +++ b/src/ImageSharp/Common/Helpers/Numerics.cs @@ -876,9 +876,7 @@ namespace SixLabors.ImageSharp // uint.MaxValue >> 27 is always in range [0 - 31] so we use Unsafe.AddByteOffset to avoid bounds check return Unsafe.AddByteOffset( ref MemoryMarshal.GetReference(Log2DeBruijn), - - // uint|long -> IntPtr cast on 32-bit platforms does expensive overflow checks not needed here - (IntPtr)(int)((value * 0x07C4ACDDu) >> 27)); + (IntPtr)(int)((value * 0x07C4ACDDu) >> 27)); // uint|long -> IntPtr cast on 32-bit platforms does expensive overflow checks not needed here } #endif } From a5210b21a5114a6622bfb5f092f3bd0f8419c8be Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Tue, 15 Jun 2021 09:31:05 +0300 Subject: [PATCH 12/12] Jpeg encoder no uses Numerics.Log2 as fallback --- src/ImageSharp/Common/Helpers/Numerics.cs | 2 +- .../Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ImageSharp/Common/Helpers/Numerics.cs b/src/ImageSharp/Common/Helpers/Numerics.cs index 9d60ac35cd..db65b84cca 100644 --- a/src/ImageSharp/Common/Helpers/Numerics.cs +++ b/src/ImageSharp/Common/Helpers/Numerics.cs @@ -861,7 +861,7 @@ namespace SixLabors.ImageSharp /// https://cstheory.stackexchange.com/questions/19524/using-the-de-bruijn-sequence-to-find-the-lceil-log-2-v-rceil-of-an-integer /// /// The value. - internal static int Log2SoftwareFallback(uint value) + private static int Log2SoftwareFallback(uint value) { // No AggressiveInlining due to large method size // Has conventional contract 0->0 (Log(0) is undefined) by default, no need for if checking diff --git a/src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs b/src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs index df1b4e4686..860a9c3236 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs @@ -420,7 +420,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Encoder // Because of the 16 bit value constraint it won't overflow // With that input value change we no longer need to add 1 before returning // And this eliminates need to check if input value is zero - it is a standard convention which Log2SoftwareFallback adheres to - return Numerics.Log2SoftwareFallback(value << 1); + return Numerics.Log2(value << 1); #endif } }