pcm: Add missing callbacks that cannot be NULL

pcm: Add missing callbacks that cannot be NULL

[Alexander E. Patrakov]

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.


-- 
Alexander E. Patrakov

[PATCH 1/4] pcm:file: add the missing htimestamp callback

[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

[PATCH 2/4] pcm: rate: add rewindable and forwardable callbacks

[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

Re: [PATCH 2/4] pcm: rate: add rewindable and forwardable callbacks

[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?

                                       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.

Re: [PATCH 2/4] pcm: rate: add rewindable and forwardable callbacks

[Alexander E. Patrakov]

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

Re: [PATCH 2/4] pcm: rate: add rewindable and forwardable callbacks

[Jaroslav Kysela]

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

Re: [PATCH 2/4] pcm: rate: add rewindable and forwardable callbacks

[Alexander E. Patrakov]

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=5256e150eb34cf599e79839feaff7398ed67a499

Thanks, that's indeed a logical step.

-- 
Alexander E. Patrakov

[PATCH 3/4] pcm: multi: implement rewindable and forwardable callbacks

[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

[PATCH 4/4] pcm: null: add rewindable and forwardable callbacks

[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

Re: pcm: Add missing callbacks that cannot be NULL

[Jaroslav Kysela]

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.