-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add ImageSharp project #7
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The generic method seems incorrect.
/// Converts the given bitmap to the library-independent representation used within the Blurhash-core | ||
/// </summary> | ||
/// <param name="sourceBitmap">The bitmap to encode</param> | ||
internal static Pixel[,] ConvertBitmap<T>(Image<T> sourceBitmap) where T : struct, IPixel<T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering the implementation, you should consume Image<Rgb24>
or use proper conversion with PixelOperations<T>.ToRgb24(...)
.
Blurhash.ImageSharp/Decoder.cs
Outdated
var height = pixelData.GetLength(1); | ||
|
||
var data = Enumerable.Range(0, height) | ||
.SelectMany(y => Enumerable.Range(0, width).Select(x => (x, y))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not a for loop? This linq expression is a perf killer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was copied from another implementation, I'll expand it, as I think perf is key if it's to be used in a Xamarin app for example, which is my primary use-case.
Co-Authored-By: Anton Firszov <[email protected]>
Has this been published to NuGet yet? Will this be merged at some point? :) |
I need to have a hack at this. I was given permission, just haven’t gotten to it yet. |
Sorry, I seeem to have notifications setup completely wrong - I didn't get notified that there was activity on the repo, so I didn't react to anything. |
I'm currently setting up GitHub actions, so all NuGet packages of the repo will be published in a feed |
Fixes #6
Needs tests!