Add FromArgb/FromBgra methods to SixLabors.ImageSharp.Color

[woutware]

In my CAD library internally all colors are represented by argb uint values (Bgra32 in SixLabors.ImageSharp).

Currently I have to cast to a pixel first to convert:

uint argb = ...;
Color color = Color.FromPixel(Unsafe.As<uint, SixLabors.ImageSharp.PixelFormats.Bgra32>(ref argb));

It works, but it would be nice if the Color struct allowed direct conversion from/to argb/bgra uint values as these are so ubiquitous.

I need the Color struct to create brushes/pens, and this is in the hot path of my CAD rendering.

[MLuc24]

You can drop the Unsafe.As today — Bgra32 already exposes the packed value as a settable property, so there is a safe path that costs nothing extra:

uint argb = ...;
Color color = Color.FromPixel(new Bgra32 { PackedValue = argb });

Bgra32 is a struct, so that object initialiser is not a heap allocation, and the type declares both

public uint Bgra { get; set; }
public uint PackedValue { get; set; }

so you can use whichever reads better. This is the library's own idiom, incidentally — its pixel conversion does the same thing:

public static Bgra32 FromBgra32(Bgra32 source) => new() { PackedValue = source.PackedValue };

That gets you off a reinterpret-cast that depends on layout assumptions, onto something the type explicitly supports, without changing the shape of your hot path.

On the request itself, you are right that Color has nothing for it: the public factories are FromPixel<TPixel>, the two FromScaledVector overloads, and the hex parsers. Nothing takes a packed integer.

And there is a decent argument in the hex parsers for adding one, because they reach for exactly the trick you did:

result = Unsafe.As<uint, Rgba32>(ref packedValue);

with the same line for Argb32 a few lines further down. So a public FromArgb/FromBgra would not be introducing a new concept to Color — it would be surfacing a conversion the type already performs internally, and saving callers from either the unsafe cast or the two-step through a pixel struct.

[woutware]

Yes, the current way to convert an Argb/Bgra uint to Color is a bit kludgy. Argb/Bgra is just so common, I wouldn't even think more than 0.1 seconds about it and add conversion methods for those. It also saves people for having to search for how to do this, so discoverability wise it's also more convenient. And conceptually it doesn't feel right you have to take an intermediate step through a TPixel to get from an Argb/Bgra uint to a Color.

[JimBobSquarePants]

I don't think Color.FromArgb(uint) or Color.FromBgra(uint) should be added. The proposal is solving the wrong problem and, more importantly, puts packed pixel representation semantics on a type that deliberately does not have them.

Color is intentionally pixel-format agnostic. Its contract is a color value that can be converted to and from any IPixel<TPixel>. The internal representation is deliberately hidden and is not ARGB, BGRA, RGBA, or any other particular packed layout.

A uint, on the other hand, is just 32 bits. It has no inherent channel order.

That distinction is important here because "ARGB uint" is already ambiguous.

For example, the conventional integer notation:

0xAARRGGBB

describes the components by numeric significance.

But ImageSharp's Bgra32 stores its components from least significant to most significant byte as:

B G R A

so its packed uint is numerically:

0xAARRGGBB

That is why the value described in this discussion as an "ARGB uint" is actually represented by Bgra32 in ImageSharp.

ImageSharp also has an actual Argb32 pixel format. Its components are stored from least significant to most significant byte as:

A R G B

so its packed uint has a completely different numeric representation:

0xBBGGRRAA

That immediately makes this API problematic:

Color.FromArgb(0xFF112233)

What exactly does Argb mean there?

Does it mean the common textual/numeric convention AARRGGBB, giving:

A = FF
R = 11
G = 22
B = 33

or does it mean "the packed value of ImageSharp's Argb32 pixel type"?

Those are different interpretations of the same uint and produce different colors.

The proposed FromArgb(uint) API therefore takes representation information that should be explicit in the type system and hides it behind an ambiguous method name.

FromBgra(uint) has the same conceptual problem. Is "BGRA" describing byte layout, component ordering, or the numeric significance of the packed integer? Those concepts are routinely conflated, as this discussion itself demonstrates.

This is precisely what the pixel types exist to solve.

When you write:

Color.FromPixel(new Bgra32(value))

Bgra32 is not some arbitrary intermediate object that we're forcing the caller through. It is the thing that gives those 32 bits meaning.

The uint says:

here are 32 bits

The Bgra32 says:

interpret those 32 bits using this specific packed pixel representation

and Color.FromPixel says:

convert that pixel representation into a format-independent Color

That separation is intentional and is exactly the abstraction we want.

It also scales correctly. ImageSharp supports many pixel formats. If packed representation conversion is placed on Color, where does that stop?

Do we add:

Color.FromRgba(...)
Color.FromBgra(...)
Color.FromArgb(...)
Color.FromAbgr(...)

and then equivalents for every other packed representation that somebody considers common?

We already have the generic API for exactly this operation:

Color.FromPixel<TPixel>(TPixel source)

The pixel type identifies the representation. Color does not need to know or care which packed format produced it.

There is also no GC argument for avoiding the pixel here.

Bgra32 is a four-byte unmanaged value type. Constructing one does not allocate an object on the managed heap.

Color.FromPixel<Bgra32> also takes the normal unboxed conversion path. Bgra32 has 8-bit components, so FromPixel converts it directly to the scaled Vector4 representation used by Color. It does not box the Bgra32.

So introducing the pixel value here isn't creating the kind of allocation that would make this problematic in a hot rendering loop.

The current example:

uint argb = ...;
Color color = Color.FromPixel(Unsafe.As<uint, Bgra32>(ref argb));

does expose a real API inconsistency, but it isn't on Color.

The Unsafe.As shouldn't be necessary.

Bgra32 already exposes its packed representation:

public uint Bgra { get; set; }

public uint PackedValue { get; set; }

so this is already safe and allocation-free:

Color color = Color.FromPixel(new Bgra32 { PackedValue = argb });

However, looking at the pixel types, Bgra32 is missing a constructor that the equivalent 32-bit packed formats already provide.

For example, Rgba32 has:

public Rgba32(uint packed)
    : this() => this.Rgba = packed;

and Argb32 has:

public Argb32(uint packed)
    : this() => this.Argb = packed;

Bgra32 exposes the same packed-value concept but doesn't have:

public Bgra32(uint packed)
    : this() => this.Bgra = packed;

That's the actual API gap here.

Adding that constructor makes the original operation:

uint argb = ...;
Color color = Color.FromPixel(new Bgra32(argb));

and converting back is equally explicit:

uint argb = color.ToPixel<Bgra32>().PackedValue;

That gives the packed representation semantics to the type whose job it is to define those semantics, while keeping Color independent of any particular pixel layout.

I also disagree with the argument above that the hex parser provides precedent for Color.FromArgb(uint).

It actually demonstrates the opposite.

ParseHex starts with a textual representation and an explicit ColorHexFormat. If the format is ARGB, the parser reads the characters as:

A A R R G G B B

then rearranges those parsed components into the actual packed layout required by Argb32, and finally does:

Color.FromPixel(argb)

In other words, the numeric value created internally by the ARGB hex parser is not simply the numeric interpretation of the input string.

For input:

FF112233

the parser reads A=FF, R=11, G=22, B=33, then packs those bytes in Argb32 layout as:

0x332211FF

before reinterpreting that value as Argb32.

The Unsafe.As there is simply an internal implementation detail used after the representation has already been explicitly established. It is not evidence that Color considers a naked uint to have intrinsic ARGB semantics.

In fact, the parser having to rearrange the components makes the ambiguity of a public Color.FromArgb(uint) especially clear.

Likewise, I don't agree that going "through a TPixel" is conceptually wrong. It is conceptually necessary because TPixel carries information that the uint does not.

Color represents a color.

Bgra32 represents that color encoded using a specific 32-bit packed pixel layout.

A uint represents neither unless somebody specifies how its bits should be interpreted.

So I don't think FromArgb(uint) or FromBgra(uint) belong on Color.

The useful issue this discussion has identified is simply that Bgra32 is missing the packed uint constructor already present on the equivalent 32-bit pixel formats. I'll add that for consistency.

[woutware]

What I'm trying to avoid is any unnecessary overhead in interfacing with ImageSharp, and even though TPixel is a struct, there is still a considerable amount of code being executed in the conversions that could be cut short, e.g. most of the Color.FromPixel implementation. And looking at the source code of e.g.

public static Vector4 operator /(Vector4 value1, float value2)
{
    return value1 / new Vector4(value2);
}

As division is usually more expensive than multiplication, it often pays off to implement as like a multiplication (e.g. in Bgra32.ToVector4()):

const float ByteClamper = 1f / 255f;

public readonly Vector4 ToVector4() => new Vector4(this.R * ByteClamper , this.G * ByteClamper , this.B * ByteClamper , this.A * ByteClamper);

This replaces 4 divisions by 4 multiplications and saves 1 Vector4 constructor call (used for new Vector4(255, 255, 255, 255)).

Looking at the Color.FromPixel implementation, this is the overhead that can be cut away:

• Color.FromPixel calls TPixel.GetPixelTypeInfo(), which creates a new PixelTypeInfo every time (see e.g. implementation of Bgra32.GetPixelTypeInfo()). Instead create a private static readonly PixelTypeInfo pixelTypeInfo = ...; field and create the PixelTypeInfo instance just once and return that.
• Color.FromPixel goes through 3 if statements before getting to where it needs to be: new Color(source.ToScaledVector4(), info.AlphaRepresentation, info.AlphaRepresentation). A dedicated FromArgb method would circumvent all this.
• Then in the Color constructor it goes through this.data = Numerics.Clamp(vector, Vector4.Zero, Vector4.One);. The data is already clamped, so this Clamp call is unnecessary. The Clamp call is another 2 Vector4 constructions, and the Min/Max calls are another 8 if statements. I'd get rid of that Clamp call in the Color constructor, and move it to a static factory method Color.FromUnclamped(...) that is only called when it is not sure if the values are clamped.

So I would like to cut out all this overhead, and construct the Color in the most direct way possible. I'm suggesting an implementation something like this:

const float ByteClamper = 1f / 255f;

private Color(Vector4 clampedData, bool isAssociated, bool dataIsAssociated)
{
    this.data = clampedData;
    this.boxedHighPrecisionPixel = null;
    this.isAssociated = isAssociated;
    this.dataIsAssociated = dataIsAssociated;
}

public static Color FromBgra32(Bgra32 value) {
    return new Color(new Vector4(ByteClamper * value.R, ByteClamper * value.G, ByteClamper * value.B, ByteClamper * value.A), false, false);
}

public static Color FromBgra32(Argb32 value) {
    return new Color(new Vector4(ByteClamper * value.R, ByteClamper * value.G, ByteClamper * value.B, ByteClamper * value.A), false, false);
}

These specific methods could perhaps go into the pixel implementations instead, e.g. Color Bgra32.ToColor(). Maybe this would be a more suitable location, as the TPixel types know more about themselves, while the Color type has to resort to the if decision tree. In this case consider making the Color constructor internal rather than private, so the TPixel types can access it.

The Unsafe.As there is simply an internal implementation detail used after the representation has already been explicitly established. It is not evidence that Color considers a naked uint to have intrinsic ARGB semantics.

This is exactly my case, I internally use an uint packed color representation that is clearly defined. But I'm fine doing an intermediate Unsafe.As<uint, SixLabors.ImageSharp.PixelFormats.Bgra32>(ref argb) cast, although uint overloads for Bgra32/Argb32 construction wouldn't hurt, it's just a little convenience so we don't have to search in the documentation for how to create these from packed representations.

Edit: here's a quick benchmark comparing Color.FromPixel to Color.FromScaledVector:

            uint argb = 0x12345678;
            const int n = 10_000_000;
            var pixel = Unsafe.As<uint, SixLabors.ImageSharp.PixelFormats.Bgra32>(ref argb);
            const float ByteClamper = 1f / 255f;

            for (int j = 0; j < 5; ++j) {
                SixLabors.ImageSharp.Color c = default;

                DateTime t1 = DateTime.Now;
                for (int i = 0; i < n; ++i) {
                    c = SixLabors.ImageSharp.Color.FromPixel(pixel);
                }
                DateTime t2 = DateTime.Now;
                for (int i = 0; i < n; ++i) {
                    c = SixLabors.ImageSharp.Color.FromScaledVector(
                        new Vector4(ByteClamper * pixel.R, ByteClamper * pixel.G, ByteClamper * pixel.B, ByteClamper * pixel.A)
                    );
                }
                DateTime t3 = DateTime.Now;

                Console.WriteLine(c.ToString());
                Console.WriteLine($"FromPixel       : {(t2 - t1).TotalSeconds}");
                Console.WriteLine($"FromScaledVector: {(t3 - t2).TotalSeconds}");
            }

Release build output:

34567812
FromPixel       : 0,4398264
FromScaledVector: 0,0352274
34567812
FromPixel       : 0,322579
FromScaledVector: 0,031597
34567812
FromPixel       : 0,3309691
FromScaledVector: 0,0325753
34567812
FromPixel       : 0,3572666
FromScaledVector: 0,0338372
34567812
FromPixel       : 0,3504202
FromScaledVector: 0,0609916

So that's about 10 times as fast (about 4 times in debug mode), and this is without removing the Clamp() part, which should give another little boost.