Fix alpha in PorterDuffImageComposite #3411
Conversation
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.
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 |
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.