Mutate BackgroundColor doesn't apply for Rgb24 Image

Turnerj commented 2 years ago

Prerequisites

[x] I have written a descriptive issue title
[x] I have verified that I am running the latest version of ImageSharp
[x] I have verified if the problem exist in both DEBUG and RELEASE mode
[x] I have searched open and closed issues to ensure it has not already been reported

Description

Admittedly I'm probably missing something obvious but while calling x.BackgroundColor() (see below) that it wasn't actually applying. While creating an MCVE, I found that it works if the pixel type is Rgba32 but not Rgb24 and I don't understand why.

Whether I use Color.Red or Color.FromRgb(255, 0, 0) (thinking that might do something different), it doesn't set the background. Because I'm not specifying an alpha channel (nor caring about one per the pixel format), I thought it would just apply the colour as if it was fully opaque.

Am I misinterpreting how the pixel formats work? The final output image I'm generating will be opaque so I figured I didn't need the "a" in "rgba" for the pixel format (wanting to save a few bytes in the output too). Should I be using Rgba32 instead or is this an actual bug?

(Note: In my "full" example that my steps below came from, many other colour operations work fine like gradients and text etc so it adds to the confusion for me.)

Steps to Reproduce

I ran this in a fresh console application in .NET 6:

using SixLabors.ImageSharp;
using SixLabors.ImageSharp.PixelFormats;
using SixLabors.ImageSharp.Processing;

using var image = new Image<Rgb24>(200, 200);
image.Mutate(x => x.BackgroundColor(Color.FromRgb(255, 0, 0)));
await image.SaveAsPngAsync("image-with-background.png");

I'm expecting the background image to be red but it is black. I've tried other overloads for BackgroundColor (thinking maybe it was an issue with the bounds to fill in) and with overloads for Color with no change in the output.

System Configuration

ImageSharp version: 1.0.4
Other ImageSharp packages and versions: N/A
Environment (Operating system, version and so on): Windows 10
.NET Framework version: (I can find the .NET Framework version but not sure why that'd help here as I'm using .NET 6)
Additional information:

Turnerj commented 2 years ago

Actually one thing I did notice when switching my full example to Rgba32 is that a gradient with alpha-channel colours didn't work where my other gradient (opaque-only) did. I'm guessing then that the pixel format plays a bigger role than I was envisaging so I'm leaning more on that I've misunderstood how certain things are meant to act with Rgb24.

I think my mental model was more the output would be Rgb24 even though quite literally in the name "pixel format", it describes how the pixels themselves are formatted/represented in memory...

Turnerj commented 2 years ago

I'm going to sanity check this tomorrow to see if I've made a mistake in my testing...

JimBobSquarePants commented 2 years ago

Am I misinterpreting how the pixel formats work?

I'm afraid so... Your expectations are a misunderstanding of what a pixel format represents.

As you've stated.

it describes how the pixels themselves are formatted/represented in memory...

That's it. Pixel formats represent how a decoded image is represented in memory, nothing else. They are totally unrepresentative of whatever image codec you choose to encode with. (We do make some educated decisions for color types and bit depths when encoding images with empty metadata based upon the pixel format).

That's the power of the API. You can decode into whatever pixel format you need, convert easily between pixel formats, and encode into whatever image codec you choose to.

Now back to your question regarding setting background color...

The BackgroundColor extension method requires an alpha component in the image pixel format to work. BackgroundColor is actually a blend of the src image rows against a span of the Color using PixelColorBlendingMode.Normal and PixelAlphaCompositionMode.SrcOver so, if you think about it, you need a non-opaque pixel in your image for the background to penetrate. Rgb24 simply lacks that alpha component.

Here's a fantastic primer on Porter/Duff Compositing and Blend Modes http://ssp.impulsetrain.com/porterduff.html

Turnerj commented 2 years ago

The BackgroundColor extension method requires an alpha component in the image pixel format to work. BackgroundColor is actually a blend of the src image rows against a span of the Color using PixelColorBlendingMode.Normal and PixelAlphaCompositionMode.SrcOver so, if you think about it, you need a non-opaque pixel in your image for the background to penetrate. Rgb24 simply lacks that alpha component.

Here's a fantastic primer on Porter/Duff Compositing and Blend Modes http://ssp.impulsetrain.com/porterduff.html

Interesting - thanks for the link! That explains your PixelAlphaCompositionMode enum a lot more and makes sense why I hit this issue.

One follow-up question - do you recommend then that, to not trip over one's self, one should use Rgba32 unless they know what they're doing? Like I did find that Rgba64 reduced banding in gradients I was using but besides that, should I think of Rgba32 as a safe default for 90% of what someone might typically use ImageSharp for?

The reason I ask is to see whether that should be guided to users (whether via inline docs or your getting started pages) or whether that should be assumed by your users.

JimBobSquarePants commented 2 years ago

do you recommend then that, to not trip over one's self, one should use Rgba32 unless they know what they're doing?

Yeah, I would. I think most people will gravitate towards it also.

tocsoft commented 2 years ago

do you recommend then that, to not trip over one's self, one should use Rgba32 unless they know what they're doing?

Yeah, I would. I think most people will gravitate towards it also.

More to the point the recommendation would be for most users to avoid Pixel Formats entirely and just use the non generic version. You generally only need to generic versions if you are dealing with manipulating the raw pixels yourself rather than utilising a processor (which handle this for you).

Turnerj commented 2 years ago

More to the point the recommendation would be for most users to avoid Pixel Formats entirely and just use the non generic version. You generally only need to generic versions if you are dealing with manipulating the raw pixels yourself rather than utilising a processor (which handle this for you).

That's good advice and I'll definitely do that in the future. In this case I was creating a new image and I can't call new Image(width, height) as the type is abstract. Not knowing about the blend behaviour relying on an alpha channel (as @JimBobSquarePants covered), I just chose the wrong pixel format.

Your actual example in your Getting Started page for ImageSharp for creating new images does specify Rgba32 but it was a case of me thinking I was smarter than I was and specifying Rgb24 instead as I thought the alpha here was relating to the output, not to the processing.

tocsoft commented 2 years ago

Oh yeah forgot it was abstract... hmm this makes me think that we might do well with making the constructors internals and shifting over to providing some Image.Create() methods, it would normalise things with our Image.LoadXX() statics too.

That would allow us to provide an api around loading the non generic images.... that would also unblock the VB use case too I believe.

antonfirsov commented 2 years ago

I don't think there a "good default pixel type for everyone" in the sense that a non-generic Image should implement by default. Note that Image.Load() returns Image<Rgba32> or Image<Rgb24> depending on the source stream's content to save memory, but it could also return grayscale Image<L8> or Image<L16> if we decide to implement further optimizations, eg. for PNG.

If a user wants to define a image eg. for drawing or other manipulations, I think it's better to expect them to be aware of their pixel types. We can in theory abstract this away by implementing a variant of non-generic Image with composition over inheritance. In that case we could exchange the internal pixel type representation when it needs to be enriched for an operation, but I don't think it's worth the trouble.

Turnerj commented 2 years ago

If the user should be more aware of the pixel formats and the effects, it might be worth expanding the pixel format docs. While that page links off to a list of pixel formats, the docs describe the bitness but not how the format affects drawing like my example of setting the background colour in the issue description.

Basically how can we convey to the user the importance so they don't shoot themselves in the foot. (Obviously relies on users reading the docs thoroughly but let's assume that at a minimum)

SixLabors / ImageSharp