Closed gavv closed 1 year ago
@gavv Hi, do you still need help with this issue? I can take over it. Sorry, I am new here, could you please give me some insights about the files that need to be modified?
@Tigojiang Hi! Yes, this issue is still help-wanted, but it should be done only after merging #417, which was not finished. If you would like to, you could try to pick up #417 and prepare a new PR taking into account unresolved discussions there.
Since the PR was not rebased quite some time, the "Files changed" tab is a bit of a mess, but you can find individual commits by @enigmaro in his fork: https://github.com/enigmaro/roc-toolkit/commits/develop (last 12 commits). I hope it's possible to cherry-pick most of those commits without conflicts.
Problem
In roc_sndio module, we have support for multiple audio backends, employing different external libraries. Currently they are PulseaudioBackend and SoxBackend.
SoxBackend uses SoX library, which have global initialization function (sox_init) and global configuration set (sox_globals). These facilities are not thread safe. sox_init() should be called once before using SoX, multiple and concurrent calls are not allowed. sox_globals can't be modified concurrently.
This limitation is OK for command-line tools, since we can perform necessary initialization in main() before everything else. Currently we don't provide C API for roc_sndio, so SoX is used only in command-line tools and everything is OK.
However, in future we will provide such an API, and this will cause problems if the user employs both Roc and SoX libraries (directly or indirectly). In this case, there is a need to ensure that SoX setup happens only once.
Solution
Add roc_init() function to the C API. It will perform necessary global pre-initialization. Roc itself doesn't need pre-initialization, but some its dependencies does. Currently the only such dependency is SoX. Later we'll also use libSRTP, which also needs pre-initialization.
Document what initialization does it perform, i.e. document that currently it calls sox_init().
We should allow to call roc_init() multiple times. It should be thread-safe. No matter how many times it was called, it should call sox_init() only once.
If the user didn't call roc_init(), initialization should be performed automatically. In case of SoX, if the user didn't call roc_init(), Roc should call sox_init() automatically when using SoxBackend first time. The user will need roc_init() only when using SoX or libSRTP bypassing Roc (directly or via other dependencies). Most likely, this is a rare case.
roc_init() should have flags that disable pre-initialization of specific libraries. Currently we should add one flag that disables calling sox_init() (call it e.g. ROC_INIT_NO_SOX). If the user calls roc_init() and specifies this flag, Roc should assume that SoX is already initialized and should never call sox_init(). The user will need this flag if some other code already calls sox_init() and the user can't control it. Or if the user want to perform SoX initialization manually.
Implementation
Earlier, sndio::BackendDispatcher and sndio::SoxBackend were both singletons. When BackendDispatcher was used first time, it accessed SoxBackend, and it called sox_init(). BackendDispatcher also has set_frame_size() method, which calls SoxBackend::set_frame_size(), which updates sox_globals. It's called in command-line tools from main().
In #417, we had to make BackendDispatcher non-singleton, while still keeping SoxBackend a singleton.
Note that this issue should be started only after merging #417.
The following improvement is suggested:
Add new sndio::BackendInitializer class. sndio::BackendInitializer should be a singleton. It should have two methods: initialize() and set_frame_size(). If SoX is disabled at compile-time, they are no-op. If SoX is enabled, corresponding methods of SoxBackend should be called. See similar
#ifdef
directives in BackendDispatcher.SoxBackend and PulseaudioBackend will remain singletons. BackendDispatcher (which is non-singleton) wont call SoxBackend::initialize() and SoxBackend::set_frame_size(). These methods will be used only from BackendInitializer (which is singleton).
roc_init() should call BackendInitializer::initialize().
Command-line tools should call BackendInitializer::initialize() and BackendInitializer::set_frame_size().
BackendInitializer::initialize() should have a flag to disable SoX initialization (to remember that it's not needed).
SoxBackend::initialize() should call sox_init(). It should be thread-safe and ensure that sox_init() is called only once, even if SoxBackend::initialize() is called multiple times.
SoxBackend::set_frame_size() should set sox_globals. It should be thread-safe as well.
SoxBackend::set_frame_size(), open_source() and open_sink() should automatically initialize SoX if it wasn't initialized before.
SoxBackend::initialize() and set_frame_size() should fail if we already created sinks or sources and hence performed SoX initialization automatically. We should return an error to roc_init(), and it should fail.