All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH - JACK PCM plugin] jack: Use boundary as hw_ptr wrap around
@ 2018-01-24 11:49 twischer
  2018-02-09 13:11 ` Wischer, Timo (ADITG/ESB)
  2018-02-13  5:40 ` Takashi Iwai
  0 siblings, 2 replies; 12+ messages in thread
From: twischer @ 2018-01-24 11:49 UTC (permalink / raw)
  To: patch; +Cc: Timo Wischer, alsa-devel

From: Timo Wischer <twischer@de.adit-jv.com>

instead of using buffer_size as wrap around.

This is required to detect Xruns.

It is also required to allow the JACK thread
to processes the whole ALSA audio buffer at once
without calling snd_pcm_avail_update() in between.

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.

Due to that the user application would not recognize
that the complete audio buffer was copied.

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

This patch is depending on
http://mailman.alsa-project.org/pipermail/alsa-devel/2018-January/130942.html

diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c
index 3aed332..c22a5d0 100644
--- a/jack/pcm_jack.c
+++ b/jack/pcm_jack.c
@@ -40,6 +40,7 @@ typedef struct {
 
 	char **port_names;
 	unsigned int num_ports;
+	snd_pcm_uframes_t boundary;
 	unsigned int hw_ptr;
 	unsigned int sample_bits;
 	snd_pcm_uframes_t min_avail;
@@ -130,6 +131,21 @@ static int snd_pcm_jack_poll_revents(snd_pcm_ioplug_t *io,
 static snd_pcm_sframes_t snd_pcm_jack_pointer(snd_pcm_ioplug_t *io)
 {
 	snd_pcm_jack_t *jack = io->private_data;
+
+	/* ALSA library is calulating the delta between the last pointer and
+	 * the current one.
+	 * Normally it is expecting a value between 0 and buffer_size.
+	 * The following example would result in an negative delta
+	 * which would result in a hw_ptr which will be reduced.
+	 *  last_hw = jack->boundary - io->buffer_size
+	 *  hw = 0
+	 * But we cannot use
+	 * return jack->hw_ptr % io->buffer_size;
+	 * because in this case an update of
+	 * hw_ptr += io->buffer_size
+	 * would not be recognized by the ALSA library.
+	 * Therefore we are using jack->boundary as the wrap around.
+	 */
 	return jack->hw_ptr;
 }
 
@@ -162,7 +178,7 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io)
 
 	while (xfer < nframes) {
 		snd_pcm_uframes_t frames = nframes - xfer;
-		snd_pcm_uframes_t offset = hw_ptr;
+		snd_pcm_uframes_t offset = hw_ptr % io->buffer_size;
 		snd_pcm_uframes_t cont = io->buffer_size - offset;
 
 		if (cont < frames)
@@ -176,7 +192,8 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io)
 		}
 		
 		hw_ptr += frames;
-		hw_ptr %= io->buffer_size;
+		if (hw_ptr >= jack->boundary)
+			hw_ptr -= jack->boundary;
 		xfer += frames;
 	}
 	jack->hw_ptr = hw_ptr;
@@ -200,6 +217,8 @@ static int snd_pcm_jack_prepare(snd_pcm_ioplug_t *io)
 	err = snd_pcm_sw_params_current(io->pcm, swparams);
 	if (err == 0) {
 		snd_pcm_sw_params_get_avail_min(swparams, &jack->min_avail);
+		/* get boundary for available calulation */
+		snd_pcm_sw_params_get_boundary(swparams, &jack->boundary);
 	}
 
 	/* deactivate jack connections if this is XRUN recovery */
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH - JACK PCM plugin] jack: Use boundary as hw_ptr wrap around
  2018-01-24 11:49 [PATCH - JACK PCM plugin] jack: Use boundary as hw_ptr wrap around twischer
@ 2018-02-09 13:11 ` Wischer, Timo (ADITG/ESB)
  2018-02-13  5:40 ` Takashi Iwai
  1 sibling, 0 replies; 12+ messages in thread
From: Wischer, Timo (ADITG/ESB) @ 2018-02-09 13:11 UTC (permalink / raw)
  To: patch; +Cc: alsa-devel

Hello all,

[1] was merged as it is (see [2])
Therefore this commit can be merged without conflicts, now.

Please have a look.

[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2018-January/130942.html
[2] http://git.alsa-project.org/?p=alsa-plugins.git;a=commit;h=21839e981a4b7c7178c1a473f20460c003e13db4

Best regards

Timo Wischer

Advanced Driver Information Technology GmbH
Engineering Software Base (ADITG/ESB)
Robert-Bosch-Str. 200
31139 Hildesheim
Germany

Tel. +49 5121 49 6938
Fax +49 5121 49 6999
twischer@de.adit-jv.com

ADIT is a joint venture company of Robert Bosch GmbH/Robert Bosch Car Multimedia GmbH and DENSO Corporation
Sitz: Hildesheim, Registergericht: Amtsgericht Hildesheim HRB 3438
Geschäftsführung: Wilhelm Grabow, Ken Yaguchi

________________________________________
From: Wischer, Timo (ADITG/ESB)
Sent: Wednesday, January 24, 2018 12:49 PM
To: patch@alsa-project.org
Cc: alsa-devel@alsa-project.org; Wischer, Timo (ADITG/ESB)
Subject: [PATCH - JACK PCM plugin] jack: Use boundary as hw_ptr wrap around

From: Timo Wischer <twischer@de.adit-jv.com>

instead of using buffer_size as wrap around.

This is required to detect Xruns.

It is also required to allow the JACK thread
to processes the whole ALSA audio buffer at once
without calling snd_pcm_avail_update() in between.

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.

Due to that the user application would not recognize
that the complete audio buffer was copied.

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

This patch is depending on
http://mailman.alsa-project.org/pipermail/alsa-devel/2018-January/130942.html

diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c
index 3aed332..c22a5d0 100644
--- a/jack/pcm_jack.c
+++ b/jack/pcm_jack.c
@@ -40,6 +40,7 @@ typedef struct {

        char **port_names;
        unsigned int num_ports;
+       snd_pcm_uframes_t boundary;
        unsigned int hw_ptr;
        unsigned int sample_bits;
        snd_pcm_uframes_t min_avail;
@@ -130,6 +131,21 @@ static int snd_pcm_jack_poll_revents(snd_pcm_ioplug_t *io,
 static snd_pcm_sframes_t snd_pcm_jack_pointer(snd_pcm_ioplug_t *io)
 {
        snd_pcm_jack_t *jack = io->private_data;
+
+       /* ALSA library is calulating the delta between the last pointer and
+        * the current one.
+        * Normally it is expecting a value between 0 and buffer_size.
+        * The following example would result in an negative delta
+        * which would result in a hw_ptr which will be reduced.
+        *  last_hw = jack->boundary - io->buffer_size
+        *  hw = 0
+        * But we cannot use
+        * return jack->hw_ptr % io->buffer_size;
+        * because in this case an update of
+        * hw_ptr += io->buffer_size
+        * would not be recognized by the ALSA library.
+        * Therefore we are using jack->boundary as the wrap around.
+        */
        return jack->hw_ptr;
 }

@@ -162,7 +178,7 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io)

        while (xfer < nframes) {
                snd_pcm_uframes_t frames = nframes - xfer;
-               snd_pcm_uframes_t offset = hw_ptr;
+               snd_pcm_uframes_t offset = hw_ptr % io->buffer_size;
                snd_pcm_uframes_t cont = io->buffer_size - offset;

                if (cont < frames)
@@ -176,7 +192,8 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io)
                }

                hw_ptr += frames;
-               hw_ptr %= io->buffer_size;
+               if (hw_ptr >= jack->boundary)
+                       hw_ptr -= jack->boundary;
                xfer += frames;
        }
        jack->hw_ptr = hw_ptr;
@@ -200,6 +217,8 @@ static int snd_pcm_jack_prepare(snd_pcm_ioplug_t *io)
        err = snd_pcm_sw_params_current(io->pcm, swparams);
        if (err == 0) {
                snd_pcm_sw_params_get_avail_min(swparams, &jack->min_avail);
+               /* get boundary for available calulation */
+               snd_pcm_sw_params_get_boundary(swparams, &jack->boundary);
        }

        /* deactivate jack connections if this is XRUN recovery */
--
2.7.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH - JACK PCM plugin] jack: Use boundary as hw_ptr wrap around
  2018-01-24 11:49 [PATCH - JACK PCM plugin] jack: Use boundary as hw_ptr wrap around twischer
  2018-02-09 13:11 ` Wischer, Timo (ADITG/ESB)
@ 2018-02-13  5:40 ` Takashi Iwai
  2018-02-13 14:56   ` Wischer, Timo (ADITG/ESB)
  1 sibling, 1 reply; 12+ messages in thread
From: Takashi Iwai @ 2018-02-13  5:40 UTC (permalink / raw)
  To: twischer; +Cc: alsa-devel

On Wed, 24 Jan 2018 12:49:05 +0100,
<twischer@de.adit-jv.com> wrote:
> 
> From: Timo Wischer <twischer@de.adit-jv.com>
> 
> instead of using buffer_size as wrap around.

No, the ioplug backend has to report the position from 0 to
buffer_size.

> This is required to detect Xruns.

The XRUN can be reported back from the backend just by returning an
error from pointer callback.

> It is also required to allow the JACK thread
> to processes the whole ALSA audio buffer at once
> without calling snd_pcm_avail_update() in between.
> 
> 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.
> 
> Due to that the user application would not recognize
> that the complete audio buffer was copied.

This has nothing to do with the reported position.  If this happens,
it simply means an XRUN.  You should report the error instead.


thanks,

Takashi

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH - JACK PCM plugin] jack: Use boundary as hw_ptr wrap around
  2018-02-13  5:40 ` Takashi Iwai
@ 2018-02-13 14:56   ` Wischer, Timo (ADITG/ESB)
  2018-02-13 17:03     ` Takashi Iwai
  0 siblings, 1 reply; 12+ messages in thread
From: Wischer, Timo (ADITG/ESB) @ 2018-02-13 14:56 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: alsa-devel

Hello Takashi,

> This has nothing to do with the reported position.  If this happens,
it simply means an XRUN.  You should report the error instead.

When the ALSA buffer is full and the JACK daemon is requesting exactly the amount of samples of the buffer size
I do not see an under run here.
After such an operation the ALSA buffer is empty
but the JACK daemon has not read more samples than available.

Exactly in this case we would increment the hw_ptr += buffer_size
but this would not be recognized by the ALSA library
when we are using a wrap around of  buffer_size.


> No, the ioplug backend has to report the position from 0 to buffer_size.

I know but I think the ioplug API implementation has possibly to be changed to allow exactly such use cases
as described above.

Or do you have another idea how to report such a hw_ptr change?

Best regards

Timo Wischer

Advanced Driver Information Technology GmbH
Engineering Software Base (ADITG/ESB)
Robert-Bosch-Str. 200
31139 Hildesheim
Germany

Tel. +49 5121 49 6938
Fax +49 5121 49 6999
twischer@de.adit-jv.com

ADIT is a joint venture company of Robert Bosch GmbH/Robert Bosch Car Multimedia GmbH and DENSO Corporation
Sitz: Hildesheim, Registergericht: Amtsgericht Hildesheim HRB 3438
Geschäftsführung: Wilhelm Grabow, Ken Yaguchi

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH - JACK PCM plugin] jack: Use boundary as hw_ptr wrap around
  2018-02-13 14:56   ` Wischer, Timo (ADITG/ESB)
@ 2018-02-13 17:03     ` Takashi Iwai
  2018-02-15 10:36       ` Wischer, Timo (ADITG/ESB)
  0 siblings, 1 reply; 12+ messages in thread
From: Takashi Iwai @ 2018-02-13 17:03 UTC (permalink / raw)
  To: Wischer, Timo (ADITG/ESB); +Cc: alsa-devel

On Tue, 13 Feb 2018 15:56:08 +0100,
Wischer, Timo (ADITG/ESB) wrote:
> 
> Hello Takashi,
> 
> > This has nothing to do with the reported position.  If this happens,
> it simply means an XRUN.  You should report the error instead.
> 
> When the ALSA buffer is full and the JACK daemon is requesting exactly the amount of samples of the buffer size
> I do not see an under run here.
> After such an operation the ALSA buffer is empty
> but the JACK daemon has not read more samples than available.

The backend has nothing to do with such a case.  This happens only
when the application uses periods=1, and then it implies that the
application must know of the situation.

The ioplug simulates the say the hardware device would behave.  It
notifies the period elapse via file-descriptor and it reports the DMA
ring-buffer position.  The ring buffer is between 0 to buffer size,
you must not exceed it in the reporting.


Takashi

> Exactly in this case we would increment the hw_ptr += buffer_size
> but this would not be recognized by the ALSA library
> when we are using a wrap around of  buffer_size.
> 
> 
> > No, the ioplug backend has to report the position from 0 to buffer_size.
> 
> I know but I think the ioplug API implementation has possibly to be changed to allow exactly such use cases
> as described above.
> 
> Or do you have another idea how to report such a hw_ptr change?
> 
> Best regards
> 
> Timo Wischer
> 
> Advanced Driver Information Technology GmbH
> Engineering Software Base (ADITG/ESB)
> Robert-Bosch-Str. 200
> 31139 Hildesheim
> Germany
> 
> Tel. +49 5121 49 6938
> Fax +49 5121 49 6999
> twischer@de.adit-jv.com
> 
> ADIT is a joint venture company of Robert Bosch GmbH/Robert Bosch Car Multimedia GmbH and DENSO Corporation
> Sitz: Hildesheim, Registergericht: Amtsgericht Hildesheim HRB 3438
> Geschäftsführung: Wilhelm Grabow, Ken Yaguchi
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH - JACK PCM plugin] jack: Use boundary as hw_ptr wrap around
  2018-02-13 17:03     ` Takashi Iwai
@ 2018-02-15 10:36       ` Wischer, Timo (ADITG/ESB)
  2018-02-15 11:32         ` Takashi Iwai
  0 siblings, 1 reply; 12+ messages in thread
From: Wischer, Timo (ADITG/ESB) @ 2018-02-15 10:36 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: alsa-devel

Hello Takashi,

> This happens only when the application uses periods=1,

It can also happen with 2 periods per buffer e.g. when the user application is too late.
I will try to illustrate the use case with the following time line:

Time line: 0--R1-W1-R2--R3-W2-W3-...

0 buffer is full (2 periods)
R1 JACK daemon reads 1 period (1 period left in the buffer)
W1 User application writes 1 period (2 periods in the buffer)
R2 JACK daemon reads 1 period (1 period left in the buffer)
R3 JACK daemon reads 1 period (buffer empty)
But it is not yet an under run because JACK has not yet read invalid data.
Due to the blocked user application (e.g low scheduling priority) the pointer() callback was not called before the second read.
In this case the next pointer() call has to result in a delta of 2 periods.
But this is not possible if pointer() is not allowed to return >=buffer_size.

W2 User application writes 1 period (1 periods in the buffer)
W3 User application writes 1 period (2 periods in the buffer)
Continue with 0

> It notifies the period elapse via file-descriptor
But this notification will be processed later if the user application is blocked at the moment (e.g. by higher priority tasks).

Do you also see my problem, now?

Best regards

Timo Wischer

Advanced Driver Information Technology GmbH
Engineering Software Base (ADITG/ESB)
Robert-Bosch-Str. 200
31139 Hildesheim
Germany

Tel. +49 5121 49 6938
Fax +49 5121 49 6999
twischer@de.adit-jv.com

ADIT is a joint venture company of Robert Bosch GmbH/Robert Bosch Car Multimedia GmbH and DENSO Corporation
Sitz: Hildesheim, Registergericht: Amtsgericht Hildesheim HRB 3438
Geschäftsführung: Wilhelm Grabow, Ken Yaguchi

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH - JACK PCM plugin] jack: Use boundary as hw_ptr wrap around
  2018-02-15 10:36       ` Wischer, Timo (ADITG/ESB)
@ 2018-02-15 11:32         ` Takashi Iwai
  2018-02-23  9:28           ` [PATCH IO plug API 1/1] ioplug: Use boundary for " twischer
  0 siblings, 1 reply; 12+ messages in thread
From: Takashi Iwai @ 2018-02-15 11:32 UTC (permalink / raw)
  To: Wischer, Timo (ADITG/ESB); +Cc: alsa-devel

On Thu, 15 Feb 2018 11:36:21 +0100,
Wischer, Timo (ADITG/ESB) wrote:
> 
> Hello Takashi,
> 
> > This happens only when the application uses periods=1,
> 
> It can also happen with 2 periods per buffer e.g. when the user application is too late.
> I will try to illustrate the use case with the following time line:
> 
> Time line: 0--R1-W1-R2--R3-W2-W3-...
> 
> 0 buffer is full (2 periods)
> R1 JACK daemon reads 1 period (1 period left in the buffer)
> W1 User application writes 1 period (2 periods in the buffer)
> R2 JACK daemon reads 1 period (1 period left in the buffer)
> R3 JACK daemon reads 1 period (buffer empty)
> But it is not yet an under run because JACK has not yet read invalid data.
> Due to the blocked user application (e.g low scheduling priority) the pointer() callback was not called before the second read.
> In this case the next pointer() call has to result in a delta of 2 periods.
> But this is not possible if pointer() is not allowed to return >=buffer_size.
> 
> W2 User application writes 1 period (1 periods in the buffer)
> W3 User application writes 1 period (2 periods in the buffer)
> Continue with 0
> 
> > It notifies the period elapse via file-descriptor
> But this notification will be processed later if the user application is blocked at the moment (e.g. by higher priority tasks).
> 
> Do you also see my problem, now?

I know that's the problem, but you're scratching in the wrong surface.
This isn't the issue to be fixed in the backend side.  The fact that
the pointer callback returns 0 to buffer_size-1 is *the* design.  You
can't ignore that.

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


Takashi

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH IO plug API 1/1] ioplug: Use boundary for wrap around
  2018-02-15 11:32         ` Takashi Iwai
@ 2018-02-23  9:28           ` twischer
  2018-02-23 10:40             ` Takashi Iwai
  0 siblings, 1 reply; 12+ messages in thread
From: twischer @ 2018-02-23  9:28 UTC (permalink / raw)
  To: tiwai; +Cc: Timo Wischer, alsa-devel

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.

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;
 }
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH IO plug API 1/1] ioplug: Use boundary for wrap around
  2018-02-23  9:28           ` [PATCH IO plug API 1/1] ioplug: Use boundary for " twischer
@ 2018-02-23 10:40             ` Takashi Iwai
  2018-02-23 13:52               ` Takashi Iwai
  0 siblings, 1 reply; 12+ messages in thread
From: Takashi Iwai @ 2018-02-23 10:40 UTC (permalink / raw)
  To: twischer; +Cc: alsa-devel

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH IO plug API 1/1] ioplug: Use boundary for wrap around
  2018-02-23 10:40             ` Takashi Iwai
@ 2018-02-23 13:52               ` Takashi Iwai
  2018-02-23 14:18                 ` [PATCH - JACK PCM plugin] jack: Use boundary as hw_ptr " twischer
  0 siblings, 1 reply; 12+ messages in thread
From: Takashi Iwai @ 2018-02-23 13:52 UTC (permalink / raw)
  To: twischer; +Cc: alsa-devel

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH - JACK PCM plugin] jack: Use boundary as hw_ptr wrap around
  2018-02-23 13:52               ` Takashi Iwai
@ 2018-02-23 14:18                 ` twischer
  2018-02-24 10:43                   ` Takashi Iwai
  0 siblings, 1 reply; 12+ messages in thread
From: twischer @ 2018-02-23 14:18 UTC (permalink / raw)
  To: tiwai; +Cc: Timo Wischer, alsa-devel

From: Timo Wischer <twischer@de.adit-jv.com>

instead of using buffer_size as wrap around.

This is required to detect Xruns.

It is also required to allow the JACK thread
to processes the whole ALSA audio buffer at once
without calling snd_pcm_avail_update() in between.

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.

Due to that the user application would not recognize
that the complete audio buffer was copied.

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

---

Here the updated version which only returns
hw_ptr >= buffer_size if supported
by the ALSA library on compile time

diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c
index 3aed332..c547cbb 100644
--- a/jack/pcm_jack.c
+++ b/jack/pcm_jack.c
@@ -40,7 +40,8 @@ typedef struct {
 
 	char **port_names;
 	unsigned int num_ports;
-	unsigned int hw_ptr;
+	snd_pcm_uframes_t boundary;
+	snd_pcm_uframes_t hw_ptr;
 	unsigned int sample_bits;
 	snd_pcm_uframes_t min_avail;
 
@@ -130,7 +131,12 @@ static int snd_pcm_jack_poll_revents(snd_pcm_ioplug_t *io,
 static snd_pcm_sframes_t snd_pcm_jack_pointer(snd_pcm_ioplug_t *io)
 {
 	snd_pcm_jack_t *jack = io->private_data;
+
+#ifdef SND_PCM_IOPLUG_FLAG_BOUNDARY_WA
 	return jack->hw_ptr;
+#else
+	return jack->hw_ptr % io->buffer_size;
+#endif
 }
 
 static int
@@ -162,7 +168,7 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io)
 
 	while (xfer < nframes) {
 		snd_pcm_uframes_t frames = nframes - xfer;
-		snd_pcm_uframes_t offset = hw_ptr;
+		snd_pcm_uframes_t offset = hw_ptr % io->buffer_size;
 		snd_pcm_uframes_t cont = io->buffer_size - offset;
 
 		if (cont < frames)
@@ -176,7 +182,8 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io)
 		}
 		
 		hw_ptr += frames;
-		hw_ptr %= io->buffer_size;
+		if (hw_ptr >= jack->boundary)
+			hw_ptr -= jack->boundary;
 		xfer += frames;
 	}
 	jack->hw_ptr = hw_ptr;
@@ -200,6 +207,8 @@ static int snd_pcm_jack_prepare(snd_pcm_ioplug_t *io)
 	err = snd_pcm_sw_params_current(io->pcm, swparams);
 	if (err == 0) {
 		snd_pcm_sw_params_get_avail_min(swparams, &jack->min_avail);
+		/* get boundary for available calulation */
+		snd_pcm_sw_params_get_boundary(swparams, &jack->boundary);
 	}
 
 	/* deactivate jack connections if this is XRUN recovery */
@@ -452,6 +461,12 @@ static int snd_pcm_jack_open(snd_pcm_t **pcmp, const char *name,
 	jack->io.poll_events = POLLIN;
 	jack->io.mmap_rw = 1;
 
+#ifdef SND_PCM_IOPLUG_FLAG_BOUNDARY_WA
+	jack->io.flags = SND_PCM_IOPLUG_FLAG_BOUNDARY_WA;
+#else
+#warning hw_ptr updates of buffer_size will not be recognized by the ALSA library. Consider to update your ALSA library.
+#endif
+
 	err = snd_pcm_ioplug_create(&jack->io, name, stream, mode);
 	if (err < 0) {
 		snd_pcm_jack_free(jack);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH - JACK PCM plugin] jack: Use boundary as hw_ptr wrap around
  2018-02-23 14:18                 ` [PATCH - JACK PCM plugin] jack: Use boundary as hw_ptr " twischer
@ 2018-02-24 10:43                   ` Takashi Iwai
  0 siblings, 0 replies; 12+ messages in thread
From: Takashi Iwai @ 2018-02-24 10:43 UTC (permalink / raw)
  To: twischer; +Cc: alsa-devel

On Fri, 23 Feb 2018 15:18:08 +0100,
<twischer@de.adit-jv.com> wrote:
> 
> From: Timo Wischer <twischer@de.adit-jv.com>
> 
> instead of using buffer_size as wrap around.
> 
> This is required to detect Xruns.
> 
> It is also required to allow the JACK thread
> to processes the whole ALSA audio buffer at once
> without calling snd_pcm_avail_update() in between.
> 
> 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.
> 
> Due to that the user application would not recognize
> that the complete audio buffer was copied.
> 
> Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
> 
> ---
> 
> Here the updated version which only returns
> hw_ptr >= buffer_size if supported
> by the ALSA library on compile time

Thanks, applied now together with your alsa-lib patch.


Takashi

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-02-24 10:43 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-24 11:49 [PATCH - JACK PCM plugin] jack: Use boundary as hw_ptr wrap around twischer
2018-02-09 13:11 ` Wischer, Timo (ADITG/ESB)
2018-02-13  5:40 ` Takashi Iwai
2018-02-13 14:56   ` Wischer, Timo (ADITG/ESB)
2018-02-13 17:03     ` Takashi Iwai
2018-02-15 10:36       ` Wischer, Timo (ADITG/ESB)
2018-02-15 11:32         ` Takashi Iwai
2018-02-23  9:28           ` [PATCH IO plug API 1/1] ioplug: Use boundary for " twischer
2018-02-23 10:40             ` Takashi Iwai
2018-02-23 13:52               ` Takashi Iwai
2018-02-23 14:18                 ` [PATCH - JACK PCM plugin] jack: Use boundary as hw_ptr " twischer
2018-02-24 10:43                   ` Takashi Iwai

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.