On Thu, 01 Mar 2018 14:14:05 +0100,
<twischer@xxxxxxxxxxxxxx> wrote:
>
> From: Timo Wischer <twischer@xxxxxxxxxxxxxx>
>
> when the JACK thread is requesting too many audio frames
>
> Playback:
> Without this commit the ALSA audio buffer
> will be played with endless repeats as long as the user application
> has not provided new audio data.
> Therefore this garbage will be played as long as the user application
> has not called snd_pcm_stop() after an Xrun.
> With this fix the rest of the JACK buffer will be filled
> with silence.
>
> Capture:
> Without this commit the audio data in the ALSA buffer would be
> overwritten.
> With this commit the new data from the JACK buffer
> will not be copied.
> Therefore the existing data in the ALSA buffer
> will not be overwritten.
Please try to format the text a bit nicer. These line breaks appear
like you're writing a poem :)
Now about the code changes:
> +static inline snd_pcm_uframes_t snd_pcm_jack_playback_avail(const snd_pcm_ioplug_t* const io,
> + const snd_pcm_uframes_t hw_ptr,
> + const snd_pcm_uframes_t appl_ptr)
You don't need to put inline unless it's really mandatory.
> +{
> + const snd_pcm_jack_t* const jack = io->private_data;
> +
> + /* cannot use io->hw_ptr without calling snd_pcm_avail_update()
> + * because it is not guranteed that snd_pcm_jack_pointer() was already
> + * called
> + */
> + snd_pcm_sframes_t avail;
> + avail = hw_ptr + io->buffer_size - appl_ptr;
> + if (avail < 0)
> + avail += jack->boundary;
> + else if ((snd_pcm_uframes_t) avail >= jack->boundary)
> + avail -= jack->boundary;
> + return avail;
> +}
> +
> +static inline snd_pcm_uframes_t snd_pcm_jack_capture_avail(const snd_pcm_ioplug_t* const io,
> + const snd_pcm_uframes_t hw_ptr,
> + const snd_pcm_uframes_t appl_ptr)
> +{
> + const snd_pcm_jack_t* const jack = io->private_data;
> +
> + /* cannot use io->hw_ptr without calling snd_pcm_avail_update()
> + * because it is not guranteed that snd_pcm_jack_pointer() was already
> + * called
> + */
> + snd_pcm_sframes_t avail;
> + avail = hw_ptr - appl_ptr;
> + if (avail < 0)
> + avail += jack->boundary;
> + return avail;
> +}
> +
> +static inline snd_pcm_uframes_t snd_pcm_jack_avail(const snd_pcm_ioplug_t* const io,
> + const snd_pcm_uframes_t hw_ptr,
> + const snd_pcm_uframes_t appl_ptr)
> +{
> + return (io->stream == SND_PCM_STREAM_PLAYBACK) ?
> + snd_pcm_jack_playback_avail(io, hw_ptr, appl_ptr) :
> + snd_pcm_jack_capture_avail(io, hw_ptr, appl_ptr);
> +}
> +
> +static inline snd_pcm_uframes_t snd_pcm_jack_hw_avail(const snd_pcm_ioplug_t* const io,
> + const snd_pcm_uframes_t hw_ptr,
> + const snd_pcm_uframes_t appl_ptr)
> +{
> + /* available data/space which can be transfered by the user application */
> + const snd_pcm_uframes_t user_avail = snd_pcm_jack_avail(io, hw_ptr,
> + appl_ptr);
> +
> + if (user_avail > io->buffer_size) {
> + /* there was an Xrun */
> + return 0;
> + }
> + /* available data/space which can be transfered by the DMA */
> + return io->buffer_size - user_avail;
> +}
Hm, this whole stuff would fit better in ioplug code itself instead of
open-coding in each plugin. Maybe providing a new helper function?
> static int pcm_poll_block_check(snd_pcm_ioplug_t *io)
> {
> static char buf[32];
> @@ -144,7 +206,6 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io)
> {
> snd_pcm_jack_t *jack = io->private_data;
> snd_pcm_uframes_t hw_ptr;
> - const snd_pcm_channel_area_t *areas;
> snd_pcm_uframes_t xfer = 0;
> unsigned int channel;
>
> @@ -154,40 +215,57 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io)
> jack->areas[channel].first = 0;
> jack->areas[channel].step = jack->sample_bits;
> }
> -
> - if (io->state != SND_PCM_STATE_RUNNING) {
> - if (io->stream == SND_PCM_STREAM_PLAYBACK) {
> - for (channel = 0; channel < io->channels; channel++)
> - snd_pcm_area_silence(&jack->areas[channel], 0, nframes, io->format);
> - return 0;
> - }
> - }
>
> hw_ptr = jack->hw_ptr;
> - areas = snd_pcm_ioplug_mmap_areas(io);
> -
> - while (xfer < nframes) {
> - snd_pcm_uframes_t frames = nframes - xfer;
> - snd_pcm_uframes_t offset = hw_ptr % io->buffer_size;
> - snd_pcm_uframes_t cont = io->buffer_size - offset;
> -
> - if (cont < frames)
> - frames = cont;
> + if (io->state == SND_PCM_STATE_RUNNING) {
> + const snd_pcm_channel_area_t *areas = snd_pcm_ioplug_mmap_areas(io);
> +
> + while (xfer < nframes) {
> + snd_pcm_uframes_t frames = nframes - xfer;
> + snd_pcm_uframes_t offset = hw_ptr % io->buffer_size;
> + snd_pcm_uframes_t cont = io->buffer_size - offset;
> + snd_pcm_uframes_t hw_avail =
> + snd_pcm_jack_hw_avail(io, hw_ptr,
> + io->appl_ptr);
> +
> + /* stop copying if there is no more data available */
> + if (hw_avail <= 0)
> + break;
> +
> + /* split the snd_pcm_area_copy() function into two parts
> + * if the data to copy passes the buffer wrap around
> + */
> + if (cont < frames)
> + frames = cont;
> +
> + if (hw_avail < frames)
> + frames = hw_avail;
> +
> + for (channel = 0; channel < io->channels; channel++) {
> + if (io->stream == SND_PCM_STREAM_PLAYBACK)
> + snd_pcm_area_copy(&jack->areas[channel], xfer, &areas[channel], offset, frames, io->format);
> + else
> + snd_pcm_area_copy(&areas[channel], offset, &jack->areas[channel], xfer, frames, io->format);
> + }
>
> - for (channel = 0; channel < io->channels; channel++) {
> - if (io->stream == SND_PCM_STREAM_PLAYBACK)
> - snd_pcm_area_copy(&jack->areas[channel], xfer, &areas[channel], offset, frames, io->format);
> - else
> - snd_pcm_area_copy(&areas[channel], offset, &jack->areas[channel], xfer, frames, io->format);
> + hw_ptr += frames;
> + if (hw_ptr >= jack->boundary)
> + hw_ptr -= jack->boundary;
> + xfer += frames;
> }
> -
> - hw_ptr += frames;
> - if (hw_ptr >= jack->boundary)
> - hw_ptr -= jack->boundary;
> - xfer += frames;
> }
> jack->hw_ptr = hw_ptr;
>
> + /* check if requested frames were copied */
> + if (xfer < nframes) {
> + /* always fill the not yet written JACK buffer with silence */
> + if (io->stream == SND_PCM_STREAM_PLAYBACK) {
> + const snd_pcm_uframes_t samples = nframes - xfer;
> + for (channel = 0; channel < io->channels; channel++)
> + snd_pcm_area_silence(&jack->areas[channel], xfer, samples, io->format);
> + }
> + }
> +
Ditto. Basically the whole procedure here is some common pattern, so
it wouldn't be too bad to have it in ioplug side.
thanks,
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
