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

Rename Pixel Shader Extensions and Delegates #1096

Merged
merged 2 commits into from
Jan 30, 2020

Conversation

JimBobSquarePants
Copy link
Member

@JimBobSquarePants JimBobSquarePants commented Jan 28, 2020

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 matches 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

The PixelShader extensions, processors and delegates were too suggestive of GPU based functionality so the decision was made to rename them.

All functionality is the same only classes and delegates were renamed.

cc. @Sergio0694

@JimBobSquarePants JimBobSquarePants added this to the 1.0.0-rc1 milestone Jan 28, 2020
@JimBobSquarePants JimBobSquarePants requested a review from a team January 28, 2020 12:42
@Sergio0694
Copy link
Member

Looks great!
And the new ProcessPixelRowsAsVector4 name is really growing on me, I like the simplicity and intuitiveness of it a lot! Hopefully this will also help drive its adoption even more, which will be a huge win for everyone here, and especially for end users struggling to figure out how to properly process abstract image instances (@JimBobSquarePants you know who I'm talking about 😆).

@codecov
Copy link

codecov bot commented Jan 28, 2020

Codecov Report

Merging #1096 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1096      +/-   ##
==========================================
+ Coverage   81.49%   81.53%   +0.03%     
==========================================
  Files         704      704              
  Lines       29389    29380       -9     
  Branches     3290     3290              
==========================================
+ Hits        23951    23954       +3     
+ Misses       4745     4733      -12     
  Partials      693      693
Flag Coverage Δ
#unittests 81.53% <100%> (+0.03%) ⬆️
Impacted Files Coverage Δ
...ng/Processors/Effects/PixelRowDelegateProcessor.cs 100% <100%> (ø)
...g/Extensions/Effects/PixelRowDelegateExtensions.cs 100% <100%> (ø)
...ssors/Effects/PixelRowDelegateProcessor{TPixel}.cs 100% <100%> (ø)
.../Effects/PositionAwarePixelRowDelegateProcessor.cs 100% <100%> (ø)
...s/Effects/PixelRowDelegateProcessorBase{TPixel}.cs 100% <100%> (ø)
.../PositionAwarePixelRowDelegateProcessor{TPixel}.cs 100% <100%> (ø)

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 71547c4...520cc5e. Read the comment docs.

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

Just a few naming-related concerns for future proofness.

In the close future, after making R ↔ G ↔ B ↔ A shuffling cheap, we can extend the API like this:

public static IImageProcessingContext ProcessPixelRowsAs<TPixel>(this IImageProcessingContext source, PixelRowOperation<TPixel> rowOperation) where TPixel : struct, IPixel<TPixel>;

It can be used like following:

image.ProcessPixelRowsAs<Rgba32>(span => { .... });

This will put the new naming into context, it's fine that is more verbose for the Vector4 case.

@Sergio0694
Copy link
Member

@antonfirsov I'm a bit confused, why do we want to add overloads that work with a specific pixel type, given that those will prevent the JITter from just compiling the Vector4 operations into SIMD instructions? Is that just to make it easier to work with for users, or am I missing something? 🤔

I mean, my fear is that a general developer presented with both the Vector4 and TPixel variants would just go for a TPixel one out of habit and write the delegate working with, say, Rgba32, and essentially doing the same exact stuff he could've done with Vector4, but losing SIMD altogether and ending up with slower code for no real reason, which would not be ideal.

@antonfirsov
Copy link
Member

@Sergio0694 it's 4x more compact in memory, conversion is much faster, if done right (NOP for Rgba32) . Advanced users can do System.Runtime.Intrinsics for SIMD, but most folks just want to play around with the good-old RGBA byte values in the easiest possible way. Believe or not, Vector4 normalized to 0..1 is an abstraction many devs just don't get 😒

@Sergio0694
Copy link
Member

@antonfirsov Ah, yeah I hadn't considered that. I personally don't get why the 0..1 range would be that confusing (it's literally just 0.255 scaled down, but that's about it), but yeah you make a very good point there. I hope some devs will take the time to look into the Vector4 APIs though instead of just settling for the pixel typed versions, because the next step which you mentioned, intrinsics, is way tougher to get to. As in, getting from TPixel to Vector4 is doable, getting to using intrinsics takes years. And getting to comfortably using intrinsics takes a lifetime 🤣

Also I might be wrong here, but I was wondering:

  1. I agree that being able to skip the pixel conversions entirely can give a nice performance bump, but isn't the (vectorized?) ToVector4 pass pretty optimized to be a reasonably small overhead anyway?
  2. In non-server scenarios where one's not worried about potential memory issues caused by too many parallel executions, isn't the allocation cost for the Vector4 buffers negligible, since they're all just just unmanaged types being pooled from the same byte buffers anyway?
  3. Doesn't working with Vector4 also have the advantage of greater "accuracy" when applying arbitrary effects? As in, since the 0..1 range with floating points is much wider than just 0..255, doesn't that essentially makes processors work in a larger color space? Which is eventually reduced back, sure, but you can retain more info during the processing steps, similar to how eg. games process intermediate buffers in HDR even if they then only display them in standard SDR 8bit 🤔

Thanks in advance, I'm just trying to learn as much as I can from you guys 😄

@JimBobSquarePants
Copy link
Member Author

@SixLabors/core Any blockers here or can I merge?

@antonfirsov
Copy link
Member

@JimBobSquarePants give me 10 minutes.

@JimBobSquarePants JimBobSquarePants merged commit 3b1266c into master Jan 30, 2020
@JimBobSquarePants JimBobSquarePants deleted the js/rename-pixel-shaders branch January 30, 2020 01:18
@antonfirsov
Copy link
Member

antonfirsov commented Jan 30, 2020

@Sergio0694 this is the .NET Core 2.1+ Rgba32 <=> Vector4 conversion code:
https://github.com/SixLabors/ImageSharp/blob/master/src/ImageSharp/Common/Helpers/SimdUtils.ExtendedIntrinsics.cs#L104

It's the fastest thing we can do, but still expensive.

In comparison, a Bgra32 -> Rgba32 conversion could be done using a few shuffle intrinsics, and you can always fit 8 (!!!) pixels into an AVX2 register instead of only 2 (2xVector4).

Of course, in case of more complex processing operations, it would be compensated by using Vector4 in the operation body, but if a user only wants to do something basic (like setting the alpha channel to some value, or recolor pixels over a specific luminosity), the RGB(A) variants will be faster.

@Sergio0694
Copy link
Member

@antonfirsov Ah, yeah I see now, makes perfect sense. Thanks! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants