Modernize and optimize pixel format operations across platforms. by JimBobSquarePants · Pull Request #2645 · SixLabors/ImageSharp

JimBobSquarePants · 2024-01-15T04:39:22Z

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

Updates API to use static interface methods for improved usability and extensibility.
Optimizes and modernizes SIMD pixel shuffle operations, adding ARM support.
~~Optimizes and modernizes SIMD Vector4 packing operations.~~ I'm leaving this for a follow up PR

I don't expect anyone to reasonably find the time to analyze this line-by-line as it's a massive piece of work. However, changes on the whole are simplistic and the developer benefit massive.

API change Elevator Pitch

The abomination that is.

TPixel pixel = default;
pixel.FromScaledVector(vector)

Becomes, as nature intended.

TPixel pixel = TPixel.FromScaledVector(vector);

All other conversion methods follow suit where applicable.

JimBobSquarePants · 2024-01-15T04:57:14Z

+    /// <param name="numBytes">The number of bytes to shift by.</param>
+    /// <returns>The <see cref="Vector128{Byte}"/>.</returns>
+    [MethodImpl(MethodImplOptions.AggressiveInlining)]
+    public static Vector128<byte> ShiftRightBytesInVector(Vector128<byte> value, [ConstantExpected(Max = (byte)15)] byte numBytes)


There's something not right in one of the methods below on Arm64.

@tannergooding @saucecontrol Would either of you be able to do a quick readthrough and set me right?

Figured it out. It was left shift. I'd forgotten to offset.

tannergooding · 2024-01-16T14:28:51Z

+    /// Calculates <paramref name="x"/> % 4
+    /// </summary>
+    [MethodImpl(MethodImplOptions.AggressiveInlining)]
+    public static nuint Modulo4(nuint x) => x & 3;


Just noting that x % 4 should already get optimized to x & 3 by the JIT since x is nuint (and therefore definitely unsigned)

Thanks yeah, I just kept it the same for consistency.

tannergooding · 2024-01-16T14:36:46Z

+                    ref Vector512<float> vs0 = ref Unsafe.Add(ref sourceBase, i);
+                    ref Vector512<float> vd0 = ref Unsafe.Add(ref destinationBase, i);

-                    vd0 = Avx.Permute(vs0, control);
-                    Unsafe.Add(ref vd0, 1) = Avx.Permute(Unsafe.Add(ref vs0, 1), control);
-                    Unsafe.Add(ref vd0, 2) = Avx.Permute(Unsafe.Add(ref vs0, 2), control);
-                    Unsafe.Add(ref vd0, 3) = Avx.Permute(Unsafe.Add(ref vs0, 3), control);
+                    vd0 = Vector512Utilities.Shuffle(vs0, control);
+                    Unsafe.Add(ref vd0, (nuint)1) = Vector512Utilities.Shuffle(Unsafe.Add(ref vs0, (nuint)1), control);
+                    Unsafe.Add(ref vd0, (nuint)2) = Vector512Utilities.Shuffle(Unsafe.Add(ref vs0, (nuint)2), control);
+                    Unsafe.Add(ref vd0, (nuint)3) = Vector512Utilities.Shuffle(Unsafe.Add(ref vs0, (nuint)3), control);


Worth noting that you could do this like the following:

Vector512Utilities.Shuffle(Vector512.LoadUnsafe(ref sourceBase, i + 0), control).StoreUnsafe(ref destinationBase, i + 0). Vector512Utilities.Shuffle(Vector512.LoadUnsafe(ref sourceBase, i + 1), control).StoreUnsafe(ref destinationBase, i + 1). Vector512Utilities.Shuffle(Vector512.LoadUnsafe(ref sourceBase, i + 2), control).StoreUnsafe(ref destinationBase, i + 2). Vector512Utilities.Shuffle(Vector512.LoadUnsafe(ref sourceBase, i + 3), control).StoreUnsafe(ref destinationBase, i + 3).

It shouldn't result in any codegen differences, but avoids needing to manipulate the byref and uses direct hardware intrinsic APIs, rather than the Unsafe helpers, so can be easier to read/understand at least IMO.

sourceBase is Vector512<float> so Vector512.LoadUnsafe(ref sourceBase, i + 0) would yield Vector512<Vector512<float>>. I could use float as the base ref type but that complicates the offsetting which confuses me and causes me to make mistakes.

tannergooding

LGTM, just left a couple comments on newer APIs that might help with readability

stefannikolei · 2024-01-20T13:21:08Z

-    /// </summary>
-    /// <returns>The <see cref="PixelTypeInfo"/>.</returns>
-#pragma warning disable CA1000
-    static abstract PixelTypeInfo GetPixelTypeInfo();


Just curious. Is there a specific reason to kick it from the interface and only have it in the implementations? Probably it was never called from the interface and only from specific implementations?

Actually, this is just temporary. I had to hack to get the solution to build because we were misusing ApproximateFloatComparer in the tests.

tannergooding · 2024-01-20T16:14:40Z

  <Include Path="..\shared-infrastructure\sixlabors.ruleset" Action="Default" />
-  <Rules AnalyzerId="StyleCop.Analyzers" RuleNamespace="StyleCop.Analyzers">
+  <Rules AnalyzerId="Microsoft.CodeAnalysis.CSharp.Features" RuleNamespace="Microsoft.CodeAnalysis.CSharp.Features">
+    <Rule Id="IDE0290" Action="None" />


This should also be suppressible via .editorconfig using csharp_style_prefer_primary_constructors = false:none (or a different severity level if you want to block use of primary constructors altogether, such as silent, suggestion, warning, or error)

Thanks. Yeah, I’ll do that upstream. Not sold on primary constructors. The syntax doesn’t sit right with me.

Updated [SixLabors.ImageSharp](https://github.com/SixLabors/ImageSharp) from 3.1.12 to 4.1.1. <details> <summary>Release notes</summary> _Sourced from [SixLabors.ImageSharp's releases](https://github.com/SixLabors/ImageSharp/releases)._ ## 4.1.1 ## What's Changed * Fix CCITT decompressor bounds checks and error handling by @JimBobSquarePants in SixLabors/ImageSharp#3176 **Full Changelog**: SixLabors/ImageSharp@v4.1.0...v4.1.1 ## 4.1.0 ## What's Changed * GIF: background handling & quantizer overflow fix by @JimBobSquarePants in SixLabors/ImageSharp#3133 * Validate PBM max pixel value by @JimBobSquarePants in SixLabors/ImageSharp#3134 * Add support for Apple CgBI PNG images by @Erik-White in SixLabors/ImageSharp#3136 * Fix GIF transparency handling and dither by @JimBobSquarePants in SixLabors/ImageSharp#3143 * Bump codecov/codecov-action from 6 to 7 by @dependabot[bot] in SixLabors/ImageSharp#3141 * Add BlendWithCoverage overloads. Optimize Rgba32 compatible shuffling. by @JimBobSquarePants in SixLabors/ImageSharp#3150 * Bump actions/checkout from 6 to 7 by @dependabot[bot] in SixLabors/ImageSharp#3146 * Bump actions/cache from 5 to 6 by @dependabot[bot] in SixLabors/ImageSharp#3149 * Fix convolution sampling for bounds smaller than the kernel radius by @JimBobSquarePants in SixLabors/ImageSharp#3152 * Enhance pixel formats and associated-alpha operations with optimizations by @JimBobSquarePants in SixLabors/ImageSharp#3154 * Bump actions/setup-dotnet from 5 to 6 by @dependabot[bot] in SixLabors/ImageSharp#3155 * Fix degeneracy check for resize transforms by @JimBobSquarePants in SixLabors/ImageSharp#3157 * Improvements to Apple CgBI PNG handling by @Erik-White in SixLabors/ImageSharp#3137 * Clone TIFF profiles into image metadata by @JimBobSquarePants in SixLabors/ImageSharp#3159 * Add AOT compiler seeds for pixel formats and ICO, CUR, QOI codecs by @JimBobSquarePants in SixLabors/ImageSharp#3160 * Normalize SIMD pipelines using TensorPrimitives_ by @JimBobSquarePants in SixLabors/ImageSharp#3161 * Add ANI decoder support by @Poker-sang in SixLabors/ImageSharp#2899 * Remove AntialiasThreshold from GraphicsOptions by @JimBobSquarePants in SixLabors/ImageSharp#3162 * Add plus and extended pixel blend modes by @JimBobSquarePants in SixLabors/ImageSharp#3163 * Fix 16-bit binary PBM sample byte order by @JimBobSquarePants in SixLabors/ImageSharp#3164 * Make MemoryAllocator limits configurable and extensible by @JimBobSquarePants in SixLabors/ImageSharp#3165 ## New Contributors * @Erik-White made their first contribution in SixLabors/ImageSharp#3136 **Full Changelog**: SixLabors/ImageSharp@v4.0.0...v4.1.0 ## 4.0.0 ## What's Changed * Update to net8 by @stefannikolei in SixLabors/ImageSharp#2583 * Handle dedup of local palette of 256 length - Main by @JimBobSquarePants in SixLabors/ImageSharp#2607 * Replace custom Crc32 by @JimBobSquarePants in SixLabors/ImageSharp#2611 * Sync 3.1 DrawImage fixes by @tocsoft in SixLabors/ImageSharp#2612 * Fix handling gif encoding for global palettes - Main by @JimBobSquarePants in SixLabors/ImageSharp#2615 * Bump actions/setup-dotnet from 3 to 4 by @dependabot[bot] in SixLabors/ImageSharp#2613 * Adjusted the casing of the Webp format name by @jscarle in SixLabors/ImageSharp#2623 * Fix Paeth Filter decode on platforms that do not support Ssse3 - Main by @JimBobSquarePants in SixLabors/ImageSharp#2620 * Fix WebP animation speed bug by @marklagendijk in SixLabors/ImageSharp#2624 * Promote PixelTypeInfo to Pixel by @stefannikolei in SixLabors/ImageSharp#2601 * TGA: Treat 32 bit True Color images always as transparent by @brianpopow in SixLabors/ImageSharp#2643 * Modernize and optimize pixel format operations across platforms. by @JimBobSquarePants in SixLabors/ImageSharp#2645 * Cleanup SimdUtils by @JimBobSquarePants in SixLabors/ImageSharp#2654 * Bump actions/cache from 3 to 4 by @dependabot[bot] in SixLabors/ImageSharp#2648 * Bump codecov/codecov-action from 3 to 4 by @dependabot[bot] in SixLabors/ImageSharp#2657 * Bump NuGet/setup-nuget from 1 to 2 by @dependabot[bot] in SixLabors/ImageSharp#2658 * Add v3.1.x fixes #2673 and #2674 into main. by @JimBobSquarePants in SixLabors/ImageSharp#2675 * Add fixes 2668, 2676, and 2677 to main by @JimBobSquarePants in SixLabors/ImageSharp#2678 * Merge 2681 to v4 Main by @JimBobSquarePants in SixLabors/ImageSharp#2690 * Add JPEG COM marker support by @RobertMut in SixLabors/ImageSharp#2641 * Bump actions/upload-artifact from 3 to 4 by @dependabot[bot] in SixLabors/ImageSharp#2625 * Only exit JPEG scan decoding after multiple EOF hits by @JimBobSquarePants in SixLabors/ImageSharp#2701 * V4 Ensure VP8X alpha flag is updated correctly. by @JimBobSquarePants in SixLabors/ImageSharp#2703 * Fix animated png handling (issue #2708) by @SpaceCheetah in SixLabors/ImageSharp#2710 * Merge latest release from v3 by @JimBobSquarePants in SixLabors/ImageSharp#2720 * Fix MacOS jobs by @antonfirsov in SixLabors/ImageSharp#2728 * Fix async-over-sync issue in Image.DecodeAsync() by @kroymann in SixLabors/ImageSharp#2725 * Fix overflow in MemoryAllocator.Create(options) by @antonfirsov in SixLabors/ImageSharp#2730 * GifDecoder: Limit lzw bits to a maximum of 12 bits by @brianpopow in SixLabors/ImageSharp#2744 * GifDecoder : Allow skipping bad metadata using identify by @JimBobSquarePants in SixLabors/ImageSharp#2749 * Add ICO and CUR file decoder. by @frg2089 in SixLabors/ImageSharp#2579 * v4 - Fix off-by-one error when centering a transform. by @JimBobSquarePants in SixLabors/ImageSharp#2761 * v4 Fix 2758 by @JimBobSquarePants in SixLabors/ImageSharp#2764 * Simplify Color Space Conversion APIs by @JimBobSquarePants in SixLabors/ImageSharp#2739 * Webp: Fix Issue 2763 by @brianpopow in SixLabors/ImageSharp#2767 * V4 Correctly break during Png decoding by @JimBobSquarePants in SixLabors/ImageSharp#2773 * V4 : Fix filtering on PNG encode. by @JimBobSquarePants in SixLabors/ImageSharp#2778 * Fix #2779 buffer overrun by @KirillAldashkin in SixLabors/ImageSharp#2780 * Fix ImageMetadata docs typo by @lofcz in SixLabors/ImageSharp#2781 * Add API for metadata conversion between formats. by @JimBobSquarePants in SixLabors/ImageSharp#2751 * Tiff decoder: Fix issue 2679 by @brianpopow in SixLabors/ImageSharp#2789 * Replace PngCrcChunkHandling by @JimBobSquarePants in SixLabors/ImageSharp#2786 * Add tagname to debugger visualization for Exif- and Iptc-values, to facilitate easier debugging and discovery by @lassevk in SixLabors/ImageSharp#2787 * V4 - Correctly handle transform spaces when building transform matrices. by @JimBobSquarePants in SixLabors/ImageSharp#2795 * Allow decoding Tiff of different frame size. by @JimBobSquarePants in SixLabors/ImageSharp#2788 * Add progressive JPEG encoder by @ardabada in SixLabors/ImageSharp#2740 * Fix using dither in BmpEncoder when bit per pixel is <= 4 by @mistoll in SixLabors/ImageSharp#2819 * Add QuadDistortion to ProjectiveTransformBuilder by @Socolin in SixLabors/ImageSharp#2748 * WEBP : Use Correct Width With AlphaDecoder by @JimBobSquarePants in SixLabors/ImageSharp#2823 ... (truncated) Commits viewable in [compare view](SixLabors/ImageSharp@v3.1.12...v4.1.1). </details> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=SixLabors.ImageSharp&package-manager=nuget&previous-version=3.1.12&new-version=4.1.1)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: iht <IhateTrains@users.noreply.github.com>

JimBobSquarePants added 3 commits January 13, 2024 10:13

Port shuffle4

3969525

Port first Shuffl3 method

4931372

Introduce new utilities to replace poor performing runtime methods

b250ac3

JimBobSquarePants added API area:pixelformats breaking Signifies a binary breaking change. arch:arm64 labels Jan 15, 2024

JimBobSquarePants added this to the v4.0.0 milestone Jan 15, 2024

Fix missed normalization

4c47a78

JimBobSquarePants commented Jan 15, 2024

View reviewed changes

saucecontrol reviewed Jan 15, 2024

View reviewed changes

Comment thread src/ImageSharp/Common/Helpers/Vector128Utilities.cs Outdated

JimBobSquarePants added 4 commits January 15, 2024 15:24

Attempt to fix left shift

d9126ea

Push the right fix!

5cf8f60

Use proper constant

c9b4edd

Update individual pixel implementations

acaebd9

tannergooding reviewed Jan 16, 2024

View reviewed changes

tannergooding approved these changes Jan 16, 2024

View reviewed changes

Update library to use new pixel API

dfd983f

stefannikolei reviewed Jan 20, 2024

View reviewed changes

tannergooding reviewed Jan 20, 2024

View reviewed changes

Restore GetPixelTypeInfo

8af75fe

JimBobSquarePants mentioned this pull request Jan 21, 2024

Investigate skipped tests in PackedPixelTests #594

Closed

4 tasks

JimBobSquarePants added 2 commits January 21, 2024 22:04

Update refs

2de4f2c

Merge branch 'main' into js/pixelsformats

cee00af

JimBobSquarePants marked this pull request as ready for review January 21, 2024 12:17

JimBobSquarePants changed the title ~~WIP : Modernize and optimize pixel format operations across platforms.~~ Modernize and optimize pixel format operations across platforms. Jan 21, 2024

JimBobSquarePants requested review from stefannikolei and tocsoft and removed request for stefannikolei January 21, 2024 12:17

Navigation Menu

Uh oh!

Uh oh!

Modernize and optimize pixel format operations across platforms. - #2645

Modernize and optimize pixel format operations across platforms.#2645
JimBobSquarePants merged 14 commits into
mainfrom
js/pixelsformats

JimBobSquarePants commented Jan 15, 2024 •

edited

Loading

Uh oh!

JimBobSquarePants Jan 15, 2024

Uh oh!

JimBobSquarePants Jan 15, 2024

Uh oh!

Uh oh!

tannergooding Jan 16, 2024 •

edited

Loading

Uh oh!

JimBobSquarePants Jan 21, 2024

Uh oh!

tannergooding Jan 16, 2024 •

edited

Loading

Uh oh!

JimBobSquarePants Jan 21, 2024

Uh oh!

tannergooding left a comment

Uh oh!

stefannikolei Jan 20, 2024

Uh oh!

JimBobSquarePants Jan 21, 2024

Uh oh!

tannergooding Jan 20, 2024

Uh oh!

JimBobSquarePants Jan 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Uh oh!

Conversation

JimBobSquarePants commented Jan 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Prerequisites

Description

API change Elevator Pitch

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tannergooding Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannergooding Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannergooding left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JimBobSquarePants commented Jan 15, 2024 •

edited

Loading

tannergooding Jan 16, 2024 •

edited

Loading

tannergooding Jan 16, 2024 •

edited

Loading