The encoding corpus contains no -fps_mode at all, and a broken option went unnoticed #141

New issue

Closed

opened 2026-08-14 15:44:46 +02:00 by Claude · 1 comment

Claude commented

2026-08-14 15:44:46 +02:00

Collaborator

jellyfin-encoding::input::video_sync_option handed the number to the option that takes a name. Upstream maps -1 → auto for ffmpeg 5.1 and later; ours emitted -fps_mode -1, which ffmpeg refuses outright — so every command line reaching that option failed, including the attachment burn-in path, where attach.rs:123 sets input_video_sync = "-1".

Found while implementing #98, and fixed there. The corpus did not catch it, and could not: neither corpus/progressive-video.json nor corpus/hls.json contains the string -fps_mode anywhere, across 5,624 cases.

That is the interesting part. The corpus is this project's strongest instrument — M5 records 5,300 checked cases and the argument that the measure is not how many pass but which branches one would notice breaking. An option that appears in none of them is a branch nobody would notice breaking, and one did.

What to do

Two things, and the second matters more than the first.

Sweep the option in, so -fps_mode and its auto/passthrough/cfr/vfr spellings appear in generated cases. The generator is a C# program linking upstream (D19), so this is a matter of which states it sweeps.
Find out what else is absent. The question is answerable mechanically: take every literal option string crates/jellyfin-encoding can emit, and check each against the corpus. Anything with zero occurrences is a branch the corpus does not defend, and the list of those is worth more than this one option. crates/jellyfin-encoding/README.md is where the answer belongs, beside what green already does not mean.

How to know it is done

Every option this crate can emit occurs in at least one corpus case, or is listed in the encoding README as knowingly unswept with a reason — and breaking video_sync_option again fails a case.

House rules

Tests run under cargo nextest; upstream's source is the checkout in upstream/ at the pin; generated artifacts are regenerated, never edited. The reference on :8096 is read-only, and Studios=/Artists=/Albums= are writes — upstream resolves them through CreateItemByName. Anything needing a write gets its own container from the pinned image over a cp -a copy of a snapshot. Kill a server by the PID ss -lntpH 'sport = :NNNN' reports; never pkill, killall, or pgrep -f with a pattern your own command line contains.

`jellyfin-encoding::input::video_sync_option` handed the **number** to the option that takes a **name**. Upstream maps `-1 → auto` for ffmpeg 5.1 and later; ours emitted `-fps_mode -1`, which ffmpeg refuses outright — so every command line reaching that option failed, including the attachment burn-in path, where `attach.rs:123` sets `input_video_sync = "-1"`. Found while implementing #98, and fixed there. **The corpus did not catch it, and could not**: neither `corpus/progressive-video.json` nor `corpus/hls.json` contains the string `-fps_mode` anywhere, across 5,624 cases. That is the interesting part. The corpus is this project's strongest instrument — M5 records 5,300 checked cases and the argument that the measure is not how many pass but which branches one would notice breaking. An option that appears in **none** of them is a branch nobody would notice breaking, and one did. ## What to do Two things, and the second matters more than the first. - Sweep the option in, so `-fps_mode` and its `auto`/`passthrough`/`cfr`/`vfr` spellings appear in generated cases. The generator is a C# program linking upstream (D19), so this is a matter of which states it sweeps. - **Find out what else is absent.** The question is answerable mechanically: take every literal option string `crates/jellyfin-encoding` can emit, and check each against the corpus. Anything with zero occurrences is a branch the corpus does not defend, and the list of those is worth more than this one option. `crates/jellyfin-encoding/README.md` is where the answer belongs, beside what green already does not mean. ## How to know it is done Every option this crate can emit occurs in at least one corpus case, or is listed in the encoding README as knowingly unswept with a reason — and breaking `video_sync_option` again fails a case. ### House rules Tests run under `cargo nextest`; upstream's source is the checkout in `upstream/` **at the pin**; generated artifacts are regenerated, never edited. The reference on `:8096` is read-only, and `Studios=`/`Artists=`/`Albums=` are *writes* — upstream resolves them through `CreateItemByName`. Anything needing a write gets its own container from the pinned image over a `cp -a` copy of a snapshot. Kill a server by the PID `ss -lntpH 'sport = :NNNN'` reports; never `pkill`, `killall`, or `pgrep -f` with a pattern your own command line contains.

Claude added the

testing

encoding

labels

2026-08-14 15:44:46 +02:00

Claude referenced this issue

2026-08-21 14:19:05 +02:00

Nothing about the write path, artwork or transcoding has been measured on a real library #117

Claude commented

2026-08-21 16:05:42 +02:00

Author

Collaborator

Landed in 4c07fd3, with review fixes in 582ed4e.

The audit, which is the half that was worth more

crates/ephyra-encoding/tests/swept.rs takes every option this crate can emit
out of its own source and looks for each in the arg strings the three corpora
record. One that occurs in none is either swept in or named in a table with the
reason nothing reaches it, and one that is neither fails the test.

Thirty-four of the hundred and eleven options this crate emits are in no
corpus case. The subtitle and attachment extractors' whole command lines
(they are not an entry point the generator drives); every option that needs a
source read at its native rate, one that requires looping, an RTSP input, or
remote HTTP headers; five audio targets the audio corpus does not reach; and a
handful needing a container, codec or hardware combination the sweep does not
produce. Each is named with its reason, and crates/ephyra-encoding/README.md
records the count beside what green already does not mean.

The review caught the audit under-reporting twice, which is worth recording
because both are easy to write:

The scanner paired quotes naively, so a '"' or an escaped quote
desynchronised it and every literal after that point in the file was read as
the wrong thing — nine options silently uncounted, including all of
-map 0:N -an -vn -c:s … -flush_packets 1 and -dump_attachment:t "" … -safe 0.
It lexes now, and finds 111 where it found 86.
It matched an option as a substring of the whole corpus file, so -re was
called swept by the case id hevc-rext-10bit-mkv. It compares tokens of the
arg strings alone.

Both numbers are pinned. A scanner that quietly stopped finding half the options
would have stayed above a floor.

Sweeping `-fps_mode` in: it cannot be done

Not an oversight in the generator. OutputVideoSync is settable and assigned
nowhere in upstream's tree, so no case can carry a state that sets it.
InputVideoSync is set — for a stream read at its native rate and for a wtv
file — but the option it becomes is applied to an input only below ffmpeg 5.1,
and both probed environments are 7.1.5.1 and 8.1.2. There is no legitimate
generated case that reaches either option.

So the acceptance's other branch applies: both are named with that reason, and
the mapping is asserted directly in input::tests — a test that fails when
-fps_mode is handed the number -vsync took, which is the defect that reached
the burn-in path with 5,624 cases green.

The review also caught that function diverging from upstream: it passed a value
it could not parse through as -fps_mode <name>, where upstream returns nothing.
The test had asserted the passthrough — certifying the wrong answer for the one
function no corpus case can check. Both fixed.

Landed in 4c07fd3, with review fixes in 582ed4e. ## The audit, which is the half that was worth more `crates/ephyra-encoding/tests/swept.rs` takes every option this crate can emit out of its own source and looks for each in the arg strings the three corpora record. One that occurs in none is either swept in or named in a table with the reason nothing reaches it, and one that is neither fails the test. **Thirty-four of the hundred and eleven options this crate emits are in no corpus case.** The subtitle and attachment extractors' whole command lines (they are not an entry point the generator drives); every option that needs a source read at its native rate, one that requires looping, an RTSP input, or remote HTTP headers; five audio targets the audio corpus does not reach; and a handful needing a container, codec or hardware combination the sweep does not produce. Each is named with its reason, and `crates/ephyra-encoding/README.md` records the count beside what green already does not mean. The review caught the audit under-reporting twice, which is worth recording because both are easy to write: - The scanner paired quotes naively, so a `'"'` or an escaped quote desynchronised it and every literal after that point in the file was read as the wrong thing — nine options silently uncounted, including all of `-map 0:N -an -vn -c:s … -flush_packets 1` and `-dump_attachment:t "" … -safe 0`. It lexes now, and finds 111 where it found 86. - It matched an option as a substring of the whole corpus file, so `-re` was called swept by the case id `hevc-rext-10bit-mkv`. It compares tokens of the arg strings alone. Both numbers are pinned. A scanner that quietly stopped finding half the options would have stayed above a floor. ## Sweeping `-fps_mode` in: it cannot be done Not an oversight in the generator. `OutputVideoSync` is settable and assigned **nowhere** in upstream's tree, so no case can carry a state that sets it. `InputVideoSync` *is* set — for a stream read at its native rate and for a `wtv` file — but the option it becomes is applied to an input only below ffmpeg 5.1, and both probed environments are 7.1.5.1 and 8.1.2. There is no legitimate generated case that reaches either option. So the acceptance's other branch applies: both are named with that reason, and the mapping is asserted directly in `input::tests` — a test that fails when `-fps_mode` is handed the number `-vsync` took, which is the defect that reached the burn-in path with 5,624 cases green. The review also caught that function diverging from upstream: it passed a value it could not parse through as `-fps_mode <name>`, where upstream returns nothing. The test had asserted the passthrough — certifying the wrong answer for the one function no corpus case can check. Both fixed.

Claude closed this issue

2026-08-21 16:05:42 +02:00