Fix grayscale alpha for Image::convert FORMAT_L8 using REC.709

[korypostma]

This PR fixes the "averaging" method previously and correctly uses REC.709 as indicated in the previous TODO comment. constexpr was added to the max_bytes because this can all be determined at compile time.

A new test case was added to convert RGBA values to L8 and then make sure the correct values were received.

Bugsquad edit:

• Fixes image.cpp _convert function returning invalid results for Image::FORMAT_L8 combined with fix_alpha_edges and fix_alpha_border #77393

[korypostma]

As can be seen, the MRP works correctly now:

[fire]

Can you squash the commits to one?

[fire]

If you can reference my branch, I put it here. https://github.com/V-Sekai/godot/commits/grayscale-fix-77393

[korypostma]

Can you squash the commits to one?

I tried and it wouldn't let me do it locally because I had already pushed the first one up to my repo (origin). Geez, I've been using git since 2008 and I still struggle with the basics...

[clayjohn]

The substantive changes look great to me! I agree with Fire about dropping the extra comment though

[fire]

Worked on git on discord with Kory.

wrong line

[fire]

Worked with Kory on discord.

wofs[0] = (uint8_t)((luminance * rgba[3] + 255) >> 8U); is now correct.

[korypostma]

Just noticed it should be 255U, not sure if it will really matter but is it worth changing again? @fire also mention about the formatting of the comment after the fact as well...

[clayjohn]

Looks good to me as is.

If you want to push one more change to fix the missing "U" on 255 you can. If you are making one more change you might as well also change the comment to // REC.709. to fit with our comment style guide.

That being said, neither change is really important here, but having perfect style is nice.

[korypostma]

Looks good to me as is.

If you want to push one more change to fix the missing "U" on 255 you can. If you are making one more change you might as well also change the comment to // REC.709. to fit with our comment style guide.

That being said, neither change is really important here, but having perfect style is nice.

Comment and unsigned issue fixed. Thanks for the help everyone, this has been a learning experience and I hope to make many more contributions, currently working on a couple more.

[clayjohn]

After looking at this again, I am realizing that this is a bigger change in behaviour than I thought.

This essentially applies pre-multiplied alpha to any texture when doing a conversion from a color format with alpha to a greyscale format without alpha (e.g RGBA -> L8). The current behaviour averages the color channels and writes them out directly, ignoring alpha. The new behaviour premultiplies the alpha when converting. Seeing as premultiply_alpha is already a function of Image (and exposed to import settings), I wonder if it would be better to remove the alpha premultiplication from convert() and instead add LA8 support to premultiply_alpha so that users can choose whether the values are premultiplied.

new comment

[korypostma]

After looking at this again, I am realizing that this is a bigger change in behaviour than I thought.

This essentially applies pre-multiplied alpha to any texture when doing a conversion from a color format with alpha to a greyscale format without alpha (e.g RGBA -> L8). The current behaviour averages the color channels and writes them out directly, ignoring alpha. The new behaviour premultiplies the alpha when converting. Seeing as premultiply_alpha is already a function of Image (and exposed to import settings), I wonder if it would be better to remove the alpha premultiplication from convert() and instead add LA8 support to premultiply_alpha so that users can choose whether the values are premultiplied.

So you are implying that the true fix for this is in fix_alpha_edges / fix_alpha_border code instead? The issue I was running into is that I convert RGBA to L8 (grayscale) and the output was as shown in Issue #77393. Basically, the fix_alpha_edges code creates a bunch of color values that should not exist in the grayscale image. Upon discussions in rocket-chat, I started thinking about where else this could be an issue and that's when it was obvious the issue was in the _convert code. When converting from RGBA to L8, we don't want all of the fake pixels and colors that were created, we only want the true values from the original image.

Also, Image::FORMAT_LA8 doesn't premultiply the alpha, from what I can see this convert code is likely broken for that format as well. I would assume that anyone converting RGBA to either L8 or LA8 would want expected behavior which would be to premultiply alpha values due to the lack of an alpha value in the converted image.

Is this code actually correct for FORMAT_LA8 (only 1 output byte and it will only store the alpha and no luminance)?

template <uint32_t read_bytes, bool read_alpha, uint32_t write_bytes, bool write_alpha, bool read_gray, bool write_gray>
static void _convert(int p_width, int p_height, const uint8_t *p_src, uint8_t *p_dst) {

case FORMAT_RGBA8 | (FORMAT_LA8 << 8):
	_convert<3, true, 1, true, false, true>(width, height, rptr, wptr);
	break;

So where does that leave us? What is the proper fix?

[korypostma]

OK, it looks like the _convert code is actually correct for FORMAT_LA8. When write_alpha == true then it will write out two bytes, the code was a bit hard to follow but it looks like it is actually correct. So the question is for converting to FORMAT_L8, should we instead premultiply alpha in the convert code, add a new parameter in convert for that option, default to false? Regarding my PR though, it is set to multiply the alpha ONLY in the case where write_gray is true, read_alpha is true, and write_alpha is false.

Anyhow, the behavior shown in the attached issue is not expected whatsoever and I think we can all agree on that.

[clayjohn]

The screenshots you post in the issue look correct for images with fix alpha border applied. What fix alpha border does is extend the colors from the border of a sprite into the clear areas so that you get proper alpha blending when using linear filtering. Without it you get black ringing around sprites because the transparent pixels are (0,0,0,1).

If you don't utilize the alpha channel, then it doesn't make sense to apply fix alpha border as it does overwrite the color channels (but only where alpha is 0).

In your MRP, you can get the correct output by checking "Premultiply Alpha" and unchecking fix alpha border

Of course your switch to REC.709 is more accurate as this screenshot is taken with the old grayscale conversion code.

In my opinion, the "correct" fix here is to ensure that premultiply_alpha works with LA8 (not just RGBA8) and to remove the premultiplication this PR adds in convert. I don't think that convert should be automatically applying a premultiplication, that should only be done if the user requests it, otherwise you take options away from the user

Here is a choice matrix that illustrates my concern

	User applies premultiplied alpha	User does not apply premultiplied alpha
convert premultiplies	color is premultiplied	color is premultiplied
convert does not premultiply	color is premultiplied	color is not premultiplied

If you automatically premultiply in convert, then the user has no choice if they don't want the image to be premultiplied, however, if you don't apply a premultiplication in convert, then the user can still choose to premultiply if they want a premultiplied image.

I dont think we should assume that every user wants a premultiplied image when converting from RGBA8 to L8 or LA8 to L8

[korypostma]

The MRP was created only to show the issue. I ran into this issue because of the SpriteFramesEditor code when it loads a spritesheet I was working on code to autoslice the spritesheet and needed to generate a binary image from a grayscale. When I used the convert function I was getting that weird output and was unable to generate contours because of the fix_alpha_edges code.

So I think that fix_alpha_edges should not apply when not using Linear filtering. While I agree that we should give the users options as to the behavior they want, we shouldn't make their life more difficult by forcing them to premultiply alphas because fix_alpha_edges messed up their imported RGBA image. The image I'm importing is actually a palette based RGB image and I would expect when converting a palette RGB to L8 that I would get a true grayscale representation of that image and not the mess that is seen due to the fix_alpha_edges code.

So, where is the true fix here?

1. Fix alpha edges the right way to avoid this issue?
2. Allow the choice in convert to pre-multiply alphas?
3. Leave this PR as-is because it produces expected behavior when converting RGB to L8?

[clayjohn]

I think I am missing something here. Why do you insist on enabling fix_alpha_edges if you do not want Godot to apply the edge dilation?

The problem is not with fix_alpha_edges, you are premultiplying by alpha in order to undo the changes made by fix_alpha_edges, but why can't you just disable fix_alpha_edges in the first place?

[korypostma]

fix_alpha_edges is on by default for all RGBA images and all RGB/RGBA images being loaded by the importer automatically get it applied by default. Because I'm developing a tool that will be used with default settings, the fix_alpha_edges code is breaking grayscale conversions. Sorry, I tried to explain this all in the issue. I, at first, thought the issue was with that code but then thinking about it more, the issue is actually that code and with the grayscale conversion. It is actually a problem with both of them. Plus, you saw the old TODO about not following a proper grayscale conversion standard so I thought the best place was to fix it there and apply the alpha there as well because that is generally what you do when you convert RGBA to grayscale, you apply the alpha values as well since the format you are converting to has no alpha channel.

So long story short, because I'm making a tool that should work with default settings both fix_alpha_edges and _convert are problematic. Actually, in my tool code I ended up making my own grayscale converter because of this issue, at least until this PR goes through.

[korypostma]

@clayjohn and I spoke over DM in RocketChat rather than here. clayjohn recommends removing the alpha multiplication for 8-bit grayscale (FORMAT_L8) and instead have users premultiply_alpha first if they want a clean grayscale image. If others agree with him here then I will modify the PR to do that and I will also unlink my issue (because this PR would no longer be a fix for my issue).

We both agree that the current grayscale method is wrong.
We both agree that fix_alpha_edges causes color bleeding and issues but it is necessary when using Linear filtering.
We both agree that modifications to core should be specific fixes to issues and not work-arounds for convenience.

@clayjohn feel free to add anything else you can think of and let me know what you guys would want me to do.

[korypostma]

I had to correct the premultiply_alpha function because it was returning the wrong values and I spotted immediately that the old code was not adding an extra 255 to the result before shifting. This newer updated one is now correct and the test-case is now valid and passing. The multiplication of the alpha value has been removed from the _convert function.

[clayjohn]

The current set of changes seems great to me! Thanks for bearing with me and being open to feedback.

[YuriSizov]

Thanks! And congrats on your first merged Godot PR!

[anvilfolk]

Yayyyyy! Congratulations on your first PR! Look at you, with a fancy new contributor tag 🥳 🥳 🥳

I didn't have a ton to contribute to the discussion, but followed this and I was so appreciative of how everyone engaged with the changes in opinion for merging, which is always awkward. Wonderfully handled, love to see it ❤️

[korypostma]

Thanks everyone and I hope to make many more contributions in the future. I had many growing pains with this first one, and it is good to learn the process. Take care and thanks again!