alsa-lib: Bug fixes to namehint, dsnoop and minor changes
I've been running these patches in some form for a while. Now tidied up for RFC. It's quite possible I misinterpreted something; the behaviours seem to be quite long-standing, hence a first pass for comments. This has taken me to the _avail() functions, which do not seem to be prepared for signed output, yet callers interpret as signed and ultimately end up in external API. Their handling of wraparound conditions etc. may not be as expected. I've already started to look at these. -- Mark
Ths "namehint" is a list, and there doesn't seem to have been any history where the separator would be a colon. --- src/control/namehint.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/control/namehint.c b/src/control/namehint.c index ecd470f3..d81d3a7e 100644 --- a/src/control/namehint.c +++ b/src/control/namehint.c @@ -543,10 +543,10 @@ static int add_software_devices(snd_config_t *config, snd_config_t *rw_config, * User-defined hints are gathered from namehint.IFACE tree like: * * <code> - * namehint.pcm {<br> + * namehint.pcm [<br> * myfile "file:FILE=/tmp/soundwave.raw|Save sound output to /tmp/soundwave.raw"<br> - * myplug "plug:front:Do all conversions for front speakers"<br> - * } + * myplug "plug:front|Do all conversions for front speakers"<br> + * ] * </code> * * Note: The device description is separated with '|' char. -- 2.17.5
Looks like the documented behaviour was broken in commit 1ba513f9 in 2006, with the introduction of multiple fields. I've chosen to match the described behaviour. Prior to this patch, using namehint could be made to work by exploiting the lack of escaping of the "name", populating the other fields: "plug:front|DESCDo all conversions for front speakers" rather than that which is documented and presumed to be the intention for asoundrc files: "plug:front|Do all conversions for front speakers" Everything seems to strongly suggest nobody is using this feature; I can find no working examples through a web search and probably someone would have hit this bug. It's not documented in configuration, only in the snd_device_name_hint() call. So it would probably clutter things to provide compatibility for the old behaviour. I have found it to be very useful since working in Chromium, where it is the only way to expose chosen ALSA devices to web applications. A temporary buffer is required to null-terminate the string. I see no use of alloca() in the code, presumably to avoid unbounded stack size. So memory is allocated on the heap. --- src/control/namehint.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/src/control/namehint.c b/src/control/namehint.c index d81d3a7e..e4f696ad 100644 --- a/src/control/namehint.c +++ b/src/control/namehint.c @@ -78,6 +78,31 @@ static int hint_list_add(struct hint_list *list, return 0; } +/** + * Add a namehint from string given in a user configuration file + */ +static int hint_list_add_custom(struct hint_list *list, + const char *entry) +{ + int err; + const char *sep; + char *name; + + assert(entry); + + sep = strchr(entry, '|'); + if (sep == NULL) + return hint_list_add(list, entry, NULL); + + name = strndup(entry, sep - entry); + if (name == NULL) + return -ENOMEM; + + err = hint_list_add(list, name, sep + 1); + free(name); + return err; +} + static void zero_handler(const char *file ATTRIBUTE_UNUSED, int line ATTRIBUTE_UNUSED, const char *function ATTRIBUTE_UNUSED, @@ -626,7 +651,7 @@ int snd_device_name_hint(int card, const char *iface, void ***hints) if (snd_config_get_string(snd_config_iterator_entry(i), &str) < 0) continue; - err = hint_list_add(&list, str, NULL); + err = hint_list_add_custom(&list, str); if (err < 0) goto __error; } -- 2.17.5
On systems with a network mounted home directory this is thoroughly useful to allow for a core set of asoundrc settings, but with different settings on different hosts. It's not possibly to implement this in our own asoundrc or local customisation, as it's too late. The installation file must be modified. This is similar to ~/.Xdefaults-* on some systems. --- src/conf/alsa.conf | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/conf/alsa.conf b/src/conf/alsa.conf index 08370108..39195f49 100644 --- a/src/conf/alsa.conf +++ b/src/conf/alsa.conf @@ -24,6 +24,17 @@ "/alsa/asoundrc" ] } + { + @func concat + strings [ + "~/.asoundrc-" + { + @func getenv + vars [ HOSTNAME ] + default "localhost" + } + ] + } ] errors false } -- 2.17.5
This was the original bug that caused me to start looking at the ring buffer functions. In the API this is documented as: "Delay is distance between current application frame position and sound frame position. It's positive and less than buffer size in normal situation, negative on playback underrun and greater than buffer size on capture overrun. " Because dsnoop was returning the buffer space available to the hardware the return value was always quite large, and moved in the wrong direction. With this patch, dsnoop now gives results which are comparable to using the "hw" device directly. My test case was with snd-echo3g (Layla3G). --- src/pcm/pcm_local.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h index 89d4125b..1fa8e61d 100644 --- a/src/pcm/pcm_local.h +++ b/src/pcm/pcm_local.h @@ -589,7 +589,7 @@ static inline snd_pcm_uframes_t snd_pcm_mmap_playback_delay(snd_pcm_t *pcm) static inline snd_pcm_uframes_t snd_pcm_mmap_capture_delay(snd_pcm_t *pcm) { - return snd_pcm_mmap_capture_hw_avail(pcm); + return snd_pcm_mmap_capture_avail(pcm); } static inline snd_pcm_sframes_t snd_pcm_mmap_delay(snd_pcm_t *pcm) -- 2.17.5
--- src/pcm/pcm_local.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h index 1fa8e61d..cf018fc0 100644 --- a/src/pcm/pcm_local.h +++ b/src/pcm/pcm_local.h @@ -582,11 +582,17 @@ static inline snd_pcm_uframes_t snd_pcm_mmap_hw_offset(snd_pcm_t *pcm) return *pcm->hw.ptr % pcm->buffer_size; } +/* + * \retval number of frames pending from application to hardware + */ static inline snd_pcm_uframes_t snd_pcm_mmap_playback_delay(snd_pcm_t *pcm) { return snd_pcm_mmap_playback_hw_avail(pcm); } +/* + * \retval number of frames pending from hardware to application + */ static inline snd_pcm_uframes_t snd_pcm_mmap_capture_delay(snd_pcm_t *pcm) { return snd_pcm_mmap_capture_avail(pcm); -- 2.17.5
The previous implementation would mean that stop_threshold behaved erratically. The intent is to detect that the buffer is too full, and stop. In practice, I don't think this was a bug in practice for applications which don't adjust the stop_threshold. The line above catches those cases. --- src/pcm/pcm_dsnoop.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c index c64df381..790d944c 100644 --- a/src/pcm/pcm_dsnoop.c +++ b/src/pcm/pcm_dsnoop.c @@ -165,7 +165,7 @@ static int snd_pcm_dsnoop_sync_ptr(snd_pcm_t *pcm) // printf("sync ptr diff = %li\n", diff); if (pcm->stop_threshold >= pcm->boundary) /* don't care */ return 0; - if ((avail = snd_pcm_mmap_capture_hw_avail(pcm)) >= pcm->stop_threshold) { + if ((avail = snd_pcm_mmap_capture_avail(pcm)) >= pcm->stop_threshold) { gettimestamp(&dsnoop->trigger_tstamp, pcm->tstamp_type); dsnoop->state = SND_PCM_STATE_XRUN; dsnoop->avail_max = avail; -- 2.17.5
This looks like a simple mistake dating back to 2003 (commit 7470a5b9) where code originated from dmix. --- src/pcm/pcm_dsnoop.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c index 790d944c..3588eb91 100644 --- a/src/pcm/pcm_dsnoop.c +++ b/src/pcm/pcm_dsnoop.c @@ -241,7 +241,7 @@ static int snd_pcm_dsnoop_delay(snd_pcm_t *pcm, snd_pcm_sframes_t *delayp) /* Fall through */ case SNDRV_PCM_STATE_PREPARED: case SNDRV_PCM_STATE_SUSPENDED: - *delayp = snd_pcm_mmap_capture_hw_avail(pcm); + *delayp = snd_pcm_mmap_capture_avail(pcm); return 0; case SNDRV_PCM_STATE_XRUN: return -EPIPE; -- 2.17.5
Match the equivalent funciton for playback. This is on the assumption that values should be capped at zero, which is what _rewindable() implements. --- src/pcm/pcm_dsnoop.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c index 3588eb91..7904314c 100644 --- a/src/pcm/pcm_dsnoop.c +++ b/src/pcm/pcm_dsnoop.c @@ -352,7 +352,7 @@ static int snd_pcm_dsnoop_pause(snd_pcm_t *pcm ATTRIBUTE_UNUSED, int enable ATTR static snd_pcm_sframes_t snd_pcm_dsnoop_rewindable(snd_pcm_t *pcm) { - return snd_pcm_mmap_capture_hw_avail(pcm); + return snd_pcm_mmap_capture_hw_rewindable(pcm); } static snd_pcm_sframes_t snd_pcm_dsnoop_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames) -- 2.17.5
I took time to understand these functions in the context of the rest of the code, which would have been a lot quicker with a comment like this. --- src/pcm/pcm_local.h | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h index cf018fc0..aae58ed3 100644 --- a/src/pcm/pcm_local.h +++ b/src/pcm/pcm_local.h @@ -480,6 +480,13 @@ static inline int snd_pcm_check_error(snd_pcm_t *pcm, int err) return err; } +/** + * \retval number of frames available to the application for playback + * + * This is how far ahead the hardware position in the ring buffer is, + * compared to the application position. ie. for playback it's the + * number of frames in the empty part of the ring buffer. + */ static inline snd_pcm_uframes_t __snd_pcm_playback_avail(snd_pcm_t *pcm, const snd_pcm_uframes_t hw_ptr, const snd_pcm_uframes_t appl_ptr) @@ -498,6 +505,13 @@ static inline snd_pcm_uframes_t snd_pcm_mmap_playback_avail(snd_pcm_t *pcm) return __snd_pcm_playback_avail(pcm, *pcm->hw.ptr, *pcm->appl.ptr); } +/* + * \retval number of frames available to the application for capture + * + * This is how far ahead the hardware position in the ring buffer is + * compared to the application position. ie. for capture, it's the + * number of frames in the filled part of the ring buffer. + */ static inline snd_pcm_uframes_t __snd_pcm_capture_avail(snd_pcm_t *pcm, const snd_pcm_uframes_t hw_ptr, const snd_pcm_uframes_t appl_ptr) @@ -529,11 +543,21 @@ static inline snd_pcm_uframes_t snd_pcm_mmap_avail(snd_pcm_t *pcm) return __snd_pcm_avail(pcm, *pcm->hw.ptr, *pcm->appl.ptr); } +/* + * \retval number of frames available to the hardware for playback + * + * ie. the filled part of the ring buffer + */ static inline snd_pcm_sframes_t snd_pcm_mmap_playback_hw_avail(snd_pcm_t *pcm) { return pcm->buffer_size - snd_pcm_mmap_playback_avail(pcm); } +/* + * \retval number of frames available to the hardware for capture + * + * ie. the empty part of the ring buffer. + */ static inline snd_pcm_sframes_t snd_pcm_mmap_capture_hw_avail(snd_pcm_t *pcm) { return pcm->buffer_size - snd_pcm_mmap_capture_avail(pcm); -- 2.17.5
On Fri, 12 Jun 2020, Mark Hills wrote:
I've been running these patches in some form for a while. Now tidied up for RFC.
It's quite possible I misinterpreted something; the behaviours seem to be quite long-standing, hence a first pass for comments. [...]
Jaroslav, I sent these patches over for RFC. No feedback came, so I'll follow this email with them as PATCH with sign-off. The only changes are a tiny rewording to commit messages. Please let me know of anything that prevents their inclusion and I'll work on it. -- Mark
Ths "namehint" is a list, and there doesn't seem to have been any history where the separator would be a colon. Signed-off-by: Mark Hills <mark@xwax.org> --- src/control/namehint.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/control/namehint.c b/src/control/namehint.c index ecd470f3..d81d3a7e 100644 --- a/src/control/namehint.c +++ b/src/control/namehint.c @@ -543,10 +543,10 @@ static int add_software_devices(snd_config_t *config, snd_config_t *rw_config, * User-defined hints are gathered from namehint.IFACE tree like: * * <code> - * namehint.pcm {<br> + * namehint.pcm [<br> * myfile "file:FILE=/tmp/soundwave.raw|Save sound output to /tmp/soundwave.raw"<br> - * myplug "plug:front:Do all conversions for front speakers"<br> - * } + * myplug "plug:front|Do all conversions for front speakers"<br> + * ] * </code> * * Note: The device description is separated with '|' char. -- 2.17.5
On Mon, 22 Jun 2020 15:15:07 +0200, Mark Hills wrote:
Ths "namehint" is a list, and there doesn't seem to have been any history where the separator would be a colon.
Signed-off-by: Mark Hills <mark@xwax.org>
Thanks, applied. Takashi
Looks like the documented behaviour was broken in commit 1ba513f9 in 2006, with the introduction of multiple fields. I've chosen to match the described behaviour. Prior to this patch, using namehint could be made to work by exploiting the lack of escaping of the "name", populating the other fields: "plug:front|DESCDo all conversions for front speakers" rather than that which is documented and presumed to be the intention for asoundrc files: "plug:front|Do all conversions for front speakers" Everything seems to strongly suggest nobody is using this feature; I can find no working examples through a web search and probably someone would have hit this bug. It's not documented in configuration, only in the snd_device_name_hint() call. So it would probably clutter things to provide compatibility for the old behaviour. I have found it to be very useful since working in Chromium, where it is the only way to expose chosen ALSA devices to web applications. A temporary buffer is required to null-terminate the string. I see no use of alloca() in the code, presumably to avoid unbounded stack size. So memory is allocated on the heap. Signed-off-by: Mark Hills <mark@xwax.org> --- src/control/namehint.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/src/control/namehint.c b/src/control/namehint.c index d81d3a7e..e4f696ad 100644 --- a/src/control/namehint.c +++ b/src/control/namehint.c @@ -78,6 +78,31 @@ static int hint_list_add(struct hint_list *list, return 0; } +/** + * Add a namehint from string given in a user configuration file + */ +static int hint_list_add_custom(struct hint_list *list, + const char *entry) +{ + int err; + const char *sep; + char *name; + + assert(entry); + + sep = strchr(entry, '|'); + if (sep == NULL) + return hint_list_add(list, entry, NULL); + + name = strndup(entry, sep - entry); + if (name == NULL) + return -ENOMEM; + + err = hint_list_add(list, name, sep + 1); + free(name); + return err; +} + static void zero_handler(const char *file ATTRIBUTE_UNUSED, int line ATTRIBUTE_UNUSED, const char *function ATTRIBUTE_UNUSED, @@ -626,7 +651,7 @@ int snd_device_name_hint(int card, const char *iface, void ***hints) if (snd_config_get_string(snd_config_iterator_entry(i), &str) < 0) continue; - err = hint_list_add(&list, str, NULL); + err = hint_list_add_custom(&list, str); if (err < 0) goto __error; } -- 2.17.5
On Mon, 22 Jun 2020 15:15:08 +0200, Mark Hills wrote:
Looks like the documented behaviour was broken in commit 1ba513f9 in 2006, with the introduction of multiple fields.
I've chosen to match the described behaviour. Prior to this patch, using namehint could be made to work by exploiting the lack of escaping of the "name", populating the other fields:
"plug:front|DESCDo all conversions for front speakers"
rather than that which is documented and presumed to be the intention for asoundrc files:
"plug:front|Do all conversions for front speakers"
Everything seems to strongly suggest nobody is using this feature; I can find no working examples through a web search and probably someone would have hit this bug. It's not documented in configuration, only in the snd_device_name_hint() call. So it would probably clutter things to provide compatibility for the old behaviour.
I have found it to be very useful since working in Chromium, where it is the only way to expose chosen ALSA devices to web applications.
A temporary buffer is required to null-terminate the string. I see no use of alloca() in the code, presumably to avoid unbounded stack size. So memory is allocated on the heap.
Signed-off-by: Mark Hills <mark@xwax.org>
Thanks, applied. Takashi
On systems with a network mounted home directory this is thoroughly useful to allow for a core set of asoundrc settings, but with different settings on different hosts. It's not possibly to implement this in our own asoundrc or local customisation, as it's too late. The installation file must be modified. This is similar to ~/.Xdefaults-* on some systems. Signed-off-by: Mark Hills <mark@xwax.org> --- src/conf/alsa.conf | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/conf/alsa.conf b/src/conf/alsa.conf index 18427ec6..4dae0e9c 100644 --- a/src/conf/alsa.conf +++ b/src/conf/alsa.conf @@ -24,6 +24,17 @@ "/alsa/asoundrc" ] } + { + @func concat + strings [ + "~/.asoundrc-" + { + @func getenv + vars [ HOSTNAME ] + default "localhost" + } + ] + } ] errors false } -- 2.17.5
On Mon, 22 Jun 2020 15:15:09 +0200, Mark Hills wrote:
On systems with a network mounted home directory this is thoroughly useful to allow for a core set of asoundrc settings, but with different settings on different hosts.
It's not possibly to implement this in our own asoundrc or local customisation, as it's too late. The installation file must be modified.
This is similar to ~/.Xdefaults-* on some systems.
Signed-off-by: Mark Hills <mark@xwax.org>
This kind of change popping up sometimes in the past, too, and I have a mixed feeling whether to take such a change globally or not. In one side, it can work, but OTOH, if you can deal with that detail, you're certainly able to set up the environment variable easily, too. thanks, Takashi
--- src/conf/alsa.conf | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/src/conf/alsa.conf b/src/conf/alsa.conf index 18427ec6..4dae0e9c 100644 --- a/src/conf/alsa.conf +++ b/src/conf/alsa.conf @@ -24,6 +24,17 @@ "/alsa/asoundrc" ] } + { + @func concat + strings [ + "~/.asoundrc-" + { + @func getenv + vars [ HOSTNAME ] + default "localhost" + } + ] + } ] errors false } -- 2.17.5
On Tue, 23 Jun 2020, Takashi Iwai wrote:
On Mon, 22 Jun 2020 15:15:09 +0200, Mark Hills wrote:
On systems with a network mounted home directory this is thoroughly useful to allow for a core set of asoundrc settings, but with different settings on different hosts.
It's not possibly to implement this in our own asoundrc or local customisation, as it's too late. The installation file must be modified.
This is similar to ~/.Xdefaults-* on some systems.
Signed-off-by: Mark Hills <mark@xwax.org>
This kind of change popping up sometimes in the past, too, and I have a mixed feeling whether to take such a change globally or not.
In one side, it can work, but OTOH, if you can deal with that detail, you're certainly able to set up the environment variable easily, too.
I'm happy for a concern to be raised. Can you clarify, which environment variable? I wasn't aware it was possible to override asoundrc with an environment variable, until I looked up just now and found ALSA_CONFIG_PATH in the code. Though I do have vague recollection of trying this some years ago. I think perhaps I ad issues using it to read other config files. I will re-visit when I have time. FYI To solve my development work, I have: ~/.asoundrc -- my personal settings ~/.asoundrc-xxx -- host specific settings and I must read both, in that order. -- Mark
On Tue, 23 Jun 2020 13:18:49 +0200, Mark Hills wrote:
On Tue, 23 Jun 2020, Takashi Iwai wrote:
On Mon, 22 Jun 2020 15:15:09 +0200, Mark Hills wrote:
On systems with a network mounted home directory this is thoroughly useful to allow for a core set of asoundrc settings, but with different settings on different hosts.
It's not possibly to implement this in our own asoundrc or local customisation, as it's too late. The installation file must be modified.
This is similar to ~/.Xdefaults-* on some systems.
Signed-off-by: Mark Hills <mark@xwax.org>
This kind of change popping up sometimes in the past, too, and I have a mixed feeling whether to take such a change globally or not.
In one side, it can work, but OTOH, if you can deal with that detail, you're certainly able to set up the environment variable easily, too.
I'm happy for a concern to be raised.
Can you clarify, which environment variable?
I wasn't aware it was possible to override asoundrc with an environment variable, until I looked up just now and found ALSA_CONFIG_PATH in the code.
Hrm, you're right, I thought we had a simple override, but it doesn't look so. OK, then it makes sense to take your patch. Or rather better to allow an own environment variable (e.g. $ASOUNDRC) instead? It's more flexible. thanks, Takashi
Though I do have vague recollection of trying this some years ago. I think perhaps I ad issues using it to read other config files. I will re-visit when I have time.
FYI To solve my development work, I have:
~/.asoundrc -- my personal settings ~/.asoundrc-xxx -- host specific settings
and I must read both, in that order.
-- Mark
On Tue, 23 Jun 2020, Takashi Iwai wrote:
On Tue, 23 Jun 2020 13:18:49 +0200, Mark Hills wrote:
On Tue, 23 Jun 2020, Takashi Iwai wrote:
On Mon, 22 Jun 2020 15:15:09 +0200, Mark Hills wrote:
On systems with a network mounted home directory this is thoroughly useful to allow for a core set of asoundrc settings, but with different settings on different hosts.
It's not possibly to implement this in our own asoundrc or local customisation, as it's too late. The installation file must be modified.
This is similar to ~/.Xdefaults-* on some systems.
Signed-off-by: Mark Hills <mark@xwax.org>
This kind of change popping up sometimes in the past, too, and I have a mixed feeling whether to take such a change globally or not.
In one side, it can work, but OTOH, if you can deal with that detail, you're certainly able to set up the environment variable easily, too.
I'm happy for a concern to be raised.
Can you clarify, which environment variable?
I wasn't aware it was possible to override asoundrc with an environment variable, until I looked up just now and found ALSA_CONFIG_PATH in the code.
Hrm, you're right, I thought we had a simple override, but it doesn't look so.
OK, then it makes sense to take your patch. Or rather better to allow an own environment variable (e.g. $ASOUNDRC) instead? It's more flexible.
Why not let me experiment, I'll check ALSA_CONFIG_PATH, and then see what patch I can come up with, at least for my own use case. Something like $ASOUNDRC will depend on whether we want it to augment, or fully replace the configuration. I did do some experiments some time ago with fully replacing ALSA configuration; I find the defaults (eg. "Surround speakers" etc.) to be not a good match for my setup. I found it quite difficult to reason about the variable-driven design of the default asoundrc files and for me it seemed to cause more problems that it was solving. But then I think I discovered I could hack it with 'namehint' and moved on. But in general something which allows policy to be passed to shell profile scripts is often not a bad thing. -- Mark
On Tue, 23 Jun 2020 13:42:12 +0200, Mark Hills wrote:
On Tue, 23 Jun 2020, Takashi Iwai wrote:
On Tue, 23 Jun 2020 13:18:49 +0200, Mark Hills wrote:
On Tue, 23 Jun 2020, Takashi Iwai wrote:
On Mon, 22 Jun 2020 15:15:09 +0200, Mark Hills wrote:
On systems with a network mounted home directory this is thoroughly useful to allow for a core set of asoundrc settings, but with different settings on different hosts.
It's not possibly to implement this in our own asoundrc or local customisation, as it's too late. The installation file must be modified.
This is similar to ~/.Xdefaults-* on some systems.
Signed-off-by: Mark Hills <mark@xwax.org>
This kind of change popping up sometimes in the past, too, and I have a mixed feeling whether to take such a change globally or not.
In one side, it can work, but OTOH, if you can deal with that detail, you're certainly able to set up the environment variable easily, too.
I'm happy for a concern to be raised.
Can you clarify, which environment variable?
I wasn't aware it was possible to override asoundrc with an environment variable, until I looked up just now and found ALSA_CONFIG_PATH in the code.
Hrm, you're right, I thought we had a simple override, but it doesn't look so.
OK, then it makes sense to take your patch. Or rather better to allow an own environment variable (e.g. $ASOUNDRC) instead? It's more flexible.
Why not let me experiment, I'll check ALSA_CONFIG_PATH, and then see what patch I can come up with, at least for my own use case.
Sure, I'm looking forward seeing a good outcome :)
Something like $ASOUNDRC will depend on whether we want it to augment, or fully replace the configuration.
I did do some experiments some time ago with fully replacing ALSA configuration; I find the defaults (eg. "Surround speakers" etc.) to be not a good match for my setup. I found it quite difficult to reason about the variable-driven design of the default asoundrc files and for me it seemed to cause more problems that it was solving. But then I think I discovered I could hack it with 'namehint' and moved on.
But in general something which allows policy to be passed to shell profile scripts is often not a bad thing.
Yes, it makes one's life sometimes easier... Takashi
This was the original bug that caused me to start looking at the ring buffer functions. In the API this is documented as: "Delay is distance between current application frame position and sound frame position. It's positive and less than buffer size in normal situation, negative on playback underrun and greater than buffer size on capture overrun. " Because dsnoop was returning the buffer space available to the hardware the return value was always quite large, and moved in the wrong direction. With this patch, dsnoop now gives results which are comparable to using the "hw" device directly. My test case was with snd-echo3g (Layla3G). Signed-off-by: Mark Hills <mark@xwax.org> --- src/pcm/pcm_local.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h index 89d4125b..1fa8e61d 100644 --- a/src/pcm/pcm_local.h +++ b/src/pcm/pcm_local.h @@ -589,7 +589,7 @@ static inline snd_pcm_uframes_t snd_pcm_mmap_playback_delay(snd_pcm_t *pcm) static inline snd_pcm_uframes_t snd_pcm_mmap_capture_delay(snd_pcm_t *pcm) { - return snd_pcm_mmap_capture_hw_avail(pcm); + return snd_pcm_mmap_capture_avail(pcm); } static inline snd_pcm_sframes_t snd_pcm_mmap_delay(snd_pcm_t *pcm) -- 2.17.5
On Mon, 22 Jun 2020 15:15:10 +0200, Mark Hills wrote:
This was the original bug that caused me to start looking at the ring buffer functions.
In the API this is documented as:
"Delay is distance between current application frame position and sound frame position. It's positive and less than buffer size in normal situation, negative on playback underrun and greater than buffer size on capture overrun. "
Because dsnoop was returning the buffer space available to the hardware the return value was always quite large, and moved in the wrong direction.
With this patch, dsnoop now gives results which are comparable to using the "hw" device directly. My test case was with snd-echo3g (Layla3G).
Signed-off-by: Mark Hills <mark@xwax.org>
Thanks, applied. Takashi
Signed-off-by: Mark Hills <mark@xwax.org> --- src/pcm/pcm_local.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h index 1fa8e61d..cf018fc0 100644 --- a/src/pcm/pcm_local.h +++ b/src/pcm/pcm_local.h @@ -582,11 +582,17 @@ static inline snd_pcm_uframes_t snd_pcm_mmap_hw_offset(snd_pcm_t *pcm) return *pcm->hw.ptr % pcm->buffer_size; } +/* + * \retval number of frames pending from application to hardware + */ static inline snd_pcm_uframes_t snd_pcm_mmap_playback_delay(snd_pcm_t *pcm) { return snd_pcm_mmap_playback_hw_avail(pcm); } +/* + * \retval number of frames pending from hardware to application + */ static inline snd_pcm_uframes_t snd_pcm_mmap_capture_delay(snd_pcm_t *pcm) { return snd_pcm_mmap_capture_avail(pcm); -- 2.17.5
On Mon, 22 Jun 2020 15:15:11 +0200, Mark Hills wrote:
Signed-off-by: Mark Hills <mark@xwax.org>
Thanks, applied. Takashi
The previous implementation would mean that stop_threshold behaved erratically. The intent is to detect that the buffer is too full, and stop. In practice, I don't think this was a bug in practice for applications which don't adjust the stop_threshold. The line above catches those cases. Signed-off-by: Mark Hills <mark@xwax.org> --- src/pcm/pcm_dsnoop.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c index c64df381..790d944c 100644 --- a/src/pcm/pcm_dsnoop.c +++ b/src/pcm/pcm_dsnoop.c @@ -165,7 +165,7 @@ static int snd_pcm_dsnoop_sync_ptr(snd_pcm_t *pcm) // printf("sync ptr diff = %li\n", diff); if (pcm->stop_threshold >= pcm->boundary) /* don't care */ return 0; - if ((avail = snd_pcm_mmap_capture_hw_avail(pcm)) >= pcm->stop_threshold) { + if ((avail = snd_pcm_mmap_capture_avail(pcm)) >= pcm->stop_threshold) { gettimestamp(&dsnoop->trigger_tstamp, pcm->tstamp_type); dsnoop->state = SND_PCM_STATE_XRUN; dsnoop->avail_max = avail; -- 2.17.5
On Mon, 22 Jun 2020 15:15:12 +0200, Mark Hills wrote:
The previous implementation would mean that stop_threshold behaved erratically. The intent is to detect that the buffer is too full, and stop.
In practice, I don't think this was a bug in practice for applications which don't adjust the stop_threshold. The line above catches those cases.
Signed-off-by: Mark Hills <mark@xwax.org>
Thanks, applied. Takashi
This looks like a simple mistake dating back to 2003 (commit 7470a5b9) where code originated from dmix. Signed-off-by: Mark Hills <mark@xwax.org> --- src/pcm/pcm_dsnoop.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c index 790d944c..3588eb91 100644 --- a/src/pcm/pcm_dsnoop.c +++ b/src/pcm/pcm_dsnoop.c @@ -241,7 +241,7 @@ static int snd_pcm_dsnoop_delay(snd_pcm_t *pcm, snd_pcm_sframes_t *delayp) /* Fall through */ case SNDRV_PCM_STATE_PREPARED: case SNDRV_PCM_STATE_SUSPENDED: - *delayp = snd_pcm_mmap_capture_hw_avail(pcm); + *delayp = snd_pcm_mmap_capture_avail(pcm); return 0; case SNDRV_PCM_STATE_XRUN: return -EPIPE; -- 2.17.5
On Mon, 22 Jun 2020 15:15:13 +0200, Mark Hills wrote:
This looks like a simple mistake dating back to 2003 (commit 7470a5b9) where code originated from dmix.
Signed-off-by: Mark Hills <mark@xwax.org>
Thanks, applied. Takashi
Match the equivalent funciton for playback. This is on the assumption that values should be capped at zero, which is what _rewindable() implements. Signed-off-by: Mark Hills <mark@xwax.org> --- src/pcm/pcm_dsnoop.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c index 3588eb91..7904314c 100644 --- a/src/pcm/pcm_dsnoop.c +++ b/src/pcm/pcm_dsnoop.c @@ -352,7 +352,7 @@ static int snd_pcm_dsnoop_pause(snd_pcm_t *pcm ATTRIBUTE_UNUSED, int enable ATTR static snd_pcm_sframes_t snd_pcm_dsnoop_rewindable(snd_pcm_t *pcm) { - return snd_pcm_mmap_capture_hw_avail(pcm); + return snd_pcm_mmap_capture_hw_rewindable(pcm); } static snd_pcm_sframes_t snd_pcm_dsnoop_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames) -- 2.17.5
On Mon, 22 Jun 2020 15:15:14 +0200, Mark Hills wrote:
Match the equivalent funciton for playback. This is on the assumption that values should be capped at zero, which is what _rewindable() implements.
Signed-off-by: Mark Hills <mark@xwax.org>
Thanks, applied. Takashi
I took time to understand these functions in the context of the rest of the code, which would have been a lot quicker with a comment like this. Signed-off-by: Mark Hills <mark@xwax.org> --- src/pcm/pcm_local.h | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h index cf018fc0..aae58ed3 100644 --- a/src/pcm/pcm_local.h +++ b/src/pcm/pcm_local.h @@ -480,6 +480,13 @@ static inline int snd_pcm_check_error(snd_pcm_t *pcm, int err) return err; } +/** + * \retval number of frames available to the application for playback + * + * This is how far ahead the hardware position in the ring buffer is, + * compared to the application position. ie. for playback it's the + * number of frames in the empty part of the ring buffer. + */ static inline snd_pcm_uframes_t __snd_pcm_playback_avail(snd_pcm_t *pcm, const snd_pcm_uframes_t hw_ptr, const snd_pcm_uframes_t appl_ptr) @@ -498,6 +505,13 @@ static inline snd_pcm_uframes_t snd_pcm_mmap_playback_avail(snd_pcm_t *pcm) return __snd_pcm_playback_avail(pcm, *pcm->hw.ptr, *pcm->appl.ptr); } +/* + * \retval number of frames available to the application for capture + * + * This is how far ahead the hardware position in the ring buffer is + * compared to the application position. ie. for capture, it's the + * number of frames in the filled part of the ring buffer. + */ static inline snd_pcm_uframes_t __snd_pcm_capture_avail(snd_pcm_t *pcm, const snd_pcm_uframes_t hw_ptr, const snd_pcm_uframes_t appl_ptr) @@ -529,11 +543,21 @@ static inline snd_pcm_uframes_t snd_pcm_mmap_avail(snd_pcm_t *pcm) return __snd_pcm_avail(pcm, *pcm->hw.ptr, *pcm->appl.ptr); } +/* + * \retval number of frames available to the hardware for playback + * + * ie. the filled part of the ring buffer + */ static inline snd_pcm_sframes_t snd_pcm_mmap_playback_hw_avail(snd_pcm_t *pcm) { return pcm->buffer_size - snd_pcm_mmap_playback_avail(pcm); } +/* + * \retval number of frames available to the hardware for capture + * + * ie. the empty part of the ring buffer. + */ static inline snd_pcm_sframes_t snd_pcm_mmap_capture_hw_avail(snd_pcm_t *pcm) { return pcm->buffer_size - snd_pcm_mmap_capture_avail(pcm); -- 2.17.5
On Mon, 22 Jun 2020 15:15:15 +0200, Mark Hills wrote:
I took time to understand these functions in the context of the rest of the code, which would have been a lot quicker with a comment like this.
Signed-off-by: Mark Hills <mark@xwax.org>
Thanks, applied. Takashi
participants (2)
-
Mark Hills -
Takashi Iwai