Promote PixelTypeInfo to Pixel by stefannikolei · Pull Request #2601 · SixLabors/ImageSharp

stefannikolei · 2023-12-03T20:45:45Z

src/ImageSharp/PixelFormats/PixelOperations{TPixel}.cs

JimBobSquarePants · 2023-12-05T01:37:47Z

src/ImageSharp/Formats/PixelTypeInfo.cs

+    public PixelAlphaRepresentation? AlphaRepresentation { get; init; }

-    internal static PixelTypeInfo Create<TPixel>(PixelAlphaRepresentation alpha)
+    internal static PixelTypeInfo Create<TPixel>(byte componentCount, PixelAlphaRepresentation pixelAlphaRepresentation)


I'm wondering whether we can remove this? I want to add the additional enum #2534 (comment) which means adding an additional property to the constructor.

Maybe just add the additional parameter as having to duplicate Unsafe.SizeOf<TPixel>() * 8 is worse.

@stefannikolei I'm going to add this for you today.

Ah. To be honest.. totally forgot about that comment

src/ImageSharp/PixelFormats/IPixel.cs

src/ImageSharp/PixelFormats/PixelImplementations/A8.cs

JimBobSquarePants · 2023-12-11T12:47:12Z

@stefannikolei I've made my changes, will need to do a sense check in the morning before merging as it's late now.

antonfirsov · 2023-12-11T16:36:46Z

src/ImageSharp/Formats/PixelTypeInfo.cs

-        this.BitsPerPixel = bitsPerPixel;
-        this.AlphaRepresentation = alpha;
-    }
+    public byte ComponentCount { get; init; }


We should consider changing this to int.

Suggested change

public byte ComponentCount { get; init; }

public int ComponentCount { get; init; }

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 int over other integral types in .NET APIs. For example, IPEndPoint.Port is an int, while technically it could be an ushort. The reason is that this removes the need for conversion which is both expensive and complicated.

On the other hand, I noticed that we use non-int integrals in other descriptor types, eg. PngMetadata. I wish I noticed this earlier.

I’ve often wondered if there was a rule. For v4 we can definitely fix this.

Unfortunately it's not codified in the Framework Design Guidelines, but it exists for BCL APIs.

All good. I've fixed the property for this PR, we can look at others.

antonfirsov · 2023-12-11T16:39:41Z

src/ImageSharp/Formats/PixelTypeInfo.cs

+    /// Gets the pixel component precision.
    /// </summary>
-    public int BitsPerPixel { get; }
+    public PixelComponentPrecision? ComponentPrecision { get; init; }


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?

It represents the maximum component precision. It’s in the description but perhaps including it in the type name or property name is better?

I looked at the usages of the property, and the only way we use it is the info.ComponentPrecision <= PixelComponentPrecision.Byte comparison, meaning that the only thing we need this for is to determine if the precision is below or above a certain treshold, and the exact precision information is practically unused. Unless we think there are use cases (proven by actual user user need) to observe the precision of the highest precision component, IMO we should try to avoid exposing such a property and explore alternative options to solve this.

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 byte vs Vector4.

The use case that I've currently implemented fixes a bug we had also.

Rgba1010102 uses a uint as its packed value which is the same size as Rgba32 but we lose precision for the RGB components if we convert to it as we were since they require 10 bit precision.

I see that ComponentCount is a similar informative property. I didn't notice this fact in the #2534 discussion. The way I view this now is that we are attempting to add a Rich Pixel Type Descriptor feature. IMO there are a couple of important points we should consider before fully committing to it:

To really get this right, it might be better to expose a struct/class ComponentInfo and a property in PixelTypeInfo that enumerates the ComponentInfos, so each component can tell its precision separately instead of telling the maximum (for which I'm not sure if there is a use-case except the conversion implementation this PR is proposing). As far as I can recall that old discussion with Clinton, WIC has such a model but I may be wrong.

Note that this feature idea doesn't necessarily overlap with the Pixel <=> Color conversion. I don't see how can we implement efficient bulk conversion using these descriptor information only. In fact, if our goal is to solve the Pixel <=> Color conversion thing, it might be better to promote the conversion (& bulk conversion) methods to PixelOperations<T>.

Considering that this isn't super trivial to get right, I'm really curious if we have enough evidence (=enough users) proving that the Rich Pixel Type Descriptor feature is worth the headache.

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.

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. Unsafe.Sizeof<B10G10R10>() should be the same size Unsafe.Sizeof<Rgba1010102>() which is 4.

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?

Right, I don't think you'll see a real-world case where BitsPerPixel - Sum(ChannelPrecision) doesn't give you the padding bits, which should always be at the end. Just trying to come up with reasons explicit masks might be beneficial, and weirdo pixel formats with interior padding are at least a possibility.

The other thing I guess that's useful in an explicit channel mask is that it also communicates the channel order. I'm not sure if WIC actually uses that information in that way, but I suppose it might. They don't expose channel order in any other way in their format definitions. It's just in the name and in the mask.

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.

I think I'll go with real world for now. If someone wants something really weird, they can find a really weird library.

Type has been introduced as PixelComponentInfo

antonfirsov · 2023-12-11T16:45:47Z

src/ImageSharp/Color/Color.cs

            return new((L16)(object)pixel);
        }
-        else if (Unsafe.SizeOf<TPixel>() <= Unsafe.SizeOf<Rgba32>())
+


While not critical, I would prefer an abstraction that can get rid of the Rgba64-L16 special cases above this line - by moving them to IPixel<T> / PixelOperations code.

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 various FromXX specialisation there also and ensure a good optimised bulk operations for all Rgba32 compatible pixel formats. That may not be possible though.

ColorFromPixel is not area for high performance operations.

Color.ToPixel already has a bulk variant, and I remember that we have found a use case for the inverse bulk operation: #2485 (comment)

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.

Boxing is very ugly, even if done <=256x typically, I would try to avoid it if there are alternatives.

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.

The reason we introduced the ability to define (higher than Vector4) arbitrary-precison pixel types was this feature request: SixLabors/ImageSharp.Drawing#165

I like the fact that ImageSharp supports such "images". (At least for some use-cases)

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.

I've updated Color to use Vector4 to avoid boxing. I've also removed the implicit casting for random pixel formats in favour of explicit methods.

JimBobSquarePants

I'm happy with this now. Would love a second or third opinion though.

saucecontrol

Really nice improvement!

stefannikolei added 3 commits December 3, 2023 21:44

Promote PixelTypeInfo to TPixel

b52ef56

Fixes SixLabors#2534

Merge branch 'main' into sn/promote-pixeltype

1d223f7

Fix Analyzer errors

2bf8d78

JimBobSquarePants reviewed Dec 5, 2023

View reviewed changes

src/ImageSharp/PixelFormats/PixelOperations{TPixel}.cs Outdated Show resolved Hide resolved

JimBobSquarePants reviewed Dec 5, 2023

View reviewed changes

src/ImageSharp/PixelFormats/IPixel.cs Outdated Show resolved Hide resolved

JimBobSquarePants reviewed Dec 5, 2023

View reviewed changes

src/ImageSharp/PixelFormats/IPixel.cs Outdated Show resolved Hide resolved

JimBobSquarePants reviewed Dec 5, 2023

View reviewed changes

src/ImageSharp/PixelFormats/PixelImplementations/A8.cs Outdated Show resolved Hide resolved

stefannikolei and others added 10 commits December 5, 2023 06:56

Made GetPixelTypeInfo non static

4b69d06

Fix typos

2949655

Fix usage of static method in ResizeProcessor

6cbbb19

Replaced comment with /// <inheritdoc />

0b2cf32

Disable SA1648 and add Ruleset to solution

153ccc3

Merge branch 'main' into sn/promote-pixeltype

9e89cc8

Add PixelComponentPrecision

db70581

Use info for color conversion

c7f18c8

Merge branch 'main' into sn/promote-pixeltype

6f9bc03

Merge branch 'main' into sn/promote-pixeltype

c80a791

antonfirsov reviewed Dec 11, 2023

View reviewed changes

JimBobSquarePants added 4 commits December 12, 2023 08:08

Rename property, use int.

d1c231b

Merge branch 'main' into sn/promote-pixeltype

07c8f7a

Merge branch 'main' into sn/promote-pixeltype

4a528c1

Introduce PixelComponentInfo + simplify Color

6b6b474

JimBobSquarePants added API area:pixelformats breaking Signifies a binary breaking change. labels Jan 2, 2024

JimBobSquarePants added 2 commits January 5, 2024 18:52

Add PixelColorType

1d35251

Use the new metadata

057edd8

JimBobSquarePants requested a review from antonfirsov January 5, 2024 10:05

JimBobSquarePants requested a review from saucecontrol January 5, 2024 10:05

Remove outdated rule overrides.

029381e

JimBobSquarePants approved these changes Jan 5, 2024

View reviewed changes

saucecontrol approved these changes Jan 5, 2024

View reviewed changes

JimBobSquarePants merged commit 77cf848 into SixLabors:main Jan 8, 2024

stefannikolei deleted the sn/promote-pixeltype branch January 23, 2024 19:14

	public byte ComponentCount { get; init; }
	public int ComponentCount { get; init; }

Uh oh!

Conversation

stefannikolei commented Dec 3, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JimBobSquarePants commented Dec 11, 2023

Uh oh!

antonfirsov Dec 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov Dec 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saucecontrol Dec 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov Dec 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov Dec 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JimBobSquarePants left a comment

Choose a reason for hiding this comment

Uh oh!

saucecontrol left a comment

antonfirsov Dec 11, 2023 •

edited

Loading

antonfirsov Dec 11, 2023 •

edited

Loading

saucecontrol Dec 14, 2023 •

edited

Loading

antonfirsov Dec 11, 2023 •

edited

Loading

antonfirsov Dec 11, 2023 •

edited

Loading

antonfirsov Dec 13, 2023 •

edited

Loading