Match ordinary Opus decoding to libopus exactly - #247
Open
TouchDown1 wants to merge 3 commits into
Open
TouchDown1 wants to merge 3 commits into
TouchDown1 wants to merge 3 commits into
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: |
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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #247 +/- ##
==========================================
- Coverage 94.11% 93.63% -0.49%
==========================================
Files 63 66 +3
Lines 11119 11703 +584
==========================================
+ Hits 10465 10958 +493
- Misses 453 533 +80
- Partials 201 212 +11
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:
|
Contributor
Author
|
@thomas-vilte ready for review at |
thomas-vilte
requested changes
Sep 9, 2026
thomas-vilte
left a comment
thomas-vilte
left a comment
Member
There was a problem hiding this comment.
Reviewed at 198f59d. I'm requesting changes here, mostly because I don't think this PR is reviewable as one unit anymore.
The diff is large, but the bigger issue isn't the line count by itself. This PR now changes several independent parts of the decoder: MDCT, arithmetic helpers, fixed-point SILK decode, PLC, pitch search, and normal decode paths.
Those are separate numerical-equivalence claims, and when they're all bundled together it's very hard to tell which change caused a mismatch. I don't feel comfortable approving that as one review.
I'd split this into at least:
- normal decode exactness
- PLC exactness
The normal decode part can be verified against the existing RFC conformance vectors, and the PLC part against the strict corpus. That makes each PR much easier to reason about and also much easier to revert if something downstream breaks.
This is different from #246, where I argued against splitting. That PR was one algorithm with one acceptance gate. Here the changes are independent enough that splitting actually improves the review.
There are also a few concrete issues I'd want fixed either way:
-
decoderRoundedProductshould usefloat32(left * right)rather than theFloat32bitsround-trip. The explicit conversion is what the Go spec actually guarantees as a rounding barrier, and it generates better code. -
TestPLCCorpusnow only applies its baseline checks to the first 502 cases even though the corpus has 1300. The strict bit-exact test covers everything, so this isn't a coverage hole today, but the old checks are silently skipped for most of the corpus. -
I don't think the strict corpus should run under
-race. Locally it's around a 63x slowdown for a deterministic single-goroutine numeric test, and the current 30-minute timeout leaves very little margin.
One other thing I'd make explicit: #246 introduced shared pitch helpers, while this PR splits the decoder copies back out. If that's required for bit-exactness, that's fine, but I think it should be an intentional design decision rather than something that happens implicitly across two PRs.
I'm happy to review the normal-decode part as soon as it's split out.
TouchDown1
force-pushed
the
codex/celt-periodic-plc
branch
from
September 9, 2026 21:14
d6c4ebd to
a403293
Compare
TouchDown1
force-pushed
the
codex/opus-plc-bitexact
branch
from
September 12, 2026 05:08
198f59d to
7364466
Compare
TouchDown1
changed the title
thomas-vilte
requested changes
Sep 15, 2026
thomas-vilte
left a comment
thomas-vilte
left a comment
Member
There was a problem hiding this comment.
I reviewed 474fff6 again. The split resolves the points from my previous review: decoderRoundedProduct is now float32(left * right), the repo-wide timeout is gone, conformance blocks, and the ordinary-decode diff stands on its own.
I found one remaining issue tied to this PR's compatibility claim: softClip is not bit-exact on arm64 (softclip.go:41, :79, :84). Go can fuse x + a*x*x and a += a*2.4e-7, while libopus v1.6.1 rounds the intermediate products separately (src/opus.c:82, :134, :139). I compared both implementations on 4,800 clipping frames: amd64 matched exactly, but arm64 produced 158 different int16 PCM frames. The strict corpus does not exercise clipping, so it would not catch this. Please wrap the three products in float32(...) and add a clipped libopus fixture.
I also left inline notes on two transition differences that predate this PR. I opened #251 for those; I only expect the compatibility claim in the description to be scoped accordingly here. The two PLC test relaxations are acceptable to me if #250 follows immediately.
One unrelated note for clarity: lshiftSat32 clamps after shifting instead of before it. That came from my #153, and I'll fix it separately on main.
| channelCount: d.channels, | ||
| }) | ||
| } | ||
| if previousMode == configurationModeHybrid && |
Member
There was a problem hiding this comment.
I think the order here differs from libopus for a Hybrid -> SILK packet that also carries SILK -> CELT redundancy. We reset CELT and decode the redundant frame above (1133-1145), then decode the Hybrid -> SILK fade-out. libopus decodes the silence frame first (src/opus_decoder.c:622-625), then resets CELT and handles redundancy (from :636). That means our fade-out uses a fresh CELT state rather than the state from the Hybrid packet. This predates the PR and is tracked in #251.
| // Exact received-frame reconstruction changes the state consumed by | ||
| // the approximate PLC from #246. Bound individual drift while the | ||
| // aggregate gate below still requires the periodic PLC improvement. | ||
| limit := max(b.RMSE+1, b.RMSE*1.5) |
Member
| @@ -1478,14 +1522,14 @@ func (d *Decoder) applySilkRedundancyFades(channelCount int) { | |||
| frameStart := fade.startSample * channelCount | |||
| if fade.celtToSilk { | |||
Member
There was a problem hiding this comment.
I noticed that libopus only overlays the CELT -> SILK redundant frame when st->prev_mode != MODE_SILK_ONLY || st->prev_redundancy (src/opus_decoder.c:650). That avoids using redundancy when the first frame of the transition was lost. We apply every celtToSilk fade unconditionally, and the Hybrid path has the same gap. This also predates the PR and is tracked in #251.
| } | ||
| } | ||
|
|
||
| func requireCompatiblePitch(t *testing.T, reference, actual int, label string) { |
Member
There was a problem hiding this comment.
On main, this test uses InDelta(ref, pitch, 1). With the arguments here, it only verifies that the lag is somewhere in [100, 720], so any valid lag passes and #247 alone stops testing pitch. #250 makes this exact, which I prefer. Please land the two PRs together, or keep the +/-1 assertion here until #250 is merged.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
This replaces the former mixed decoder/PLC change with the first half of a two-PR stack. It is rebased directly onto current
mainafter #246 and is limited to ordinary packet decoding. PLC and recovery exactness now live in #250.This PR aligns supported ordinary Opus decode output with pinned generic floating-point libopus by adding:
There is no public API expansion, encoder behavior change, runtime cgo dependency, or neural PLC/DRED implementation.
Stack and scope
b8ebd659d671eae11ea7262b3f66d79f103539ec(Implement periodic CELT packet loss concealment and recovery #246)22244de5a79bd1d6d623c32e72bf1954b56235beThe reference uses generic floating point with fast float approximations enabled, intrinsics/RTCD and neural PLC/DRED disabled, and
OPUS_SET_PHASE_INVERSION_DISABLED(0).Because exact pre-loss decoder history changes the input state seen by #246's approximate PLC, this PR adjusts only the intermediate PLC compatibility assertions needed for the first PR to remain independently green. The stacked #250 replaces those compatibility bounds with exact PLC/reference-state assertions. The final stack does not weaken the bit-exact claim.
Implementation notes
Decoder float32 products that feed reference-ordered additions use explicit rounding barriers.
decoderRoundedProductis deliberately the directfloat32(left * right)operation requested in review. Decoder-only arithmetic is isolated from shared encoder helpers; #250 carries the corresponding PLC-only pitch implementation so encoder packet output remains unchanged.Verification performed before publication
go test ./... -count=1on Windows/amd64CGO_ENABLED=1 go test -race ./... -count=1 -timeout=10mon Linux/amd64 for the final stackgo vet ./...go build ./...go mod verifyThe RFC 6716/8251 workflow checks all 120 published bitstream/output-rate/channel combinations against
opus_compareand now fails the PR on a mismatch.