fix: resample and downmix WAV files for Tencent Silk encoding - #9100
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- wav_to_tencent_silk assumes 16‑bit samples (hardcoded width=2 in tomono/ratecv and in the duration calculation); consider reading sampwidth from the WAV header and either handling other widths explicitly or raising a clear error when unsupported.
- The downmix logic only triggers for exactly 2 channels and silently passes through any other channel count; it may be safer to either normalize all multi-channel inputs to mono or explicitly reject unsupported channel configurations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- wav_to_tencent_silk assumes 16‑bit samples (hardcoded width=2 in tomono/ratecv and in the duration calculation); consider reading sampwidth from the WAV header and either handling other widths explicitly or raising a clear error when unsupported.
- The downmix logic only triggers for exactly 2 channels and silently passes through any other channel count; it may be safer to either normalize all multi-channel inputs to mono or explicitly reject unsupported channel configurations.
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/tencent_record_helper.py" line_range="76-85" />
<code_context>
rate = wav.getframerate()
- frames = wav.getnframes()
- pcm_data = wav.readframes(frames)
+ channels = wav.getnchannels()
+ pcm_data = wav.readframes(wav.getnframes())
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Hardcoded 16‑bit sample width can break for non‑16‑bit WAVs and skews duration calculation.
This logic assumes 16‑bit PCM (`audioop.tomono(..., 2, ...)`, `audioop.ratecv(..., 2, 1, ...)`, `len(pcm_data) / (2 * rate)`). For files where `wav.getsampwidth() != 2`, mixing, resampling, and duration will be wrong. Please read `sampwidth = wav.getsampwidth()` and pass it into `tomono`/`ratecv`, then compute duration as `len(pcm_data) / (sampwidth * rate)` after downmixing to mono so it works for any PCM width.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request introduces audio downmixing and resampling capabilities to wav_to_tencent_silk using the audioop module, ensuring compatibility with the SILK encoder's supported sample rates and mono channel requirement. It also adds comprehensive test coverage for these scenarios. The review feedback correctly identifies a potential issue where the sample width is hardcoded to 2 (16-bit) during audio operations, which could cause corruption or crashes for WAV files with different sample widths, and provides a robust suggestion to dynamically handle and convert the sample width.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…tDevs#9100) * fix: resample and downmix WAV files for Tencent Silk encoding * fix: improve WAV to Tencent Silk conversion by handling sample width and resampling
…tDevs#9100) * fix: resample and downmix WAV files for Tencent Silk encoding * fix: improve WAV to Tencent Silk conversion by handling sample width and resampling
…tDevs#9100) * fix: resample and downmix WAV files for Tencent Silk encoding * fix: improve WAV to Tencent Silk conversion by handling sample width and resampling
Modifications / 改动点
Fixes: #9089
astrbot/core/utils/tencent_record_helper.py: In wav_to_tencent_silk, added resampling logic before pysilk.encode:
Stereo audio is downmixed to mono via audioop.tomono
Unsupported sample rates (e.g. 44100, 22050 Hz) are resampled to 24000 Hz via audioop.ratecv
Duration calculation updated to use the actual resampled PCM length
Zero external dependencies — audioop is in the Python standard library (3.12) and audioop-lts is already declared in pyproject.toml for Python 3.13+
tests/test_media_utils.py: Enhanced tests:
Parametrized the existing real pysilk end-to-end test to cover 5 scenarios: 24k-mono, 44.1k-mono, 22.05k-mono, 48k-stereo, 44.1k-stereo
Added 3 mock tests verifying resample triggers (unsupported rate), downmix triggers (stereo), and no-op passthrough (supported rate)
Screenshots or Test Results / 运行截图或测试结果
test environment: python3.12
tests/test_media_utils.py::test_tencent_silk_encoding_uses_pysilk_tencent_format[24k-mono] PASSED [ 12%]
tests/test_media_utils.py::test_tencent_silk_encoding_uses_pysilk_tencent_format[44.1k-mono] PASSED [ 25%]
tests/test_media_utils.py::test_tencent_silk_encoding_uses_pysilk_tencent_format[22.05k-mono] PASSED [ 37%]
tests/test_media_utils.py::test_tencent_silk_encoding_uses_pysilk_tencent_format[48k-stereo] PASSED [ 50%]
tests/test_media_utils.py::test_tencent_silk_encoding_uses_pysilk_tencent_format[44.1k-stereo] PASSED [ 62%]
tests/test_media_utils.py::test_wav_to_tencent_silk_resamples_unsupported_rate PASSED [ 75%]
tests/test_media_utils.py::test_wav_to_tencent_silk_resamples_stereo PASSED [ 87%]
tests/test_media_utils.py::test_wav_to_tencent_silk_skips_resample_for_supported_rate PASSED [100%]
================= 8 passed, 41 deselected, 1 warning in 11.89s =================
注:这个 warning 是 Python 3.13 将移除 audioop 的弃用警告
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能,已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试,并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Handle WAV preprocessing for Tencent Silk encoding to support more input formats and ensure correct duration reporting.
Bug Fixes:
Tests: