-
-
Notifications
You must be signed in to change notification settings - Fork 854
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
Promote PixelTypeInfo to Pixel #2601
Changes from 13 commits
b52ef56
1d223f7
2bf8d78
4b69d06
2949655
6cbbb19
0b2cf32
153ccc3
9e89cc8
db70581
c7f18c8
6f9bc03
c80a791
d1c231b
07c8f7a
4a528c1
6b6b474
1d35251
057edd8
029381e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -4,50 +4,51 @@ | |||||
using System.Runtime.CompilerServices; | ||||||
using SixLabors.ImageSharp.PixelFormats; | ||||||
|
||||||
// TODO: Review this class as it's used to represent 2 different things. | ||||||
// TODO: Review this type as it's used to represent 2 different things. | ||||||
// 1.The encoded image pixel format. | ||||||
// 2. The pixel format of the decoded image. | ||||||
namespace SixLabors.ImageSharp.Formats; | ||||||
|
||||||
/// <summary> | ||||||
/// Contains information about the pixels that make up an images visual data. | ||||||
/// </summary> | ||||||
public class PixelTypeInfo | ||||||
/// <remarks> | ||||||
/// Initializes a new instance of the <see cref="PixelTypeInfo"/> struct. | ||||||
/// </remarks> | ||||||
/// <param name="bitsPerPixel">Color depth, in number of bits per pixel.</param> | ||||||
public readonly struct PixelTypeInfo(int bitsPerPixel) | ||||||
{ | ||||||
/// <summary> | ||||||
/// Initializes a new instance of the <see cref="PixelTypeInfo"/> class. | ||||||
/// Gets color depth, in number of bits per pixel. | ||||||
/// </summary> | ||||||
/// <param name="bitsPerPixel">Color depth, in number of bits per pixel.</param> | ||||||
public PixelTypeInfo(int bitsPerPixel) | ||||||
=> this.BitsPerPixel = bitsPerPixel; | ||||||
public int BitsPerPixel { get; init; } = bitsPerPixel; | ||||||
|
||||||
/// <summary> | ||||||
/// Initializes a new instance of the <see cref="PixelTypeInfo"/> class. | ||||||
/// Gets the count of the color components | ||||||
/// </summary> | ||||||
/// <param name="bitsPerPixel">Color depth, in number of bits per pixel.</param> | ||||||
/// <param name="alpha">The pixel alpha transparency behavior.</param> | ||||||
public PixelTypeInfo(int bitsPerPixel, PixelAlphaRepresentation alpha) | ||||||
{ | ||||||
this.BitsPerPixel = bitsPerPixel; | ||||||
this.AlphaRepresentation = alpha; | ||||||
} | ||||||
public byte ComponentCount { get; init; } | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should consider changing this to
Suggested change
Unless there is a reason to compress data for optimization (=the type is expected to be instantiated very frequently), it is generally recommended to prefer On the other hand, I noticed that we use non- There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’ve often wondered if there was a rule. For v4 we can definitely fix this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately it's not codified in the Framework Design Guidelines, but it exists for BCL APIs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All good. I've fixed the property for this PR, we can look at others. |
||||||
|
||||||
/// <summary> | ||||||
/// Gets color depth, in number of bits per pixel. | ||||||
/// Gets the pixel component precision. | ||||||
/// </summary> | ||||||
public int BitsPerPixel { get; } | ||||||
public PixelComponentPrecision? ComponentPrecision { get; init; } | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIK there might be pixel types with asymmetric component precision. (I remember briefly touching this topic in an old discussion with Clinton.) How would this model handle such pixel types? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It represents the maximum component precision. It’s in the description but perhaps including it in the type name or property name is better? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked at the usages of the property, and the only way we use it is the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My thought process is that users can implement custom performance optimized processors based upon this implementation. If they know the precision, they could operate over The use case that I've currently implemented fixes a bug we had also.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see that
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's neat! I think you might have to be able to account for padding bits, so I'm not sure you can get away with not having a mask per channel. I'm trying to think of tricky examples... I've seen some BMPs that have gaps in the channel masks, but those were of the stress-test variety, not real world. You'll have real-world formats like B10G10R10, which is 32-bit with 2 bits padding, though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure that's required for our use cases which is what precision can I safly store this pixel in. We can tell the actual size of the pixel format from the info since the types themselves must handle padding. Theoretically we could add padding to the info. I'm assuming though that world is not completely mad and padding is always at the end? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, I don't think you'll see a real-world case where
Edit: Never mind, there's no channel order conveyed in the mask. BGR and RGB have the same masks, so you can only know how to interpret them by knowing the name. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'll go with real world for now. If someone wants something really weird, they can find a really weird library. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Type has been introduced as |
||||||
|
||||||
/// <summary> | ||||||
/// Gets the pixel alpha transparency behavior. | ||||||
/// <see langword="null"/> means unknown, unspecified. | ||||||
/// </summary> | ||||||
public PixelAlphaRepresentation? AlphaRepresentation { get; } | ||||||
|
||||||
internal static PixelTypeInfo Create<TPixel>() | ||||||
where TPixel : unmanaged, IPixel<TPixel> | ||||||
=> new(Unsafe.SizeOf<TPixel>() * 8); | ||||||
public PixelAlphaRepresentation? AlphaRepresentation { get; init; } | ||||||
|
||||||
internal static PixelTypeInfo Create<TPixel>(PixelAlphaRepresentation alpha) | ||||||
internal static PixelTypeInfo Create<TPixel>( | ||||||
byte componentCount, | ||||||
PixelComponentPrecision componentPrecision, | ||||||
PixelAlphaRepresentation pixelAlphaRepresentation) | ||||||
where TPixel : unmanaged, IPixel<TPixel> | ||||||
=> new(Unsafe.SizeOf<TPixel>() * 8, alpha); | ||||||
=> new() | ||||||
{ | ||||||
BitsPerPixel = Unsafe.SizeOf<TPixel>() * 8, | ||||||
ComponentCount = componentCount, | ||||||
ComponentPrecision = componentPrecision, | ||||||
AlphaRepresentation = pixelAlphaRepresentation | ||||||
}; | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
// Copyright (c) Six Labors. | ||
// Licensed under the Six Labors Split License. | ||
|
||
namespace SixLabors.ImageSharp.PixelFormats; | ||
|
||
/// <summary> | ||
/// Provides enumeration of the maximum precision of individual components within a pixel format. | ||
/// </summary> | ||
public enum PixelComponentPrecision | ||
{ | ||
/// <summary> | ||
/// 8-bit signed integer. | ||
/// </summary> | ||
SByte, | ||
|
||
/// <summary> | ||
/// 8-bit unsigned integer. | ||
/// </summary> | ||
Byte, | ||
|
||
/// <summary> | ||
/// 16-bit signed integer. | ||
/// </summary> | ||
Short, | ||
|
||
/// <summary> | ||
/// 16-bit unsigned integer. | ||
/// </summary> | ||
UShort, | ||
|
||
/// <summary> | ||
/// 32-bit signed integer. | ||
/// </summary> | ||
Int, | ||
|
||
/// <summary> | ||
/// 32-bit unsigned integer. | ||
/// </summary> | ||
UInt, | ||
|
||
/// <summary> | ||
/// 16-bit floating point. | ||
/// </summary> | ||
Half, | ||
|
||
/// <summary> | ||
/// 32-bit floating point. | ||
/// </summary> | ||
Float | ||
} |
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.
While not critical, I would prefer an abstraction that can get rid of the
Rgba64-L16
special cases above this line - by moving them toIPixel<T>
/PixelOperations
code.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.
Me also. In this case I think we should ditch the specialisation and box anything greater than byte.
ColorFromPixel
is not area for high performance operations.On
IPixel
I would, if we can, drop all the variousFromXX
specialisation there also and ensure a good optimised bulk operations for allRgba32
compatible pixel formats. That may not be possible though.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.
Color.ToPixel
already has a bulk variant, and I remember that we have found a use case for the inverse bulk operation: #2485 (comment)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.
Well remembered, but those are really short loops (max 256) in real terms. I'd hate to limit our flexibility and extensibility based on introducing a requirement there.
Ideally, I want to introduce as much agnostity as possible in our APIs.
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.
Boxing is very ugly, even if done
<=256x
typically, I would try to avoid it if there are alternatives.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.
Maybe we just back Color with
Vector4
? it already takes up the same amount of space and means we would be able to use bulk operations.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 reason we introduced the ability to define (higher than
Vector4
) arbitrary-precison pixel types was this feature request: SixLabors/ImageSharp.Drawing#165I like the fact that ImageSharp supports such "images". (At least for some use-cases)
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.
Then use
Vector4
as the default and box anything bigger. People expected double precision are in for a rough time with the library since almost everything is converted to use single precision at some point.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've updated
Color
to useVector4
to avoid boxing. I've also removed the implicit casting for random pixel formats in favour of explicit methods.