-
Notifications
You must be signed in to change notification settings - Fork 378
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
ADIT alsa improvement backports #949
base: master
Are you sure you want to change the base?
ADIT alsa improvement backports #949
Conversation
sample_move_d32u24_sS() converts into samples like 0x00****** but S32 format expects samples like 0x********. Therefore it will not use the full volume range when also using sample_move_d32u24_sS() for S32. Change-Id: I4afe9bd0c3b342975536f5f98fa4dadd2eaee02d Signed-off-by: Timo Wischer <[email protected]>
this extraction allowed easier multiplatform implementation, even thought other platform such as QNX are not a functional target by Jack2, it would be beneficial to allow this abstraction as the cost for upstream is minimal Change-Id: I92760ed539b475e210bc877b335afdbb4a982a04 Signed-off-by: Timo Wischer <[email protected]>
75a2f11
to
08c5c42
Compare
@@ -2062,6 +2062,97 @@ discover_alsa_using_apps () | |||
} | |||
} | |||
|
|||
static int | |||
alsa_driver_open (alsa_driver_t *driver, bool is_capture) |
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.
Using SND_PCM_STREAM_PLAYBACK
and SND_PCM_STREAM_CAPTURE
as arguments to a boolean function parameter makes the whole thing unnecessarily complicated, and is at least in theory susceptible to breakage if the ALSA API changes (unlikely but not impossible). You can remove a lot of the if-else boilerplate by being more straightforward with the typing, and adding a couple of variables:
static int
alsa_driver_open (alsa_driver_t *driver, snd_pcm_stream_t stream)
{
snd_pcm_t **phandle;
const char *name;
if (stream == SND_PCM_STREAM_CAPTURE) {
phandle = &driver->capture_handle;
name = driver->alsa_name_capture;
} else {
phandle = &driver->playback_handle;
name = driver->alsa_name_playback;
}
err = snd_pcm_open (phandle, name, stream, SND_PCM_NONBLOCK);
if (err < 0) {
switch (errno) {
case EBUSY:
#ifdef __ANDROID__
jack_error ("\n\nATTENTION: The device \"%s\" is "
"already in use. Please stop the"
" application using it and "
"run JACK again", name);
#else
current_apps = discover_alsa_using_apps ();
if (current_apps) {
jack_error ("\n\nATTENTION: The device \"%s\" is "
"already in use. The following applications "
" are using your soundcard(s) so you should "
" check them and stop them as necessary before "
" trying to start JACK again:\n\n%s", name,
current_apps);
free (current_apps);
} else {
jack_error ("\n\nATTENTION: The device \"%s\" is "
"already in use. Please stop the"
" application using it and "
"run JACK again", name);
}
#endif
break;
case EPERM:
jack_error ("you do not have permission to open "
"the audio device \"%s\" for playback", name);
break;
case EINVAL:
jack_error ("the state of handle or the mode is invalid "
"or invalid state change occured \"%s\" for playback", name);
break;
case ENOENT:
jack_error ("device \"%s\" does not exist for playback", name);
break;
case ENOMEM:
jack_error ("Not enough memory available for allocation for \"%s\" for playback",
name);
break;
case SND_ERROR_INCOMPATIBLE_VERSION:
jack_error ("Version mismatch \"%s\" for playback", name);
break;
}
alsa_driver_delete (driver);
*phandle = NULL;
}
if (*phandle)
snd_pcm_nonblock (*phandle, 0);
return err;
}
@@ -1055,6 +1055,12 @@ alsa_driver_get_channel_addresses (alsa_driver_t *driver, | |||
return 0; | |||
} | |||
|
|||
static int | |||
alsa_driver_stream_start(snd_pcm_t *pcm, bool is_capture) |
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.
What purpose does it serve to add an unused function parameter? If the idea is to make function calls somehow more uniform, then I think applying such an unwavering ideal to even these sort of internal static functions crosses the line from reasonable to dogmatic.
Is there something I'm not seeing here? Does adding bool is_capture
actually lay groundwork for an actually useful change down the line? How?
Also: this is literally a wrapper around calling a function with the same signature. It simply adds a static function effectively to rename an ALSA API function. In my opinion the negative outcome of adding more LOC weighs more than whatever the subjective benefit of this is supposed to be.
But again, if I'm missing some bigger picture, please let me know.
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.
yeah this is a groundwork for additional features. iirc the general feature is adding multiple playback/capture devices that can be started in alsa_driver. this was done some years ago and I still have the patches laying around somewhere on my drive.
it didnt get merged in previous PR due to large amount of changes, so I split in and this is one of the PRs that is a pre-requisite for that work.
I think its this one: https://github.com/miartad/jack2/commits/ami-alsa-multidev/
Hello,
please consider merging these changes developed internally at ADIT.
These changes are part of a larger effort by ADIT to port jack2 to QNX, improve responsiveness of jack2, improve error handling, hardening of jack2 and adding alsa multi device functionality and more.
This specific PR includes general refactoring and code improvements for the alsa driver. This is a precursor for further improvements like QNX port and 'alsa multi device feature' discussed in PR-608
Although QNX port is not going to be merged to upstream, as discussed in previous PRs, this PR is still needed for the PR-608
If we can merge this I have prepared another PR that contains only the alsa multi dev feature patches.
The changed are used in production, but I had to backport them to newest master branch (it was a couple of years since I looked into this) and I have tested the PR on my laptop with a simple test:
Result: audio capture by card is audible on output
Please let me know what you think.
Best Regards,
Adam