ADIT alsa improvement backports

[amiartus]

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:

jackd -r -d alsa -C hw:1 -P hw:1 -r 48000 -i 2 -o 2
jack_connect system:capture_1 system:playback_2
jack_connect system:capture_1 system:playback_2

Result: audio capture by card is audible on output

Please let me know what you think.

Best Regards,
Adam

[imaami]

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;
}

[imaami]

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.

[amiartus]

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/