-
-
Notifications
You must be signed in to change notification settings - Fork 123
fix(voip/mlow): multi-frame admission, coded-inactive decode, malformed-frame concealment, and regenerable vectors #1111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
jlucaso1
wants to merge
18
commits into
main
Choose a base branch
from
fix/voip-mlow-multi-frame-packets
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 14 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
f1e7191
fix(voip/mlow): decode multi-frame packets instead of dropping them
jlucaso1 d111081
chore(voip/mlow): make the decoder cross-check vectors regenerable
jlucaso1 fb71117
chore(voip/mlow): fail a short vector regeneration instead of committ…
jlucaso1 feb1d0e
fix(voip/mlow): decode frames coded inactive instead of silencing them
jlucaso1 7725186
fix(voip/mlow): size the playout cushion to the peer's packet
jlucaso1 6c31c4e
docs(voip): keep the playout sizing rationale at one site
jlucaso1 8c581ff
fix(voip/mlow): size the playout cushion from the declared duration
jlucaso1 9c98522
fix(voip/mlow): let the playout ceiling lag a shrinking packet
jlucaso1 f111744
chore(voip/mlow): refuse to regenerate vectors from a dirty oracle
jlucaso1 79ecd92
test(voip/mlow): commit the live 120ms repro, still failing
jlucaso1 3a73661
fix(voip/mlow): conceal a frame whose decode overruns its body
jlucaso1 3e9565e
fix(voip/mlow): keep a concealed frame from leaking into the next
jlucaso1 537a4b6
chore(voip/mlow): reject a stale oracle archive, and stop repeating a…
jlucaso1 9fe0767
docs(voip/mlow): drop the captured-speech fixture from provenance
jlucaso1 a927ada
perf(voip/mlow): keep the test trace out of the rollback snapshot
jlucaso1 da20678
chore(voip/mlow): check the oracle archive against sources, not git m…
jlucaso1 8845bf3
fix(voip): take the endpoint window from the shipped decoder, not the…
jlucaso1 89249a7
Merge branch 'main' into fix/voip-mlow-multi-frame-packets
jlucaso1 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,153 @@ | ||
| // Encode a raw PCM file as MLow/smpl packets and decode each one back through the same | ||
| // reference, so a single run produces both halves of a decoder cross-check vector. | ||
| // | ||
| // Output, one line per packet, on stdout: | ||
| // | ||
| // <hex payload> <hex s16le pcm> | ||
| // | ||
| // Diagnostics (packet index, byte count, TOC, sample count) go to stderr, so stdout can be | ||
| // redirected straight into a fixture builder. | ||
| // | ||
| // Two things are easy to get wrong and cost an afternoon each: | ||
| // | ||
| // * smpl_CreateCodec() must run before the first encode. Without it every encode fails with | ||
| // SMPL_ENC_NO_GLOBAL_DATA (-112), which opus_encode flattens into a bare OPUS_INTERNAL_ERROR | ||
| // (-3) with no hint that global tables were the problem. | ||
| // * The output buffer handed to opus_encode is a hard bound, not a capacity. Under CBR the | ||
| // encoder pads to exactly that size, so an oversized buffer fails the pad. Keep it near the | ||
| // real packet size. | ||
|
|
||
| #include <stdio.h> | ||
| #include <stdlib.h> | ||
| #include <string.h> | ||
|
|
||
| #include "opus.h" | ||
|
|
||
| // Declared here rather than pulled from a private header: the reference exposes it as a plain | ||
| // symbol and the harness only needs the one entry point. | ||
| extern int smpl_CreateCodec(void); | ||
|
|
||
| #define FS 16000 | ||
| #define MAX_PACKET 400 | ||
|
|
||
| static void usage(const char *argv0) { | ||
| fprintf(stderr, | ||
| "usage: %s <input.raw> <frame_ms> [packets] [bitrate]\n" | ||
| " input.raw s16le mono @ 16 kHz\n" | ||
| " frame_ms 10, 20, 60 or 120\n" | ||
| " packets how many to emit (default: until EOF)\n" | ||
| " bitrate encoder bitrate in bps (default 20000)\n", | ||
| argv0); | ||
| } | ||
|
|
||
| int main(int argc, char **argv) { | ||
| if (argc < 3) { | ||
| usage(argv[0]); | ||
| return 2; | ||
| } | ||
| const char *path = argv[1]; | ||
| const int frame_ms = atoi(argv[2]); | ||
| const int want = argc > 3 ? atoi(argv[3]) : -1; | ||
| const int bitrate = argc > 4 ? atoi(argv[4]) : 20000; | ||
|
|
||
| if (frame_ms != 10 && frame_ms != 20 && frame_ms != 60 && frame_ms != 120) { | ||
| fprintf(stderr, "error: frame_ms must be 10, 20, 60 or 120 (got %d)\n", frame_ms); | ||
| return 2; | ||
| } | ||
| const int samps = FS / 1000 * frame_ms; | ||
|
|
||
| if (smpl_CreateCodec() != 0) { | ||
| fprintf(stderr, "error: smpl_CreateCodec failed\n"); | ||
| return 1; | ||
| } | ||
|
|
||
| FILE *f = fopen(path, "rb"); | ||
| if (!f) { | ||
| perror("open input"); | ||
| return 2; | ||
| } | ||
|
|
||
| int err = 0; | ||
| OpusEncoder *enc = opus_encoder_create(FS, 1, OPUS_APPLICATION_VOIP, &err); | ||
| if (err != OPUS_OK || !enc) { | ||
| fprintf(stderr, "error: encoder create: %s\n", opus_strerror(err)); | ||
| return 1; | ||
| } | ||
| opus_encoder_ctl(enc, OPUS_SET_USING_SMPL(1)); | ||
| opus_encoder_ctl(enc, OPUS_SET_BITRATE(bitrate)); | ||
| opus_encoder_ctl(enc, OPUS_SET_FORCE_CHANNELS(1)); | ||
|
|
||
| OpusDecoder *dec = opus_decoder_create(FS, 1, &err); | ||
| if (err != OPUS_OK || !dec) { | ||
| fprintf(stderr, "error: decoder create: %s\n", opus_strerror(err)); | ||
| return 1; | ||
| } | ||
| opus_decoder_ctl(dec, OPUS_SET_USING_SMPL(1)); | ||
|
|
||
| opus_int16 *pcm = malloc((size_t)samps * sizeof(opus_int16)); | ||
| opus_int16 *out = malloc((size_t)samps * sizeof(opus_int16)); | ||
| unsigned char packet[MAX_PACKET]; | ||
| if (!pcm || !out) { | ||
| fprintf(stderr, "error: out of memory\n"); | ||
| return 1; | ||
| } | ||
|
|
||
| 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) { | ||
| // A partial frame at EOF, or a read error. Either way this run cannot produce the | ||
| // vector that was asked for, and callers overwrite committed fixtures with whatever | ||
| // comes out of it -- so remember it and fail below rather than emit a short vector. | ||
| short_read = 1; | ||
| if (ferror(f)) { | ||
| perror("read input"); | ||
| } | ||
| break; | ||
| } | ||
| opus_int32 len = opus_encode(enc, pcm, samps, packet, sizeof(packet)); | ||
| if (len <= 0) { | ||
| fprintf(stderr, "error: encode packet %d: %s\n", n, opus_strerror(len)); | ||
| return 1; | ||
| } | ||
| int ns = opus_decode(dec, packet, len, out, samps, 0); | ||
| if (ns <= 0) { | ||
| fprintf(stderr, "error: decode packet %d: %s\n", n, opus_strerror(ns)); | ||
| return 1; | ||
| } | ||
| for (opus_int32 i = 0; i < len; i++) { | ||
| printf("%02x", packet[i]); | ||
| } | ||
| printf(" "); | ||
| for (int i = 0; i < ns; i++) { | ||
| unsigned short v = (unsigned short)out[i]; | ||
| printf("%02x%02x", v & 0xff, (v >> 8) & 0xff); | ||
| } | ||
| printf("\n"); | ||
| fprintf(stderr, "packet %d: %d bytes, TOC 0x%02x, %d samples\n", n, len, packet[0], ns); | ||
| emitted++; | ||
| } | ||
|
|
||
| fprintf(stderr, "emitted %d packet(s) of %d ms\n", emitted, frame_ms); | ||
|
|
||
| // Exiting 0 with fewer packets than requested would let a regeneration quietly replace a | ||
| // fixture with a smaller one: the relative length checks downstream stay satisfied, so the | ||
| // coverage loss is invisible. A short count is a failure, not a partial success. | ||
| int ok = 1; | ||
| if (emitted == 0) { | ||
| fprintf(stderr, "error: no packets emitted\n"); | ||
| ok = 0; | ||
| } else if (want >= 0 && emitted != want) { | ||
| fprintf(stderr, | ||
| "error: asked for %d packet(s) but the input only yielded %d%s\n", | ||
| want, emitted, short_read ? " (short read)" : ""); | ||
| ok = 0; | ||
| } | ||
|
|
||
| free(pcm); | ||
| free(out); | ||
| opus_encoder_destroy(enc); | ||
| opus_decoder_destroy(dec); | ||
| fclose(f); | ||
| return ok ? 0 : 1; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,124 @@ | ||
| #!/usr/bin/env bash | ||
| # Regenerate the MLow decoder cross-check fixtures from the `smpl` C reference. | ||
| # | ||
| # Consumers never run this — the fixtures are committed and the tests read them directly. It | ||
| # exists so the vectors stop being a documented recipe with a missing binary: everything needed | ||
| # to reproduce them, including the two non-obvious setup steps, lives in | ||
| # `scripts/mlow-vectors/mlow_frames.c`. | ||
| # | ||
| # The committed fixtures were produced with this exact oracle: | ||
| # | ||
| # https://github.com/edgardmessias/opus_mlow at 84b076e0809412df22e8a0d26f944610c4a3e40f | ||
| # | ||
| # Byte-for-byte reproduction is only claimed against that revision. A different checkout can | ||
| # change the output because the ORACLE changed, which is not the same thing as a decoder change, | ||
| # so the script prints the revision it actually built against and warns when it differs. | ||
| # | ||
| # Requires a built `smpl` reference (the WhatsApp MLow fork of libopus): | ||
| # | ||
| # cd "$MLOW_REFERENCE" && ./autogen.sh && ./configure --disable-shared --disable-doc \ | ||
| # --disable-extra-programs && make -j"$(nproc)" | ||
| # | ||
| # Then: | ||
| # | ||
| # MLOW_REFERENCE=/path/to/opus_mlow scripts/regenerate-mlow-vectors.sh | ||
| # | ||
| # The script refuses to run against a reference worktree with uncommitted changes, or against a | ||
| # library older than the checkout, because neither can be attributed to a recorded revision. Set | ||
| # MLOW_ALLOW_DIRTY_REFERENCE=1 when generating from a modified oracle is the actual intent. | ||
| # | ||
| # Regenerating changes committed fixtures. Re-run the decoder suite afterwards and treat any | ||
| # correlation change as a finding, not as a number to paper over: | ||
| # | ||
| # cargo test -p wacore --features voip-mlow --lib | ||
|
|
||
| set -euo pipefail | ||
|
|
||
| repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" | ||
| testdata="$repo_root/wacore/src/voip/mlow/testdata" | ||
| harness_src="$repo_root/scripts/mlow-vectors/mlow_frames.c" | ||
|
|
||
| ref="${MLOW_REFERENCE:-}" | ||
| if [[ -z "$ref" ]]; then | ||
| echo "error: set MLOW_REFERENCE to the built smpl/opus reference checkout" >&2 | ||
| exit 1 | ||
| fi | ||
| if [[ ! -f "$ref/.libs/libopus.a" ]]; then | ||
| echo "error: $ref/.libs/libopus.a not found; build the reference first (see the header of this script)" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| # 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)" | ||
| if [[ "$actual_rev" == "unknown" ]]; then | ||
| echo "warning: $ref is not a git checkout; cannot confirm the oracle revision" >&2 | ||
| else | ||
| # The revision alone does not identify the build: uncommitted changes still compile into the | ||
| # static library while rev-parse keeps reporting the pinned commit, so a modified oracle would | ||
| # silently rewrite the fixtures and look like a legitimate update. Refuse unless told otherwise. | ||
| if [[ -n "$(git -C "$ref" status --porcelain 2>/dev/null)" ]]; then | ||
| if [[ "${MLOW_ALLOW_DIRTY_REFERENCE:-}" == "1" ]]; then | ||
| echo "warning: oracle worktree is DIRTY; output does not correspond to $actual_rev" >&2 | ||
| else | ||
| echo "error: oracle worktree has uncommitted changes, so the fixtures it produces would not" >&2 | ||
| echo " correspond to any recorded revision. Commit or stash them, or re-run with" >&2 | ||
| echo " MLOW_ALLOW_DIRTY_REFERENCE=1 if you mean to generate from a modified oracle." >&2 | ||
| exit 1 | ||
| fi | ||
| fi | ||
| if [[ "$actual_rev" != "$expected_rev" ]]; then | ||
| echo "warning: oracle is $actual_rev, fixtures were generated with $expected_rev" >&2 | ||
| echo " differences below may come from the reference, not from this repository" >&2 | ||
| fi | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| fi | ||
| # A clean worktree at the right commit still proves nothing about the ARCHIVE: switching revisions | ||
| # without rebuilding leaves a stale .a that links fine and attributes its output to a commit it was | ||
| # never built from. Compare against the last thing that changed the checkout. | ||
| lib="$ref/.libs/libopus.a" | ||
| if [[ -e "$ref/.git" ]]; then | ||
| head_file="$ref/.git/HEAD" | ||
| [[ -f "$ref/.git" ]] && head_file="$(git -C "$ref" rev-parse --git-dir)/HEAD" | ||
| if [[ -f "$head_file" && "$head_file" -nt "$lib" ]]; then | ||
|
jlucaso1 marked this conversation as resolved.
Outdated
|
||
| echo "error: $lib predates the current checkout, so it was not built from $actual_rev." >&2 | ||
| echo " Rebuild it (make -j\"\$(nproc)\" in \$MLOW_REFERENCE) and re-run." >&2 | ||
| exit 1 | ||
| fi | ||
| fi | ||
|
|
||
| echo "==> oracle: $ref @ $actual_rev" | ||
|
|
||
| work="$(mktemp -d)" | ||
| trap 'rm -rf "$work"' EXIT | ||
|
|
||
| echo "==> building the harness against $ref" | ||
| cc -O2 \ | ||
| -I "$ref/include" -I "$ref/src" -I "$ref/celt" -I "$ref" \ | ||
| -o "$work/mlow_frames" "$harness_src" "$ref/.libs/libopus.a" -lm | ||
|
jlucaso1 marked this conversation as resolved.
|
||
|
|
||
| # One packet per line: "<hex payload> <hex s16le pcm>". Split into the frames fixture and the | ||
| # reference PCM the decoder test compares against. | ||
| emit() { | ||
| local frame_ms="$1" packets="$2" frames_json="$3" pcm_raw="$4" | ||
| echo "==> ${frame_ms}ms x ${packets} -> $(basename "$frames_json"), $(basename "$pcm_raw")" | ||
| "$work/mlow_frames" "$testdata/synth_mic.raw" "$frame_ms" "$packets" > "$work/vectors.txt" | ||
| python3 - "$work/vectors.txt" "$frames_json" "$pcm_raw" <<'PY' | ||
| import json, sys | ||
| src, frames_path, pcm_path = sys.argv[1:4] | ||
| frames, pcm = [], bytearray() | ||
| for line in open(src): | ||
| payload, samples = line.split() | ||
| frames.append(payload) | ||
| pcm += bytes.fromhex(samples) | ||
| json.dump(frames, open(frames_path, "w"), indent=1) | ||
| open(pcm_path, "wb").write(pcm) | ||
| print(f" {len(frames)} frames, {len(pcm)//2} samples, TOCs: {sorted({f[:2] for f in frames})}") | ||
| PY | ||
| } | ||
|
|
||
| emit 120 8 "$testdata/mlow_120ms_frames.json" "$testdata/ref_120ms_expected.raw" | ||
| # 60 ms with DTX off: the encoder emits VoA=00 frames (TOC 0x10) for the silent stretches of | ||
| # synth_mic.raw, which is the only in-repo coverage of the DTX-off decode path. | ||
| emit 60 110 "$testdata/mlow_dtx_off_frames.json" "$testdata/ref_dtx_off_expected.raw" | ||
|
|
||
| echo "==> done; re-run: cargo test -p wacore --features voip-mlow --lib" | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
MLOW_REFERENCEhas uncommitted source changes and its static library was built from them,rev-parse HEADstill reports the pinned revision, so this script emits no warning and overwrites the committed fixtures with output from a modified oracle. That defeats the script's byte-for-byte provenance guarantee and can make decoder regressions appear to be legitimate fixture updates; check the reference worktree for modifications and abort or explicitly mark it dirty before generating files.Useful? React with 👍 / 👎.