Support High Precision TPixel formats via Color - Boxed #1801
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1801 +/- ##
==========================================
+ Coverage 87.12% 87.31% +0.18%
==========================================
Files 936 936
Lines 47944 48015 +71
Branches 6018 6034 +16
==========================================
+ Hits 41771 41923 +152
+ Misses 5176 5092 -84
- Partials 997 1000 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
src/ImageSharp/Color/Color.cs
Outdated
| [MethodImpl(InliningOptions.ShortMethod)] | ||
| public static Color FromPixel<TPixel>(TPixel pixel) | ||
| where TPixel : unmanaged, IPixel<TPixel> | ||
| => new(pixel); |
There was a problem hiding this comment.
I'm surprised we didn't have this method before. We should avoid boxing in case user provides a typical pixel like Rgba32, which I would expect to be the more common than advanced scenarios like SixLabors/ImageSharp.Drawing#165
I realized that we don't need to add new stuff to PixelOperations<T> unless we see users complaining about the default heuristic, which is very unlikely.
| => new(pixel); | |
| => Unsafe.SizeOf<TPixel> > sizeof(Rgba64) ? new(pixel) : new(pixel.ToRgba64()); |
There was a problem hiding this comment.
Does sizeof work here? Does the compiler see that as a constant?
There was a problem hiding this comment.
Does the compiler see that as a constant?
As far as I'm aware it should, since it's not a generic argument, however we need to mark the method unsafe too which is not in my recommendation.
There was a problem hiding this comment.
Just remembered why I wrote it this way. ToRgba64 doesn't exist on IPixel<TSelf>;
Probably why the method doesn't already exist also.
There was a problem hiding this comment.
ToRgba64doesn't exist onIPixel<TSelf>;
I missed that. We can implement this as following:
[MethodImpl(InliningOptions.ShortMethod)]
public unsafe static Color FromPixel<TPixel>(TPixel pixel)
where TPixel : unmanaged, IPixel<TPixel>
{
// Avoid boxing in case we can convert to Rgba64 safely and efficently
if (typeof(TPixel) == typeof(Rgba64))
{
Rgba64 p = (Rgba64)(object)pixel;
return new(p);
}
else if (typeof(TPixel) == typeof(Rgb48))
{
Rgb48 p = (Rgb48)(object)pixel;
return new(new Rgba64(p.R, p.G, p.B, ushort.MaxValue)); // or add a new Color(Rgb48) constructor
}
else if (typeof(TPixel) == typeof(L16))
{
Rgb48 p = (L16)(object)pixel;
return new(new Rgba64(p.PackedValue, p.PackedValue, p.PackedValue, ushort.MaxValue)); // or add a new Color(L16) constructor
}
else if (Unsafe.SizeOf(TPixel) <= sizeof(Rgba32))
{
return new(pixel.ToRgba32());
}
else
{
return new(pixel);
}
}The JIT will optimize away the unnecessary branches and the boxing from the (Rgba64)(object)pixel conversion.
Update: Also special-case L16
| for (int i = 0; i < source.Length; i++) | ||
| { | ||
| destination[i] = source[i].ToPixel<TPixel>(); | ||
| } |
There was a problem hiding this comment.
Just a note that it's possible to detect the common boxedHighPrecisionPixel == null case with SIMD and bulk convert, if we really want to.
There was a problem hiding this comment.
Wasn’t aware of that. SIMD is still very much a Google event for me
There was a problem hiding this comment.
object is basically a pointer so it's unsafe convertible to long on 64 bits. We can check 4 Colors with bitwise masks, if all of them are NULL we can shuffle stuff so the data fields are together, then bulk convert them.
Super unsure if it's worth the trouble.
There was a problem hiding this comment.
Super unsure if it's worth the trouble.
Yeah, not attempting that just now. Not worth the trouble.
|
Thanks to everyone who helped with this! 🎉 |
Prerequisites
Description
An alternative to #1797 that uses a boxed
IPixelrepresentation to hold any Color created via newColor.FromPixel<TPixel>(TPixel pixel)method or viaColor(Vector4). This allows supporting all the pixel formats in the library without loss of precision.See SixLabors/ImageSharp.Drawing#165
CC. @tocsoft @antonfirsov