From 401c6ca91cf582904cf38d6bfe1d82a073eff559 Mon Sep 17 00:00:00 2001 From: "Steinar H. Gunderson" Date: Tue, 3 Mar 2015 23:03:54 +0100 Subject: [PATCH 1/1] 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. --- effect_chain.cpp | 11 ++++------- effect_chain_test.cpp | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 7 deletions(-) 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 -- 2.39.5