* [PATCH 1/4] pcm:file: add the missing htimestamp callback
2014-06-12 10:34 pcm: Add missing callbacks that cannot be NULL Alexander E. Patrakov
@ 2014-06-12 10:34 ` Alexander E. Patrakov
2014-06-12 10:34 ` [PATCH 2/4] pcm: rate: add rewindable and forwardable callbacks Alexander E. Patrakov
` (3 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Alexander E. Patrakov @ 2014-06-12 10:34 UTC (permalink / raw)
To: alsa-devel; +Cc: Alexander E. Patrakov
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,
};
/**
--
2.0.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/4] pcm: rate: add rewindable and forwardable callbacks
2014-06-12 10:34 pcm: Add missing callbacks that cannot be NULL Alexander E. Patrakov
2014-06-12 10:34 ` [PATCH 1/4] pcm:file: add the missing htimestamp callback Alexander E. Patrakov
@ 2014-06-12 10:34 ` Alexander E. Patrakov
2014-06-13 7:13 ` Jaroslav Kysela
2014-06-12 10:34 ` [PATCH 3/4] pcm: multi: implement " Alexander E. Patrakov
` (2 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Alexander E. Patrakov @ 2014-06-12 10:34 UTC (permalink / raw)
To: alsa-devel; +Cc: Alexander E. Patrakov
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,
--
2.0.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/4] pcm: rate: add rewindable and forwardable callbacks
2014-06-12 10:34 ` [PATCH 2/4] pcm: rate: add rewindable and forwardable callbacks Alexander E. Patrakov
@ 2014-06-13 7:13 ` Jaroslav Kysela
2014-06-13 8:44 ` Alexander E. Patrakov
0 siblings, 1 reply; 10+ messages in thread
From: Jaroslav Kysela @ 2014-06-13 7:13 UTC (permalink / raw)
To: Alexander E. Patrakov; +Cc: ALSA development
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,
>
--
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project; Red Hat, Inc.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/4] pcm: rate: add rewindable and forwardable callbacks
2014-06-13 7:13 ` Jaroslav Kysela
@ 2014-06-13 8:44 ` Alexander E. Patrakov
2014-06-13 9:19 ` Jaroslav Kysela
0 siblings, 1 reply; 10+ messages in thread
From: Alexander E. Patrakov @ 2014-06-13 8:44 UTC (permalink / raw)
To: Jaroslav Kysela; +Cc: ALSA development
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.
--
Alexander E. Patrakov
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/4] pcm: rate: add rewindable and forwardable callbacks
2014-06-13 8:44 ` Alexander E. Patrakov
@ 2014-06-13 9:19 ` Jaroslav Kysela
2014-06-13 9:46 ` Alexander E. Patrakov
0 siblings, 1 reply; 10+ messages in thread
From: Jaroslav Kysela @ 2014-06-13 9:19 UTC (permalink / raw)
To: Alexander E. Patrakov; +Cc: ALSA development
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:
>
> 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.
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=5256e150eb34cf599e79839feaff7398ed67a499
Thanks,
Jaroslav
--
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project; Red Hat, Inc.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/4] pcm: multi: implement rewindable and forwardable callbacks
2014-06-12 10:34 pcm: Add missing callbacks that cannot be NULL Alexander E. Patrakov
2014-06-12 10:34 ` [PATCH 1/4] pcm:file: add the missing htimestamp callback Alexander E. Patrakov
2014-06-12 10:34 ` [PATCH 2/4] pcm: rate: add rewindable and forwardable callbacks Alexander E. Patrakov
@ 2014-06-12 10:34 ` Alexander E. Patrakov
2014-06-12 10:34 ` [PATCH 4/4] pcm: null: add " Alexander E. Patrakov
2014-06-13 7:14 ` pcm: Add missing callbacks that cannot be NULL Jaroslav Kysela
4 siblings, 0 replies; 10+ messages in thread
From: Alexander E. Patrakov @ 2014-06-12 10:34 UTC (permalink / raw)
To: alsa-devel; +Cc: Alexander E. Patrakov
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,
--
2.0.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/4] pcm: null: add rewindable and forwardable callbacks
2014-06-12 10:34 pcm: Add missing callbacks that cannot be NULL Alexander E. Patrakov
` (2 preceding siblings ...)
2014-06-12 10:34 ` [PATCH 3/4] pcm: multi: implement " Alexander E. Patrakov
@ 2014-06-12 10:34 ` Alexander E. Patrakov
2014-06-13 7:14 ` pcm: Add missing callbacks that cannot be NULL Jaroslav Kysela
4 siblings, 0 replies; 10+ messages in thread
From: Alexander E. Patrakov @ 2014-06-12 10:34 UTC (permalink / raw)
To: alsa-devel; +Cc: Alexander E. Patrakov
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,
--
2.0.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: pcm: Add missing callbacks that cannot be NULL
2014-06-12 10:34 pcm: Add missing callbacks that cannot be NULL Alexander E. Patrakov
` (3 preceding siblings ...)
2014-06-12 10:34 ` [PATCH 4/4] pcm: null: add " Alexander E. Patrakov
@ 2014-06-13 7:14 ` Jaroslav Kysela
4 siblings, 0 replies; 10+ messages in thread
From: Jaroslav Kysela @ 2014-06-13 7:14 UTC (permalink / raw)
To: Alexander E. Patrakov, alsa-devel
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
--
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project; Red Hat, Inc.
^ permalink raw reply [flat|nested] 10+ messages in thread