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 a5c3bbdcb08fc29700119c1625892519da73b39a..81613e69fba3c602ad9116b4b66e6f0cc8a239c5 100644 (file)
--- 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 50d004db1c52c1192946b87add1b785149f585fc..7b86d1601467f8e302763898aaf3b8bfc9d4f268 100644 (file)
--- 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