Implement periodic CELT packet loss concealment and recovery - #246
Conversation
Encoder Quality ReportStatus: pass Tier 1 — SNR regression (96 kbps, pion encode → pion decode)Delta = baseline − current SNR; positive = regression. Fail threshold: 1.5 dB.
Tier 2 — opus_compare vs libopus (96 kbps CBR)Weighted error: lower is better. The gap reflects pion lacking constrained VBR; libopus ships with it enabled by default.
Run outputBaseline: |
78cb47d to
cca94d4
Compare
RFC 6716 / 8251 conformationStatus: pass The action extracts the RFC 6716 reference implementation, applies the RFC 8251 decoder update patch, and then builds the patched reference tools. Legend: numeric cells are Inputs use the shared RFC 6716 / RFC 8251 bitstream corpus; accepted references follow RFC 8251 Section 11.
Run output |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #246 +/- ##
==========================================
+ Coverage 93.51% 94.11% +0.60%
==========================================
Files 61 63 +2
Lines 10973 11119 +146
==========================================
+ Hits 10261 10465 +204
+ Misses 503 453 -50
+ Partials 209 201 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@thomas-vilte ready for review: all 19 current checks are green at 9e3044b, including same-compiler/same-race before-after comparisons on Linux and macOS. This is the standard non-neural periodic/noise CELT PLC follow-up, stacked on pending #245; it does not change the public API or its accepted durations. On the pinned 502-stream corpus, periodic loss RMSE improves 1562.55 -> 3.56 and recovery 1020.25 -> 0.74, with no per-step regressions and unchanged no-loss PCM. The description includes energy/recovery-boundary measurements, exact pins and CPU costs (first loss is more expensive; warm paths remain allocation-free). SILK-based/Hybrid PLC residuals and non-bit-exact limitations are stated explicitly. Please review the history/postfilter-overlap handling and recovery state transitions in particular. |
|
Downstream integration validation at 9e3044b (2026-09-09). We tested this exact revision in our experimental Go receive pipeline and call executor, using Go 1.26.1 on Linux/amd64 with Module consumption and build: PASS
Short PLC through the actual receive backend: PASS After priming with a synthetic 20 ms CELT packet, the 16 kHz mono backend accepts Application-level compatibility is still open In a separate offline codec-transition experiment, a 5 ms PLC call produces the required 80 samples. Comparing its first 40 samples with a saved original transition prefix gives 27 unequal samples, maximum absolute error 2 in int16 units. The priming packet itself differs from the saved original at 13/320 samples, maximum error 1. These are preliminary consumer observations: the original harness and transition-state contract are still being qualified, so this is not a bit-exact claim or an attribution of those residuals to this PR. The complete MLow -> Opus -> MLow PCM comparison remains failing (913/960, 89/320, and 956/960 unequal samples respectively). MLow PLC and the original reset/overlap sequence remain consumer-side work; the new PLC has not yet been wired into that transition path. The public PLC API returns quantized int16 output and provides no float PLC or state snapshot/restore API; integration must respect those boundaries. No live-call validation was performed with this rebuilt binary. |
thomas-vilte
left a comment
thomas-vilte
left a comment
There was a problem hiding this comment.
Reviewed at 9e3044b. There's one blocking issue from my side; otherwise this looks good and I'd be happy to approve once that's fixed.
On the size of the PR: GitHub shows 39 files and +1417/-32, but the production diff is really +270/-21 across 8 files, and 220 of those added lines are the two new self-contained files, plc.go and recovery_energy.go.
I don't think this needs to be split. The pitch-scratch refactor itself is small — mostly moving fields between structs and changing two signatures — and pulling it into a separate PR wouldn't really make the PLC change easier to review.
I also went through the periodic path line by line against libopus v1.6.1 (22244de5 resolves to that tag) and didn't find any behavioral differences. I checked the periodic/noise decision, the decay calculation and offsets, extrapolation_len = N + overlap, the S1 source after the buffer move, the float explosion check, the gain calculation, autocorrelation and lag windowing, FIR/IIR state layout, background-energy updates, and the recovery-energy path.
The deferred plc.fold also looks right to me. It matches libopus's prefilter_and_fold flag, and it reads the postfilter parameters before updatePostfilterState changes them, so it still uses the pre-loss values.
The one thing I'd block on is:
-
decoder_plc_corpus_test.go:120float64(delta * delta)does the multiplication asintbefore converting tofloat64. On 32-bit, a delta of 65535 overflows and turns negative, which can make the RMSENaN.This matters here because the repo runs
test-i386for Go 1.24 and 1.25, andTestPLCCorpusisn't gated out there. The current baseline doesn't hit it — the largest peak is 34367, below the 46341 overflow point — but a louder corpus sample could.Please use:
float64(delta) * float64(delta)
like the short PLC test already does.
A few other things I'd like to see before merging, but I wouldn't block on them:
-
Please call out the noise-path changes in the PR description.
decodeLostFrameisn't only adding periodic PLC; the existing noise fallback also changes in four places:info.channelCount = info.outputChannelCount- the
max(plc.background...)floor on energy decay - removal of the mono
previousLogEmirror - removal of the
seed == 0fallback
I checked all four against v1.6.1 and they look correct, but they're still behavior changes to existing code. It'd be good to mention them explicitly and point to the tests that exercise the noise path.
-
I'd also document the resident-memory cost next to the warm allocation benchmarks.
On amd64 I get:
Decoder: 744 → 18472 BdecoderScratch: 117696 → 140936 BencoderScratch: 8488 → 9640 B
The decoder increase is basically the new
plcState, and oncescratchBuffer()is materialized that's around 41 KiB more per decoder instance. That's perfectly compatible with0 allocs/opwhen warm; I just think the memory tradeoff should be visible.
A few smaller nits:
plcMeasurement.Energyand.Boundaryare written to the baseline but never asserted, andsummarize.mjsonly readsPeak. I'd drop them.baseline.jsonis uncompressed at about 1.5 MB while the two arm64 baselines are already gzipped at around 512 KB. I'd gzip this one too.- It'd be worth documenting that GOARCH values outside amd64/arm64 fall back to the amd64 golden even though the test compares byte-exact PCM hashes. Everything is green today; this is mostly a maintenance note for future Go upgrades.
regenerate.shdoesn't verify the libopus SHA or that the checkout is clean. Checking the pinned commit would make the fixture provenance stronger.
The plc-quality.yml setup looks good to me. Rebuilding the base commit with the same compiler, architecture, and race mode is the right comparison, and I'd keep that approach.
Once the 32-bit overflow issue is fixed, I'm good with this.
|
@thomas-vilte addressed the i386 RMSE overflow at |
|
Thanks for turning this around so quickly — I went through Two things left, then I'll approve:
On the conflicts. #245 landed as a squash ( Please don't use the web editor GitHub is suggesting: that merges That replays only this PR's nine commits onto Once CI is green on the new base I'll switch my review to approve. Note that #247 will need the same treatment against its base after this one merges. |
d6c4ebd to
a403293
Compare
thomas-vilte
left a comment
thomas-vilte
left a comment
There was a problem hiding this comment.
Rebase looks right — linear, c0d7ee6 dropped, merge_base is 7aa7a85, and the comment fix in a403293 removes exactly the wrong sentence and leaves the correct explanation intact. 23/23 checks green on the new base.
Agreed on leaving baseline.json as is; not worth the fixture churn for 1 MB.
Approving.
2 participants
Summary
Implement standard, non-neural CELT periodic PLC and its recovery state machine:
The existing noise fallback is aligned with libopus too: it synthesizes the decoder output-channel count, floors decay at tracked background energy, does not mirror mono log-energy into an inactive channel, and advances the reference LCG directly from zero. TestPLCCorpus, the long-loss/noise cases, and the same-build Linux/macOS comparison exercise this path.
No public API or runtime dependency changes; no cgo. SILK PLC/encoder algorithms are unchanged.
Stacked on #245 (
c0d7ee63cecdc35aa81b83cbde40c70148da9e74): this PR targetscodex/short-celt-plcso its diff contains only the periodic PLC follow-up. Retarget to main after #245 lands; neither PR should be merged automatically.Evidence
Reference: Xiph libopus
22244de5a79bd1d6d623c32e72bf1954b56235be(1.6.1), float, intrinsics/neural PLC/DRED disabled. Observation-only and unchanged reference builds emit byte-identical fixtures, verified before recording internal states.502 deterministic streams / 11,044 steps cover periodic/variable-frequency/decaying signals, noise, impulses, silence, all five output rates, mono/stereo conversion and switching, short and mixed recovery frames, long loss bursts, one/two received frames between losses, and mode transitions/Hybrid.
The corpus explicitly enables signaled intensity-stereo phase inversion in libopus (
OPUS_SET_PHASE_INVERSION_DISABLED(0)) to match Pion's existing policy. libopus 1.6.1 otherwise disables it by default for mono output, causing different pre-loss PCM and pitch-search input. No PLC mode is forced and Pion's no-loss behavior is unchanged.Weighted RMSE vs reference, int16 units:
Every step must remain within baseline RMSE + one int16 unit; every periodic-signal scenario must at least halve aggregate loss/recovery RMSE. Pre-loss/no-loss PCM hashes must stay unchanged. Mode/duration/readiness are checked independently from PCM against reference traces; energy tolerance is 0.001 log2 units, pitch interpolation tolerance one sample.
The PLC quality CI additionally builds the unchanged base and current code on the same Linux/macOS runner with the same compiler and race mode, then enforces the exact no-loss hashes and the same loss/recovery gates. ARM64 ordinary/race snapshots are recorded from the old commit, not from new-code output; this avoids confusing existing compiler-dependent rounding with a regression. Independent pitch/downsampling/windowed-autocorrelation/LPC reference fixtures and decoder reset/isolation checks are included.
This is not bit-exact or perceptual parity: peak differences remain (periodic loss 440, noise/impulse loss 2459), and SILK-based loss remains substantially different. The original #245 short fixture now differs by at most 3 int16 units during loss and 1 during recovery.
Independent energy and recovery-boundary measurements on the same corpus:
Energy error is
sum(abs(E_step - E_reference_step)) / sum(E_reference_step)over loss/recovery steps, withE = sum(PCM^2). Boundary error compares the first-channel last-lost-sample -> first-received-sample jump against the same reference jump (not against zero). These are separate measurements, not additional relaxed acceptance thresholds or a listening-test score.Validation and cost
Local Ubuntu WSL, Go 1.26.1:
go test ./...,go vet ./...,go build ./...,go mod verify: PASS.go test -race ./...: PASS.Median of 3 x 200 iterations, 48 kHz / 20 ms on Ryzen 9 9950X3D:
First-loss CPU increases because pitch/LPC analysis replaces unconditional noise. Small normal-decode differences are measurement noise, not a speedup claim. Exact regeneration, baseline provenance, measurements and remaining limitations are documented in
testdata/short-plc/README.md. The translated PLC retains Xiph/CSIRO/Gregory Maxwell BSD notices.