alsa-project / alsa-lib

The Advanced Linux Sound Architecture (ALSA) - library
GNU Lesser General Public License v2.1
344 stars 173 forks source link

sw_params_set_start_threshold(ULONG_MAX) is bugged #374

Closed z-s-e closed 6 months ago

z-s-e commented 6 months ago

To disable automatic start of the stream, it seems natural to just use the largest possible value, i.e. call snd_pcm_sw_params_set_start_threshold with ULONG_MAX. However in pcm.c currently the value is cast to snd_pcm_sframes_t (which is long) in the comparison where it is decided to start the stream, which then overflows and produces exactly the wrong result.

From looking at the implementation, it seems the intended way to disable automatic stream start is rather by setting that value to the boundary. I'm not sure whether the boundary value is guaranteed to be not larger than LONG_MAX (which is the largest value that currently works correctly) or not however...

So I see two way to fix this, either improve the comparison to not cast to signed, or amend the documentation of sw_params_set_start_threshold to use either the boundary or LONG_MAX to disable. If you decide on a solution I could prepare a patch if you want.

borine commented 6 months ago

The documentation here states:

If you want to use explicit start (snd_pcm_start), you can set this value greater than ring buffer size (in samples), but use the constant MAXINT is not a bad idea.

Clearly that text is outdated, so your suggestion of LONG_MAX would be better. However, I prefer to use the boundary value for this purpose, which is in fact guaranteed to be no larger than LONG_MAX - buffer_size, but that is just my personal preference.

z-s-e commented 6 months ago

A patch to just simply also allow ULONG_MAX as well is pretty trivial, fyi. I'd recommend going with that, as then you don't have to warn users in the documentation to not use too large values.

diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c
index 7c123379..8457f1ae 100644
--- a/src/pcm/pcm.c
+++ b/src/pcm/pcm.c
@@ -7720,7 +7720,8 @@ snd_pcm_sframes_t snd_pcm_write_areas(snd_pcm_t *pcm, const snd_pcm_channel_area
                        /* some plugins might automatically start the stream */
                        state = __snd_pcm_state(pcm);
                        if (state == SND_PCM_STATE_PREPARED &&
-                           hw_avail >= (snd_pcm_sframes_t) pcm->start_threshold) {
+                               hw_avail >= 0 &&
+                               (snd_pcm_uframes_t)hw_avail >= pcm->start_threshold) {
                                err = __snd_pcm_start(pcm);
                                if (err < 0)
                                        goto _end;
perexg commented 6 months ago

Thanks. Fixed in c3fec78dddc09d82cbceb0dfa1d09d25f05aa92d .