[pulseaudio-discuss] [PATCH] alsa: Check respect return value of snd_pcm_mmap_commit()

[David Henningsson]

On 2014-11-12 14:15, Peter Meerwald wrote:
> From: Peter Meerwald <p.meerwald at bct-electronic.com>
>
> snd_pcm_mmap_commit() actually transfers the memory area prepared by snd_pcm_mmap_begin(),
> it returns the 'count of transferred frames' which should be equal to the number of frames
> returned by snd_pcm_mmap_begin()
>
> however, this identify is not checked and the number of frames prepared are accounted for,
> not the number of frames commited -- this is wrong; the ALSA example codes bothers to
> check snd_pcm_mmap_commit()'s returned number of frames
>
> this patch outputs a warning and uses the commited number of frames to update the bytes
> read or written, resp.

It's not that easy, I think...

In case you get fewer frames written, you should retry the mmap_begin + 
mmap_commit cycle but *without* calling pa_sink_render_into_full, and 
only with the remaining frames. Right?


>
> Signed-off-by: Peter Meerwald <pmeerw at pmeerw.net>
> ---
>   src/modules/alsa/alsa-sink.c   |   11 +++++++++--
>   src/modules/alsa/alsa-source.c |   17 +++++++++++++----
>   2 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c
> index e256bbd..fc05232 100644
> --- a/src/modules/alsa/alsa-sink.c
> +++ b/src/modules/alsa/alsa-sink.c
> @@ -639,8 +639,7 @@ static int mmap_write(struct userdata *u, pa_usec_t *sleep_usec, bool polled, bo
>
>               p = (uint8_t*) areas[0].addr + (offset * u->frame_size);
>
> -            written = frames * u->frame_size;
> -            chunk.memblock = pa_memblock_new_fixed(u->core->mempool, p, written, true);
> +            chunk.memblock = pa_memblock_new_fixed(u->core->mempool, p, frames * u->frame_size, true);
>               chunk.length = pa_memblock_get_length(chunk.memblock);
>               chunk.index = 0;
>
> @@ -660,6 +659,14 @@ static int mmap_write(struct userdata *u, pa_usec_t *sleep_usec, bool polled, bo
>
>               work_done = true;
>
> +            if (PA_UNLIKELY((snd_pcm_uframes_t) sframes != frames)) {
> +                PA_ONCE_BEGIN {
> +                    pa_log_warn("ALSA mmap write was set up up for %lu frames, but unexpectedly commited %lu frames.\n",
> +                        (unsigned long) frames, (unsigned long) sframes);
> +                } PA_ONCE_END;
> +            }
> +
> +            written = sframes * u->frame_size;
>               u->write_count += written;
>               u->since_start += written;
>
> diff --git a/src/modules/alsa/alsa-source.c b/src/modules/alsa/alsa-source.c
> index 111c517..36e7585 100644
> --- a/src/modules/alsa/alsa-source.c
> +++ b/src/modules/alsa/alsa-source.c
> @@ -562,6 +562,7 @@ static int mmap_read(struct userdata *u, pa_usec_t *sleep_usec, bool polled, boo
>               const snd_pcm_channel_area_t *areas;
>               snd_pcm_uframes_t offset, frames;
>               snd_pcm_sframes_t sframes;
> +            size_t got;
>
>               frames = (snd_pcm_uframes_t) (n_bytes / u->frame_size);
>   /*             pa_log_debug("%lu frames to read", (unsigned long) frames); */
> @@ -613,16 +614,24 @@ static int mmap_read(struct userdata *u, pa_usec_t *sleep_usec, bool polled, boo
>
>               work_done = true;
>
> -            u->read_count += frames * u->frame_size;
> +            if (PA_UNLIKELY((snd_pcm_uframes_t) sframes != frames)) {
> +                PA_ONCE_BEGIN {
> +                    pa_log_warn("ALSA mmap read was set up up for %lu frames, but unexpectedly commited %lu frames.\n",
> +                        (unsigned long) frames, (unsigned long) sframes);
> +                } PA_ONCE_END;
> +            }
> +
> +            got = sframes * u->frame_size;
> +            u->read_count += got;
>
>   #ifdef DEBUG_TIMING
> -            pa_log_debug("Read %lu bytes (of possible %lu bytes)", (unsigned long) (frames * u->frame_size), (unsigned long) n_bytes);
> +            pa_log_debug("Read %lu bytes (of possible %lu bytes)", (unsigned long) got, (unsigned long) n_bytes);
>   #endif
>
> -            if ((size_t) frames * u->frame_size >= n_bytes)
> +            if (got >= n_bytes)
>                   break;
>
> -            n_bytes -= (size_t) frames * u->frame_size;
> +            n_bytes -= got;
>           }
>       }
>
>

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic