Clamp alpha in MixEffect.

This struck me suddenly one evening as a fix for the issues with alpha in glow,
and seems to work incredibly well for unsharp mask, too. The rationale is
outlined in the comment in the frag.

I'll probably be adding some tests for GlowEffect to make sure that it keeps
working fine, but visual tests so far look very good. The only issue is that
with destination alpha always being in linear light (as it should be),
programs that don't blend in linear light, like GIMP and ImageMagick,
get somewhat dull-looking results with e.g. glow.

diff --git a/mix_effect.frag b/mix_effect.frag
index f76881ba2519012fae38f65f3981f10ed009c6d4..450ca21f5ed285f349f330f431740fb6d75785b0 100644 (file)
--- a/mix_effect.frag
+++ b/mix_effect.frag
@@ -1,5 +1,33 @@
 vec4 FUNCNAME(vec2 tc) {
        vec4 first = INPUT1(tc);
        vec4 second = INPUT2(tc);
 vec4 FUNCNAME(vec2 tc) {
        vec4 first = INPUT1(tc);
        vec4 second = INPUT2(tc);

-       return vec4(PREFIX(strength_first)) * first + vec4(PREFIX(strength_second)) * second;
+       vec4 result = vec4(PREFIX(strength_first)) * first + vec4(PREFIX(strength_second)) * second;
+
+       // Clamping alpha at some stage, either here or in AlphaDivisionEffect,
+       // is actually very important for some use cases. Consider, for instance,
+       // the case where we have additive blending (strength_first = strength_second = 1),
+       // and add two 50% gray 100% opaque (0.5, 0.5, 0.5, 1.0) pixels. Without
+       // alpha clamping, we'd get (1.0, 1.0, 1.0, 2.0), which would then in
+       // conversion to postmultiplied be divided back to (0.5, 0.5, 0.5)!
+       // Clamping alpha to 1.0 fixes the problem, and we get the expected result
+       // of (1.0, 1.0, 1.0). Similarly, adding (0.5, 0.5, 0.5, 0.5) to itself
+       // yields (1.0, 1.0, 1.0, 1.0) (100% white 100% opaque), which makes sense.
+       //
+       // The classic way of doing additive blending with premultiplied alpha
+       // is to give the additive component alpha=0, but this also doesn't make
+       // sense in a world where we could end up postmultiplied; just consider
+       // the case where we have first=(0, 0, 0, 0) (ie., completely transparent)
+       // and second=(0.5, 0.5, 0.5, 0.5) (ie., white at 50% opacity).
+       // Zeroing out the alpha of second would yield (0.5, 0.5, 0.5, 0.0),
+       // which has undefined RGB values in postmultiplied storage; certainly
+       // e.g. (0, 0, 0, 0) would not be an expected output. Also, it would
+       // break the expectation that A+B = B+A.
+       //
+       // Note that we do _not_ clamp RGB, since it might be useful to have
+       // out-of-gamut colors. We could choose to do the alpha clamping in
+       // AlphaDivisionEffect instead, though; I haven't thought a lot about
+       // if that would be better or not.
+       result.a = clamp(result.a, 0.0, 1.0);
+
+       return result;

 }
 }

diff --git a/mix_effect_test.cpp b/mix_effect_test.cpp
index d49efd754b56a964ac171f2cd9b572f4b28c2f5f..096ecd5ae03169ee5e9052bfe2207a0140cc6f53 100644 (file)
--- a/mix_effect_test.cpp
+++ b/mix_effect_test.cpp
@@ -80,6 +80,34 @@ TEST(MixEffectTest, DoesNotSumToOne) {
        expect_equal(expected_data, out_data, 4, 1);
 }
 
        expect_equal(expected_data, out_data, 4, 1);
 }
 

+TEST(MixEffectTest, AdditiveBlendingWorksForBothTotallyOpaqueAndPartiallyTranslucent) {
+       float data_a[] = {
+               0.0f, 0.5f, 0.75f, 1.0f,
+               1.0f, 1.0f, 1.0f, 0.2f,
+       };
+       float data_b[] = {
+               1.0f, 0.25f, 0.15f, 1.0f,
+               1.0f, 1.0f, 1.0f, 0.5f,
+       };
+
+       float expected_data[] = {
+               1.0f, 0.75f, 0.9f, 1.0f,
+               1.0f, 1.0f, 1.0f, 0.7f,
+       };
+
+       float out_data[4];
+       EffectChainTester tester(data_a, 1, 2, FORMAT_RGBA_POSTMULTIPLIED_ALPHA, COLORSPACE_sRGB, GAMMA_LINEAR);
+       Effect *input1 = tester.get_chain()->last_added_effect();
+       Effect *input2 = tester.add_input(data_b, FORMAT_RGBA_POSTMULTIPLIED_ALPHA, COLORSPACE_sRGB, GAMMA_LINEAR);
+
+       Effect *mix_effect = tester.get_chain()->add_effect(new MixEffect(), input1, input2);
+       ASSERT_TRUE(mix_effect->set_float("strength_first", 1.0f));
+       ASSERT_TRUE(mix_effect->set_float("strength_second", 1.0f));
+       tester.run(out_data, GL_RGBA, COLORSPACE_sRGB, GAMMA_LINEAR);
+
+       expect_equal(expected_data, out_data, 4, 2);
+}
+

 TEST(MixEffectTest, MixesLinearlyDespitesRGBInputsAndOutputs) {
        float data_a[] = {
                0.0f, 0.25f,
 TEST(MixEffectTest, MixesLinearlyDespitesRGBInputsAndOutputs) {
        float data_a[] = {
                0.0f, 0.25f,