-
-
Notifications
You must be signed in to change notification settings - Fork 884
Handle hex parsing in Color with format support #2964
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
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.
Pull Request Overview
This PR refactors hex parsing/formatting to live on the semantic Color
type with explicit format support, removing hex APIs from Rgba32
.
- Moved all hex parse/format logic from
Rgba32
intoColor
, introducingColorHexFormat
(Rgba
/Argb
) - Updated tests to use
Color.ParseHex(...).ToPixel<>()
and new format overloads - Added parsing/formatting for both CSS‐style RRGGBBAA and Microsoft‐style AARRGGBB
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/ImageSharp/Color/ColorHexFormat.cs | Introduces ColorHexFormat enum and XML docs |
src/ImageSharp/Color/Color.cs | Implements Color.ParseHex /TryParseHex and ToHex(format) |
src/ImageSharp/PixelFormats/PixelImplementations/Rgba32.cs | Removes hex parse/format from Rgba32 |
tests/ImageSharp.Tests/PixelFormats/UnPackedPixelTests.cs | Switches to Color.ParseHex(...).ToPixel<>() |
tests/ImageSharp.Tests/PixelFormats/Rgba32Tests.cs | Removes deprecated Rgba32.ParseHex tests |
tests/ImageSharp.Tests/Color/ColorTests.cs | Adds ToHexArgb , reorganizes FromHex tests |
Comments suppressed due to low confidence (3)
src/ImageSharp/Color/ColorHexFormat.cs:33
- The docs for
ColorHexFormat.Argb
list only#ARGB
and#AARRGGBB
, but the code also treats 6-digit inputs (e.g.#RRGGBB
) by prependingFF
for alpha. Update the XML docs to accurately reflect 6-digit support or change the implementation to match the docs.
/// <item><description><c>#ARGB</c> expands to <c>AARRGGBB</c></description></item>
tests/ImageSharp.Tests/Color/ColorTests.cs:191
- There aren’t tests covering 6-digit ARGB parsing (i.e. default opaque
#RRGGBB
underColorHexFormat.Argb
). Adding a case to verify thatColor.ParseHex("RRGGBB", ColorHexFormat.Argb)
yields an alpha ofFF
would ensure the implementation remains correct.
[Fact]
src/ImageSharp/Color/Color.cs:458
- The helper
ToRgbaHex
(andToArgbHex
) returnsnull
when the input length is invalid, but the signature isReadOnlySpan<char>
. Returning a null string may lead to a NullReferenceException at runtime. Consider returningdefault(ReadOnlySpan<char>)
explicitly or throwing anArgumentException
instead of returning null.
private static ReadOnlySpan<char> ToRgbaHex(string value)
src/ImageSharp/Color/Color.cs
Outdated
? ('F', hex[0], hex[1], hex[2]) | ||
: (hex[0], hex[1], hex[2], hex[3]); | ||
|
||
return new string([a, a, r, r, g, g, b, b]); |
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.
Similar to #2963 (comment), I'd use string.Create
here (note: due the collection expression, C# will emit a inline here and no allocation happens, but it's unneded copying around the value).
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.
@gfoidl I wrote the faster one:
static (bool success, byte a, byte r, byte g, byte b) ConvertToColor(string input) //ARGB format
{
bool startWithPoundSign = input.StartsWith('#');
var colorStringLength = input.Length;
if (startWithPoundSign) colorStringLength -= 1;
int currentOffset = startWithPoundSign ? 1 : 0;
// Formats:
// #FFDFD991 8 chars with Alpha
// #DFD991 6 chars
// #FD92 4 chars with Alpha
// #DAC 3 chars
if (colorStringLength == 8
|| colorStringLength == 6
|| colorStringLength == 4
|| colorStringLength == 3)
{
bool success;
byte result;
byte a;
int readCount;
// #DFD991 6 chars
// #FFDFD991 8 chars with Alpha
//if (colorStringLength == 8 || colorStringLength == 6)
if (colorStringLength > 5)
{
readCount = 2;
}
else
{
readCount = 1;
}
bool includeAlphaChannel = colorStringLength == 8 || colorStringLength == 4;
if (includeAlphaChannel)
{
(success, result) = HexCharToNumber(input, currentOffset, readCount);
if (!success) return default;
a = result;
currentOffset += readCount;
}
else
{
a = 0xFF;
}
(success, result) = HexCharToNumber(input, currentOffset, readCount);
if (!success) return default;
byte r = result;
currentOffset += readCount;
(success, result) = HexCharToNumber(input, currentOffset, readCount);
if (!success) return default;
byte g = result;
currentOffset += readCount;
(success, result) = HexCharToNumber(input, currentOffset, readCount);
if (!success) return default;
byte b = result;
return (true, a, r, g, b);
}
return default;
}
static (bool success, byte result) HexCharToNumber(string input, int offset, int readCount)
{
Debug.Assert(readCount == 1 || readCount == 2);
byte result = 0;
for (int i = 0; i < readCount; i++, offset++)
{
var c = input[offset];
byte n;
if (c >= '0' && c <= '9')
{
n = (byte)(c - '0');
}
else if (c >= 'a' && c <= 'f')
{
n = (byte)(c - 'a' + 10);
}
else if (c >= 'A' && c <= 'F')
{
n = (byte)(c - 'A' + 10);
}
else
{
return default;
}
result *= 16;
result += n;
}
if (readCount == 1)
{
result = (byte)(result * 16 + result);
}
return (true, result);
}
We parse to string, and then parse string to uint. In fact, we can achieve zero alloc.
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.
👍🏻
Instead of the if (c >= 'a' && c <= 'f')
you can use pattern matching (is
), as Roslyn will emit optimized tests (at least in a future version, but some of them are already enabled).
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.
Dammit you nerd sniped me!! 🤣
I've updated the code to use a zero-allocation implementation for both RGBA and ARGB layout.
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.
@JimBobSquarePants The code I shown is the zero-allocation implementation. See my benchmark for the code I shown, and you can find my benchmark code in https://github.com/lindexi/lindexi_gd/blob/8422ee2ff82386e57eeb8bb43735a7ef3121782f/Workbench/RenalwhuchewelneHukawine/ColorParserBenchmark.cs
Method | colorText | Mean | Error | StdDev | Allocated |
---|---|---|---|---|---|
Test | #123456 | 4.8096 ns | 0.0112 ns | 0.0105 ns | - |
Test | #AABBCC | 5.3978 ns | 0.0150 ns | 0.0141 ns | - |
Test | #AABBCC1 | 0.3928 ns | 0.0059 ns | 0.0055 ns | - |
Test | #AABBCCDD | 6.7899 ns | 0.0187 ns | 0.0175 ns | - |
Test | #ABC | 3.9403 ns | 0.0245 ns | 0.0217 ns | - |
Test | #FF123456 | 6.3917 ns | 0.0213 ns | 0.0189 ns | - |
Test | #FFAABBCC | 6.7806 ns | 0.0152 ns | 0.0143 ns | - |
Test | #FFAABBCC1 | 0.3235 ns | 0.0033 ns | 0.0031 ns | - |
The origin code in ImageSharp should alloc some memory.
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.
I think you may have misunderstood what I said. The code in this PR is now zero allocation for both RGBA and ARGB scenarios.
…/ImageSharp into js/color-hex-normalize
Prerequisites
Description
This PR cleans up and refactors the hex parsing and formatting APIs to separate semantic color operations from pixel storage formats, ensuring clarity and consistency across the library.
See #2962 for discussion.
Key Changes
Removed hex parsing and formatting from
Rgba32
. These operations do not belong on pixel storage types.Added
Color.ToHex(ColorHexFormat)
, supporting two semantic formats:ColorHexFormat.Rgba
: Outputs RRGGBBAA, matching CSS4/Web conventions.ColorHexFormat.Argb
: Outputs AARRGGBB, matching XAML/Microsoft conventions.Centralized hex string parsing into the
Color
type, supporting normalized input for both formats:#rgb
,#rgba
,#rrggbb
,#rrggbbaa
→ parsed as RRGGBBAA#argb
,#aarrggbb
→ parsed as AARRGGBBClarified the intended usage in the XML docs for
ColorHexFormat
, detailing the supported input forms and output layout for each format.This removes ambiguity between pixel channel layout and semantic hex formats, making the API surface more predictable and easier to use correctly.