Match ordinary Opus decoding to libopus exactly - #247
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:
|
|
@thomas-vilte ready for review at |
thomas-vilte
left a comment
thomas-vilte
left a comment
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.
d6c4ebd to
a403293
Compare
198f59d to
7364466
Compare
TouchDown1
changed the title
thomas-vilte
left a comment
thomas-vilte
left a comment
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 && |
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) |
| @@ -1478,14 +1522,14 @@ func (d *Decoder) applySilkRedundancyFades(channelCount int) { | |||
| frameStart := fade.startSample * channelCount | |||
| if fade.celtToSilk { | |||
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) { |
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.
1f91658 to
8cb3950
Compare
thomas-vilte
left a comment
thomas-vilte
left a comment
There was a problem hiding this comment.
Re-reviewed at 8cb3950. Approving.
The soft-clipping products now round explicitly, and I checked that end to end instead of reading it: I extracted opus_pcm_soft_clip_impl and opus_limit2_checkwithin1_c from the pinned v1.6.1, built them with -ffp-contract=off, and compared 4,800 clipping frames against this tree. amd64 and arm64 both match libopus bit for bit — float32 output, int16 conversion, and the carried clip memory. Before the fix, arm64 differed on 4,227 frames in float32 and 158 in int16 PCM. The new non-race step on ubuntu and native macOS/arm64 keeps that gated.
The description and EXACTNESS.md scope the claim and name the three #251 cases as excluded, which is what I asked for.
The two temporary PLC relaxations are still here (decoder_plc_corpus_test.go:130 and requireCompatiblePitch), and the description already commits to landing #250 immediately afterwards. I'm approving on that condition: if #250 can't land right away, these shouldn't sit on main on their own.
Locally go test ./... passes on amd64, and also on arm64 without -race under qemu.
2 participants
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 #252 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
ba49dcbcd2afae4b131def3e836d4932c04b28d4(Fix TestExpRotation1BlockOfFour on arm64 #252)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 exactness claim is limited to the pinned profile and tested sequences, with the #251 exclusions below. Merge #250 immediately after #247; do not leave these temporary RMSE and pitch relaxations on main independently.
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.September review follow-up
Explicit exclusions and landing dependency
The pre-existing transition differences in #251 are not fixed or included in this PR's bit-exact compatibility claim: PLC mode/redundancy bookkeeping, Hybrid-to-SILK fade-out ordering around redundancy, and the previous-mode/redundancy guard for CELT-to-SILK fades. RFC conformance is tolerance-based and does not establish universal bit exactness.
Thomas's separate
lshiftSat32issue from #153 is also outside this change. The two intermediate PLC assertion relaxations are acceptable only as a paired landing with #250 immediately following this PR. This update does not merge either PR or substitute for reviewer approval.Stack review boundaries
The stack is #247 → #250 → #256: ordinary decode, PLC/recovery correctness and evidence, then isolated SILK dead-code cleanup. #250 must still land immediately after this PR; the cleanup in #256 adds no numerical fix and can be reviewed separately.