On 2014-11-12 14:31, Peter Meerwald wrote:
> Hi,
>
>>> 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.
>
> regards, 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;
>>> }
>>> }
>>>
>>>
>>
>>
>
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
