Skip to content

Use premultiplied colours in rendering#6456

Open
frenzibyte wants to merge 19 commits into
ppy:masterfrom
frenzibyte:operation-go-premultiplied
Open

Use premultiplied colours in rendering#6456
frenzibyte wants to merge 19 commits into
ppy:masterfrom
frenzibyte:operation-go-premultiplied

Conversation

@frenzibyte

@frenzibyte frenzibyte commented Dec 14, 2024

Copy link
Copy Markdown
Member

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

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

Copy link
Copy Markdown
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

smoogipoo commented Dec 19, 2024

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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?

Source = BlendingType.One,
Destination = BlendingType.OneMinusSrcAlpha,
SourceAlpha = BlendingType.One,
DestinationAlpha = BlendingType.One,

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

frenzibyte commented Dec 19, 2024

Copy link
Copy Markdown
Member Author

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
Copy Markdown
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

frenzibyte commented Dec 22, 2024

Copy link
Copy Markdown
Member Author

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

Development

Successfully merging this pull request may close these issues.

BufferedContainer should blend using pre-multiplied alpha iOS image loading using incorrect premultiplied mode for skinned resources

2 participants