Ensure the parsing works properly with different locales by diegonieto · Pull Request #1430 · Netflix/vmaf

diegonieto · 2025-07-21T10:38:07Z

The library had locale-dependent numerical parsing/writing issues where systems using comma (,) as decimal separator (e.g., European locales like de_DE, fr_FR) instead of period (.) would produce incorrect JSON model files and output files.

Solution:
Implemented a cross-platform abstraction layer for thread-safe locale handling that ensures consistent numeric formatting (period as decimal separator) across all platforms while being thread-safe and not affecting other parts of the application.

Platform-specific implementations:

Linux/BSD/macOS: Uses POSIX.1-2008 uselocale() with LC_ALL_MASK
Windows: Uses _configthreadlocale() + setlocale()
Fallback: Graceful degradation for platforms without thread-local locale

diegonieto · 2025-09-30T13:33:19Z

Hi @kylophone , do you think it is possible to have a review of this one?

sdroege · 2025-12-02T18:43:09Z

This is not a good idea. It changes process-global state, is not thread-safe (setlocale() is MT-Unsafe const) and will temporarily break localization in other threads.

I assume this is necessary because vmaf is using C string parsing functions, which are locale-dependent? The correct solution here would be to use locale-independent parsing functions.

rgonzalezfluendo · 2025-12-02T18:55:31Z

Good point @sdroege. IIRC, the main issue is that json_get_number uses strtod, which is locale-dependent.

Idea: using strtod_l instead of strtod.

sdroege · 2025-12-02T18:56:05Z

Similarly, this then probably also affects writing of JSON numbers?

rgonzalezfluendo · 2025-12-02T18:59:13Z

FYI: Current implementation of svm_save_model is also calling setlocale(LC_ALL, "C");

https://github.com/Netflix/vmaf/blob/master/libvmaf/src/svm.cpp#L2651-L2660

Mr-Clam · 2025-12-02T19:32:39Z

    memset(m->score_transform.knots.list, 0, knots_sz);

-    return model_parse(s, m, cfg->flags);
+    char *old_locale = setlocale(LC_NUMERIC, NULL);


As @sdroege sdroege said, setlocale changes the locale for the whole process, so it's probably not good for a library to invoke this function. It's good that it is restored later via old_locale, but there still a period that another thread in the process could behave unexpectedly. setlocale isn't thread safe either.

Perhaps it's better to use uselocale instead because it only affects the current thread? Maybe svm.cpp should be revised too.

I wasn't aware of uselocale(). How portable is that?

That would seem like a good option for a fast fix.

I've just pushed the changes with uselocale() for both read_json_model.c and svm.cpp

At least vmaf_write_output_json() is also affected, probably CSV and XML too.

I don't know if there's more string formatting / parsing code elsewhere. Please check

I've added:

vmaf_write_output_xml()

vmaf_write_output_json()

vmaf_write_output_csv()

vmaf_write_output_sub()

I think with these ones we are good to go. Does this sounds good to you @Mr-Clam ?

I wasn't aware of uselocale(). How portable is that?

To answer my own question: it doesn't seem to be available at least on Windows and macOS. It's supposed to be POSIX.1-2008

I'm sorry, you're right: I can't find it in the Microsoft documentation of their C library, nor in the headers from MSVC 2022 or Windows SDK. It is available on my Mac though via xlocale.h (present on my system in Xcode 14 and 26) for macOS and other iDevices (e.g. see this man page.)

On Windows, you can make setlocale thread local by using _configthreadlocale. So the best I can offer after all is to do one thing on Windows and another on others.

sdroege · 2025-12-02T19:37:39Z

Idea: using strtod_l instead of strtod.

That's unfortunately not very portable. It exists on most platforms in some form (Windows has it was a leading underscore), but AFAIU it's not standard C.

Also, it's not just floating point formatting/parsing that is locale-dependent. It's just the most obvious one because many European languages use , instead of . for the decimal separator.

sdroege · 2025-12-03T09:38:05Z

    memset(m->score_transform.knots.list, 0, knots_sz);

-    return model_parse(s, m, cfg->flags);
+    locale_t c_locale = newlocale(LC_NUMERIC_MASK, "C", NULL);


You probably want to use LC_ALL here for not getting any surprises later

true, I've updated all of them. Thanks

kylophone · 2025-12-03T17:52:24Z

If possible, could a test be added for this?

Problem: The library had locale-dependent numerical parsing/writing issues where systems using comma (,) as decimal separator (e.g., European locales like de_DE, fr_FR) instead of period (.) would produce incorrect JSON model files and output files. Solution: Implemented a cross-platform abstraction layer for thread-safe locale handling that ensures consistent numeric formatting (period as decimal separator) across all platforms while being thread-safe and not affecting other parts of the application. Platform-specific implementations: - Linux/BSD/macOS: Uses POSIX.1-2008 uselocale() with LC_ALL_MASK - Windows: Uses _configthreadlocale() + setlocale() - Fallback: Graceful degradation for platforms without thread-local locale Changes: - libvmaf/src/thread_locale.h: New abstraction layer API - libvmaf/src/thread_locale.c: Platform-specific implementations - libvmaf/src/meson.build: Build system integration - libvmaf/src/svm.cpp: Update numeric I/O - libvmaf/src/read_json_model.c: Update JSON model parsing - libvmaf/src/output.c: Update all output functions - libvmaf/test/test_locale_handling.c: Test suite - libvmaf/test/meson.build: Test integration This ensures consistent numeric formatting across all platforms and locales while maintaining full thread-safety and backward compatibility.

diegonieto · 2025-12-09T18:55:15Z

Just updated the PR covering compatibility across platforms, and added a test to ensure the new functionality works as expected

lusoris · 2026-10-01T17:30:51Z

Tested at 054a97e merged onto master 6ec23e8 (the two meson.build conflicts are adjacent additions to the source and test lists; keeping both resolves them). x86-64 Linux, gcc 16.2.1 (with -Dc_args=-Wno-error=incompatible-pointer-types, #1630): release meson test 24/24; ASan+UBSan 22 pass, test_predict and test_pic_preallocation fail under LeakSanitizer exactly as on unpatched master.

The bug and the fix reproduce. To get a comma locale into the unmodified CLI I preloaded a four-line library that calls setlocale(LC_ALL, "de_DE.utf8") before main, and scored src01 for 4 frames with --model version=vmaf_v0.6.1 --threads 8:

master, --json: "vmaf": 100,000000 for all four frames (83.86, 82.64, 81.04, 82.20 in the C-locale run) and 145 lines with comma decimals; --xml 20 lines, --csv 4 lines
this PR, --json, --xml, --csv: output identical to the C-locale run (fps is the only differing field)

The uselocale path is the one exercised here. I did not run the Windows _configthreadlocale path against real Windows; a mingw-w64 build (msvcrt and ucrt) compiles it, and a small program under wine shows the same mechanism working (global Italian/German, _configthreadlocale(_ENABLE_PER_THREAD_LOCALE) then setlocale(LC_ALL, "C"): %.2f of 1.5 prints 1.50).

One problem in the new test: test_locale_handling passes here only because four of its five cases skip (es_ES, fr_FR and it_IT are not installed; only de_DE is), and the skipped cases print pass. When the locale does exist, the JSON and CSV cases fail on valid output. To check, I changed the locale names in the test to de_DE: test_output_json_with_comma_locale fails with "JSON output should contain period decimals", and, with the JSON case disabled, so does the CSV one. The cause is contains_period_decimals() (libvmaf/test/test_locale_handling.c:57-75): it returns 0 on any digit directly followed by a comma, which JSON ("frameNum": 0,) and CSV (0,34.76,) always contain. The same failure shows up under wine with an Italian locale. The check could look only at digits that are followed by , and then a digit, or the tests could compare against the C-locale output.

Overlap with ours: #1632 (model: free the partially built model when reading a JSON model fails) edits vmaf_read_json_model() in read_json_model.c as well; the two conflict textually in that function (push/pop around the parse here, a split into model_alloc_and_parse() there). Either order resolves by wrapping the call to model_alloc_and_parse().

diegonieto force-pushed the json-parse-add-setlocate branch 4 times, most recently from eb03b8a to aa47ce8 Compare July 21, 2025 16:49

diegonieto force-pushed the json-parse-add-setlocate branch from febb785 to 9ede68e Compare October 30, 2025 17:32

diegonieto changed the title ~~read_json_model.c: use setlocale to ensure parsing model properly~~ Ensure the model's parsing works properly with the locale Oct 30, 2025

Mr-Clam reviewed Dec 2, 2025

View reviewed changes

diegonieto force-pushed the json-parse-add-setlocate branch from 9ede68e to ad4e4aa Compare December 3, 2025 09:29

sdroege reviewed Dec 3, 2025

View reviewed changes

diegonieto force-pushed the json-parse-add-setlocate branch from ad4e4aa to 5f40053 Compare December 3, 2025 10:25

diegonieto force-pushed the json-parse-add-setlocate branch from 5f40053 to cf58c30 Compare December 9, 2025 18:47

diegonieto force-pushed the json-parse-add-setlocate branch from cf58c30 to 054a97e Compare December 9, 2025 18:54

diegonieto changed the title ~~Ensure the model's parsing works properly with the locale~~ Ensure the parsing works properly with different locales Dec 9, 2025

Conversation

diegonieto commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

diegonieto commented Sep 30, 2025

Uh oh!

sdroege commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rgonzalezfluendo commented Dec 2, 2025

Uh oh!

sdroege commented Dec 2, 2025

Uh oh!

rgonzalezfluendo commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mr-Clam Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdroege Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

diegonieto Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

sdroege Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

diegonieto Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

sdroege Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Mr-Clam Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdroege commented Dec 2, 2025

Uh oh!

sdroege Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

diegonieto Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

kylophone commented Dec 3, 2025

Uh oh!

diegonieto commented Dec 9, 2025

Uh oh!

lusoris commented Oct 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

diegonieto commented Jul 21, 2025 •

edited

Loading

sdroege commented Dec 2, 2025 •

edited

Loading

rgonzalezfluendo commented Dec 2, 2025 •

edited

Loading

Mr-Clam Dec 2, 2025 •

edited

Loading

Mr-Clam Dec 3, 2025 •

edited

Loading