> > > > 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?
> >
> > ah, yes
> >
> > I think the question is: does it ever happen?
> >
> > currently, the code won't notice any glitches and does the wrong thing if
> > sframes != frames
> >
> > the proposed code logs a warning and does the wrong thing if sframes !=
> > frames
> >
> > the ALSA example just treats sframes != frames as an error -- probably
> > that's the best thing to do (with some logging)?
>
> Since we don't know if this ever happens, and if so, why, I think maybe act
> conservatively and just write something to the log?
>
> And if this actually happens, we'll try to analyze it? Instead of guessing.
yes, sounds good
I came across this while developing the related optimization patch,
snd_pcm_mmap_commit() succeeding with sframes == frames is a necessary
precondition (and I was surprised that PA lacks rigidity in its most
important loop)
thanks, p.
> > > > ---
> > > > 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;
> > > > }
> > > > }
> > > >
> > > >
> > >
> > >
> >
>
>
--
Peter Meerwald
+43-664-2444418 (mobile)
