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
Handle SoX init/shutdown automatically #572
Conversation
# defined at | ||
# https://fossies.org/dox/sox-14.4.2/sox_8h.html#a8e07e80cebeff3339265d89c387cea93a9ef2b87ec303edfe40751d9a85fadeeb | ||
|
||
|
||
@_audio_backend_guard("sox") | ||
def initialize_sox() -> int: |
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.
I think the whole logic of initialize_sox
/shutdown_sox
can be moved to effect chain and mark them as internal, because users no longer need to worry about it.
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.
Essentially, you suggest closing sox on exit only, right? We can discuss that on a separate pull request.
|
||
_SOX_SUCCESS_CODE = 0 | ||
# defined at | ||
# https://fossies.org/dox/sox-14.4.2/sox_8h.html#a8e07e80cebeff3339265d89c387cea93a9ef2b87ec303edfe40751d9a85fadeeb |
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.
It kind of bugs me to copy constant defined in somewhere else like this.
An alternative approach would be, changing the CPP impl of initialize_sox
to return boolean, indicating success / failure by performing return code check on CPP side.
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 can be done in a separate PR later.
""" | ||
global _SOX_INITIALIZED | ||
if _SOX_INITIALIZED is None: | ||
raise RuntimeError('SoX effects chain has been already shut down. Can not initialize again.') |
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.
We do not need to do this, but raising RuntimeError
with a message would be better than segfault.
if _SOX_INITIALIZED is None: | ||
raise RuntimeError('SoX effects chain has been already shut down. Can not initialize again.') | ||
if not _SOX_INITIALIZED: | ||
import _torch_sox |
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.
It would be better to have debug log here.
logger.debug('Initializing SoX effects chain.')
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.
We could make an explicit commitment to add debugging everywhere, but let's not do that here.
""" | ||
global _SOX_INITIALIZED | ||
if _SOX_INITIALIZED: | ||
import _torch_sox |
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.
Similarly, it would be better to have debug log here.
logger.debug('Shutting down SoX effects chain.')
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.
LGTM
[WIP] Audio preprocessing tutorial. Yay!
This PR
atexit
initialize_sox
/shutdown_sox
, yet they can still do so. This gives better backend abstraction.