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

ADIT alsa improvement backports #949

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

amiartus
Copy link

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

Timo Wischer and others added 12 commits July 24, 2023 21:42
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 <twischer@de.adit-jv.com>
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 <twischer@de.adit-jv.com>
@amiartus amiartus force-pushed the adit-alsa-improvement-backports branch from 75a2f11 to 08c5c42 Compare July 26, 2023 06:35
@@ -2062,6 +2062,97 @@ discover_alsa_using_apps ()
}
}

static int
alsa_driver_open (alsa_driver_t *driver, bool is_capture)
Copy link
Contributor

@imaami imaami Dec 19, 2023

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)
Copy link
Contributor

@imaami imaami Dec 19, 2023

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.

Copy link
Author

@amiartus amiartus Dec 21, 2023

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/

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.

2 participants