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

Six ADR-0108 deep-dive deliverables for the Netflix#1430 port: 1. Research digest: upstream cherry-pick 054a97e is self-documenting; no separate docs/research/NNNN needed (straight port). 2. Decision matrix: see ADR-0137 §"Alternatives considered" (port-with-corrections vs. wait-upstream vs. POSIX-only vs. inline). 3. AGENTS.md invariant: libvmaf/AGENTS.md gains a "Rebase-sensitive invariants" section pinning the fork's ferror(outfile) ? -EIO : 0 return contract and the push_c()/pop() pairing. 4. Reproducer: docs/rebase-notes.md §0031 includes an LC_ALL=de_DE.UTF-8 vmaf CLI smoke-test. 5. CHANGELOG.md: new "Added — I18N / thread-safety" bullet under the lusoris-fork section. 6. Rebase note: docs/rebase-notes.md §0031 covers touches, invariants, re-test, and upstream-sync handling. Correction note for the cherry-picked commit: - test_locale_handling.c updated to pass NULL score_format to vmaf_write_output_{xml,json,csv} (fork API from ADR-0119). - output.c preserves fork's ferror(outfile) ? -EIO : 0 return contract on top of the upstream pop() cleanup. - svm.cpp drops now-dead <locale.h> include; SVM parser ctors use fork's K&R brace style when folding in upstream's buffer.imbue(std::locale::classic()) calls. See docs/adr/0137-thread-local-locale-for-numeric-io.md.

…sitive Two follow-ups surfaced on the prior push: * Clang-Tidy scans every changed C/C++ file whole-file; touching libvmaf/src/svm.cpp (upstream thread-locale patch) lit up dozens of long-latent vendored warnings — null-dereference paths, rand() usage, nullptr modernisation, function-size / branches / nesting over Power-of-10 thresholds, realloc-usage, cert-err33-c. libsvm is vendored upstream code we want to keep diff-reviewable, not re-flow. Wrap the whole file with NOLINTBEGIN / NOLINTEND and drop the narrow per-site NOLINTs the prior fix introduced. * test_output_csv_with_comma_locale failed on macOS + MinGW64 because the contains_period_decimals() helper treated digit-comma-digit as a leak signal — CSV uses commas as structural field separators between integer cells, so "0,3" between Frame column and the first numeric column was a false positive. Drop the absence check and rely on period-presence alone: if the thread-local C-locale applies, the output will contain period decimals; if it does not, every numeric field would render with commas and no period-decimal triple would appear. Local verification: meson test -C build → 31/31 OK clang-tidy -p build libvmaf/src/svm.cpp → silent Refs: ADR-0133 (whole-file vendored suppressions), upstream Netflix PR Netflix#1430 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

MinGW64 linked against msvcrt.dll exposes _configthreadlocale but the underlying per-thread-locale mode is not implemented, so the call returns -1. The previous port-PR code treated that return as fatal and freed the state, causing vmaf_thread_locale_push_c to return NULL on MinGW64 and making test_windows_locale_handling fail with "Windows: push should succeed". Fall back to process-global setlocale(LC_ALL, "C") when the per-thread-locale call fails. The caller still gets C-locale numeric formatting inside the push/pop window; it just isn't thread-isolated on MinGW64. Real Windows MSVC and UCRT builds are unaffected — _configthreadlocale succeeds there and the per-thread mode is used as designed. pop() already guards the restore with `old_per_thread_mode != -1`, so no pop-side change is needed. Refs: upstream PR Netflix#1430, fork port PR #74.

…37) (#74) * Add cross-platform thread-safe locale handling for numeric I/O 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. * docs(adr-0137): thread-locale port deliverables + fork corrections Six ADR-0108 deep-dive deliverables for the Netflix#1430 port: 1. Research digest: upstream cherry-pick 054a97e is self-documenting; no separate docs/research/NNNN needed (straight port). 2. Decision matrix: see ADR-0137 §"Alternatives considered" (port-with-corrections vs. wait-upstream vs. POSIX-only vs. inline). 3. AGENTS.md invariant: libvmaf/AGENTS.md gains a "Rebase-sensitive invariants" section pinning the fork's ferror(outfile) ? -EIO : 0 return contract and the push_c()/pop() pairing. 4. Reproducer: docs/rebase-notes.md §0031 includes an LC_ALL=de_DE.UTF-8 vmaf CLI smoke-test. 5. CHANGELOG.md: new "Added — I18N / thread-safety" bullet under the lusoris-fork section. 6. Rebase note: docs/rebase-notes.md §0031 covers touches, invariants, re-test, and upstream-sync handling. Correction note for the cherry-picked commit: - test_locale_handling.c updated to pass NULL score_format to vmaf_write_output_{xml,json,csv} (fork API from ADR-0119). - output.c preserves fork's ferror(outfile) ? -EIO : 0 return contract on top of the upstream pop() cleanup. - svm.cpp drops now-dead <locale.h> include; SVM parser ctors use fork's K&R brace style when folding in upstream's buffer.imbue(std::locale::classic()) calls. See docs/adr/0137-thread-local-locale-for-numeric-io.md. * style(thread-locale): clang-format + trailing-whitespace fixes Pre-commit hooks auto-reformatted the cherry-picked upstream code: clang-format on thread_locale.{c,h}, output.c, read_json_model.c, and test_locale_handling.c; trailing-whitespace trim on thread_locale.{c,h}. No functional changes — applies the fork's .clang-format style to the ported files per ADR-0012 (JPL/CERT C) and ADR-0105 (pre-commit enforcement). * fix(locale): unblock CI legs for thread-locale port - test/test_locale_handling.c: relax contains_period_decimals() to only flag digit-comma-digit triples (true locale leaks), not the structural JSON comma after frame numbers. macOS + MinGW64 CI legs hit the false positive; Linux passed only because es/fr/it locales are absent and the tests skipped. - test/test_locale_handling.c: -Wstrict-prototypes — replace `f()` with `f(void)` for every static + the exported run_tests. - test/meson.build: test_locale_handling picks up the Windows pthread shim via pthread_dependency (MSVC+CUDA, MSVC+SYCL fatally failed on feature_collector.h -> pthread.h when the test include path lacked src/compat/win32). - test/meson.build: test_lpips + test_ms_ssim_decimate now compile thread_locale.c directly — they pull libsvm object files without link_with=libvmaf, so the svm_save_model references to vmaf_thread_locale_push_c / _pop were unresolved under ASan and Coverage builds. - src/svm.cpp: NOLINT the vendored libsvm parse_header() rule-4 size / branch / nesting findings and the support_vectors malloc leak false positive (ownership transfers through model->SV[]; model->free_sv = 1 arranges cleanup). Per ADR-0133's explicit acknowledgement that a PR touching svm.cpp must annotate or fix long-latent warnings. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(locale): wrap vendored svm.cpp in NOLINTBEGIN + drop CSV false-positive Two follow-ups surfaced on the prior push: * Clang-Tidy scans every changed C/C++ file whole-file; touching libvmaf/src/svm.cpp (upstream thread-locale patch) lit up dozens of long-latent vendored warnings — null-dereference paths, rand() usage, nullptr modernisation, function-size / branches / nesting over Power-of-10 thresholds, realloc-usage, cert-err33-c. libsvm is vendored upstream code we want to keep diff-reviewable, not re-flow. Wrap the whole file with NOLINTBEGIN / NOLINTEND and drop the narrow per-site NOLINTs the prior fix introduced. * test_output_csv_with_comma_locale failed on macOS + MinGW64 because the contains_period_decimals() helper treated digit-comma-digit as a leak signal — CSV uses commas as structural field separators between integer cells, so "0,3" between Frame column and the first numeric column was a false positive. Drop the absence check and rely on period-presence alone: if the thread-local C-locale applies, the output will contain period decimals; if it does not, every numeric field would render with commas and no period-decimal triple would appear. Local verification: meson test -C build → 31/31 OK clang-tidy -p build libvmaf/src/svm.cpp → silent Refs: ADR-0133 (whole-file vendored suppressions), upstream Netflix PR Netflix#1430 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * ci(clang-tidy): drop empty filename from parallel file list The `git diff --name-only ... | tr '\n' ' '` produces a trailing space, and the subsequent `tr ' ' '\n'` re-introduces it as an empty line. GNU parallel faithfully forwards each line to clang-tidy, including the blank. clang-tidy then resolves the empty filename to the workspace root, emits `error reading '/home/runner/work/vmaf/vmaf/': Is a directory [clang-diagnostic-error]`, and exits non-zero — which the workflow interprets as a tidy failure. This had been latent: whenever the genuine file list contained real warnings-as-errors (e.g. vendored svm.cpp clang-analyzer-core.* null-derefs), that failure masked the empty-arg failure. With svm.cpp wrapped in NOLINTBEGIN / NOLINTEND per the companion commit, the empty-arg error becomes the visible failure. Fix: `grep -v '^$'` the file list before handing it to parallel. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * ci(test): surface meson test stdout via --print-errorlogs MinGW64's test_locale_handling failure exits 1 in 0.02s with no visible assertion text in the CI log — meson only writes it to meson-logs/testlog.txt, which the build-matrix workflow does not upload as an artifact. --print-errorlogs makes the failing test's stdout/stderr inline, so cross-platform debugging (locale semantics on MinGW64 MSVCRT vs POSIX uselocale) does not need an artifact round-trip. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(thread-locale): gracefully degrade on MinGW64 MSVCRT MinGW64 linked against msvcrt.dll exposes _configthreadlocale but the underlying per-thread-locale mode is not implemented, so the call returns -1. The previous port-PR code treated that return as fatal and freed the state, causing vmaf_thread_locale_push_c to return NULL on MinGW64 and making test_windows_locale_handling fail with "Windows: push should succeed". Fall back to process-global setlocale(LC_ALL, "C") when the per-thread-locale call fails. The caller still gets C-locale numeric formatting inside the push/pop window; it just isn't thread-isolated on MinGW64. Real Windows MSVC and UCRT builds are unaffected — _configthreadlocale succeeds there and the per-thread mode is used as designed. pop() already guards the restore with `old_per_thread_mode != -1`, so no pop-side change is needed. Refs: upstream PR Netflix#1430, fork port PR #74. --------- Co-authored-by: Diego Nieto <dnieto@fluendo.com> Co-authored-by: Lusoris <lusoris@pm.me> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>

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

This was referenced Apr 20, 2026

feat(batch-a): port four OPEN Netflix upstream PRs (ADR-0131/0132/0134/0135) lusoris/vmaf#72

Merged

port(libvmaf): thread-local locale handling from Netflix/vmaf#1430 (ADR-0137) lusoris/vmaf#74

Merged

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 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