-
-
Notifications
You must be signed in to change notification settings - Fork 122
fix(voip): pick the inbound codec from the negotiation, and never go silent without saying so #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
48
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.
+8,878
−477
Open
Changes from 10 commits
Commits
Show all changes
48 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 26594bd
Merge branch 'main' into fix/voip-codec-negotiation
jlucaso1 6a97793
feat(voip): count every discard on the receive path and alarm on a si…
jlucaso1 c2de31f
feat(voip): select the audio codec from the peer's capability
jlucaso1 86e1fd9
feat(voip): decode what the peer actually sends, and say so when we c…
jlucaso1 cb4c082
fix(voip/mlow): decode the multiframe envelope instead of hearing a SID
jlucaso1 c1e5e1d
fix(voip/mlow): make silence occupy the time the packet claims
jlucaso1 6365f4c
fix(voip/mlow): read the CELT escape with the layout this crate writes
jlucaso1 ed63a42
fix(voip): key the inbound SRTP rollover counter and replay window by…
jlucaso1 8eebd90
docs(voip): document how the codec is actually chosen
jlucaso1 c1bc6c1
fix(voip): close the holes five parallel reviews found
jlucaso1 2520dcc
perf(voip/mlow): give back the memory the concealment snapshot cost
jlucaso1 d652290
perf(voip/mlow): narrow the concealment snapshot to what the decode c…
codspeedbot feb4df4
fix(voip): address the CodeRabbit review
jlucaso1 02d44c1
fix(voip): close the receive-path gaps the review found
jlucaso1 5e4b6af
fix(voip): six more receive-path findings from the second review pass
jlucaso1 3f3acf5
fix(voip): six findings from the third review pass
jlucaso1 011bb57
fix(voip): eight findings from the fourth review pass
jlucaso1 559562c
refactor(voip): arm the watchdog through its own entry point
jlucaso1 0b23b00
fix(voip): three findings from the fifth review pass
jlucaso1 3e5fcdc
fix(voip): three more findings from the fifth review pass
jlucaso1 6df1490
docs(voip): say why AudioCodecSwitched is not force-sent
jlucaso1 36b5946
fix(voip): four findings from the sixth review pass
jlucaso1 84f0355
fix(voip): the send path and the libopus-only build
jlucaso1 38711b8
fix(voip): source format, playout without MLOW, and two smaller holes
jlucaso1 501f92b
fix(voip): a saturated tap must not swallow the disconnect
jlucaso1 9966dd7
fix(voip): decode a group participant's Opus with the installed codec
jlucaso1 8f270ef
fix(voip): probe each group participant's content, not just the escape
claude 85c8f7b
fix(voip): give the group path the rest of the direct path's treatment
claude 040baee
fix(voip): finish the group frame label, and drain its mixer without …
claude 494f635
refactor(voip): ask the group probe about the packet's own codec
claude 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
Some comments aren't visible on the classic Files Changed page.
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,106 @@ | ||
| #!/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 | ||
| # | ||
| # 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)" | ||
|
jlucaso1 marked this conversation as resolved.
|
||
| 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 | ||
| 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.
Uh oh!
There was an error while loading. Please reload this page.