auto-detect audio backend

[enigmaro]

PulseaudioBackend probe performs runtime check for Pulseaudio Daemon. SoxBackend chooses next available driver upon failure to open sink or source.

[TravisBuddy]

Build status: FAILED Build log: https://travis-ci.org/roc-streaming/roc-toolkit/builds/710807689

[gavv]

macOS travis build is fixed in fresh develop, please rebase.

[gavv]

198

[TravisBuddy]

Build status: PASSED Build log: https://travis-ci.org/roc-streaming/roc-toolkit/builds/712021634

[TravisBuddy]

Build status: PASSED Build log: https://travis-ci.org/roc-streaming/roc-toolkit/builds/714603330

[gavv]

Thanks for update, I'll take a look.

BTW, how did you test it?

[enigmaro]

I tested this on my fedora 30 laptop machine and RPi3. I wrote shell scripts to spawn the roc-recv on my laptop and roc-send on the RPi3 (vice-versa as well). I tried them first with default commands and then added the testing args for specifying driver and device name. Finally, also added testing args for file devices. I tested this with pulseaudio enabled first on both machines and then disabled. I know this is not extensive, Do you suggest that I should write dedicated pipeline tests for this?

[gavv]

I tested this on my fedora 30 laptop machine and RPi3. I wrote shell scripts to spawn the roc-recv on my laptop and roc-send on the RPi3 (vice-versa as well). I tried them first with default commands and then added the testing args for specifying driver and device name. Finally, also added testing args for file devices. I tested this with pulseaudio enabled first on both machines and then disabled. I know this is not extensive, Do you suggest that I should write dedicated pipeline tests for this?

Sounds good. Before merging the PR, I'll test in my environment too.

Currently we don't have any tests that work with real PulseAudio/ALSA devices, I think adding them is out of the scope of this PR. We have a bunch of tasks for adding "real-time" tests that will live in a separate repo and run on real hardware: https://github.com/roc-streaming/roc-toolkit/labels/rt-tests. I think it's a good place for tests with real audio devices too. For now we don't have an issue for that, but this would be definitely be a good addition. If you'd like to work on it in future, feel free to file an issue.

[TravisBuddy]

Build status: FAILED Build log: https://travis-ci.org/roc-streaming/roc-toolkit/builds/715333954

[TravisBuddy]

Build status: PASSED Build log: https://travis-ci.org/roc-streaming/roc-toolkit/builds/715344907

[enigmaro]

I tested this on my fedora 30 laptop machine and RPi3. I wrote shell scripts to spawn the roc-recv on my laptop and roc-send on the RPi3 (vice-versa as well). I tried them first with default commands and then added the testing args for specifying driver and device name. Finally, also added testing args for file devices. I tested this with pulseaudio enabled first on both machines and then disabled. I know this is not extensive, Do you suggest that I should write dedicated pipeline tests for this?

Sounds good. Before merging the PR, I'll test in my environment too.

Currently we don't have any tests that work with real PulseAudio/ALSA devices, I think adding them is out of the scope of this PR. We have a bunch of tasks for adding "real-time" tests that will live in a separate repo and run on real hardware: https://github.com/roc-streaming/roc-toolkit/labels/rt-tests. I think it's a good place for tests with real audio devices too. For now we don't have an issue for that, but this would be definitely be a good addition. If you'd like to work on it in future, feel free to file an issue.

That sounds interesting. I will work on creating tests once I am done with the couple other issues that I am working on now.

[gavv]

That sounds interesting. I will work on creating tests once I am done with the couple other issues that I am working on now.

Awesome!

[TravisBuddy]

Build status: FAILED Build log: https://travis-ci.org/roc-streaming/roc-toolkit/builds/715518302

[TravisBuddy]

Build status: FAILED Build log: https://travis-ci.org/roc-streaming/roc-toolkit/builds/715580489

[TravisBuddy]

Build status: PASSED Build log: https://travis-ci.org/roc-streaming/roc-toolkit/builds/715599744

[gavv]

It seems after removing probe, we have one new problem.

If the user specifies pulse://somename, we'll first go to PulseaudioBackend, and if it returns NULL, we'll then go to SoxBackend.

It means that if there is some bug in PulseaudioSink which causes open failure, instead of reporting failure, we now will silently switch to SoX backend and pretend that everything is OK and continue working, but with higher latency.

That's not very good. If Roc is built with PulseAudio support, I'd prefer to be sure that it will always use PulseaudioSink and will never use SoxSink for PulseAudio, to avoid confusion.

The problem is that now we can't distinguish two cases: 1) backend doesn't support given driver and device; 2) backend supports them but open failed.

In case (1), we should try another backend. In case (2), we should report failure and exit. Earlier we distinguished them because (1) was handled by probe, and (2) was handler by open. But now we just get NULL from open in both cases.

To fix this, we can, for example, introduce status codes returned by open_source() and open_sink(), like: StatusOK, StatusNotSupported, StatusOpenFailed; and handle them in backend dispatcher loop.

Thoughts?

[enigmaro]

It seems after removing probe, we have one new problem.

If the user specifies pulse://somename, we'll first go to PulseaudioBackend, and if it returns NULL, we'll then go to SoxBackend.

It means that if there is some bug in PulseaudioSink which causes open failure, instead of reporting failure, we now will silently switch to SoX backend and pretend that everything is OK and continue working, but with higher latency.

That's not very good. If Roc is built with PulseAudio support, I'd prefer to be sure that it will always use PulseaudioSink and will never use SoxSink for PulseAudio, to avoid confusion.

The problem is that now we can't distinguish two cases: 1) backend doesn't support given driver and device; 2) backend supports them but open failed.

In case (1), we should try another backend. In case (2), we should report failure and exit. Earlier we distinguished them because (1) was handled by probe, and (2) was handler by open. But now we just get NULL from open in both cases.

To fix this, we can, for example, introduce status codes returned by open_source() and open_sink(), like: StatusOK, StatusNotSupported, StatusOpenFailed; and handle them in backend dispatcher loop.

Thoughts?

I agree we cannot distinguish the two but if we want to distinguish case (2) and (1), there is another problem I think. We won't be able to say if open failed because of a bug or because the backend supports the driver and device but daemon was not running right? Status codes seem like a good way to go, but I think we might need to handle them in PulseaudioBackend::open_sink() as well to distinguish whether open failed because of a bug or because pulse daemon failed. Does that concern make sense?

[enigmaro]

Oh no, I think my previous comment was wrong. I think what you said makes sense. This is when the user specifies the driver so yes, I think what you said makes complete sense. Its not important if the daemon failed in the case when user doesn't mention default driver and device anyway. So yes, I can try what you said.

[gavv]

Hmm, good catch, and it seems it ruins the idea. If driver and device are NULL, and PulseaudioBackend::open_sink() returned StatusOpenFailed, we can't report error, we have to go to SoxBackend. However SoxBackend doesn't know that it shouldn't try pulseaudio driver.

One way to fix it is to move iteration through default drivers from backends to backend dispatcher. Backend dispatcher can iterate all backends during initialization, ask each one for the list of default drivers, and collect all default drivers in a single list. Then, when the user asks to open a sink without specifying driver, backend dispatcher will iterate default driver list and try to open default device using each of them.

The last step can be implemented using status codes (StatusNotSupported means go to next backend; StatusOpenFails means stop and report error).

However, if we're going this way and are collecting driver list in backend dispatcher, we can make one step further and just build a mapping of drivers to backends in backend dispatcher. E.g. a list of triplets <driver, backend, is_default>.

Then we can use this list for two purposes 1) iterate default drivers; 2) find appropriate backend by driver name.

[gavv]

Oh no, I think my previous comment was wrong. I think what you said makes sense. This is when the user specifies the driver so yes, I think what you said makes complete sense. Its not important if the daemon failed in the case when user doesn't mention default driver and device anyway. So yes, I can try what you said.

See above, there is a problem indeed.

[enigmaro]

So what would happen when open fails with PulseaudioBackend and user doesn't specify driver. Do we mark that as StatusNotSupported and continue try open with other drivers with Sox then?

[gavv]

If the user doesn't specify driver, we should iterate default drivers until pen succeeds. Since we will do it in backend dispatcher, not in backend, we can be sure that each driver (e.g. "pulseaudio") will be tried only once.

[enigmaro]

Hmm, good catch, and it seems it ruins the idea. If driver and device are NULL, and PulseaudioBackend::open_sink() returned StatusOpenFailed, we can't report error, we have to go to SoxBackend. However SoxBackend doesn't know that it shouldn't try pulseaudio driver.

One way to fix it is to move iteration through default drivers from backends to backend dispatcher. Backend dispatcher can iterate all backends during initialization, ask each one for the list of default drivers, and collect all default drivers in a single list. Then, when the user asks to open a sink without specifying driver, backend dispatcher will iterate default driver list and try to open default device using each of them.

The last step can be implemented using status codes (StatusNotSupported means go to next backend; StatusOpenFails means stop and report error).

However, if we're going this way and are collecting driver list in backend dispatcher, we can make one step further and just build a mapping of drivers to backends in backend dispatcher. E.g. a list of triplets <driver, backend, is_default>.

What do you suppose the is_default flag can help for? Aren't we only querying backends for default drivers? So won't they all be default drivers anyway? Am I missing something?

Then we can use this list for two purposes 1) iterate default drivers; 2) find appropriate backend by driver name.

[gavv]

In two last paragraphs, I'm talking about more generic idea. We can not only accumulate default drivers, but build a list of all drivers and their backends. This mapping can be used for opening both default and other drivers, and also can be used to iterate default drivers. Instead of iterating backends in open, we will iterate drivers in this list. So opening default and non-default drivers will use the same mechanism, for better consistency. What do you think?

[enigmaro]

In two last paragraphs, I'm talking about more generic idea. We can not only accumulate default drivers, but build a list of all drivers and their backends. This mapping can be used for opening both default and other drivers, and also can be used to iterate default drivers. Instead of iterating backends in open, we will iterate drivers in this list. So opening default and non-default drivers will use the same mechanism, for better consistency. What do you think?

I think that makes sense. I will go ahead and cumulate the list of default drivers by querying the backends in the dispatcher, do you have any directions on how we can accumulate all non-default drivers for a given backend though? Is it correct to assume that PulseaudioBackend will not return any non-default drivers when a call is made to query the supported drivers? Also, in the case of SoxBackend, the non-default drivers would be the hidden drivers?

[gavv]

We already have get_driver(StringList&) that can be used to obtain the list of all drivers supported by backend.

We can enhance it by replacing StringList with an array of structs:

bool get_driver(Array<DriverInfo>&);

struct DriverInfo {
    char name[16];
    unsigned flags;
    IBackend* backend;
};

enum DriverFlags {
    DriverDefault, // try this driver when no driver specified
    DriverFile, // this is file driver
    DriverDevice, // this is device driver
    DriverSource, // this driver supports sources
    DriverSink, // this driver supports sinks
};

(You can choose different names).

PulseaudioBackend will add a single driver with name "pulse" and flags DriverDefault|DriverDevice|DriverSink.

SoxBackend will add multiple drivers with different flags. It will add DriverDefault flag for drivers that are currently in default_drivers list.

Hidden drivers are drivers supported by sox but excluded from driver list, i.e. roc pretends that there are no such drivers at all. The reason is that they are redundant or wont work properly.

[enigmaro]

@gavv -- I plan on making the Array of DriverInfo objects a member of the BackendDispatcher class. However, this requires an allocator to be initialized right?

When BackendDispatcher is instantiated in roc_{recv,send,conv}, I see that it passes the context.allocator() to open_sink() and open_source() methods. Would it be okay to pass the same allocator for Array as well? The other way I assume is to create a separate HeapAllocator but I am not sure which one is better?

[enigmaro]

Just realized I could do this with a small embedded capacity.

[gavv]

Yeah, all allocations should go through context allocator. Currently sndio is not used in C API, but it will be in future, and in C API it's possible to have multiple context objects, each one with its own allocator. Maybe we'll allow to provide custom allocator for context.

Currently BackendDispatcher is a singleton. We can make it non-singleton and pass allocator to ctor instead of open.

[enigmaro]

@gavv, I changed the implementation based on our discussion. However, I think we can differentiate cases without the need for status codes.

I separated the cases when user specifies input and does not. When user does not specify input, we iterate until success for a default driver (sorted by priority) and ensured that each driver is tried exactly once. For eg., if we build with pulse support and if opening pulse fails with PulseaudioBackend, SoxBackend will not try to open pulse again.

Otherwise, when user provides input, we now find the appropriate backend for the given device driver and try open. If it is a file input, currently we just use SoxBackend to open file type devices. If an appropriate backend cannot be found for a given driver this means that the driver is not supported by any of the available backends and we report this and exit. If open fails, we report failure for open and exit.

Is there any other reason that we would want these status codes? It seems to me like they may be redundant unless I am missing a point. We can discuss this further when you review the code.

[gavv]

@enigmaro Sorry for huge delay, I finally have time for Roc again. I'm now reviewing the changes.

Is there any other reason that we would want these status codes? It seems to me like they may be redundant unless I am missing a point. We can discuss this further when you review the code.

I think you're right and status codes are not needed with the new approach. StatusNotSupported is now impossible because backend dispatcher wont try unsupported drivers. So only two codes remain: StatusOK and StatusOpenFailed, and it can be just a boolean. So no changes are needed here.

[github-actions[bot]]

:umbrella: The latest upstream change (presumably these) made this pull request unmergeable. Please resolve the merge conflicts.

[gavv]

I've finished and merged commit from this PR manually. Thanks!