Support 40 and 60 ms SILK-only packets

[TouchDown1]

Summary

• extend EncodeSILK to accept one, two, or three 20 ms coding units and emit the RFC 6716 TOC configuration for 20, 40, or 60 ms;
• encode multi-unit packets in one continuous range stream with one VAD/LBRR header;
• add the range-coder initial-bit patch required for delayed SILK VAD flags;
• preserve per-stream predictor state and reject invalid frame sizes.

The public scope remains mono SILK-only Narrowband, Mediumband, and Wideband. CELT and hybrid paths are unchanged.

Verification

• CGO_ENABLED=0 go test -count=1 ./...
• CGO_ENABLED=0 go vet ./...
• 16 kHz mono, 960-sample / 60 ms output was independently decoded by native libopus, including mixed VAD patterns.

Native libopus was used only as an external interoperability oracle; this implementation is pure Go.

Related to #9.

[codecov]

Codecov Report

❌ Patch coverage is 91.42857% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.28%. Comparing base (25f7cc7) to head (442fc4d).

Files with missing lines	Patch %	Lines
internal/rangecoding/encoder.go	78.94%	2 Missing and 2 partials ⚠️
internal/silk/enc_frame.go	94.87%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #219      +/-   ##
==========================================
+ Coverage   93.24%   93.28%   +0.04%     
==========================================
  Files          58       58              
  Lines       10503    10553      +50     
==========================================
+ Hits         9793     9844      +51     
+ Misses        502      500       -2     
- Partials      208      209       +1     

Flag	Coverage Δ
go	93.28% <91.42%> (+0.04%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[thomas-vilte]

Went through this properly and ran it locally. Verdict up front: I'd like some changes, but nothing here looks like a blocker.

Starting with interop, since that's what I wanted to know first. I didn't want to take it on faith, so I checked against libopus 1.6.1: dumped the TOC, payload and the encoder's FinalRange for NB/MB/WB at 20/40/60 ms over periodic, mixed-VAD and noise input, then decoded all 27 with libopus and compared OPUS_GET_FINAL_RANGE. All 27 match, sample counts too. So libopus is walking the same symbol sequence we are, which is about as strong as this gets. I also threw roughly 13,500 packets at it looking for a payload over 1275 bytes and never got one; worst was 1070

The fix commit looks right too. I reverted it locally and the new test does reproduce the desync (0x8b0eaa vs 0x812f4f at repeat 10). That's the assertion I'd want here

What I'd like changed:

Lint. First because it's a minute of work. 9 goconst hits. The cause is a bit indirect: "20ms", "40ms" and "60ms" each already appear once in table_of_contents_header.go and once in decoder_test.go. The new table adds a third of each, which crosses goconst's threshold, and then every site gets reported. Constants in silk_encode_test.go alone drops it back to zero. CI hasn't run on the branch, so nothing has caught it

Nothing tests the VAD header. I stubbed out PatchInitialBits with a bare return and the entire SILK suite still passes. Only TestEncoderPatchInitialBits fails. The new test doesn't reach it either: the promotion sets active = true, so the patch writes 1 over the reserved 1 and does nothing

Reserving zeros instead, like enc_API.c:358-360, is worth doing for parity, but I tried it and it doesn't fix the coverage by itself. A packet with header 0x0b instead of 0xeb, so every VAD flag wrong, still passes everything. What's missing is a test that decodes the VAD bits, on input where a unit actually stays inactive

The VAD/pitch policy doesn't match libopus. find_pitch_lags_FIX.c:115 gates the pitch search on signalType != TYPE_NO_VOICE_ACTIVITY && first_frame_after_reset == 0, and the else branch zeroes pitchL, lagIndex, contourIndex and LTPCorr_Q15. We run the search unconditionally and then promote active to match it. So a VAD-inactive frame can turn voiced here and never there, and our first frame after a reset can come out voiced where libopus keeps it unvoiced

Whether you notice depends on the input. On a fresh encoder we match libopus exactly across an amplitude sweep: 32 and 128 inactive, 512 and up active, same on both sides. On this test's own signal, amp 6000 followed by amp 1024 ten times, we emit VAD=1 on all ten and libopus emits it on four. Caveat on that one: our packets are 108-225 bytes there against libopus's 60, so we're nowhere near the same operating point

I don't think this has to be fixed here. But I'd like it decided and written down, because at the moment it's a bitstream difference that just happened rather than one anyone chose

LTP scale on the conditional units. ltpScaleControl runs for every voiced unit and its Q14 goes into the NSQ, but the index only gets transmitted for the first. The decoder assumes zero when it reads nothing (decode_indices.c:142), so units 2 and 3 quantise against a scale the decoder never applies. libopus forces the index to zero in the non-independent branch and derives the Q14 from it (LTP_scale_ctrl_FIX.c:52-56)

It can't fire today, since SetLossRate only reaches CELT and packetLossPerc stays zero. I forced it: at 25% the units come out [1 2 2] and only the first is sent. This PR is what creates the multi-unit case, so I'd rather the guard go in here than wait for whoever wires loss rate into SILK

EncodeSILK commits state before checking the output buffer. Not yours, it's already there, but 60 ms makes it worse. The DC blocker and the whole SILK predictor advance before the size check at encoder.go:499, so if you get errOutBufferTooSmall, enlarge the buffer and retry, you get a different packet than a fresh encoder would. 295 vs 300 bytes at 20 ms, 613 vs 654 at 60 ms. Happy for this to be its own PR

[TouchDown1]

Got it. Thank you for the thorough review - I'll work through the requested changes.

[TouchDown1]

@thomas-vilte

[TouchDown1]

Sorry if there are too many comments in some places.

[TouchDown1]

Update after incorporating the review feedback and rerunning the exact current PR head:

• df0f7da82e16b38eee4dbe11687a8280bcc659c8 (b191d65 plus the merge from main) passes locally with CGO_ENABLED=0: go test -count=1 ./..., go vet ./..., and git diff --check.
• b191d65 adds/adjusts focused coverage for the VAD header patch, range-patcher failures and invalid inputs, and forces the transmitted/used LTP scale to zero on non-independent units. I agree that state advancement after an output-buffer-too-small error is broader than this PR and should be handled separately.
• Our downstream Meowcaller direct-60-ms tree is green against the original 39935723 commit. I have not yet rerun that complete downstream tree against df0f7da, so I am not claiming that combined result yet.
• We are also exploring a separate, much broader normal-Encode 48 kHz -> SILK 60 ms path. Its current failures are experimental and are not regressions in this PR. One rejected experiment tried to pre-size ICDF use with a zero-valued rangecoding.Encoder{}; the uninitialized range state made normalization/carry-out grow the output without termination. That code is not in this PR, and we have rejected the approach. For context, the verified baseline includes libopus-interoperable CELT mono encoding at 48 kHz/20 ms, SILK mono round-trips at 8/12/16 kHz and 20/40/60 ms, and regression coverage across all SILK/Hybrid/CELT decoder modes, channels, and packet codes.

@thomas-vilte, when you have time, could you please take another look at the current head?

[TouchDown1]

Reworked the review commit and pushed a minimal follow-up:

• wrapped the review commit body to satisfy the metadata gate;
• added the requested duration20Ms, duration40Ms, and duration60Ms test constants (goconst: 0 issues);
• retained the explicit PatchInitialBits success/failure contract and regression coverage;
• retained zero VAD placeholders, mixed inactive/active header coverage, and FinalRange assertions;
• retained fail-closed invalid-size handling and conditional-unit LTP scale parity;
• kept the existing 20 ms VAD/pitch policy explicitly documented and scoped full libopus policy parity to a separate change;
• kept output-buffer retry/state rollback out of this PR, as discussed.

The production diff is unchanged from the previously reviewed b191d65; the only source diff versus the previous PR head is in silk_encode_test.go, so the 27-vector libopus/FinalRange interop result still applies.

Local verification on the rewritten head:

• go test ./...
• go vet ./...
• focused SILK/range-coder regression suite
• golangci-lint v2.10.1 goconst-only: 0 issues
• diff check and commit-message line-length check

Could you please take another look once CI finishes?

[TouchDown1]

The rewritten head created fresh fork workflow runs, and GitHub has put all of them in action_required pending maintainer approval. I confirmed the fork author cannot approve them (403: admin rights required). Could a maintainer approve the runs so CI can proceed? Thanks.

[TouchDown1]

All checks are green now, including Go lint, metadata, the full Go platform matrix, RFC conformance, encoder quality, API compatibility, CodeQL, Codecov, REUSE, and tidy. All review conversations have been addressed/resolved. The fork author cannot enable auto-merge or request a reviewer through the API, so this is ready for maintainer re-review and merge when you are satisfied.

[thomas-vilte]

All green.

Thanks for working through all of that — the review had a lot in it and you
took the substantive parts seriously rather than just making the linter happy.
The VAD header coverage in particular is real now: I stubbed PatchInitialBits
to return true without patching and seven SILK tests fail, where before the
whole suite passed with the mechanism completely disabled. I also re-ran the
libopus interop check after the header reservation flipped to zeros and it
still matches on every vector, so nothing regressed there.

If you haven't already, worth opening issues for the two things we scoped out —
the VAD/pitch policy parity with libopus, and the output-buffer state rollback
in EncodeSILK. Both are real and both are easier to reason about on their own.

[TouchDown1]

Thanks, Thomas. I opened the two scoped follow-ups:

• Align SILK VAD/pitch policy with libopus #223 for SILK VAD/pitch policy parity with libopus
• Make EncodeSILK output-buffer failures state-rollback safe #224 for output-buffer state rollback in EncodeSILK

All checks are green on 442fc4d, and the PR is ready to merge. My fork account does not have MergePullRequest permission, so the final merge needs a maintainer.

[JoTurk]

@TouchDown1 do you plan to contribute and work on opus beyond this and help with the review? Do you want an invite to the org? So you can merge your prs and review other.

[TouchDown1]

@JoTurk Yes, I’d be happy to contribute and help with reviews when I can. My workflow is AI-assisted, and I can’t promise constant availability, but I’d be glad to accept an invite. Thank you!

[JoTurk]

@TouchDown1 thank you, check your email, after you accept the invite you should be able to merge this pr :)

[TouchDown1]

Merged — thank you for the thorough review and for the invite. Glad to join Pion!