chore(voip/mlow): make the decoder cross-check vectors regenerable - #1112
Conversation
The MLow fixtures were documented recipes pointing at binaries that live on nobody's machine. PROVENANCE.md described each oracle in prose, but the harnesses themselves were never in the tree, so refreshing a vector meant rediscovering how to drive the C reference. That cost real time: the encoder fails every call with SMPL_ENC_NO_GLOBAL_DATA unless smpl_CreateCodec() runs first, and opus_encode flattens that into a bare OPUS_INTERNAL_ERROR with no hint of the cause. Vendor the harness and a script that drives it. Running the script against a built reference reproduces the committed 120 ms fixtures byte for byte, so a regeneration that changes them is a real change rather than tool drift. Add a tripwire for the two halves of that vector. They come out of one run and mean nothing apart: refreshing only one leaves the comparison reading mismatched data, which shows up as a correlation number that moved rather than as an obvious failure. The test pins the PCM length against the frame count and pins that every frame is still the TOC the fixture exists to cover. Record honestly what the harness does NOT reproduce. Run at 60 ms with DTX off it emits the same TOC mix as the older capture fixture, but not the same bytes: packet sizes bracket the committed ones without landing on them, so the original encoder configuration was never recorded. Regenerating those would replace the vectors rather than reproduce them.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| scripts/mlow-vectors/mlow_frames.c | Adds the reference-codec harness and now rejects explicit-count runs that encounter short input before producing every requested packet. |
| scripts/regenerate-mlow-vectors.sh | Builds the harness, stages its output in a temporary file, and only rewrites the paired fixtures after a successful eight-packet run. |
| wacore/src/voip/mlow/decoder.rs | Adds an eight-frame absolute-count tripwire alongside TOC and paired-PCM geometry checks. |
| wacore/src/voip/mlow/testdata/PROVENANCE.md | Documents the reproducible oracle workflow and clearly identifies fixtures that remain non-reproducible. |
Reviews (2): Last reviewed commit: "chore(voip/mlow): fail a short vector re..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d111081366
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (fread(pcm, sizeof(opus_int16), (size_t)samps, f) != (size_t)samps) { | ||
| break; |
There was a problem hiding this comment.
When EOF or an I/O error occurs after at least one complete frame, this break still lets the harness exit successfully because the final status only checks emitted > 0. The regeneration script explicitly requests eight packets with emit 120 8, so it will then overwrite both fixtures with a shorter vector, and the new tripwire accepts any nonempty matching frame count; an intermittent read failure can therefore silently reduce regression coverage. When want >= 0, return an error unless exactly want packets were emitted, and also distinguish ferror(f) from normal EOF.
Useful? React with 👍 / 👎.
| halves of the vector in one pass. Verified to reproduce the committed files byte for byte, so a | ||
| regeneration that changes them is a real change and not tool drift. |
There was a problem hiding this comment.
The regeneration instructions accept an arbitrary opus_mlow checkout, but the repository contains no source URL, commit, tag, or archive checksum identifying the reference build that produced the committed bytes. If a later regeneration uses a newer or differently patched fork, both fixtures may change because the oracle changed, while this text explicitly tells the developer that such a difference cannot be tool drift. Record the exact reference source revision and build identity, or have the script validate it, before making that guarantee.
Useful? React with 👍 / 👎.
…ing it Review found the hole this tooling was supposed to close. The harness exited successfully whenever it emitted at least one packet, so a truncated input or a read error produced a shorter vector and the script overwrote both committed fixtures with it. The tripwire did not catch that either: it checked the PCM length against the frame count, a relative property that a shorter vector still satisfies. Treat a short count as the failure it is, distinguishing a read error from a clean EOF, and assert the absolute packet count in the tripwire so a shrunken fixture fails loudly. Also pin the oracle. The claim that a changed fixture means a real change only holds against the reference revision the bytes came from, which was never recorded; a different checkout could move the output and the text told the reader that could not happen. Name the revision and have the script report the one it built against, warning when they differ.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb71117c38
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| # Recorded so a regeneration says which oracle produced it; see the header. | ||
| expected_rev="84b076e0809412df22e8a0d26f944610c4a3e40f" | ||
| actual_rev="$(git -C "$ref" rev-parse HEAD 2>/dev/null || echo unknown)" |
There was a problem hiding this comment.
When a developer has checked out expected_rev but .libs/libopus.a is stale from another commit—or was built from dirty sources—rev-parse HEAD reports the expected oracle even though the compile on line 64 links different code, and the script proceeds to overwrite both reference fixtures. Fresh evidence after the earlier revision comment is that this new check validates only the checkout's HEAD, not the pre-existing archive it actually consumes; require a clean checkout plus rebuild, or validate an archive checksum, before claiming byte-for-byte regeneration.
Useful? React with 👍 / 👎.
| int emitted = 0; | ||
| int short_read = 0; | ||
| for (int n = 0; want < 0 || n < want; n++) { | ||
| if (fread(pcm, sizeof(opus_int16), (size_t)samps, f) != (size_t)samps) { |
There was a problem hiding this comment.
On a big-endian host, this fread places the little-endian bytes from synth_mic.raw directly into native opus_int16 elements, so the encoder sees every sample byte-swapped even though the harness advertises an s16le input; the output path, by contrast, explicitly serializes little-endian samples. The same pinned oracle can therefore regenerate different frames and reference PCM solely because of host endianness, so convert each input sample from little endian or explicitly reject non-little-endian hosts.
Useful? React with 👍 / 👎.
1 participant
Stacked on #1111 — it adds the 120 ms fixtures this makes regenerable. Review that one first; this diff is the two files under
scripts/plus a tripwire.Summary
The MLow fixtures were documented recipes pointing at binaries that live on nobody's machine.
PROVENANCE.mddescribes each oracle in prose, but the harnesses were never in the tree, so refreshing a vector meant rediscovering how to drive the C reference from scratch.That cost is not hypothetical. Producing the 120 ms vector for #1111 took an afternoon of reverse engineering for two reasons that are invisible from the outside:
smpl_CreateCodec()must run before the first encode, or every call failsSMPL_ENC_NO_GLOBAL_DATA(-112) — whichopus_encodeflattens into a bareOPUS_INTERNAL_ERROR(-3) with no hint that global tables were the problem.Neither is discoverable without instrumenting the reference. Both are now in the harness header.
Changes
scripts/mlow-vectors/mlow_frames.c— encodes a raw PCM file at a requested duration through thesmplreference and decodes each packet back, emitting both halves of a cross-check vector in one pass (<hex payload> <hex s16le pcm>per line).scripts/regenerate-mlow-vectors.sh— builds it against a reference checkout and rewrites the fixtures. Follows the existingscripts/regenerate-*.shconvention: consumers never run it, editors run it once and commit the result.multi_frame_fixture_halves_stay_in_step— a tripwire. The two halves come out of one run and mean nothing apart: refreshing only one leaves the comparison reading mismatched data, which surfaces as a correlation number that moved rather than as an obvious failure. Pins the PCM length against the frame count, and pins that every frame is still TOC0x58.PROVENANCE.md— the multi-frame section now points at the command instead of describing it.Guarantees
Running the script against a built reference reproduces the committed 120 ms fixtures byte for byte (identical md5, clean
git status). So a regeneration that changes them is a real change, not tool drift — which is the property that makes refreshing a fixture safe.What this deliberately does not do
The older fixtures stay non-reproducible, and the doc now says so instead of implying otherwise. The harness reproduces their shape — run at 60 ms with DTX off it emits the same TOC mix as
inbound_capture_frames.json(13x0x10, 2x0x12, 95x0x50) — but not their bytes: packet sizes bracket the committed ones without landing on them (a 165-byte first frame sits between the 159 and 168 the reference produces at adjacent bitrates), so the original run used a configuration that was never recorded. Regenerating those would replace the vectors rather than reproduce them, which is a decision to make deliberately with the correlation thresholds in hand.The oversized JSON fixtures are left alone.
gennoise_vectors.jsonis 5.8 MB across 1320 records, but it feeds a bit-exact PRNG/gen_noisecheck whose coverage is not characterised anywhere. Truncating it without a coverage analysis is guesswork, and the failure mode — quietly losing a rarenp/fcbgcase — is exactly what a fixture is supposed to prevent. Worth doing after the coverage is measurable, not before.Validation
Full matrix left to CI.