Add support for latest DPDFNet models and offline attenuation limit - #3824
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe DPDFNet attenuation limit moved from the top-level offline denoiser configuration into the nested DPDFNet model configuration. Native processing, C/C++ and language bindings, WASM layouts, examples, CLI guidance, and model documentation were updated accordingly. ChangesDPDFNet attenuation limit
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant APIConfig
participant NativeConfig
participant DPDFNetImpl
participant STFT
APIConfig->>NativeConfig: set model.dpdfnet.attenuation_limit_db
NativeConfig->>DPDFNetImpl: initialize attenuation_limit_db_
DPDFNetImpl->>STFT: produce enhanced STFT
DPDFNetImpl->>STFT: blend enhanced STFT with noisy STFT
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@c-api-examples/speech-enhancement-dpdfnet-c-api.c`:
- Around line 20-30: Add the missing wget command for dpdfnet2_48khz_hr.onnx
alongside the existing 48 kHz model downloads in
c-api-examples/speech-enhancement-dpdfnet-c-api.c (lines 20-30) and
python-api-examples/offline-speech-enhancement-dpdfnet.py (lines 18-26), using
the same Hugging Face model location and preserving the existing download
instructions.
In `@sherpa-onnx/csrc/offline-speech-denoiser-dpdfnet-impl.h`:
- Around line 108-115: Update ApplyAttenuationLimit() to return a failure/broken
result when noisy and enhanced STFT shapes differ instead of calling
SHERPA_ONNX_EXIT(-1). Propagate that result through Run() so denoiser.Run()
reports failure without terminating the host process, while preserving the
existing successful attenuation path.
In `@sherpa-onnx/csrc/offline-speech-denoiser-dpdfnet-model-config.cc`:
- Around line 18-20: Update the CLI help text in the offline denoiser model
configuration to list the actual DPDFNet .onnx filenames, replacing the
shorthand baseline and model names with the filenames used elsewhere and keeping
the supported 16 kHz, 8 kHz, and 48 kHz variants consistent with the online
denoiser help.
In `@wasm/speech-enhancement/sherpa-onnx-speech-enhancement.js`:
- Around line 148-161: Update initSherpaOnnxOnlineSpeechDenoiserConfig to return
the allocation wrapper containing ptr, matching the shape expected by
freeConfig, rather than returning the modelConfig directly. Preserve the
existing model defaults and initialization flow.
- Around line 137-138: Update the Module.setValue call for
config.dpdfnetAttenuationLimitDb to use an undefined/null-only fallback rather
than truthiness, preserving NaN for native validation while still defaulting
missing values to 0.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e72c305-aa3c-4d5c-bd48-ca09af3f9380
📒 Files selected for processing (41)
c-api-examples/speech-enhancement-dpdfnet-c-api.ccxx-api-examples/speech-enhancement-dpdfnet-cxx-api.ccdart-api-examples/speech-enhancement-dpdfnet/bin/speech_enhancement_dpdfnet.dartdotnet-examples/speech-enhancement-dpdfnet/Program.csflutter/sherpa_onnx/lib/src/offline_speech_denoiser.dartflutter/sherpa_onnx/lib/src/sherpa_onnx_bindings.dartgo-api-examples/speech-enhancement-dpdfnet/main.goharmony-os/SherpaOnnxHar/sherpa_onnx/src/main/cpp/non-streaming-speech-denoiser.ccjava-api-examples/NonStreamingSpeechEnhancementDpdfNet.javakotlin-api-examples/test_offline_speech_denoiser_dpdfnet.ktnodejs-addon-examples/test_offline_speech_enhancement_dpdfnet.jsnodejs-examples/test-offline-speech-enhancement-dpdfnet.jspascal-api-examples/speech-enhancement-dpdfnet/dpdfnet.paspython-api-examples/README.mdpython-api-examples/offline-speech-enhancement-dpdfnet.pyrust-api-examples/examples/offline_speech_enhancement_dpdfnet.rsscripts/dotnet/OfflineSpeechDenoiserConfig.csscripts/go/sherpa_onnx.goscripts/node-addon-api/lib/types.jssherpa-onnx/c-api/c-api.ccsherpa-onnx/c-api/c-api.hsherpa-onnx/c-api/cxx-api.ccsherpa-onnx/c-api/cxx-api.hsherpa-onnx/c-api/docs/speech-enhancement.doxsherpa-onnx/csrc/offline-speech-denoiser-dpdfnet-impl.hsherpa-onnx/csrc/offline-speech-denoiser-dpdfnet-model-config.ccsherpa-onnx/csrc/offline-speech-denoiser.ccsherpa-onnx/csrc/offline-speech-denoiser.hsherpa-onnx/csrc/online-speech-denoiser-dpdfnet-impl.hsherpa-onnx/csrc/sherpa-onnx-offline-denoiser.ccsherpa-onnx/csrc/sherpa-onnx-online-denoiser.ccsherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/OfflineSpeechDenoiserConfig.javasherpa-onnx/jni/speech-denoiser.ccsherpa-onnx/kotlin-api/OfflineSpeechDenoiser.ktsherpa-onnx/pascal-api/sherpa_onnx.passherpa-onnx/python/csrc/offline-speech-denoiser.ccsherpa-onnx/rust/sherpa-onnx-sys/src/speech_denoiser.rssherpa-onnx/rust/sherpa-onnx/src/offline_speech_denoiser.rsswift-api-examples/speech-enhancement-dpdfnet.swiftwasm/speech-enhancement/sherpa-onnx-speech-enhancement.jswasm/speech-enhancement/sherpa-onnx-wasm-main-speech-enhancement.cc
| Module.setValue( | ||
| ptr + offset, config.dpdfnetAttenuationLimitDb || 0, 'float'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve NaN so native validation can reject it.
config.dpdfnetAttenuationLimitDb || 0 converts NaN to 0, silently bypassing native std::isnan rejection. Use an undefined/null fallback instead of truthiness for this numeric field.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wasm/speech-enhancement/sherpa-onnx-speech-enhancement.js` around lines 137 -
138, Update the Module.setValue call for config.dpdfnetAttenuationLimitDb to use
an undefined/null-only fallback rather than truthiness, preserving NaN for
native validation while still defaulting missing values to 0.
| } | ||
|
|
||
| auto sd_config = GetOfflineSpeechDenoiserConfig(config); | ||
| if (!sd_config.Validate()) { |
There was a problem hiding this comment.
For harmonyOS, we don't validate it since model files can be in a sandbox. Please keep the original code.
| return false; | ||
| } | ||
|
|
||
| if (dpdfnet_attenuation_limit_db > 0.0f && |
There was a problem hiding this comment.
Is there a reasonable range for it?
E.g., < 10? < 100? < 1000?
In C API, this value may be unintialized and the garbage value may be super large. We need to catch it here.
| struct OfflineSpeechDenoiserConfig { | ||
| OfflineSpeechDenoiserModelConfig model; | ||
| // DPDFNet-only offline attenuation limit in dB. A value of 0 disables it. | ||
| float dpdfnet_attenuation_limit_db = 0.0f; |
There was a problem hiding this comment.
This parameter is specific to dpdfnet. Can you move it to the struct OfflineSpeechDenoiserDpdfNetModelConfig?
I suggest that you use AI coding to do the change.
|
@csukuangfj Addressed the comments: moved the attenuation limit into the DPDFNet-specific model config across all bindings, added validation for finite values above 100 dB, and preserved HarmonyOS sandbox behavior by skipping file validation there. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
c-api-examples/speech-enhancement-dpdfnet-c-api.c (1)
20-30: 📐 Maintainability & Code Quality | 🟡 MinorComplete the 48 kHz model download instructions in both examples.
The documentation names
dpdfnet2_48khz_hr.onnxwithout downloading it, while downloading only thedpdfnet848 kHz variant.
c-api-examples/speech-enhancement-dpdfnet-c-api.c#L20-L30: add the missingdpdfnet2_48khz_hr.onnxdownload command.python-api-examples/offline-speech-enhancement-dpdfnet.py#L18-L26: add the same missing download command.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@c-api-examples/speech-enhancement-dpdfnet-c-api.c` around lines 20 - 30, Add the missing dpdfnet2_48khz_hr.onnx download command beside the existing 48 kHz dpdfnet8 download in c-api-examples/speech-enhancement-dpdfnet-c-api.c (lines 20-30) and python-api-examples/offline-speech-enhancement-dpdfnet.py (lines 18-26), matching each example’s existing download style.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@c-api-examples/speech-enhancement-dpdfnet-c-api.c`:
- Around line 20-30: Add the missing dpdfnet2_48khz_hr.onnx download command
beside the existing 48 kHz dpdfnet8 download in
c-api-examples/speech-enhancement-dpdfnet-c-api.c (lines 20-30) and
python-api-examples/offline-speech-enhancement-dpdfnet.py (lines 18-26),
matching each example’s existing download style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fbc3e7ed-8ff3-44a0-9c92-564c0af54183
📒 Files selected for processing (38)
c-api-examples/speech-enhancement-dpdfnet-c-api.ccxx-api-examples/speech-enhancement-dpdfnet-cxx-api.ccdart-api-examples/speech-enhancement-dpdfnet/bin/speech_enhancement_dpdfnet.dartdotnet-examples/speech-enhancement-dpdfnet/Program.csflutter/sherpa_onnx/lib/src/offline_speech_denoiser.dartflutter/sherpa_onnx/lib/src/sherpa_onnx_bindings.dartgo-api-examples/speech-enhancement-dpdfnet/main.goharmony-os/SherpaOnnxHar/sherpa_onnx/src/main/cpp/speech-denoiser.hjava-api-examples/NonStreamingSpeechEnhancementDpdfNet.javakotlin-api-examples/test_offline_speech_denoiser_dpdfnet.ktnodejs-addon-examples/test_offline_speech_enhancement_dpdfnet.jsnodejs-examples/test-offline-speech-enhancement-dpdfnet.jspascal-api-examples/speech-enhancement-dpdfnet/dpdfnet.paspython-api-examples/offline-speech-enhancement-dpdfnet.pyrust-api-examples/examples/offline_speech_enhancement_dpdfnet.rsrust-api-examples/examples/streaming_speech_enhancement_dpdfnet.rsscripts/dotnet/OfflineSpeechDenoiserDpdfNetModelConfig.csscripts/go/sherpa_onnx.goscripts/node-addon-api/lib/types.jssherpa-onnx/c-api/c-api.ccsherpa-onnx/c-api/c-api.hsherpa-onnx/c-api/cxx-api.ccsherpa-onnx/c-api/cxx-api.hsherpa-onnx/c-api/docs/speech-enhancement.doxsherpa-onnx/csrc/offline-speech-denoiser-dpdfnet-impl.hsherpa-onnx/csrc/offline-speech-denoiser-dpdfnet-model-config.ccsherpa-onnx/csrc/offline-speech-denoiser-dpdfnet-model-config.hsherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/OfflineSpeechDenoiserDpdfNetModelConfig.javasherpa-onnx/jni/speech-denoiser.ccsherpa-onnx/kotlin-api/OfflineSpeechDenoiser.ktsherpa-onnx/pascal-api/sherpa_onnx.passherpa-onnx/python/csrc/offline-speech-denoiser-dpdfnet-model-config.ccsherpa-onnx/python/csrc/offline-speech-denoiser.ccsherpa-onnx/rust/sherpa-onnx-sys/src/speech_denoiser.rssherpa-onnx/rust/sherpa-onnx/src/offline_speech_denoiser.rsswift-api-examples/speech-enhancement-dpdfnet.swiftwasm/speech-enhancement/sherpa-onnx-speech-enhancement.jswasm/speech-enhancement/sherpa-onnx-wasm-main-speech-enhancement.cc
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/node-addon-api/lib/types.js
|
Can you fix the CI errors for swift? |
|
@csukuangfj, the errors have been fixed. |
csukuangfj
left a comment
There was a problem hiding this comment.
Can you first rebase or merge the master branch into your current branch dpdfnet_2 to make it easier for review the changes?
|
By the way, you can ignore CI errors not related to your PR. |
This reverts commit aa299c4.
csukuangfj
left a comment
There was a problem hiding this comment.
Thank you for your contribution!
Summary
Summary by CodeRabbit
New Features
Bug Fixes