the RGB values of transparent pixels are indeterminate

This is very much an assumption. A very good one that is true in many cases, but an assumption nonetheless. Whether the color of full transparent pixels matters very much depends on the current use case.

That being said, if the color of fully transparent pixels is undefined, then the current implementation of Average Color Fix (ACF) is indeed incorrect. But I am not convinced that your fix is correct either.

Some background: ACF is supposed to be a post-process clean-up step after upscaling an image with an AI model. AI models tend to not be too accurate with color while upscaling. Models adding certain tints to upscaled images is a common issue, and this node is supposed to fix it. To make the node easier to use, I choose to ignore to leave the alpha of the input image unchanged. This is because the input image might have an alpha channel while the reference does not, or vise versa. So it doesn't make sense to fix the alpha anyway.

So how should we deal with fully transparent pixels? I think there is not one answer to this question - there are two. If the color of fully transparent pixels is defined, then the current implementation is correct. If the color of fully transparent pixels is undefined, then we need to do something. Since we can't know this just by looking at pixel data, we have to let the user tell us which one it is. So let's add a "Separate Alpha" option (checkbox/bool). If the option is enabled, then the color of fully transparent pixels is defined and we use the current implementation. If not, then we use the new implementation.
(The name "Separate Alpha" is taken from the Save Image node. When creating mip maps for DDS images, we have to downscale the image and there we have the exact same issue. We need to use alpah-corrected downscaling to get correct colors in the mips, but some game engines use the alpha channel to store other information and not transparency, so we also have an option to opt out of alpha-corrected downscaling.)

Now, what to do when we don't separate alpha and have fully transparent pixels with undefined color: Ideally, we want the following (I think):

• If a pixel is fully transparent in the input image, it shouldn't affect other pixels in the input image.
• If a pixel is fully transparent in the reference image, it shouldn't affect pixels in the input image.

Frankly, I'm not sure how we could do this. There is also the issue that the output isn't defined when the input image has an opaque pixel at a position where the reference image has a fully transparent pixel.

What do you think?

Thanks for the analysis!

So it doesn't make sense to fix the alpha anyway.

For what it's worth, I've seen at least one ESRGAN model that causes opacity of the background to increase from 0% to around 3%. I haven't tested to see how well ACF's algorithm works to fix that, but there are definitely legit use cases for correcting the alpha channel.

So let's add a "Separate Alpha" option (checkbox/bool).

Sounds reasonable.

Frankly, I'm not sure how we could do this.

On a theoretical basis, I think my implementation is an improvement over the status quo (for the case where Separate Alpha is False), since the only differences in behavior are that it throws away indeterminate data rather than trying to use it. On a practical basis, there was a specific image I was working with (from the wild, not a contrived case) that gave me major halo-effect issues with the status quo implementation (which is why I wrote this PR); with the implementation in this PR, I can't see any issues with it by eye. I suppose it's entirely possible that we could get visually better results by guessing values for the indeterminate inputs rather than throwing them away, but I suspect we'll very quickly run into diminishing returns, and analyzing such approaches is pretty far outside my area of competence.

For what it's worth, I've seen at least one ESRGAN model that causes opacity of the background to increase from 0% to around 3%.

That's not really something ACF would truly fix anyway. Rather than bringing it down to 0%, it would bring it down to 1% in some places and 0% in others.

Fixing alpha is pretty difficult. I don't think ACF is up for the task, nor do I think that it should that that anyway.

I think my implementation is an improvement

Then let's split ACF into 2 methods with Separate Alpha and use yours for the Separate Alpha=false case.

The 2 methods should probably be separate functions as well.

For what it's worth, I've seen at least one ESRGAN model that causes opacity of the background to increase from 0% to around 3%

In this case I think the threshold node is better suited for fixing the alpha channel.

Sounds good, I'll make the behavior configurable as requested. It might take a few days, I'm mildly busy this week.

Added "Separate Alpha" option as requested; sorry this took me so long.

Sorry for the delay. I just tested it a little and average_color_fix_alpha_aware does not handle pixels around transparent edges well. Discolorations are still clearly present.

In my testing, I used the following chain to give the upscaled image a red tint. Adding a color to the image is the type of discoloration ACF should be able to 100% fix.
Unfortunately, neither the old method nor the new method are doing this correctly for image with alpha. While the old method unnaturally brightens edges, the new method simply doesn't remove their tint.

Original (untinted)	Old method	New method

I'm not sure how to fix this.

Chain:

Interesting, thanks for testing @RunDevelopment. I'll do some experiments with your sample image over the next few days and see if I can make any improvements.

@RunDevelopment Maybe I'm missing something, but I'm a little confused about what images you're looking at. Your chain screenshot assigns the "Old method" label to Separate Alpha False, and the "Separate Alpha" label to Separate Alpha True. Your table then describes the "Separate Alpha" image as "New method". This doesn't make sense to me, since setting Separate Alpha to True yields the behavior from before this PR, while setting it to False yields the new behavior (which I think is consistent with the Save Image node's semantics).

(Sorry it took so long for me to reply.)

Rebased to fix merge conflict.

@RunDevelopment @Splendide-Imaginarius what's the status on this? Is it in a finished enough state, or does it need more work?

@joeyballentine It works fine for me and I'm using it in production, but it sounds like @RunDevelopment had some concerns. I'm not able to reproduce the issue that @RunDevelopment described, pending some clarification on how he tested it (it looks to me like his test chain may have swapped the before-this-PR and after-this-PR state).

So, I guess wait for @RunDevelopment to reply?

Sorry for the late reply. I actually tested my chain again 2 days ago, but forgot to reply that day....

So firstly, you were 100% correct. I messed up in my testing. The old method is completely unusable for RGBA images, so let's always use your method for them. In fact, your method produced the same result for RGB images in my testing as well (which is a little surprising since PIL downscaling is gamma-corrected AFAIK, while OpenCV's is not). So let's always always use your method.

This is exactly what you had at first, so I'm sorry for making you do all that extra work for nothing, all because of my mistake.

The only thing I'm still a bit worried about is alpha fixing. It doesn't really make sense to me (applying the invalid mask on alpha just doesn't feel right), but it's also not bad, so let keep it. If we ever find a better way to do this, we can change it in the future.

No worries @RunDevelopment, I learned some stuff while revising this PR (both about image processing and chaiNNer internals), so it's not time wasted. :) So do you want me to force-push to this PR, removing 6c04f41 ?

So do you want me to force-push to this PR, removing 6c04f41 ?

Maybe? I think so? Force pushes make it hard for me to understand your commit history, but I think that's the one.

OK, I'll just do a standard revert commit rather than a force-push.