Add pure-Go SILK encoder#147
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #147 +/- ##
==========================================
+ Coverage 87.72% 90.81% +3.09%
==========================================
Files 32 55 +23
Lines 7566 9573 +2007
==========================================
+ Hits 6637 8694 +2057
+ Misses 705 641 -64
- Partials 224 238 +14
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:
|
fa2c322 to
97d2298
Compare
JoTurk
left a comment
JoTurk
left a comment
There was a problem hiding this comment.
Can you please break this into smaller prs so it's easier to reason / review?
97d2298 to
77e753c
Compare
| predQ10 := int32(0) | ||
| if k+1 < order { | ||
| predQ10 = (int32(predTable[predSelect[index1][k]][k]) * prevOut) >> 8 //nolint:gosec // G115 | ||
| } | ||
| ind := clamp((invQstepQ6*(target-predQ10))>>16, -nlsfQuantMaxAmplitudeExt, nlsfQuantMaxAmplitudeExt-1) |
There was a problem hiding this comment.
spent a bit of time diffing some of the riskier files against libopus (a2nlsf, nsq, gains, ltp_scale, control_snr, nlsf_encode — mostly picked based on coverage and bitstream impact). Everything else looked good, but I think I found a bug here
clamp is func clamp(low, in, high int32) int32, but the arguments are swapped:
ind := clamp(raw, -nlsfQuantMaxAmplitudeExt, nlsfQuantMaxAmplitudeExt-1)that means -10 ends up as the value being clamped, not raw, so the upper bound is never applied. I added some logging and saw raw reaching +31 on valid (just tightly clustered) NLSF vectors, with stage-2 indices ending up as high as +15
the nasty part is that it doesn't fail loudly. EncodeSymbolWithICDF just ignores out-of-range symbols, so the extension marker gets written but the extension symbol doesn't. After that the decoder starts reading the following bits as the extension symbol and the rest of the frame is out of sync
swapping the first two arguments fixes it:
ind := clamp(
-nlsfQuantMaxAmplitudeExt,
(invQstepQ6*(target-predQ10))>>16,
nlsfQuantMaxAmplitudeExt-1,
)might be worth adding a regression test too. A tightly clustered NLSF vector (strong formants / tonal content) seems to reproduce it pretty reliably
|
Thanks @thomas-vilte, good catch! You're right, the clamp arguments were inverted — the raw index was going into the lower-bound slot, so the stage-2 indices were never actually bounded and could run past the decodable I've tried to fix it in 06256a2, and added a regression test over tightly clustered NLSF vectors (the case that triggers it most reliably). @JoTurk I'll also split this into smaller stacked PRs to make it easier to review — I'll open them and link them back here shortly. |
Encoder scaffold and range-coding of the per-field SILK bitstream: subframe gains, shell/pulse excitation, NLSF stage-1/2 indices, and pitch/LTP indices.
Voice activity detection with adaptive high-pass, float LPC and signal-analysis primitives, the 3-stage pitch lag search, Burg LPC analysis, A2NLSF conversion, the find_LPC wrapper, and LTP correlation analysis (find_LTP).
Warped-autocorrelation noise-shape primitives, fixed-point helpers, and the noise-shaping quantizer that range-codes the excitation.
Wire the analysis and quantization stages into a full frame encoder, expose it via opus.Encoder.EncodeSILK, and add the voiced/LTP path.
…tion, LTP scaling Faithful noise-shaping analysis and prediction coefficients, NLSF interpolation across subframes, and LTP state scaling control.
End-to-end encode/decode fidelity tests, encoder documentation, and golangci-lint directives for the Q-format conversions and short DSP variable names.
The raw quantization index was passed as the clamp lower bound instead of the value being clamped, so stage-2 indices were never bounded and could exceed the decodable [-10, 10] range, silently desyncing the range decoder. Add a regression test over tightly clustered NLSF vectors.
06256a2 to
6bd3d82
Compare
|
Sounds good thank you |
|
@JoTurk @thomas-vilte I reorganized the work so it's easier to review. Instead of the one big squashed commit, it's now a series of coherent commits grouped by subsystem: scaffold + bitstream coding, then signal analysis, noise shaping + NSQ, frame assembly + I kept everything in a single PR for now instead of several stacked ones. The encoder was built bottom-up and the lint fixes only really apply to the finished code, so if I split it into separate PRs the intermediate ones would fail golangci-lint. This way CI stays green and the history still tells the story. That said, if you'd rather have multiple PRs I'm happy to do that, just let me know. |
|
IMO it's still super hard for human review, and the direction is still more vibe coding than ai assisted work, I have many comments on the code itself from the design to small stuff like all the unnecessary nolints. |
|
@thomas-vilte : would you be able to manage that split into multiple PRs ? I am not sure I can do it efficiently |
|
I'll read the outlines of this pr and the related specs and try to send you a plan to break it by today |
|
Sorry I thought the request was for me if @thomas-vilte can break it, it would be great |
|
yeah, i'm happy to take care of the split i was planning to work on the SILK encoder anyway, so when i saw this PR i spent some time yesterday and today reading through it to avoid duplicating work. i also mapped the call graph while i was at it, which made it a lot easier to see where the natural split points are i think it breaks down pretty cleanly into: math/bit primitives → shared float DSP → VAD → LPC→NLSF → pitch analysis → Burg/LPC → NLSF quantization → LTP → gains → noise shaping → NSQ → pulses → pitch/LTP bitstream → the nice part is that there are only two small moves i'd make first so everything stands on its own. my plan would be to cherry-pick each piece from this branch, keep François as the author (i'm just repackaging the work), keep each PR small and testable, and review them against the corresponding libopus code as they go @JoTurk i think this will also make the if it helps, i can also open a tracking issue with the detailed breakdown so we can tick pieces off as they land if everyone's happy with that, I'd mark this PR as draft and use it as the reference while the smaller PRs land. i can start opening the first couple today |
3 participants
This adds a pure-Go SILK encoder as the counterpart to the existing SILK decoder, exposed as
opus.Encoder.EncodeSILK(pcm, bandwidth, out). It produces decodable SILK-only Opus packets that round-trip through the existing decoder.Scope
Mono, 20 ms frames, narrowband / mediumband / wideband. The full analysis pipeline is ported from the libopus float SILK encoder:
find_pred_coefs: LTP residual for voiced, gain-normalized input for unvoiced)silk_NSQ_c) and range coding of every field in decode orderVerification
internal/silkunit tests + an end-to-end fidelity test, ~22 dB reconstruction SNR on a voiced signal).go test ./...passes; lint clean under the repo config.Not included yet (follow-ups)
Delayed-decision NSQ (
silk_NSQ_del_dec), NLSF trellis MSVQ (this uses a full stage-1 search with a greedy stage-2 quantizer), the rate-control loop, stereo, hybrid, and 10/40/60 ms frame sizes.