From 6a8b7d750dff76dee320ad973a52d4d9720510b9 Mon Sep 17 00:00:00 2001
From: "Steinar H. Gunderson" <sgunderson@bigfoot.com>
Date: Sat, 19 Jan 2013 20:13:51 +0100
Subject: [PATCH 1/1] 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.
---
 mix_effect.frag     | 30 +++++++++++++++++++++++++++++-
 mix_effect_test.cpp | 28 ++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/mix_effect.frag b/mix_effect.frag
index f76881b..450ca21 100644
--- a/mix_effect.frag
+++ b/mix_effect.frag
@@ -1,5 +1,33 @@
 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 d49efd7..096ecd5 100644
--- 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);
 }
 
+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,
-- 
2.39.2