[alsa-devel] pcm: Add missing callbacks that cannot be NULL
Some plugins don't implement some mandatory fast_ops. Thus, these fast_ops get defaulted to NULL, and programs crash when calling the corresponding snd_pcm_* functions.
The following callbacks were missing:
file: htimestamp multi: rewindable, forwardable null: rewindable, forwardable rate: rewindable, forwardable
After this series, all required callbacks are present, but I cannot guarantee that they return meaningful values.
All patches in this series are only compile-tested.
Signed-off-by: Alexander E. Patrakov patrakov@gmail.com --- src/pcm/pcm_file.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/pcm/pcm_file.c b/src/pcm/pcm_file.c index b1f2330..b139f7f 100644 --- a/src/pcm/pcm_file.c +++ b/src/pcm/pcm_file.c @@ -697,6 +697,7 @@ static const snd_pcm_fast_ops_t snd_pcm_file_fast_ops = { .poll_descriptors_count = snd_pcm_generic_poll_descriptors_count, .poll_descriptors = snd_pcm_generic_poll_descriptors, .poll_revents = snd_pcm_generic_poll_revents, + .htimestamp = snd_pcm_generic_real_htimestamp, };
/**
This commit does not fix nonsense values returned by the rewind and forward callbacks. E.g., with period_size = 1024 and buffer_size = 4096, an attempt to rewind 1024 samples from the nearly-full buffer returns 4090.
Due to these nonsense values, the current rate plugin should be treated as non-rewindable. That's why the new callbacks return 0.
Signed-off-by: Alexander E. Patrakov patrakov@gmail.com --- src/pcm/pcm_rate.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c index 2eb4b1b..58ed842 100644 --- a/src/pcm/pcm_rate.c +++ b/src/pcm/pcm_rate.c @@ -689,6 +689,16 @@ static int snd_pcm_rate_reset(snd_pcm_t *pcm) return 0; }
+static snd_pcm_sframes_t snd_pcm_rate_rewindable(snd_pcm_t *pcm ATTRIBUTE_UNUSED) +{ + return 0; +} + +static snd_pcm_sframes_t snd_pcm_rate_forwardable(snd_pcm_t *pcm ATTRIBUTE_UNUSED) +{ + return 0; +} + static snd_pcm_sframes_t snd_pcm_rate_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames) { snd_pcm_rate_t *rate = pcm->private_data; @@ -1221,7 +1231,9 @@ static const snd_pcm_fast_ops_t snd_pcm_rate_fast_ops = { .drop = snd_pcm_generic_drop, .drain = snd_pcm_rate_drain, .pause = snd_pcm_generic_pause, + .rewindable = snd_pcm_rate_rewindable, .rewind = snd_pcm_rate_rewind, + .forwardable = snd_pcm_rate_forwardable, .forward = snd_pcm_rate_forward, .resume = snd_pcm_generic_resume, .writei = snd_pcm_mmap_writei,
Date 12.6.2014 12:34, Alexander E. Patrakov wrote:
This commit does not fix nonsense values returned by the rewind and forward callbacks. E.g., with period_size = 1024 and buffer_size = 4096, an attempt to rewind 1024 samples from the nearly-full buffer returns 4090.
Due to these nonsense values, the current rate plugin should be treated as non-rewindable. That's why the new callbacks return 0.
Signed-off-by: Alexander E. Patrakov patrakov@gmail.com
I don't agree. The snd_pcm_rate_move_applptr() should be fixed instead this blocking attempt. Do you have any simple use case?
Jaroslav
src/pcm/pcm_rate.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c index 2eb4b1b..58ed842 100644 --- a/src/pcm/pcm_rate.c +++ b/src/pcm/pcm_rate.c @@ -689,6 +689,16 @@ static int snd_pcm_rate_reset(snd_pcm_t *pcm) return 0; }
+static snd_pcm_sframes_t snd_pcm_rate_rewindable(snd_pcm_t *pcm ATTRIBUTE_UNUSED) +{
- return 0;
+}
+static snd_pcm_sframes_t snd_pcm_rate_forwardable(snd_pcm_t *pcm ATTRIBUTE_UNUSED) +{
- return 0;
+}
static snd_pcm_sframes_t snd_pcm_rate_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames) { snd_pcm_rate_t *rate = pcm->private_data; @@ -1221,7 +1231,9 @@ static const snd_pcm_fast_ops_t snd_pcm_rate_fast_ops = { .drop = snd_pcm_generic_drop, .drain = snd_pcm_rate_drain, .pause = snd_pcm_generic_pause,
- .rewindable = snd_pcm_rate_rewindable, .rewind = snd_pcm_rate_rewind,
- .forwardable = snd_pcm_rate_forwardable, .forward = snd_pcm_rate_forward, .resume = snd_pcm_generic_resume, .writei = snd_pcm_mmap_writei,
13.06.2014 13:13, Jaroslav Kysela пишет:
Date 12.6.2014 12:34, Alexander E. Patrakov wrote:
This commit does not fix nonsense values returned by the rewind and forward callbacks. E.g., with period_size = 1024 and buffer_size = 4096, an attempt to rewind 1024 samples from the nearly-full buffer returns 4090.
Due to these nonsense values, the current rate plugin should be treated as non-rewindable. That's why the new callbacks return 0.
Signed-off-by: Alexander E. Patrakov patrakov@gmail.com
I don't agree. The snd_pcm_rate_move_applptr() should be fixed instead this blocking attempt. Do you have any simple use case?
I don't understand what you mean by a "simple use case".
And anyway, due to reasons expressed in http://permalink.gmane.org/gmane.linux.alsa.devel/122179 , I insist that 0 is the only correct return value even if someone fixes snd_pcm_rate_move_applptr(), which is not trivial. Also, 0 is not a regression - it was "crash" previously, and I have not changed the currently-broken snd_pcm_rate_move_applptr logic (just for the impossible case if someone does rely on it).
The full fix, after which I do agree to change 0 to something else, would involve:
1. Writing a rewindable resampler library, or a resampler library that offers a public API to save its state. 2. Dropping all other resampler implementations. 3. Extending the snd_pcm_rate_ops_t structure in order to forward either rewinds or save-requests to the resampler. 4. Using these new callbacks.
This translates to several months, or maybe a year of work at the current rate. It has to be done, and I do plan to do this. But it should not block the crash-fix: a suboptimal but valid return value of snd_pcm_rewindable() is better than a crash inside this function.
Date 13.6.2014 10:44, Alexander E. Patrakov wrote:
13.06.2014 13:13, Jaroslav Kysela пишет:
Date 12.6.2014 12:34, Alexander E. Patrakov wrote:
This commit does not fix nonsense values returned by the rewind and forward callbacks. E.g., with period_size = 1024 and buffer_size = 4096, an attempt to rewind 1024 samples from the nearly-full buffer returns 4090.
Due to these nonsense values, the current rate plugin should be treated as non-rewindable. That's why the new callbacks return 0.
Signed-off-by: Alexander E. Patrakov patrakov@gmail.com
I don't agree. The snd_pcm_rate_move_applptr() should be fixed instead this blocking attempt. Do you have any simple use case?
I don't understand what you mean by a "simple use case".
I meant a test case (a simple test program).
And anyway, due to reasons expressed in http://permalink.gmane.org/gmane.linux.alsa.devel/122179 , I insist that 0 is the only correct return value even if someone fixes snd_pcm_rate_move_applptr(), which is not trivial. Also, 0 is not a regression - it was "crash" previously, and I have not changed the currently-broken snd_pcm_rate_move_applptr logic (just for the impossible case if someone does rely on it).
The full fix, after which I do agree to change 0 to something else, would involve:
- Writing a rewindable resampler library, or a resampler library that
offers a public API to save its state. 2. Dropping all other resampler implementations. 3. Extending the snd_pcm_rate_ops_t structure in order to forward either rewinds or save-requests to the resampler. 4. Using these new callbacks.
This translates to several months, or maybe a year of work at the current rate. It has to be done, and I do plan to do this. But it should not block the crash-fix: a suboptimal but valid return value of snd_pcm_rewindable() is better than a crash inside this function.
I see, I agree that the current code is/was tricky and probably broken as you noted.
But along with your patch, the code in rewind/forward callbacks should be also removed. I put yours and this patch to the alsa-lib repository:
http://git.alsa-project.org/?p=alsa-lib.git;a=commit;h=5256e150eb34cf599e798...
Thanks, Jaroslav
13.06.2014 15:19, Jaroslav Kysela wrote:
I see, I agree that the current code is/was tricky and probably broken as you noted.
But along with your patch, the code in rewind/forward callbacks should be also removed. I put yours and this patch to the alsa-lib repository:
http://git.alsa-project.org/?p=alsa-lib.git;a=commit;h=5256e150eb34cf599e798...
Thanks, that's indeed a logical step.
Signed-off-by: Alexander E. Patrakov patrakov@gmail.com --- src/pcm/pcm_multi.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)
diff --git a/src/pcm/pcm_multi.c b/src/pcm/pcm_multi.c index f58852c..a84e0ce 100644 --- a/src/pcm/pcm_multi.c +++ b/src/pcm/pcm_multi.c @@ -555,6 +555,42 @@ static int snd_pcm_multi_channel_info(snd_pcm_t *pcm, snd_pcm_channel_info_t *in return err; }
+static snd_pcm_sframes_t snd_pcm_multi_rewindable(snd_pcm_t *pcm) +{ + snd_pcm_multi_t *multi = pcm->private_data; + unsigned int i; + snd_pcm_sframes_t frames = LONG_MAX; + + for (i = 0; i < multi->slaves_count; ++i) { + snd_pcm_sframes_t f = snd_pcm_rewindable(multi->slaves[i].pcm); + if (f <= 0) + return f; + if (f < frames) + frames = f; + } + + return frames; + +} + +static snd_pcm_sframes_t snd_pcm_multi_forwardable(snd_pcm_t *pcm) +{ + snd_pcm_multi_t *multi = pcm->private_data; + unsigned int i; + snd_pcm_sframes_t frames = LONG_MAX; + + for (i = 0; i < multi->slaves_count; ++i) { + snd_pcm_sframes_t f = snd_pcm_forwardable(multi->slaves[i].pcm); + if (f <= 0) + return f; + if (f < frames) + frames = f; + } + + return frames; + +} + static snd_pcm_sframes_t snd_pcm_multi_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames) { snd_pcm_multi_t *multi = pcm->private_data; @@ -932,7 +968,9 @@ static const snd_pcm_fast_ops_t snd_pcm_multi_fast_ops = { .writen = snd_pcm_mmap_writen, .readi = snd_pcm_mmap_readi, .readn = snd_pcm_mmap_readn, + .rewindable = snd_pcm_multi_rewindable, .rewind = snd_pcm_multi_rewind, + .forwardable = snd_pcm_multi_forwardable, .forward = snd_pcm_multi_forward, .resume = snd_pcm_multi_resume, .link = snd_pcm_multi_link,
Dirty, but consistent with avail_update.
Signed-off-by: Alexander E. Patrakov patrakov@gmail.com --- src/pcm/pcm_null.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/src/pcm/pcm_null.c b/src/pcm/pcm_null.c index a154a5c..f1d2f91 100644 --- a/src/pcm/pcm_null.c +++ b/src/pcm/pcm_null.c @@ -168,6 +168,17 @@ static int snd_pcm_null_pause(snd_pcm_t *pcm, int enable) return 0; }
+static snd_pcm_sframes_t snd_pcm_null_rewindable(snd_pcm_t *pcm) +{ + return pcm->buffer_size; +} + +static snd_pcm_sframes_t snd_pcm_null_forwardable(snd_pcm_t *pcm ATTRIBUTE_UNUSED) +{ + return 0; +} + + static snd_pcm_sframes_t snd_pcm_null_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames) { snd_pcm_null_t *null = pcm->private_data; @@ -325,7 +336,9 @@ static const snd_pcm_fast_ops_t snd_pcm_null_fast_ops = { .drop = snd_pcm_null_drop, .drain = snd_pcm_null_drain, .pause = snd_pcm_null_pause, + .rewindable = snd_pcm_null_rewindable, .rewind = snd_pcm_null_rewind, + .forwardable = snd_pcm_null_forwardable, .forward = snd_pcm_null_forward, .resume = snd_pcm_null_resume, .writei = snd_pcm_null_writei,
Date 12.6.2014 12:34, Alexander E. Patrakov wrote:
Some plugins don't implement some mandatory fast_ops. Thus, these fast_ops get defaulted to NULL, and programs crash when calling the corresponding snd_pcm_* functions.
The following callbacks were missing:
file: htimestamp multi: rewindable, forwardable null: rewindable, forwardable
I applied above three patches. Thanks.
rate: rewindable, forwardable
A comment sent.
Jaroslav
participants (2)
-
Alexander E. Patrakov -
Jaroslav Kysela