Skip to content
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

Use premultiplied colours in rendering #6456

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

frenzibyte
Copy link
Member

@frenzibyte frenzibyte commented Dec 14, 2024

RFC, cc @ppy/team-client.

This will be an extremely breaking change to all framework consumers, I will write down breaking notes for it after we all agree with the direction.

The efforts of this started when I noticed that iOS always outputs premultiplied image pixels when loading them via UIImage. Searching on the internet for methods to avoid this behaviour has shown nothing, and after opening a PR manually "fixing" this, #6454 (comment) came up, which shown that we can take a different route and switch permanently to premultiplied colours for textures and shaders, as discussed.

Consider this an initial approach meant to stir discussions on the introduced API and intended direction.

Multiple classes have been introduced to emphasize on the concept of "premultiplied colours" in rendering, most of which are one-way conversion, meaning that a regular (aka straight-alpha) colour transformed into a premultiplied-alpha colour cannot be transformed back, as the conversion is technically lossy when given zero alpha, and I generally want to avoid implicit conversions from/to Color4 as much as possible.

There is not much to note about the changes, but, as a breakdown of introduced classes:

  • PremultipliedImage: Exposes an image with alpha multiplication applied. The image may originally be in straight-alpha form converted using FromStraight, or already premultiplied (aka iOS) and wrapped in the structure using FromPremultiplied.
  • PremultipliedColour: Exposes a Color4 with alpha multiplication applied. Works similar to PremultipliedImage.
  • UniformColour: Special version of UniformVector4 for colours specifically, stores colours as premultiplied. Implicitly converts from/to PremultipliedColour.
  • UniformColourInfo: This is introduced specifically for border colour specifications, as it was extremely too long and adding premultiplied API warranted this.

TestSceneBlendingCorrectness

Inspired by #3429, this PR adds a test scene that shows blending comparisons between buffered and non-buffered rendering. Final result is achieved by operating using premultiplied colours and fixing alpha blending.

master this PR
CleanShot 2024-12-19 at 12 17 37 CleanShot 2024-12-19 at 12 16 21
CleanShot 2024-12-19 at 12 17 39 CleanShot 2024-12-19 at 12 16 25

@frenzibyte frenzibyte force-pushed the operation-go-premultiplied branch from afd49bc to 6f10e23 Compare December 14, 2024 21:49
@@ -34,7 +35,7 @@ protected override Image<TPixel> ImageFromStream<TPixel>(Stream stream)
}

bitmap.Recycle();
return Image.LoadPixelData<TPixel>(result, bitmap.Width, bitmap.Height);
return PremultipliedImage.FromPremultiplied(Image.LoadPixelData<Rgba32>(result, bitmap.Width, bitmap.Height));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to https://developer.android.com/reference/android/graphics/BitmapFactory.Options#inPremultiplied which states that it is enabled by default, this line should be correct (cc @Susko3 if you can confirm this).

@frenzibyte frenzibyte self-assigned this Dec 18, 2024
@smoogipoo
Copy link
Contributor

smoogipoo commented Dec 19, 2024

My primary concern reading through these changes is that the alpha channel is still present in some parts of the code. For example...

TestSceneDrawablePath.cs
SmoothPath.cs

In these two, the alpha channel is still present in the image itself, even though image.Premultiplied[] is used. There's a few other similar cases, such as in FrameStatisticsDisplay.

PremultipliedColour.cs

When constructing the premult colour, we're still taking on the source alpha channel which is then being passed to the shader. i.e. a colour (255, 255, 255, 0.8) will be passed as (204, 204, 204, 0.8) to the shader as far as I can tell.

Am I misunderstanding something here?

/// <summary>
/// The <see cref="Color4"/> after alpha multiplication.
/// </summary>
public readonly Color4 Premultiplied;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Referring to my prior comment, #6456 (comment), it might be more intuitive to decompose this into RGB and a private float A=1. Unless it becomes annoying to handle raw components in usages as opposed to a Color4 bundle.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I was under the thought that any colour value that is represented by red-green-blue-alpha channels should be bundled in a Color4, but I can agree with the argument that premult colours do not fall within that category particularly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure about making the alpha component private, it's still an effective value of the colour and needs to be exposed for UniformColour to turn it to a Vector4 for shaders. The alpha component in premultiplied colours is referred to as the level of "occlusion" in the "Alpha compositing" wikipedia page:

CleanShot 2024-12-22 at 08 19 54

I've went with that name for the alpha component field just to disassociate it from the name "Alpha" which we commonly recognize as the opacity of a pixel rather than "occlusion". Up for discussion.

@smoogipoo
Copy link
Contributor

Reading through more, I am guessing this is just my misunderstanding because the blending is dC = (1 * sC) + ((1-sA) * dC), whereas I was thinking of (incorrectly) blending in colour space as dC = (1 * sC) + ((1-sC) * dC).

Appears to make sense as it is?

@@ -72,7 +72,7 @@ public struct BlendingParameters : IEquatable<BlendingParameters>

public static BlendingParameters Mixture => new BlendingParameters
{
Source = BlendingType.SrcAlpha,
Source = BlendingType.One,
Destination = BlendingType.OneMinusSrcAlpha,
SourceAlpha = BlendingType.One,
DestinationAlpha = BlendingType.One,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a side-node, I'm pretty sure this is incorrect and the alpha equation should be (1, 1-sA). That said, this is also incorrect on master but it may be what's causing your colour issues in ppy/osu#31129 (comment) ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not seem to have any effect on the issue (I've ensured the inherited property is also updated to OneMinusSrcAlpha). That being said, I don't see any differences in game so the change can happen here.

I did not notice that the mixing algorithm included alpha so that's why I was wondering why you mentioned DestinationAlpha being wrong in #3429. But now that I search again, from the very definition of the mixing algorithm (Porter–Duff's over operator) it does seem to be correct.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied to Mixture blending in dc00efe. Also added a test scene that can be ran to show blending differences between master and this PR with the fix applied.

After some self ranting and figuring out whether this change should be applied to Additive blending as well, I've settled with a weak no because I cannot find any reference source to compare against, and the case where this matters is very specific (see 1ff2b65#diff-d2aa04480a7566b4c82977db2d05575222822b78e79807a9b3c607fe374fc254R288-R293).

@frenzibyte
Copy link
Member Author

frenzibyte commented Dec 19, 2024

My primary concern reading through these changes is that the alpha channel is still present in some parts of the code. For example...

TestSceneDrawablePath.cs SmoothPath.cs

In these two, the alpha channel is still present in the image itself, even though image.Premultiplied[] is used. There's a few other similar cases, such as in FrameStatisticsDisplay.

In the usages that directly modify image.Premultiplied[], the colours constructed are assumed to be premultiplied already. As you can see in the diff, I've modified said usages to be multiplied by their alpha.

Alternatively I can provide an indexer from PremultipliedImage that accepts PremultipliedColours, therefore it becomes extremely clear what kind of colour is being passed and make things easier to read:

- raw.Premultiplied[i, 0] = new Rgba32(r * a, g * a, b * a, a);
+ raw[i, 0] = PremultipliedColour.FromStraight(new Color4(r, g, b, a));

And technically you can still pass down premultiplied colours if you wish:

- raw.Premultiplied[i, 0] = new Rgba32(r * a, g * a, b * a, a);
+ raw[i, 0] = PremultipliedColour.FromPremultiplied(new Color4(r * a, g * a, b * a, a));

I think that might be a better approach as we don't want to expose the framework codebase to the idea that colours can be premultiplied, and leave it as an internal detail that is done right before the shader reads said colours.

PremultipliedColour.cs

When constructing the premult colour, we're still taking on the source alpha channel which is then being passed to the shader. i.e. a colour (255, 255, 255, 0.8) will be passed as (204, 204, 204, 0.8) to the shader as far as I can tell.

Am I misunderstanding something here?

Worthy of note that PremultipliedColour intentionally does not allow direct construction, it can be constructed via two methods:

  • FromStraight, which accepts colours that are not multiplied by alpha yet
  • FromPremultiplied, which accepts colours that have been multiplied already

If you use PremultipliedColour.FromStraight(255, 255, 255, 0.8) then the resulting colour that is passed to the shader will be (204, 204, 204, 0.8)

@frenzibyte
Copy link
Member Author

@smoogipoo To solidify the API, I've removed access to the underlying image of PremultipliedImage and mirrored the necessary methods in the class itself. See df53238 and ccd0269. All mentioned concerns should be addressed hopefully. My intention with these changes is to make it 100% clear at which point during the rendering API everything operates in "premultiplied colour" format, and I've done so by attaching the name Premultiplied to every component as such.

@frenzibyte
Copy link
Member Author

frenzibyte commented Dec 22, 2024

59493e3 is pushed as colours with alpha>1 can be used to give strong glow effects. See https://discord.com/channels/188630481301012481/589331078574112768/1320398450457841754.

This is also the premise of how BlurEffect.Strength works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants