Bug in snd_pcm_ioplug_avail_update()?

Bug in snd_pcm_ioplug_avail_update()?

[Rob Duncan]

It seems to me that there's something wrong in
snd_pcm_ioplug_avail_update() for capture IO plugins.

Once a bunch of conditions are met it makes this call:

	__snd_pcm_mmap_begin(pcm, &areas, &offset, &size);
	result = io->data->callback->transfer(io->data, areas, offset, size);
	if (result < 0)
		return result;

This transfers size frames from the IO plugin, which in general is not
all the available data: it's limited by the amount of contiguous space
in the mmap.

However, the function returns like this:

	avail = snd_pcm_mmap_avail(pcm);
	if (avail > io->avail_max)
		io->avail_max = avail;
	return (snd_pcm_sframes_t)avail;

But this is all the available data in the IO plugin, without considering
the contiguous space limitation. This means that there is now
uninitialized data in the mmap, and some data still in the IO plugin
that has yet to be transferred.

I'm running into this misbehaviour with a rate conversion plugin pulling
data from an IO plugin.  I get chunks of old data showing up later in
the stream, I think because the mmap buffer is not being fully written
to.

I suspect that this avail mismatch tricks snd_pcm_rate_avail_update()
into thinking that it can grab an integral number of periods of data
from the mmap, which is not the case.

But I'm quite tangled up trying to follow the control flow here so I
could certainly be mistaken.  I'd appreciate any insight into what might
be going wrong.

Thanks,

Rob.

Re: Bug in snd_pcm_ioplug_avail_update()?

[Takashi Iwai]

On Sat, 14 Jul 2018 00:10:29 +0200,
Rob Duncan wrote:
> 
> It seems to me that there's something wrong in
> snd_pcm_ioplug_avail_update() for capture IO plugins.
> 
> Once a bunch of conditions are met it makes this call:
> 
> 	__snd_pcm_mmap_begin(pcm, &areas, &offset, &size);
> 	result = io->data->callback->transfer(io->data, areas, offset, size);
> 	if (result < 0)
> 		return result;
> 
> This transfers size frames from the IO plugin, which in general is not
> all the available data: it's limited by the amount of contiguous space
> in the mmap.
> 
> However, the function returns like this:
> 
> 	avail = snd_pcm_mmap_avail(pcm);
> 	if (avail > io->avail_max)
> 		io->avail_max = avail;
> 	return (snd_pcm_sframes_t)avail;
> 
> But this is all the available data in the IO plugin, without considering
> the contiguous space limitation. This means that there is now
> uninitialized data in the mmap, and some data still in the IO plugin
> that has yet to be transferred.
> 
> I'm running into this misbehaviour with a rate conversion plugin pulling
> data from an IO plugin.  I get chunks of old data showing up later in
> the stream, I think because the mmap buffer is not being fully written
> to.
> 
> I suspect that this avail mismatch tricks snd_pcm_rate_avail_update()
> into thinking that it can grab an integral number of periods of data
> from the mmap, which is not the case.
> 
> But I'm quite tangled up trying to follow the control flow here so I
> could certainly be mistaken.  I'd appreciate any insight into what might
> be going wrong.

Yeah, this looks like a bug.  The code there supposed to copy the
whole data that is available, while snd_pcm_mmap_begin() gives the
range only for a single copy action, so it won't fill if the region to
fill is split across the buffer boundary.

I guess we need some open-code there to achieve the whole data copy.
The avail value from snd_pcm_mmap_avail() can be moved before the
action, so that we can avoid to calculate this twice.

If you have already some fix patch, it'd be helpful.  Otherwise I'll
fix it up some time later.


thanks,

Takashi

Re: Bug in snd_pcm_ioplug_avail_update()?

[Rob Duncan]

At 11:38 on Mon, Jul 16 2018, Takashi wrote:

> Yeah, this looks like a bug.  The code there supposed to copy the
> whole data that is available, while snd_pcm_mmap_begin() gives the
> range only for a single copy action, so it won't fill if the region to
> fill is split across the buffer boundary.
>
> I guess we need some open-code there to achieve the whole data copy.
> The avail value from snd_pcm_mmap_avail() can be moved before the
> action, so that we can avoid to calculate this twice.
>
> If you have already some fix patch, it'd be helpful.  Otherwise I'll
> fix it up some time later.

How about this?

Rob.

>From 467766fc0bd2fb0981d699d018114fa348ddc6ac Mon Sep 17 00:00:00 2001
From: Rob Duncan <rduncan@teslamotors.com>
Date: Mon, 16 Jul 2018 16:35:23 -0700
Subject: [PATCH] pcm: ioplug: Transfer all available data

The snd_pcm_mmap_begin() call returns the amount of contiguous data,
which is less than the total available if it wraps around the buffer
boundary.

If we don't handle this split we leave stale data in the buffer that
should have been overwritten, as well as unread data in the io_plugin
that gets transferred on a subsequent call at the wrong offset.

Signed-off-by: Rob Duncan <rduncan@teslamotors.com>
---
 src/pcm/pcm_ioplug.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c
index 6d52c27b..90e55d0a 100644
--- a/src/pcm/pcm_ioplug.c
+++ b/src/pcm/pcm_ioplug.c
@@ -713,6 +713,8 @@ static snd_pcm_sframes_t snd_pcm_ioplug_avail_update(snd_pcm_t *pcm)
 	ioplug_priv_t *io = pcm->private_data;
 	snd_pcm_uframes_t avail;
 
+	avail = snd_pcm_mmap_avail(pcm);
+
 	snd_pcm_ioplug_hw_ptr_update(pcm);
 	if (io->data->state == SND_PCM_STATE_XRUN)
 		return -EPIPE;
@@ -728,9 +730,15 @@ static snd_pcm_sframes_t snd_pcm_ioplug_avail_update(snd_pcm_t *pcm)
 			result = io->data->callback->transfer(io->data, areas, offset, size);
 			if (result < 0)
 				return result;
+
+			if (size < avail) {
+				result = io->data->callback->transfer(io->data, areas,
+								      0, avail - size);
+				if (result < 0)
+					return result;
+			}
 		}
 	}
-	avail = snd_pcm_mmap_avail(pcm);
 	if (avail > io->avail_max)
 		io->avail_max = avail;
 	return (snd_pcm_sframes_t)avail;
-- 
2.15.1 (Apple Git-101)

Re: Bug in snd_pcm_ioplug_avail_update()?

[Takashi Iwai]

On Tue, 17 Jul 2018 01:53:36 +0200,
Rob Duncan wrote:
> 
> 
> At 11:38 on Mon, Jul 16 2018, Takashi wrote:
> 
> > Yeah, this looks like a bug.  The code there supposed to copy the
> > whole data that is available, while snd_pcm_mmap_begin() gives the
> > range only for a single copy action, so it won't fill if the region to
> > fill is split across the buffer boundary.
> >
> > I guess we need some open-code there to achieve the whole data copy.
> > The avail value from snd_pcm_mmap_avail() can be moved before the
> > action, so that we can avoid to calculate this twice.
> >
> > If you have already some fix patch, it'd be helpful.  Otherwise I'll
> > fix it up some time later.
> 
> How about this?
> 
> Rob.
> 
> >From 467766fc0bd2fb0981d699d018114fa348ddc6ac Mon Sep 17 00:00:00 2001
> From: Rob Duncan <rduncan@teslamotors.com>
> Date: Mon, 16 Jul 2018 16:35:23 -0700
> Subject: [PATCH] pcm: ioplug: Transfer all available data
> 
> The snd_pcm_mmap_begin() call returns the amount of contiguous data,
> which is less than the total available if it wraps around the buffer
> boundary.
> 
> If we don't handle this split we leave stale data in the buffer that
> should have been overwritten, as well as unread data in the io_plugin
> that gets transferred on a subsequent call at the wrong offset.
> 
> Signed-off-by: Rob Duncan <rduncan@teslamotors.com>

The code change is similar as what I had in my mind, but...


> ---
>  src/pcm/pcm_ioplug.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c
> index 6d52c27b..90e55d0a 100644
> --- a/src/pcm/pcm_ioplug.c
> +++ b/src/pcm/pcm_ioplug.c
> @@ -713,6 +713,8 @@ static snd_pcm_sframes_t snd_pcm_ioplug_avail_update(snd_pcm_t *pcm)
>  	ioplug_priv_t *io = pcm->private_data;
>  	snd_pcm_uframes_t avail;
>  
> +	avail = snd_pcm_mmap_avail(pcm);

This call should be put after the hwptr update.  That is...


> +
>  	snd_pcm_ioplug_hw_ptr_update(pcm);
>  	if (io->data->state == SND_PCM_STATE_XRUN)
>  		return -EPIPE;

... put here.

In addition:

> @@ -728,9 +730,15 @@ static snd_pcm_sframes_t snd_pcm_ioplug_avail_update(snd_pcm_t *pcm)
>  			result = io->data->callback->transfer(io->data, areas, offset, size);
>  			if (result < 0)
>  				return result;
> +
> +			if (size < avail) {
> +				result = io->data->callback->transfer(io->data, areas,
> +								      0, avail - size);
> +				if (result < 0)
> +					return result;
> +			}

Please give a short comment about the buffer boundary wrap for
readers.


Thanks!

Takashi

Re: Bug in snd_pcm_ioplug_avail_update()?

[Rob Duncan]

At 22:04 on Mon, Jul 16 2018, Takashi wrote:

> The code change is similar as what I had in my mind, but...

Version 2!

Thanks,

Rob.

>From 8890fbf81c27ed5fded12349b274c7c008c53249 Mon Sep 17 00:00:00 2001
From: Rob Duncan <rduncan@teslamotors.com>
Date: Mon, 16 Jul 2018 16:35:23 -0700
Subject: [PATCH] pcm: ioplug: Transfer all available data

The snd_pcm_mmap_begin() call returns the amount of contiguous data,
which is less than the total available if it wraps around the buffer
boundary.

If we don't handle this split we leave stale data in the buffer that
should have been overwritten, as well as unread data in the io_plugin
that gets transferred on a subsequent call at the wrong offset.

Signed-off-by: Rob Duncan <rduncan@teslamotors.com>
---
 src/pcm/pcm_ioplug.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c
index 6d52c27b..881a1a85 100644
--- a/src/pcm/pcm_ioplug.c
+++ b/src/pcm/pcm_ioplug.c
@@ -716,6 +716,8 @@ static snd_pcm_sframes_t snd_pcm_ioplug_avail_update(snd_pcm_t *pcm)
 	snd_pcm_ioplug_hw_ptr_update(pcm);
 	if (io->data->state == SND_PCM_STATE_XRUN)
 		return -EPIPE;
+
+	avail = snd_pcm_mmap_avail(pcm);
 	if (pcm->stream == SND_PCM_STREAM_CAPTURE &&
 	    pcm->access != SND_PCM_ACCESS_RW_INTERLEAVED &&
 	    pcm->access != SND_PCM_ACCESS_RW_NONINTERLEAVED) {
@@ -728,9 +730,19 @@ static snd_pcm_sframes_t snd_pcm_ioplug_avail_update(snd_pcm_t *pcm)
 			result = io->data->callback->transfer(io->data, areas, offset, size);
 			if (result < 0)
 				return result;
+
+			/* If the available data doesn't fit in the
+			   contiguous area at the end of the mmap we
+			   must transfer the remaining data to the
+			   beginning of the mmap. */
+			if (size < avail) {
+				result = io->data->callback->transfer(io->data, areas,
+								      0, avail - size);
+				if (result < 0)
+					return result;
+			}
 		}
 	}
-	avail = snd_pcm_mmap_avail(pcm);
 	if (avail > io->avail_max)
 		io->avail_max = avail;
 	return (snd_pcm_sframes_t)avail;
-- 
2.15.1

Re: Bug in snd_pcm_ioplug_avail_update()?

[Takashi Iwai]

On Tue, 17 Jul 2018 17:46:35 +0200,
Rob Duncan wrote:
> 
> At 22:04 on Mon, Jul 16 2018, Takashi wrote:
> 
> > The code change is similar as what I had in my mind, but...
> 
> Version 2!

Applied, thanks!


Takashi