[IO,plug,API,1/1] ioplug: Use boundary for wrap around
diff mbox

Message ID 1519378131-18139-1-git-send-email-twischer@de.adit-jv.com
State New
Headers show

Commit Message

Timo Wischer Feb. 23, 2018, 9:28 a.m. UTC
From: Timo Wischer <twischer@de.adit-jv.com>

if requested by the IO plugin

Without this changes an IO plugin is not able to report
that buffer_size frames were read from the buffer.
When the buffer was full this is a valid action and
has not to be handled as an under run.

For example when the hw_ptr will be updated with
hw_ptr += buffer_size
and it is using the buffer_size as wrap around
hw_ptr %= buffer_size
would result in the same value as before the add operation.

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
---
Hello Takashi,

> If you need to improve such a situation, you'd have to fix in the
> ioplug implementation itself, not the jack plugin.

A have attached a patch which improves the IO plug API.
If you are happy with this solution I would also adapt the pending
JACK plugin patch and set the SND_PCM_IOPLUG_FLAG_BOUNDARY_WA flag.

Comments

Takashi Iwai Feb. 23, 2018, 10:40 a.m. UTC | #1
On Fri, 23 Feb 2018 10:28:51 +0100,
<twischer@de.adit-jv.com> wrote:
> 
> From: Timo Wischer <twischer@de.adit-jv.com>
> 
> if requested by the IO plugin
> 
> Without this changes an IO plugin is not able to report
> that buffer_size frames were read from the buffer.
> When the buffer was full this is a valid action and
> has not to be handled as an under run.
> 
> For example when the hw_ptr will be updated with
> hw_ptr += buffer_size
> and it is using the buffer_size as wrap around
> hw_ptr %= buffer_size
> would result in the same value as before the add operation.
> 
> Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
> ---
> Hello Takashi,
> 
> > If you need to improve such a situation, you'd have to fix in the
> > ioplug implementation itself, not the jack plugin.
> 
> A have attached a patch which improves the IO plug API.
> If you are happy with this solution I would also adapt the pending
> JACK plugin patch and set the SND_PCM_IOPLUG_FLAG_BOUNDARY_WA flag.

Yep, this looks better now.

A slightly complex procedure when extending an API is that you need
to check the existence of SND_PCM_IOPLUG_FLAG_BOUNDARY_WA in jack
plugin side at the compilation time.  Or make AM_PATH_ALSA() in
alsa-utils/configure.ac aligned with this change, but it won't work
until the release of alsa-lib itself.


thanks,

Takashi
Takashi Iwai Feb. 23, 2018, 1:52 p.m. UTC | #2
On Fri, 23 Feb 2018 11:40:01 +0100,
Takashi Iwai wrote:
> 
> On Fri, 23 Feb 2018 10:28:51 +0100,
> <twischer@de.adit-jv.com> wrote:
> > 
> > From: Timo Wischer <twischer@de.adit-jv.com>
> > 
> > if requested by the IO plugin
> > 
> > Without this changes an IO plugin is not able to report
> > that buffer_size frames were read from the buffer.
> > When the buffer was full this is a valid action and
> > has not to be handled as an under run.
> > 
> > For example when the hw_ptr will be updated with
> > hw_ptr += buffer_size
> > and it is using the buffer_size as wrap around
> > hw_ptr %= buffer_size
> > would result in the same value as before the add operation.
> > 
> > Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
> > ---
> > Hello Takashi,
> > 
> > > If you need to improve such a situation, you'd have to fix in the
> > > ioplug implementation itself, not the jack plugin.
> > 
> > A have attached a patch which improves the IO plug API.
> > If you are happy with this solution I would also adapt the pending
> > JACK plugin patch and set the SND_PCM_IOPLUG_FLAG_BOUNDARY_WA flag.
> 
> Yep, this looks better now.
> 
> A slightly complex procedure when extending an API is that you need
> to check the existence of SND_PCM_IOPLUG_FLAG_BOUNDARY_WA in jack
> plugin side at the compilation time.  Or make AM_PATH_ALSA() in
> alsa-utils/configure.ac aligned with this change, but it won't work
> until the release of alsa-lib itself.

That said, I'd happily merge the patch once when seeing the
corresponding change in jack plugin side.


thanks,

Takashi

Patch
diff mbox

diff --git a/include/pcm_ioplug.h b/include/pcm_ioplug.h
index 1c84594..e75f973 100644
--- a/include/pcm_ioplug.h
+++ b/include/pcm_ioplug.h
@@ -65,6 +65,8 @@  typedef snd_pcm_ioplug_callback snd_pcm_ioplug_callback_t;
  */
 #define SND_PCM_IOPLUG_FLAG_LISTED	(1<<0)		/**< list up this PCM */
 #define SND_PCM_IOPLUG_FLAG_MONOTONIC	(1<<1)		/**< monotonic timestamps */
+/** hw pointer wrap around at boundary instead of buffer_size */
+#define SND_PCM_IOPLUG_FLAG_BOUNDARY_WA	(1<<2)
 
 /*
  * Protocol version
@@ -133,6 +135,9 @@  struct snd_pcm_ioplug_callback {
 	int (*stop)(snd_pcm_ioplug_t *io);
 	/**
 	 * get the current DMA position; required, called inside mutex lock
+	 * \return buffer position up to buffer_size or
+	 * when #SND_PCM_IOPLUG_FLAG_BOUNDARY_WA flag is set up to boundary or
+	 * a negative error code for Xrun
 	 */
 	snd_pcm_sframes_t (*pointer)(snd_pcm_ioplug_t *io);
 	/**
diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c
index 7a782e6..9970646 100644
--- a/src/pcm/pcm_ioplug.c
+++ b/src/pcm/pcm_ioplug.c
@@ -42,7 +42,7 @@  const char *_snd_module_pcm_ioplug = "";
 typedef struct snd_pcm_ioplug_priv {
 	snd_pcm_ioplug_t *data;
 	struct snd_ext_parm params[SND_PCM_IOPLUG_HW_PARAMS];
-	unsigned int last_hw;
+	snd_pcm_uframes_t last_hw;
 	snd_pcm_uframes_t avail_max;
 	snd_htimestamp_t trigger_tstamp;
 } ioplug_priv_t;
@@ -56,13 +56,18 @@  static void snd_pcm_ioplug_hw_ptr_update(snd_pcm_t *pcm)
 
 	hw = io->data->callback->pointer(io->data);
 	if (hw >= 0) {
-		unsigned int delta;
-		if ((unsigned int)hw >= io->last_hw)
+		snd_pcm_uframes_t delta;
+
+		if ((snd_pcm_uframes_t)hw >= io->last_hw)
 			delta = hw - io->last_hw;
-		else
-			delta = pcm->buffer_size + hw - io->last_hw;
+		else {
+			const snd_pcm_uframes_t wrap_point =
+				(io->data->flags & SND_PCM_IOPLUG_FLAG_BOUNDARY_WA) ?
+					pcm->boundary : pcm->buffer_size;
+			delta = wrap_point + hw - io->last_hw;
+		}
 		snd_pcm_mmap_hw_forward(io->data->pcm, delta);
-		io->last_hw = hw;
+		io->last_hw = (snd_pcm_uframes_t)hw;
 	} else
 		io->data->state = SNDRV_PCM_STATE_XRUN;
 }