Skip to content

tools: keep a drive-letter colon inside the --model path - #1638

Open
lusoris wants to merge 1 commit into
Netflix:masterfrom
VMAFx:fix/cli-model-path-drive-letter
Open

lusoris wants to merge 1 commit into
Netflix:masterfrom
VMAFx:fix/cli-model-path-drive-letter

Conversation

@lusoris

@lusoris lusoris commented Oct 1, 2026 •

Copy link
Copy Markdown

vmaf --model path=C:\dir\model.json and path=C:/dir/model.json fail because the colon after the drive letter is taken as an option separator. This keeps that colon inside the path. Addresses #761.

Cause

parse_model_config() in libvmaf/tools/cli_parse.c splits the --model argument with strsep(&optarg_copy, ":"). The colon of a drive letter therefore ends the path= option, and the rest of the path is parsed as a new option. Forward slashes make no difference.

Reproducer

Master 6ec23e8f2 (reproduced there; the code is unchanged in 9e48141), release build. The parse step does not depend on the file existing, and on Linux a directory named C: is legal, so the same path form can be loaded for real:

meson setup build libvmaf --buildtype release
ninja -C build
mkdir -p 'C:/models' && cp model/vmaf_v0.6.1.json 'C:/models/'
build/tools/vmaf -r src01_hrc00_576x324.yuv -d src01_hrc01_576x324.yuv -w 576 -h 324 -p 420 -b 8 \
    --frame_cnt 3 -m 'path=C:/models/vmaf_v0.6.1.json' -q --json -o out.json
-m argument master this branch
path=C:/models/vmaf_v0.6.1.json Problem parsing model, bad option string "/models/vmaf_v0.6.1.json"., exit 1 loads, vmaf mean 82.564227 (first 3 frames of src01), exit 0
path=C:/models/vmaf_v0.6.1.json:name=x same error loads, score reported as x
name=a:path=C:/models/vmaf_v0.6.1.json same error loads, score reported as a
path=C:\models\vmaf_v0.6.1.json bad option string "\models\vmaf_v0.6.1.json" parses as one path; on Linux that file does not exist, so the model read then fails with could not read model from path: "C:\models\vmaf_v0.6.1.json" (the full string reaches the loader)

Fix

A small helper, next_option(), replaces that strsep() call. It ends an option at the next colon, except a colon that follows = plus a single letter and is followed by \ or /: that one is the drive-letter colon of the value and stays in it.

Strings that parsed before parse the same way: an option key never starts with / or \, so a one-letter value followed by the next option (name=a:path=/x.json, path=m:name=b) still splits. Only a string that previously failed to parse changes meaning.

Not covered: a drive-relative path (path=C:model.json, no separator after the colon) still splits at the colon, and parse_feature_config() uses the same strsep() for --feature option strings and is left alone (a path option of a feature such as cambi's heatmaps_path would meet the same problem on Windows). This PR is limited to --model.

Tests

Four cases added to libvmaf/test/test_cli_parse.c, which call cli_parse() with --model and check path, name, flags and overloads:

  • drive-letter path alone: C:\..., C:/..., lowercase d:\..., Z:/...;
  • the path with options before and after it (name=, disable_clip, enable_transform, a feature overload vif.vif_enhn_gain_limit=1.5);
  • one-letter values that must still split: name=a:path=/tmp/vmaf.json, path=m:name=b:disable_clip;
  • existing strings unchanged: version=...:name=...:disable_clip:enable_transform, path=model/vmaf_v0.6.1.json:name=rel, path=/opt/vmaf/model.json.

On master's cli_parse.c the new tests fail: test_model_path_drive_letter exits through the usage error above. With this change test_cli_parse passes 11/11, also under ASan/UBSan with leak detection.

Validation

x86-64 Linux, GCC 16.2.1, on master 9e48141b. Rebased on master 9e48141 (2026-10-02).

  • Release build meson test (-Denable_float=true -Denable_checkasm=true): 25/25 on master, 25/25 here. -Db_sanitize=address,undefined -Db_lto=false build: 22 pass and 3 fail on master, 22 pass and 3 fail here; test_predict and test_pic_preallocation fail on LeakSanitizer reports and checkasm aborts on a heap-buffer-overflow in adm_dwt2_16 (integer_adm.c:2603 on master), on both, and this change does not touch them.
  • Scores unchanged: the three Netflix reference pairs give identical per-frame and pooled output on master and on this branch, with the default CPU dispatch and with SIMD masked off. No golden assertion changes.

Not tested: Windows itself. I have not run vmaf.exe or a Windows build. The parse logic is plain C on the argument string and was exercised on Linux only, with the C: directory above and the unit tests, which feed the same strings through cli_parse().

The workflow run on this PR needs a maintainer's approval.

@lusoris
lusoris force-pushed the fix/cli-model-path-drive-letter branch 3 times, most recently from 0a20183 to 27236ff Compare October 2, 2026 18:43
parse_model_config() splits the --model argument on every ':', so a
Windows path such as path=C:\models\vmaf_v0.6.1.json or
path=C:/models/vmaf_v0.6.1.json was cut at the drive letter and failed
with 'Problem parsing model, bad option string "\models\...".'

Split with a small helper, next_option(), that does not end the option
at a colon which follows '=' and a single letter and is followed by '\'
or '/'. An option key never starts with a path separator, so a one-letter
value followed by another option (name=a:path=/x) still splits, and
every string that parsed before parses the same way.

Add cli_parse tests: drive-letter path alone, with options before and
after it, forward and back slash, one-letter values that must still
split, and existing option strings.
@lusoris
lusoris force-pushed the fix/cli-model-path-drive-letter branch from 27236ff to c743d9f Compare October 7, 2026 10:03
@lusoris

lusoris commented Oct 7, 2026

Copy link
Copy Markdown
Author

Rebased onto acdd937; the new head is c743d9f. libvmaf/tools/cli_parse.c merged cleanly. The conflict was in libvmaf/test/test_cli_parse.c, where the per-input colorimetry tests were added; the new tests are placed after upstream's tests and the colorimetry tests are unchanged, including the ones under VMAF_BUILT_IN_MODELS.

Release build with checkasm: all 27 meson tests pass, and test_cli_parse runs 19 tests (upstream's 15 and this PR's 4), all passing. ASan/UBSan build: 23 pass and 4 fail (test_predict, checkasm, test_read_pictures_convert, test_pic_preallocation); the same 4 fail on unpatched master acdd937.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant