Fix alpha in PorterDuffImageComposite #3411
Conversation
There was a problem hiding this comment.
Code looks good, alpha premultiplication is required by spec for the non-blending PorterDuff functions.
However, I question the assumption that the input is a mask that needs alpha conversion as the inputs are labeled source_alpha and destination_alpha in the node.
That to me implies that the input should have already been converted to alpha before being passed into the node (especially as PorterDuff are alpha based functions).
There is probably merit in the assumption that most people are passing non alpha converted mask to the node and getting unexpected results, but that is an incorrect use issue.
|
@shawnington Thank you for checking it out!
Please see the commit c16f574 where @comfyanonymous fixed the same issue for two other nodes. I think the author who added these 3 nodes ( The quick experiment that you can do to verify this is to load two transparent images and run But in current version you will see a very strange result. If I'm not mistaken it will look something like this: opaque areas will become invisible, invisible ares will still be invisible and only half-transparent areas will be visible. This is clearly not how the composite should behave. Inverting mask, on other other hand, does the right thing. |
Yes, the PorterDuffImageComposite node does need the alpha premultiplication fix you committed to work properly. I actually checked every function in the node to make sure the math needed the alpha pre-multiplication, and you are 100% correct that its required for proper function of the node, and the node does not currently function properly. I also looked at those commits before I commented. The first commit to split_image_with_alpha is good, the intended output is alpha, so alpha conversion makes sense, and seems quite correct. the commit to JoinImageWithAlpha is a bug that was introduced probably unintentionally and actually demonstrate the problem I am discussing in a very concrete way. If you take the output of split_image_with_alpha and put it into JoinImageWithAlpha, you will not get expected behavior, because JoingImageWithAlpha will invert the alpha. Alpha has a very specific meaning in terms of imaging, and yes its basically just the mask inverted, but it being in its inverted state is essential to the math working properly with a proper input. Anything specifying alpha as an input should not do alpha conversion on the alpha input. This simply inverts proper input. As you pointed out, if you input a mask, the output is the opposite of what you expected, because the specified input is alpha. If you convert the mask to alpha before input into the node, you will get expected behavior, well not quite because of the properly identified alpha premultiplication bug, but closer. I understand that people are confusing them and some nodes convert improperly and some dont. However with a library of this size, there should definitely be an effort to adhere to terminology, and associated behavior, just as you properly identified that PorterDuff does specify alpha pre-multiplication. Standard adherence makes everyone's life easier. Something that specifies alpha as input, should not do alpha conversion. Im perfectly fine with you changing the input to mask, and doing alpha conversion just as you proposed. I however feel strongly that if input is labeled as alpha, it should behave as expected when given alpha as input. |
Okay, I see. But renaming the field will likely break serialization for that node, right? I think having name inconsistency is slightly better than breaking existing workflows. |
Yeah probably, which is why I would rather alpha be treated as alpha. I think we can agree its easier for other people that want to work with the nodes and with the code, if the indicated input type is treated as the indicated input type. Thats my only issue with your commit. The output doesn't indicate alpha out even if the variable is called out_alpha, its labeled mask in the node, so your output conversion back to mask, 100% acceptable, it does whats expected. The input is labeled alpha in the node though, so conversion is inappropriate. Good job identifying the lack of premultiliplied alpha though, reviewing the code was one of the longest code reviews Ive ever done, checking every PorterDuff function, and all the blending mode math to make sure that premultiplied alpha was indeed correct. Very solid commit, once again my only issue is not treating the input as the indicated type. |
|
|
|
Hmm, strange, maybe I'm missing |
The banding is also present in the output mask ( and Im presuming the input mask ). is placing zeros into the image where the mask is full density. The area that is not replaced with zeros indicates the alpha pre multiplication is not working properly for some reason, as that area should not be present in the out_image after the multiply function. |
|
Maybe changing the condition to |
Im assuming it's just done to avoid a divide by zero error for out_image / out_alpha. Is that correct? If so, the error is not in this line. That line is just obfuscating what is happening. Assuming the masked area is black or 0 for the alpha pre-multiplication. src_img = src_img * src_alpha should result in areas that have values of zero now where the src_alpha was zero. out_image = src_image * dst_image should result in 0's in the area that is masked before undoing alpha premultiplication. But it appears to be getting multiplied as if the alpha premultiplication didn't happen at all. Im not sure why, I just re-checked the math. It should be working correctly. |
after reviewing again, shouldn't this actually be: It's a divide by zero check, out_image / 0 != 0, shouldn't it just be getting the value from out_image, or am I missing something? |
Yes, it's for div by zero check. This change was made two weeks ago, so I don't remember exactly why zeros are important. But I think I was getting some strange output without this 🤔 |
dennwc
force-pushed
the
fix_composite
branch
2 times, most recently
from
5ed59f6
to
299a668
Compare
There were two bugs in PorterDuffImageComposite. The first one is the fact that it uses the mask input directly as alpha, missing the conversion (`1-a`). The fix is similar to c16f574. The second one is that all color composition formulas assume alpha premultiplied values, while the input is not premultiplied. This change fixes both of these issue.
|
Indeed, switching from |
|
TBH porter_duff_composite() was meant to work exclusively on alphas, as that's how this method was originally designed (#1577 + back in 1984 - no single mention of "mask" concept in the original paper, just working on alpha channel). So personally I'd rather keep any kind of alpha <-> mask conversions (side effect of ComfyUI design choices) out of this function (it should be like mathematically clean, not influenced by app-level issues). |
|
Since pairing nodes were changed to work with alphas, I think it made sense to update the composite too. It just doesn't make sense to but InvertMask every time. |
|
@dennwc The porter_duff_composite() I mentioned earlier was implemented in such a way it could be for example moved to external graphics library (knowing nothing about ComfyUI working on inverted alpha channel as mask) - and now it's kinda "contaminated" with app-specific logic. That invertion could be made for example in more ComfyUI-specific PorterDuffImageComposite.composite(), instead. But taking into account this file belongs to ComfyUI, and whole idea of using both alphas and mask as the same type is already pretty messy - not really a big deal. It just bugged me a bit from code aesthetics point of view 🙃. |
I agree with you, Mask and Alpha are two different things, it's very messy to use the terms interchangeably in an image generation library. |
There were two bugs in PorterDuffImageComposite.
The first one is the fact that it uses the mask input directly as alpha, missing the conversion (
1-a). The fix is similar to c16f574.The second one is that all color composition formulas assume alpha premultiplied values, while the input is not premultiplied.
This change fixes both of these issue.