From: Steinar H. Gunderson <sgunderson@bigfoot.com>
Date: Tue, 3 Mar 2015 22:03:54 +0000 (+0100)
Subject: Use thread-local locale.
X-Git-Tag: 1.1.3~4
X-Git-Url: https://git.sesse.net/?p=movit;a=commitdiff_plain;h=401c6ca91cf582904cf38d6bfe1d82a073eff559

Use thread-local locale.

setlocale() affects the whole process, not just the current thread
as I assumed; uselocale() (available since glibc 2.3, so basically
forever) is per-thread, and also conveniently seems to avoid the
issue of the returned pointer being destroyed (unless the driver
uses the return value of uselocale() as a base, which I really hope
it doesn't).

I'm slightly worried that since this overrides setlocale(), buggy drivers
might get confused when they try to do setlocale() and something else
overrides that precedence, but hopefully this shouldn't be a case.

Also add a unit test for locale handling while we're at it. It doesn't
test multi-threaded behavior, though, only the simple case.

Reported by Christophe Thommeret.
---

diff --git a/effect_chain.cpp b/effect_chain.cpp
index a5c3bbd..81613e6 100644
--- a/effect_chain.cpp
+++ b/effect_chain.cpp
@@ -1332,11 +1332,8 @@ void EffectChain::finalize()
 {
 	// Save the current locale, and set it to C, so that we can output decimal
 	// numbers with printf and be sure to get them in the format mandated by GLSL.
-	// Note that the OpenGL driver might call setlocale() behind-the-scenes,
-	// and that might corrupt the returned pointer, so we need to take our own
-	// copy of it here.
-	char *saved_locale = strdup(setlocale(LC_NUMERIC, NULL));
-	setlocale(LC_NUMERIC, "C");
+	locale_t c_locale = newlocale(LC_NUMERIC_MASK, "C", (locale_t)0);
+	locale_t saved_locale = uselocale(c_locale);
 
 	// Output the graph as it is before we do any conversions on it.
 	output_dot("step0-start.dot");
@@ -1400,8 +1397,8 @@ void EffectChain::finalize()
 	assert(phases[0]->inputs.empty());
 	
 	finalized = true;
-	setlocale(LC_NUMERIC, saved_locale);
-	free(saved_locale);
+	uselocale(saved_locale);
+	freelocale(c_locale);
 }
 
 void EffectChain::render_to_fbo(GLuint dest_fbo, unsigned width, unsigned height)
diff --git a/effect_chain_test.cpp b/effect_chain_test.cpp
index 50d004d..7b86d16 100644
--- a/effect_chain_test.cpp
+++ b/effect_chain_test.cpp
@@ -1041,4 +1041,41 @@ TEST(EffectChainTest, IdentityWithOwnPool) {
 	movit_debug_level = MOVIT_DEBUG_OFF;
 }
 
+// A dummy effect whose only purpose is to test sprintf decimal behavior.
+class PrintfingBlueEffect : public Effect {
+public:
+	PrintfingBlueEffect() {}
+	virtual string effect_type_id() const { return "PrintfingBlueEffect"; }
+	string output_fragment_shader() {
+		char buf[256];
+		snprintf(buf, sizeof(buf), "vec4 FUNCNAME(vec2 tc) { return vec4(%f, %f, %f, %f); }\n",
+			0.0f, 0.0f, 1.0f, 1.0f);
+		return buf;
+	}
+};
+
+TEST(EffectChainTest, LocaleIsIgnoredDuringFinalize) {
+	// An example of a locale with comma instead of period as decimal separator.
+	// Obviously, if you run on a machine without this locale available,
+	// the test will always succeed. Note that the OpenGL driver might call
+	// setlocale() behind-the-scenes, and that might corrupt the returned
+	// pointer, so we need to take our own copy of it here.
+	char *saved_locale = strdup(setlocale(LC_ALL, "nb_NO.UTF_8"));
+	float data[] = {
+		0.0f, 0.0f, 0.0f, 0.0f,
+	};
+	float expected_data[] = {
+		0.0f, 0.0f, 1.0f, 1.0f,
+	};
+	float out_data[4];
+	EffectChainTester tester(data, 1, 1, FORMAT_RGBA_PREMULTIPLIED_ALPHA, COLORSPACE_sRGB, GAMMA_LINEAR);
+	tester.get_chain()->add_effect(new PrintfingBlueEffect());
+	tester.run(out_data, GL_RGBA, COLORSPACE_sRGB, GAMMA_LINEAR);
+
+	expect_equal(expected_data, out_data, 4, 1);
+
+	setlocale(LC_ALL, saved_locale);
+	free(saved_locale);
+}
+
 }  // namespace movit