-
Notifications
You must be signed in to change notification settings - Fork 8.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Skip trim_long_silences in preprocess_wav if webrtcvad not available #376
Conversation
Can do, but add a warning when the import fails because denoising should be enabled by default |
Good idea. I added a print statement if the import fails. Let me know if you prefer an actual warning instead. |
encoder/audio.py
Outdated
@@ -5,10 +5,12 @@ | |||
import numpy as np | |||
import librosa | |||
import struct | |||
import warnings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import warnings | |
from warninings import warn |
encoder/audio.py
Outdated
|
||
try: | ||
import webrtcvad | ||
except: | ||
print("WARNING: Unable to import 'webrtcvad'. Please install for better noise removal.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
print("WARNING: Unable to import 'webrtcvad'. Please install for better noise removal.") | |
warn("Unable to import 'webrtcvad'. This package enables noise removal and is recommended.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This latest commit implements your suggestions. Let me know if you think of anything else
I added a check in each of the preprocess scripts to verify that webrtcvad is installed before proceeding. The check can be disabled but the user is encouraged to install it. Can you take a look at this and tell me whether you want to keep this feature, or modify it? We can also update the training wiki page to remind the user to install webrtcvad since it is no longer in requirements.txt |
Here is an idea for resolving #375 . I also suggest removing webrtcvad from requirements.txt to make installation easier for Windows users. The training instructions will need to include a step to install webrtcvad if using LibriSpeech or LibriTTS. Since training setup is already manual I find that preferable to including it in requirements.txt and complicating the install for everyone else.