Re: [PATCH - JACK plugin 1/4] jack: Do not Xrun the ALSA buffer

From: Takashi Iwai <tiwai@suse.de>
To: twischer@de.adit-jv.com
Cc: alsa-devel@alsa-project.org
Subject: Re: [PATCH - JACK plugin 1/4] jack: Do not Xrun the ALSA buffer
Date: Thu, 01 Mar 2018 16:24:48 +0100	[thread overview]
Message-ID: <s5hfu5jizsv.wl-tiwai@suse.de> (raw)
In-Reply-To: <1519910048-19732-2-git-send-email-twischer@de.adit-jv.com>

On Thu, 01 Mar 2018 14:14:05 +0100,
<twischer@de.adit-jv.com> wrote:
> 
> From: Timo Wischer <twischer@de.adit-jv.com>
> 
> 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