fix(silk): PLC output saturates to full scale — SAT16 lower bound was +0x8000 instead of -0x8000 - #3
Open
pagci wants to merge 1 commit into
Open
Conversation
…ead of -0x8000 The c2rust expansion of the frame-write line in silk/PLC.rs (upstream C: `frame[i] = (opus_int16)silk_SAT16(silk_SAT16(silk_RSHIFT_ROUND( silk_SMULWW(sLPC_Q14_ptr[MAX_LPC_ORDER + i], prevGain_Q10[1]), 8)));`) rendered silk_SAT16's LOWER bound `(opus_int16)0x8000` (= -32768) as the bare literal `0x8000` (= +32768 as i32). The comparison `x < 0x8000` is therefore true for every sample that is not positively saturated, so the "clamped low" arm fires almost always and every concealed frame on a SILK/hybrid stream comes out as full-scale garbage. Measured on a 25-frame SILK stream (VOIP @ 12 kbps, 48 kHz mono) followed by NULL-packet concealment: PLC frames come out at RMS ~0.98 (saturated), where reference libopus 1.3.1 conceals the same stream at RMS ~0.11 decaying. Normal (non-concealment) decode is unaffected — this expansion only exists on the PLC output path, which is why the packet test vectors never caught it. Fix: replace the whole expanded expression with the existing helper fns (silk_SAT16 / silk_RSHIFT_ROUND / silk_SMULWW), mirroring the same structural cleanup already applied to silk/CNG.rs — the sign bug class cannot recur once the literal expansion is gone. Adds tests/silk_plc_regression.rs: decodes a SILK stream, requests 3 concealment frames, and asserts they are a decaying continuation instead of saturated output (fails with RMS 0.9807 before this fix; passes after). Note: the published 0.2.0 crate carries the same bug in six sites (three in the pre-refactor CNG.rs as well) — a backport/patch release would help crates.io users; happy to prepare it if wanted.
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
1 participant
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
silk/PLC.rs's concealment frame-write is a c2rust expansion of the upstream C lineand the expansion rendered
silk_SAT16's lower bound(opus_int16)0x8000(=-32768) as the bare literal0x8000(=+32768asi32). The comparisonx < 0x8000is therefore true for every sample that is not positively saturated, so the "clamped low" arm fires almost always — every concealed (PLC) frame on a SILK or hybrid stream comes out as full-scale garbage.Repro / measurement
Decode 25 frames of any SILK-mode stream (e.g. VOIP application at 12 kbps, 48 kHz mono), then call
opus_decode_floatwith a NULL packet (packet-loss concealment):Normal packet decode is unaffected (it tracks reference libopus essentially bit-exactly) — this expansion only exists on the PLC output path, which is why the packet-based test vectors never caught it.
Fix
Replace the whole ~160-line expanded expression with the existing helper functions (
silk_SAT16/silk_RSHIFT_ROUND/silk_SMULWW), mirroring the structural cleanup already applied tosilk/CNG.rs— with the literal expansion gone, this sign-loss bug class cannot recur at this site.Regression test
tests/silk_plc_regression.rs: encodes/decodes a SILK stream, requests 3 concealment frames, and asserts they are a decaying continuation instead of saturated output. Fails withPLC step 0 saturated: rms 0.9807before the fix; passes after.Note on the published crate
unsafe-libopus 0.2.0on crates.io carries the same bug in six sites — the three fixed here plus three in the pre-refactorCNG.rs(master's CNG rewrite already eliminated those). A patch release / backport would help crates.io users (we hit this in production integration and currently carry a vendored patch); happy to prepare the backport if you want it.