Fix blending math #169
Fix blending math #169
Conversation
The blending math had two errors: first, colors were not separated for the purpose of blending (blending was wrongly applied to premultiplied values), and second, alpha was applied over-aggressively to the alpha channel. This PR does *not* address the issue of gamma correctness. That is a complex issue and should probably be handled in the short term by disabling sRGB conversions and doing the internal math in sRGB color space rather than linear. This will degrade the quality of antialiasing but on the other hand give spec-compliant results for compositing. We remove the plus-darker mode as its specification does not appear to be valid. The plus-lighter mode remains as it is quite useful for cross-fading effects. Also the generated shaders were compiled on mac so the DXIL is unsigned. Those should be compiled on Windows before this PR is merged. (and we should figure out a better strategy for all that)
|
Another note: the normal + src_over mode is now special-cased with an optimized implementation, but there are other combinations where the math to un-premultiply could be optimized out. I'm noodling on that now but I would consider it a fairly low priority, as I believe the code as written gives correct results (modulo the gamma issue mentioned above) and I suspect the performance difference is not massive. |
Adds a test to visualize the blend modes. Fixes a dumb bug in blend.h and also a more subtle issue where default blending is not the same as clipping, as the former needs to always push a blend group (to cause isolation) and the latter does not. This might be something we need to get back to. This should fix the rendering, so it fairly closely resembles the Mozilla reference image. There's also a compile-time switch to disable sRGB conversion, which is (sadly) needed for compatible rendering.
Updates DXIL on generated shaders.
dfrg
left a comment
dfrg
left a comment
There was a problem hiding this comment.
This all looks good to me. The scene crate will need to be updated to support the new clip mode but that's an easy fix. Just to clarify: this mode was added to provide two versions of normal/source-over that are differentiated by whether they cull tiles? Presumably the non-culling version is used to establish an isolated tile set on the blend stacks.
|
Yes, this distinction wasn't at all clear to me before digging into the artifacts, and is probably imperfectly explained now, as well as incomplete in the scene crate (I didn't dig into that but am happy to make changes before committing this). If you're not doing blending, then doing begin + end with normal + src_over + 100% alpha (and this reminds me that multiplying by blend group alpha is an enhancement we'll want soon, though it's not especially relevant to COLRv1 rendering) is equivalent to no-op, ie just rendering the children with no extra blend operations. But if the children contain any blending, then the two are not equivalent, as the blend leaves the top-of-stack rgb value as clear, while the no-op does not. The former is the correct operation to make sure the blend group is isolated. I also believe that non-isolated blending is fairly straightforward, you just elide that operation, but I didn't test it in detail because again it's not relevant for COLRv1. |
|
It looks like I can commit this then you'll take care of the fixups to the scene crate in #171? Possibly some merging will be needed. |
|
Good to merge! I just pushed a commit to add the clip mode in #171. If there are conflicts after this merge I’ll correct them (looks like rustfmt may have touched some of those files). |
The blending math had two errors: first, colors were not separated for the purpose of blending (blending was wrongly applied to premultiplied values), and second, alpha was applied over-aggressively to the alpha channel.
This PR does not address the issue of gamma correctness. That is a complex issue and should probably be handled in the short term by disabling sRGB conversions and doing the internal math in sRGB color space rather than linear. This will degrade the quality of antialiasing but on the other hand give spec-compliant results for compositing.
We remove the plus-darker mode as its specification does not appear to be valid. The plus-lighter mode remains as it is quite useful for cross-fading effects.
Also the generated shaders were compiled on mac so the DXIL is unsigned. Those should be compiled on Windows before this PR is merged. (and we should figure out a better strategy for all that)