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
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 28 additions & 10 deletions linux/alsa/alsa_driver.c
Original file line number Diff line number Diff line change
Expand Up @@ -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/

{
return snd_pcm_start(pcm);
}

int
alsa_driver_start (alsa_driver_t *driver)
{
Expand Down Expand Up @@ -1161,7 +1167,7 @@ alsa_driver_start (alsa_driver_t *driver)
driver->user_nperiods
* driver->frames_per_cycle);

if ((err = snd_pcm_start (driver->playback_handle)) < 0) {
if ((err = alsa_driver_stream_start (driver->playback_handle, SND_PCM_STREAM_PLAYBACK)) < 0) {
jack_error ("ALSA: could not start playback (%s)",
snd_strerror (err));
return -1;
Expand All @@ -1170,7 +1176,7 @@ alsa_driver_start (alsa_driver_t *driver)

if ((driver->capture_handle && driver->capture_and_playback_not_synced)
|| !driver->playback_handle) {
if ((err = snd_pcm_start (driver->capture_handle)) < 0) {
if ((err = alsa_driver_stream_start (driver->capture_handle, SND_PCM_STREAM_CAPTURE)) < 0) {
jack_error ("ALSA: could not start capture (%s)",
snd_strerror (err));
return -1;
Expand Down Expand Up @@ -1352,6 +1358,18 @@ alsa_driver_set_clock_sync_status (alsa_driver_t *driver, channel_t chn,
alsa_driver_clock_sync_notify (driver, chn, status);
}

static int
alsa_driver_poll_descriptors(snd_pcm_t *pcm, struct pollfd *pfds, unsigned int space, bool is_capture)
{
return snd_pcm_poll_descriptors(pcm, pfds, space);
}

static snd_pcm_sframes_t
alsa_driver_avail(alsa_driver_t *driver, snd_pcm_t *pcm, bool is_capture)
{
return snd_pcm_avail_update(pcm);
}

static int under_gdb = FALSE;

jack_nframes_t
Expand Down Expand Up @@ -1392,16 +1410,16 @@ alsa_driver_wait (alsa_driver_t *driver, int extra_fd, int *status, float
nfds = 0;

if (need_playback) {
snd_pcm_poll_descriptors (driver->playback_handle,
alsa_driver_poll_descriptors (driver->playback_handle,
&driver->pfd[0],
driver->playback_nfds);
driver->playback_nfds, SND_PCM_STREAM_PLAYBACK);
nfds += driver->playback_nfds;
}

if (need_capture) {
snd_pcm_poll_descriptors (driver->capture_handle,
alsa_driver_poll_descriptors (driver->capture_handle,
&driver->pfd[nfds],
driver->capture_nfds);
driver->capture_nfds, SND_PCM_STREAM_CAPTURE);
ci = nfds;
nfds += driver->capture_nfds;
}
Expand Down Expand Up @@ -1578,8 +1596,8 @@ alsa_driver_wait (alsa_driver_t *driver, int extra_fd, int *status, float
}

if (driver->capture_handle) {
if ((capture_avail = snd_pcm_avail_update (
driver->capture_handle)) < 0) {
if ((capture_avail = alsa_driver_avail (driver,
driver->capture_handle, SND_PCM_STREAM_CAPTURE)) < 0) {
if (capture_avail == -EPIPE) {
xrun_detected = TRUE;
} else {
Expand All @@ -1593,8 +1611,8 @@ alsa_driver_wait (alsa_driver_t *driver, int extra_fd, int *status, float
}

if (driver->playback_handle) {
if ((playback_avail = snd_pcm_avail_update (
driver->playback_handle)) < 0) {
if ((playback_avail = alsa_driver_avail (driver,
driver->playback_handle, SND_PCM_STREAM_PLAYBACK)) < 0) {
if (playback_avail == -EPIPE) {
xrun_detected = TRUE;
} else {
Expand Down