Skip to content
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

ALSA backend: Multi ALSA device support #608

Open
wants to merge 42 commits into
base: develop
Choose a base branch
from

Conversation

twischer
Copy link

This PR depends currently on #463

This PR contains the following features for the ALSA audio backend

  • Support multiple devices without using ALSA multi plug-in. Compared to ALSA multi plugin it reduces the CPU load because snd_pcm_avail_update() only needs to be called once for each device per period. alsa_in/out applications use additional audio buffers and therefore increase the audio latency. audio devices attached with the ALSA multi device feature have the same latency as using the original backend implementation. This extension is also required to be able to open and close audio devices dynamically at JACK runtime (see underneath).
  • Rework error handling including retry for read and write
  • Support open close feature (With this feature it is possible to close or stop ALSA devices independently if they are not required but the JACK daemon can run continuously and all JACK client connects stay active. For clients it looks only like a short Xrun. See new -c and -x option)

ALSA multi plugin versus this new ALSA multi device feature
In case of ALSA multi plugin poll() might return when not all slave devices of ALSA multi plugin have frames available because the poll() event of each devices will be forwarded to JACK. Therefore we have to check with snd_pcm_avail_update()
This means a high probability that snd_pcm_avail_update() has to be called for the same devices at the same period multiple times.

With the multi devices support we can repeat poll() till all devices are done (after this poll() the data will be available for all devices.)

Timo Wischer and others added 30 commits June 25, 2020 16:17
Change-Id: Ia6aee61aa7059a70d2014f7de66a41cbaca8c7ed
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Change-Id: I1cab18816980f704b6ae73a37960c1421e46ed9b
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Change-Id: Ic0b9c377e111cb59ed4859819b810842347e8525
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Change-Id: I69bc2a73849431925c14d865bc970e2cbfa2d843
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
because it is not available in QNX

Change-Id: I94a7586c148aa4f9b6cd6fef98a8637e7fea717c
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Change-Id: I40287828155d7b7530d8760724b3fb3b06ae15b7
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Change-Id: I7ea831b96cb774fc69b7934c65797a51e9b9ec6a
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Change-Id: I92760ed539b475e210bc877b335afdbb4a982a04
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Change-Id: Id1dda94d4621a8e77c76c2cd15f08e26a31b87e5
Signed-off-by: Laxmi Devi <Laxmi.Devi@in.bosch.com>
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
shave off a few usec from processing time

Change-Id: I052d168b11caa54d881d651c32822d6db33383b8
Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
(cherry picked from commit 138925dfd5c45c99d7978ba39f99d9d0fc349727)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Change-Id: I9ee88d6b2531622c8d46f69872d30750168c4e4a
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
vsnprintf implementation on QNX can not handle NULL string parameter and
will interrupt, This is a workaround for the same

Change-Id: I18c57ec4b596336cea6f3061caf49c0b33afe198
Signed-off-by: Laxmi Devi <Laxmi.Devi@in.bosch.com>
linux release 1.0

what was tested:
 - playing and recording from multiple devices
 - linking devices, fallback for case when some or all devices are not linked
 - xrun recovery

notes:
 - we should be using epoll instead of poll, ~ +1 manday to implement
 - solution can be simplified to remove doubling parameters for capture and playback
device, instead one struct for abstract "device" could be created that has parameter
capture or playaback, this would simplify code and reduce duplicate lines of code

Change-Id: I457b8e7e8ff0d04db18333b44bf64bf26a253bac
Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Signed-off-by: Kapildev Patel <kapildev.patel@wipro.com>

- adapted to latest version
- removed redundant check

Change-Id: Idf5208ac2d40ece6f1598f8fa2a7d8b80da52309
Signed-off-by: Adam Miartus <external.Adam.Miartus@de.bosch.com>
(cherry picked from commit 58775de5353c9a198ed22ce41775015ef9f95211)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
allowed by compiler and helps with clean commit history when
adding changes

Change-Id: Iaafbe0a01970cf1b176a24d8fdc8cc6ee5c97e3b
Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
later used to restart JackAlsaDriver and allow closing unused devices
(devices whose ports are are not connected)

Change-Id: I886e4b23949e2a5e321194c18043f00f434537cc
Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
(cherry picked from commit 39a31b50b3efbf0a118cf36f679001991147306d)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
will be used to ease transporting data between functions, later will be
used as part of alsa_driver_t struct

parsing of input devices moved to driver_initialize

Change-Id: I17ab2f51595aa3c8a77e36f68adfd4b60d8220a1
Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
(cherry picked from commit 9414ef5852f98af27e3d78b66f198b14e54c4da0)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
on restart request desired state of devices is evaluated, devices will be
closed, left prepared, started depending on selected options or their usage
(number or connected ports)

Change-Id: Id512e013556169dfc3837fd5f10845848a211d5b
Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
(cherry picked from commit 9bdb9d09de12d5b11000d23d0276687a43a36ab2)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
in case ALSA_DRIVER_FEAT_CLOSE_IDLE_DEVS is defined unused devices are
closed on restart request, otherwise unused devices will be kept in
prepared state

Change-Id: I0cd91deb957a4839c5ea8c679bdabe1d0a3c6720
Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
(cherry picked from commit ee4fe3fde6902c41a3a086bba20a75f37ae96373)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
in case ALSA_DRIVER_FEAT_LINK_DEVS is defined, driver will attempt to
snd_pcm_link all devices

Change-Id: I7bd54b8414d0b614786bee1078667a9aa595b58f
Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
(cherry picked from commit da45a136f556e1aeb3b12d3f10e4ba7687c389c8)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Change-Id: I607eba702288677f8401c396c93d9c2d790cd362
Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
(cherry picked from commit efe557426bb9d7fe88f37be27df0f8d681fc8d44)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
allows additional logic to be added to function later

Change-Id: I30a8de53faa66883a7976f69ccc2b312d9f7ecce
Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
(cherry picked from commit 2fb38bca8d19f777a20e6124a41bdfffc9e6a1a2)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
allow discard duplicate array members as a configurable feature

Change-Id: Ibbbfed9ec5633d2bdd23cc516f726ff4ad98c4ed
Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
(cherry picked from commit 65c157936ae94024d9bcd29e82080f53ccee6613)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Change-Id: I3d79e5f2516d8709b70dc700e5c9a04534619661
Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
(cherry picked from commit 7a7d3b3524877533c0d5a9d3490d862d4323d280)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
if enabled with '-x' option starts jackd with all devices closed, when
configured, user must provide number of channels for all devices, otherwise
we would not be able to create any jack ports.

we need to create all jack ports, since the only way to start any device
in this condition is to connect a jack port and issue a call
jack_client_reload_master

Change-Id: I63736eff20a2f2db7405ad512fec11e7bd0a9860
Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
(cherry picked from commit 55d03f05d05e438f89f8e57658c7593d3ec56a41)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Change-Id: I9c4724ca8b0b464dc3b38bac9f1fa6d8b05c186e
Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
(cherry picked from commit 73a34ffc49a9ca7c6fb6f291c365ac80859d44c7)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Change-Id: I68228ae98b4b074847abf547810e35e8e9267f31
Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
(cherry picked from commit d890538d322acbdd4c1b09b37a86b2bdb0f31d75)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
changes signature of function to allow extending the possible modes
of TargetState()

Change-Id: I394d85f4aac9fabb669fb03eedbfc3c6587a2a8e
Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
(cherry picked from commit 9d215ce938556aead96db81126aa5c20be1aa5e3)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
for cases when we need to shutdown jack server or disconnect all devices

Change-Id: Idb3bede50e9f5e86d5997d5f675724846226b0c5
Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
(cherry picked from commit 2d7d8b93d0268bc243a6ad83aa788b57ea6d480c)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
do not close alsa devices that should stay open
do not prepare already prepared device
do not drop frames on device that is not running

Change-Id: Ibb6463bf6604a9e5e0038266bc99d2f8d838e135
Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
(cherry picked from commit f5f8d29ef0d6b934b0a2022e924dafc8894382a5)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Adam Miartus and others added 12 commits June 30, 2020 14:34
'c' decides what state idle device should be put to during reload
api call, when this option is present logic is to 'close' the idle
device, otherwise leave it 'prepared'

'x' enables the idle devices check for jackd init phase, the check
will behave the same way reload api call check call behaves,
therefore the state of the device will depend on 'c' option, if 'x'
option is not provided, devices will be configured to 'running' state
during jack init.

Change-Id: I3f8461884ef0569229f689412d0ec6ff8aeb39e9
Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
Signed-off-by: Adam Miartus <external.adam.miartus@de.bosch.com>
(cherry picked from commit ce2146ef1b2dd1f7fd5c12b288c6b044f9628cc9)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
the upper layer is responsible for deleting the driver in case of failed
initialization

Change-Id: I2fa0850375b1397d0b6907f210ca68d7a73acd03
Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
(cherry picked from commit 8e6734a2b038c502d323154bcc306e70a95c6dd5)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
snd_pcm_plugin_flush will not flush all linked devices on qnx,
each device has to be flushed separately

Change-Id: Ie2921ecc9d9f7cdd88e6b80f6a51e7d7bfd3d954
Signed-off-by: Adam Miartus <external.Adam.Miartus@de.bosch.com>
(cherry picked from commit 8ff259557563bbc9098c5d5435228a51e2b0c4bf)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
error code can now be checked making error handling more
explicit

this change also fixes confusing log in case of idle mode of
operation

the specific error code numbers are not significant because they
were never used in handling errors and in general there is error
log printed each time error occurs

Change-Id: Ieef6c2d72301ba582bfbd101bba7f3a5e228e980
Signed-off-by: Adam Miartus <external.Adam.Miartus@de.bosch.com>
(cherry picked from commit 2ec88d7054e8a985875e33ee49b1241bc55e88f2)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
add more logs to error cases

Change-Id: Ie369972a177b869ea1ca8375556f608304d52a44
Signed-off-by: Adam Miartus <external.Adam.Miartus@de.bosch.com>
(cherry picked from commit 9abd7adb57abdbc1596f47ee9ef0b1a92421f9d4)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
to be used in JackAlsaDriver.cpp

Change-Id: Ica92fe1a1366664e5a9f5eff54dc8d1c8a0405e1
Signed-off-by: Adam Miartus <external.Adam.Miartus@de.bosch.com>
(cherry picked from commit a88639d3e9dade9f2100c93376f28c694786b45b)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
upper layer will be responsible for handling recovery

Change-Id: I507df2976ae569b4953a16e6e334ae5f9c78b59f
Signed-off-by: Adam Miartus <external.Adam.Miartus@de.bosch.com>
(cherry picked from commit aff8dd6e215df7023918dc47264da70b3252ad91)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
read and write can fail, allow retry in process method

Change-Id: I6d29b5cb1a6b849b44c9e0128dd3de7269a35873
Signed-off-by: Adam Miartus <external.Adam.Miartus@de.bosch.com>
(cherry picked from commit e24bd05411b61f6914a601bbd8861882b50065fe)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
both read and write can fail, therefore allow recovery

Change-Id: I32462c68aaa2395da11d2969a73ab5b5f28af879
Signed-off-by: Adam Miartus <external.Adam.Miartus@de.bosch.com>
(cherry picked from commit 1996a103ac2b14062d81ef222cf902c1c45b8812)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
previously some state variables were reused for this
purpose which caused hidden bugs in multi device
implementation, this would cause issues with reloading
the jack server in certain conditions

now separate variable is used and the driver correctly
evaluates sample bytes and format

Change-Id: Ic82ba475c4eaa7baffea032106b62bc0b7e86535
Signed-off-by: Adam Miartus <external.Adam.Miartus@de.bosch.com>
(cherry picked from commit b0dfe85ef1e29fd6d6ff9d69992985a22b2d098f)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
otherwise incorrect sample rate (0) would be used for
comparisons

Change-Id: I93b733f820171fe3a169a5e9c92b416348e90c5e
Signed-off-by: Adam Miartus <external.Adam.Miartus@de.bosch.com>
(cherry picked from commit 94eaf8b0232c0bcda444e9201f36723adbcaeb48)
Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
backend of JACK. Currently this is only supported for the ALSA
backend.

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
Change-Id: I4b634056ea1774db0a264fcff121d2854f6b1a60
@falkTX
Copy link
Member

falkTX commented Jun 30, 2020

You have all the qnx stuff mixed in, so it is impossible to review.

Did you try jackadapter? even though it uses an extra buffer like alsa_in/out, it is an internal client and thus does not have the issue of IPC context switches.

@amiartus
Copy link

amiartus commented Jul 2, 2020

@falkTX with regards to "multi-device" support in alsa backend, are you open to it on concept level? I could find the time to separate the QNX stuff away in case there was a chance it would be accepted...

@falkTX
Copy link
Member

falkTX commented Jul 2, 2020

It is hard to say. Again, did you try jackadapter? how does it compare to this?

@twischer
Copy link
Author

twischer commented Jul 2, 2020

Hello @falkTX,

Did you try jackadapter? even though it uses an extra buffer like alsa_in/out, it is an internal client and thus does not have the issue of IPC context switches.

Our main focus is low latency between all devices. Therefore we used a solution which avoids this additional buffering. Due to our hard audio latency requirements, we are not able to spend an additional audio buffer here. One example use case is Bluetooth Hands free call. Therefore the audio data has to be pre and post processed by echo cancellation noise reduction algorithms and forwarded from mic to Bluetooth and from Bluetooth to speaker (4 audio devices).

@falkTX
Copy link
Member

falkTX commented Jul 2, 2020

quickly looking at the changes, they are just too intrusive.
they also change the parameter types for number of input and output channels, breaking DBus compatibility.
and introduce new arguments, which would need to be documented

did you made some measurements to compare the latency between audioadapter and your approach?
I am very interested to see such numbers
just wondering how worth all of this is..

@amiartus
Copy link

amiartus commented Jul 2, 2020

I have done some testing on my not-so-ideal HW, maybe someone could pick up on better HW later...

I was able to open the additional audio device with audioadapter but was getting errors like

../linux/alsa/JackAlsaAdapter.h:377, reading samples : Invalid argument(-22)
JackRingBuffer::Write : consumer too slow, skip frames = 96

Interestingly enough for the same devices "alsa multi-dev" solution worked without xruns

here are my commands for audioadapter:

jackd -R -d alsa -C hw:1,0 -P hw:1,0 -i 2 -o 2 -r 48000 -p 96 -n 2 &
jack_load audioadapter -i "-i 0 -o 2 -r 48000 -p 96 -n 2 -C null -d hw:0,3"

and for comparison alsa "multi-dev" command:

jackd -R -d alsa -C hw:1,0 -P "hw:1,0 hw:0,3" -i 2 -o "2 2" -r 48000 -p 96 -n 2

this will print jack audioadapter help when jackd is running

jack_load audioadapter -i "-h"

I'm not fully aware of capabilities of the audioadapter, but as far as I remember the multidev solution was running with 6 or more devices at the same time with period sizes of 2ms on both linux and QNX, so it is working quite well and is tested, it would be great to find a way to fit it into upstream. As far as parameters being changes and breaking DBUS - it could be solved by adding more parameters and leaving the existing as is.

@falkTX
Copy link
Member

falkTX commented Oct 11, 2020

I did some tests of my own with audio-adapter, alsa_in/out and even zita-a2j/j2a.
not impressed at all with the audio-adapter, I get same results as you most of the time.
zita one (when running as internal client) works mostly okay, but takes very long to recover when anything at all goes wrong.
alsa_in/out ends up working the best, though it has some issues (does not handle device disconnect for one)

all of those have the issue of an extra latency cycle.
so I am very curious to see how the multi-dev setup would work instead.
from what I can see, it is "yes please!" on that front.

The change of arguments is a blocker though, as mentioned before.
Does this PR accept multiple capture and playback devices separately? (in your example CLI args, you only use "-P")

@twischer
Copy link
Author

Does this PR accept multiple capture and playback devices separately?

additional capture and playback devices can be listed space separated e.g.
... -C "hw:1,0 hw:0,0" -P "hw:1,0 hw:0,3" ...

Currently -P and -C can only specified once. But we could also think about modifying it to support multiple -P and -C options.

@amiartus
Copy link

The change of arguments is a blocker though, as mentioned before.

@falkTX I'm keen to have this merged eventually and I can find the time to adapt the arguments to whatever the acceptable requirements are. Could you let me know what would be acceptable change in the parameters?

As Timo has commented -C and -P work as string list of devices separated by a space, however the original behavior of having only one device in -C or -P is still preserved, so it should not break usage in scripts that were using the parameters before this change.

@falkTX
Copy link
Member

falkTX commented May 20, 2021

Could you let me know what would be acceptable change in the parameters?

I can no longer remember why I was so against this use of "spaces". It does look ugly, but it is also functional I guess.

But anyway if this is going in, it really needs to be in smaller chunks and not one super big PR.

@amiartus
Copy link

amiartus commented May 24, 2021

@twischer I think we could separate the first 10 or so commits into QNX support / bugfix PR

I could have a look at the other half and separate it to couple of PRs

do you still would like to work on this to have it merged?

@twischer
Copy link
Author

@miartad

I think we could separate the first 10 or so commits into QNX support

Yes removing QNX support from this PR is fine for me because QNX is not in scope of community.
(See #463 (comment))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants