qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/6] audio/jack: fixes to overall jack behaviour
@ 2020-06-13  4:05 Geoffrey McRae
  2020-06-13  4:05 ` [PATCH 1/6] audio/jack: fix invalid minimum buffer size check Geoffrey McRae
                   ` (6 more replies)
  0 siblings, 7 replies; 16+ messages in thread
From: Geoffrey McRae @ 2020-06-13  4:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: kraxel, geoff

This patch set addresses several issues that cause inconsistent
behaviour in the guest when the sound device is stopped and started or
the JACK server stops responding on the host.

Geoffrey McRae (6):
  audio/jack: fix invalid minimum buffer size check
  audio/jack: remove unused stopped state
  audio/jack: remove invalid set of input support bool
  audio/jack: do not remove ports when finishing
  audio/jack: honour the enable state of the audio device
  audio/jack: simplify the re-init code path

 audio/jackaudio.c | 73 ++++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 35 deletions(-)

-- 
2.20.1



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

* [PATCH 1/6] audio/jack: fix invalid minimum buffer size check
  2020-06-13  4:05 [PATCH 0/6] audio/jack: fixes to overall jack behaviour Geoffrey McRae
@ 2020-06-13  4:05 ` Geoffrey McRae
  2020-06-13  4:05 ` [PATCH 2/6] audio/jack: remove unused stopped state Geoffrey McRae
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Geoffrey McRae @ 2020-06-13  4:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: kraxel, geoff

JACK does not provide us with the configured buffer size until after
activiation which was overriding this minimum value. JACK itself doesn't
have this minimum limitation, but the QEMU virtual hardware and as such
it must be enforced, failure to do so results in audio discontinuities.

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>
---
 audio/jackaudio.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 722ddb1dfe..d0b6f748f2 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -434,17 +434,6 @@ static int qjack_client_init(QJackClient *c)
     jack_set_xrun_callback(c->client, qjack_xrun, c);
     jack_on_shutdown(c->client, qjack_shutdown, c);
 
-    /*
-     * ensure the buffersize is no smaller then 512 samples, some (all?) qemu
-     * virtual devices do not work correctly otherwise
-     */
-    if (c->buffersize < 512) {
-        c->buffersize = 512;
-    }
-
-    /* create a 2 period buffer */
-    qjack_buffer_create(&c->fifo, c->nchannels, c->buffersize * 2);
-
     /* allocate and register the ports */
     c->port = g_malloc(sizeof(jack_port_t *) * c->nchannels);
     for (int i = 0; i < c->nchannels; ++i) {
@@ -468,6 +457,17 @@ static int qjack_client_init(QJackClient *c)
     jack_activate(c->client);
     c->buffersize = jack_get_buffer_size(c->client);
 
+    /*
+     * ensure the buffersize is no smaller then 512 samples, some (all?) qemu
+     * virtual devices do not work correctly otherwise
+     */
+    if (c->buffersize < 512) {
+        c->buffersize = 512;
+    }
+
+    /* create a 2 period buffer */
+    qjack_buffer_create(&c->fifo, c->nchannels, c->buffersize * 2);
+
     qjack_client_connect_ports(c);
     c->state = QJACK_STATE_RUNNING;
     return 0;
-- 
2.20.1



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

* [PATCH 2/6] audio/jack: remove unused stopped state
  2020-06-13  4:05 [PATCH 0/6] audio/jack: fixes to overall jack behaviour Geoffrey McRae
  2020-06-13  4:05 ` [PATCH 1/6] audio/jack: fix invalid minimum buffer size check Geoffrey McRae
@ 2020-06-13  4:05 ` Geoffrey McRae
  2020-06-13  4:05 ` [PATCH 3/6] audio/jack: remove invalid set of input support bool Geoffrey McRae
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Geoffrey McRae @ 2020-06-13  4:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: kraxel, geoff

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>
---
 audio/jackaudio.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index d0b6f748f2..fb8efd7af7 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -38,7 +38,6 @@ struct QJack;
 
 typedef enum QJackState {
     QJACK_STATE_DISCONNECTED,
-    QJACK_STATE_STOPPED,
     QJACK_STATE_RUNNING,
     QJACK_STATE_SHUTDOWN
 }
@@ -549,9 +548,6 @@ static void qjack_client_fini(QJackClient *c)
 {
     switch (c->state) {
     case QJACK_STATE_RUNNING:
-        /* fallthrough */
-
-    case QJACK_STATE_STOPPED:
         for (int i = 0; i < c->nchannels; ++i) {
             jack_port_unregister(c->client, c->port[i]);
         }
-- 
2.20.1



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

* [PATCH 3/6] audio/jack: remove invalid set of input support bool
  2020-06-13  4:05 [PATCH 0/6] audio/jack: fixes to overall jack behaviour Geoffrey McRae
  2020-06-13  4:05 ` [PATCH 1/6] audio/jack: fix invalid minimum buffer size check Geoffrey McRae
  2020-06-13  4:05 ` [PATCH 2/6] audio/jack: remove unused stopped state Geoffrey McRae
@ 2020-06-13  4:05 ` Geoffrey McRae
  2020-06-13  4:05 ` [PATCH 4/6] audio/jack: do not remove ports when finishing Geoffrey McRae
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Geoffrey McRae @ 2020-06-13  4:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: kraxel, geoff

Initial code for JACK did not support audio input and as such this
boolean was set to let QEMU know, however JACK ended up including input
support making this invalid. Further investigation shows it was invalid
to set it in the first instance anyway due to a failure on my part
understand properly what this was for when the audodev was initially
developed.

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>
---
 audio/jackaudio.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index fb8efd7af7..58c7344497 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -607,9 +607,6 @@ static int qjack_thread_creator(jack_native_thread_t *thread,
 static void *qjack_init(Audiodev *dev)
 {
     assert(dev->driver == AUDIODEV_DRIVER_JACK);
-
-    dev->u.jack.has_in = false;
-
     return dev;
 }
 
-- 
2.20.1



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

* [PATCH 4/6] audio/jack: do not remove ports when finishing
  2020-06-13  4:05 [PATCH 0/6] audio/jack: fixes to overall jack behaviour Geoffrey McRae
                   ` (2 preceding siblings ...)
  2020-06-13  4:05 ` [PATCH 3/6] audio/jack: remove invalid set of input support bool Geoffrey McRae
@ 2020-06-13  4:05 ` Geoffrey McRae
  2020-06-13  4:05 ` [PATCH 5/6] audio/jack: honour the enable state of the audio device Geoffrey McRae
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Geoffrey McRae @ 2020-06-13  4:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: kraxel, geoff

This fixes a hang when there is a communications issue with the JACK
server. Simply closing the connection is enough to completely clean up
and as such we do not need to remove the ports first. As JACK uses a
socket based protocol that relies on the `select` call, if there is a
communication breakdown with the server the client library waits
forever for a response to the unregister request.

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>
---
 audio/jackaudio.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 58c7344497..249cbd3265 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -548,9 +548,6 @@ static void qjack_client_fini(QJackClient *c)
 {
     switch (c->state) {
     case QJACK_STATE_RUNNING:
-        for (int i = 0; i < c->nchannels; ++i) {
-            jack_port_unregister(c->client, c->port[i]);
-        }
         jack_deactivate(c->client);
         /* fallthrough */
 
-- 
2.20.1



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

* [PATCH 5/6] audio/jack: honour the enable state of the audio device
  2020-06-13  4:05 [PATCH 0/6] audio/jack: fixes to overall jack behaviour Geoffrey McRae
                   ` (3 preceding siblings ...)
  2020-06-13  4:05 ` [PATCH 4/6] audio/jack: do not remove ports when finishing Geoffrey McRae
@ 2020-06-13  4:05 ` Geoffrey McRae
  2020-06-17 12:44   ` Gerd Hoffmann
  2020-06-13  4:05 ` [PATCH 6/6] audio/jack: simplify the re-init code path Geoffrey McRae
  2020-06-14  5:07 ` [PATCH 0/6] audio/jack: fixes to overall jack behaviour no-reply
  6 siblings, 1 reply; 16+ messages in thread
From: Geoffrey McRae @ 2020-06-13  4:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: kraxel, geoff

When the guest closes the audio device we must start dropping input
samples from JACK and zeroing the output buffer samples. Failure to do
so causes sound artifacts during operations such as guest OS reboot, and
causes a hang of the input pipeline breaking it until QEMU is restated.

Closing and reconnecting to JACK was tested during these enable/disable
calls which works well for Linux guests, however Windows re-opens the
audio hardware repeatedly even when doing simple tasks like playing a
system sounds. As such it was decided it is better to feed silence to
JACK while the device is disabled.

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>
---
 audio/jackaudio.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 249cbd3265..b2b53985ae 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -56,7 +56,7 @@ typedef struct QJackClient {
     AudiodevJackPerDirectionOptions *opt;
 
     bool out;
-    bool finished;
+    bool enabled;
     bool connect_ports;
     int  packets;
 
@@ -271,9 +271,17 @@ static int qjack_process(jack_nframes_t nframes, void *arg)
     }
 
     if (c->out) {
-        qjack_buffer_read_l(&c->fifo, buffers, nframes);
+        if (likely(c->enabled)) {
+            qjack_buffer_read_l(&c->fifo, buffers, nframes);
+        } else {
+            for(int i = 0; i < c->nchannels; ++i) {
+                memset(buffers[i], 0, nframes * sizeof(float));
+            }
+        }
     } else {
-        qjack_buffer_write_l(&c->fifo, buffers, nframes);
+        if (likely(c->enabled)) {
+            qjack_buffer_write_l(&c->fifo, buffers, nframes);
+        }
     }
 
     return 0;
@@ -314,8 +322,8 @@ static void qjack_client_recover(QJackClient *c)
     if (c->state == QJACK_STATE_DISCONNECTED &&
         c->packets % 100 == 0) {
 
-        /* if not finished then attempt to recover */
-        if (!c->finished) {
+        /* if enabled then attempt to recover */
+        if (c->enabled) {
             dolog("attempting to reconnect to server\n");
             qjack_client_init(c);
         }
@@ -387,7 +395,6 @@ static int qjack_client_init(QJackClient *c)
     char client_name[jack_client_name_size()];
     jack_options_t options = JackNullOption;
 
-    c->finished      = false;
     c->connect_ports = true;
 
     snprintf(client_name, sizeof(client_name), "%s-%s",
@@ -483,8 +490,10 @@ static int qjack_init_out(HWVoiceOut *hw, struct audsettings *as,
     }
 
     jo->c.out       = true;
+    jo->c.enabled   = false;
     jo->c.nchannels = as->nchannels;
     jo->c.opt       = dev->u.jack.out;
+
     int ret = qjack_client_init(&jo->c);
     if (ret != 0) {
         return ret;
@@ -519,8 +528,10 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings *as,
     }
 
     ji->c.out       = false;
+    ji->c.enabled   = false;
     ji->c.nchannels = as->nchannels;
     ji->c.opt       = dev->u.jack.in;
+
     int ret = qjack_client_init(&ji->c);
     if (ret != 0) {
         return ret;
@@ -568,23 +579,25 @@ static void qjack_client_fini(QJackClient *c)
 static void qjack_fini_out(HWVoiceOut *hw)
 {
     QJackOut *jo = (QJackOut *)hw;
-    jo->c.finished = true;
     qjack_client_fini(&jo->c);
 }
 
 static void qjack_fini_in(HWVoiceIn *hw)
 {
     QJackIn *ji = (QJackIn *)hw;
-    ji->c.finished = true;
     qjack_client_fini(&ji->c);
 }
 
 static void qjack_enable_out(HWVoiceOut *hw, bool enable)
 {
+    QJackOut *jo = (QJackOut *)hw;
+    jo->c.enabled = enable;
 }
 
 static void qjack_enable_in(HWVoiceIn *hw, bool enable)
 {
+    QJackIn *ji = (QJackIn *)hw;
+    ji->c.enabled = enable;
 }
 
 static int qjack_thread_creator(jack_native_thread_t *thread,
-- 
2.20.1



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

* [PATCH 6/6] audio/jack: simplify the re-init code path
  2020-06-13  4:05 [PATCH 0/6] audio/jack: fixes to overall jack behaviour Geoffrey McRae
                   ` (4 preceding siblings ...)
  2020-06-13  4:05 ` [PATCH 5/6] audio/jack: honour the enable state of the audio device Geoffrey McRae
@ 2020-06-13  4:05 ` Geoffrey McRae
  2020-06-14  5:07 ` [PATCH 0/6] audio/jack: fixes to overall jack behaviour no-reply
  6 siblings, 0 replies; 16+ messages in thread
From: Geoffrey McRae @ 2020-06-13  4:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: kraxel, geoff

Instead of checking for the audodev state in each code path, centralize
the check into the initialize function itself to make it safe to call it
at any time.

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>
---
 audio/jackaudio.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index b2b53985ae..72ed7c4929 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -395,6 +395,10 @@ static int qjack_client_init(QJackClient *c)
     char client_name[jack_client_name_size()];
     jack_options_t options = JackNullOption;
 
+    if (c->state == QJACK_STATE_RUNNING) {
+        return 0;
+    }
+
     c->connect_ports = true;
 
     snprintf(client_name, sizeof(client_name), "%s-%s",
@@ -485,9 +489,7 @@ static int qjack_init_out(HWVoiceOut *hw, struct audsettings *as,
     QJackOut *jo  = (QJackOut *)hw;
     Audiodev *dev = (Audiodev *)drv_opaque;
 
-    if (jo->c.state != QJACK_STATE_DISCONNECTED) {
-        return 0;
-    }
+    qjack_client_fini(&jo->c);
 
     jo->c.out       = true;
     jo->c.enabled   = false;
@@ -523,9 +525,7 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings *as,
     QJackIn  *ji  = (QJackIn *)hw;
     Audiodev *dev = (Audiodev *)drv_opaque;
 
-    if (ji->c.state != QJACK_STATE_DISCONNECTED) {
-        return 0;
-    }
+    qjack_client_fini(&ji->c);
 
     ji->c.out       = false;
     ji->c.enabled   = false;
-- 
2.20.1



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

* Re: [PATCH 0/6] audio/jack: fixes to overall jack behaviour
  2020-06-13  4:05 [PATCH 0/6] audio/jack: fixes to overall jack behaviour Geoffrey McRae
                   ` (5 preceding siblings ...)
  2020-06-13  4:05 ` [PATCH 6/6] audio/jack: simplify the re-init code path Geoffrey McRae
@ 2020-06-14  5:07 ` no-reply
  6 siblings, 0 replies; 16+ messages in thread
From: no-reply @ 2020-06-14  5:07 UTC (permalink / raw)
  To: geoff; +Cc: geoff, qemu-devel, kraxel

Patchew URL: https://patchew.org/QEMU/20200613040518.38172-1-geoff@hostfission.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20200613040518.38172-1-geoff@hostfission.com
Subject: [PATCH 0/6] audio/jack: fixes to overall jack behaviour
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
c962352 audio/jack: simplify the re-init code path
1b80c6e audio/jack: honour the enable state of the audio device
d549d5e0 audio/jack: do not remove ports when finishing
d9f3c84 audio/jack: remove invalid set of input support bool
c612550 audio/jack: remove unused stopped state
62d579e audio/jack: fix invalid minimum buffer size check

=== OUTPUT BEGIN ===
1/6 Checking commit 62d579e00e55 (audio/jack: fix invalid minimum buffer size check)
2/6 Checking commit c612550cb58e (audio/jack: remove unused stopped state)
3/6 Checking commit d9f3c846ec2f (audio/jack: remove invalid set of input support bool)
4/6 Checking commit d549d5e05a30 (audio/jack: do not remove ports when finishing)
5/6 Checking commit 1b80c6eb42f5 (audio/jack: honour the enable state of the audio device)
ERROR: space required before the open parenthesis '('
#42: FILE: audio/jackaudio.c:277:
+            for(int i = 0; i < c->nchannels; ++i) {

total: 1 errors, 0 warnings, 91 lines checked

Patch 5/6 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

6/6 Checking commit c9623522cfe0 (audio/jack: simplify the re-init code path)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200613040518.38172-1-geoff@hostfission.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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

* Re: [PATCH 5/6] audio/jack: honour the enable state of the audio device
  2020-06-13  4:05 ` [PATCH 5/6] audio/jack: honour the enable state of the audio device Geoffrey McRae
@ 2020-06-17 12:44   ` Gerd Hoffmann
  2020-06-18  3:11     ` Geoffrey McRae
  0 siblings, 1 reply; 16+ messages in thread
From: Gerd Hoffmann @ 2020-06-17 12:44 UTC (permalink / raw)
  To: Geoffrey McRae; +Cc: qemu-devel

On Sat, Jun 13, 2020 at 02:05:17PM +1000, Geoffrey McRae wrote:
> When the guest closes the audio device we must start dropping input
> samples from JACK and zeroing the output buffer samples. Failure to do
> so causes sound artifacts during operations such as guest OS reboot, and
> causes a hang of the input pipeline breaking it until QEMU is restated.
> 
> Closing and reconnecting to JACK was tested during these enable/disable
> calls which works well for Linux guests, however Windows re-opens the
> audio hardware repeatedly even when doing simple tasks like playing a
> system sounds. As such it was decided it is better to feed silence to
> JACK while the device is disabled.

Hmm, I guess feeding silence into jack needs some cpu cycles?
Maybe add a timer to close the jack server connection?  Keep the
connection open for re-use for a while, but in case the guest stops
playing sound altogether close the jack connection after being unused
for a few minutes?

[ Doesn't render the patch invalid, consider it a suggestion for future
  improvements ]

take care,
  Gerd



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

* Re: [PATCH 5/6] audio/jack: honour the enable state of the audio device
  2020-06-17 12:44   ` Gerd Hoffmann
@ 2020-06-18  3:11     ` Geoffrey McRae
  2020-06-19  9:29       ` Gerd Hoffmann
  0 siblings, 1 reply; 16+ messages in thread
From: Geoffrey McRae @ 2020-06-18  3:11 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: qemu-devel



On 2020-06-17 22:44, Gerd Hoffmann wrote:
> On Sat, Jun 13, 2020 at 02:05:17PM +1000, Geoffrey McRae wrote:
>> When the guest closes the audio device we must start dropping input
>> samples from JACK and zeroing the output buffer samples. Failure to do
>> so causes sound artifacts during operations such as guest OS reboot, 
>> and
>> causes a hang of the input pipeline breaking it until QEMU is 
>> restated.
>> 
>> Closing and reconnecting to JACK was tested during these 
>> enable/disable
>> calls which works well for Linux guests, however Windows re-opens the
>> audio hardware repeatedly even when doing simple tasks like playing a
>> system sounds. As such it was decided it is better to feed silence to
>> JACK while the device is disabled.
> 
> Hmm, I guess feeding silence into jack needs some cpu cycles?
> Maybe add a timer to close the jack server connection?  Keep the
> connection open for re-use for a while, but in case the guest stops
> playing sound altogether close the jack connection after being unused
> for a few minutes?
> 
> [ Doesn't render the patch invalid, consider it a suggestion for future
>   improvements ]

Thanks, I had considered that however there is a usability issue to 
consider. Each time a jack client registers, it removes the client 
entirely and disconnects any custom plumbing that may have been done by 
the user. JACK does not remember this custom routing and as such it's 
lost until the user re-establishes it, or they have some automation set 
up to do it. While this could likely be worked around, it would likely 
cause more headaches then the few CPU cycles lost in a memset.

Further to this, while I have added some automation to connect ports via 
regex matching, this is likely not going to be used by many as it's not 
the normal method of connecting things up. This was added to open up 
this for general use for those that don't care for custom filters but 
just want the reliable audio interface.

> 
> take care,
>   Gerd


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

* Re: [PATCH 5/6] audio/jack: honour the enable state of the audio device
  2020-06-18  3:11     ` Geoffrey McRae
@ 2020-06-19  9:29       ` Gerd Hoffmann
  2020-06-21  4:06         ` Geoffrey McRae
  0 siblings, 1 reply; 16+ messages in thread
From: Gerd Hoffmann @ 2020-06-19  9:29 UTC (permalink / raw)
  To: Geoffrey McRae; +Cc: qemu-devel

  Hi,

> > Hmm, I guess feeding silence into jack needs some cpu cycles?
> > Maybe add a timer to close the jack server connection?  Keep the
> > connection open for re-use for a while, but in case the guest stops
> > playing sound altogether close the jack connection after being unused
> > for a few minutes?
> > 
> > [ Doesn't render the patch invalid, consider it a suggestion for future
> >   improvements ]
> 
> Thanks, I had considered that however there is a usability issue to
> consider. Each time a jack client registers, it removes the client entirely
> and disconnects any custom plumbing that may have been done by the user.

Hmm, ok, that is bad indeed.

Can you stop the stream without closing the connection?

> JACK does not remember this custom routing and as such it's lost until the
> user re-establishes it, or they have some automation set up to do it. While
> this could likely be worked around, it would likely cause more headaches
> then the few CPU cycles lost in a memset.

I'm more concerned about the frequent wakeups to feed the next chunk of
(silence) data to jack.

take care,
  Gerd



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

* Re: [PATCH 5/6] audio/jack: honour the enable state of the audio device
  2020-06-19  9:29       ` Gerd Hoffmann
@ 2020-06-21  4:06         ` Geoffrey McRae
  2020-06-22  9:05           ` Gerd Hoffmann
  0 siblings, 1 reply; 16+ messages in thread
From: Geoffrey McRae @ 2020-06-21  4:06 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: qemu-devel



On 2020-06-19 19:29, Gerd Hoffmann wrote:
> Hi,
> 
>> > Hmm, I guess feeding silence into jack needs some cpu cycles?
>> > Maybe add a timer to close the jack server connection?  Keep the
>> > connection open for re-use for a while, but in case the guest stops
>> > playing sound altogether close the jack connection after being unused
>> > for a few minutes?
>> >
>> > [ Doesn't render the patch invalid, consider it a suggestion for future
>> >   improvements ]
>> 
>> Thanks, I had considered that however there is a usability issue to
>> consider. Each time a jack client registers, it removes the client 
>> entirely
>> and disconnects any custom plumbing that may have been done by the 
>> user.
> 
> Hmm, ok, that is bad indeed.
> 
> Can you stop the stream without closing the connection?

Not as far as I can tell, it seems the JACK API doesn't allow for this 
in a way that is useful to us.

> 
>> JACK does not remember this custom routing and as such it's lost until 
>> the
>> user re-establishes it, or they have some automation set up to do it. 
>> While
>> this could likely be worked around, it would likely cause more 
>> headaches
>> then the few CPU cycles lost in a memset.
> 
> I'm more concerned about the frequent wakeups to feed the next chunk of
> (silence) data to jack.
> 
> take care,
>   Gerd


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

* Re: [PATCH 5/6] audio/jack: honour the enable state of the audio device
  2020-06-21  4:06         ` Geoffrey McRae
@ 2020-06-22  9:05           ` Gerd Hoffmann
  2020-06-22  9:27             ` Geoffrey McRae
  0 siblings, 1 reply; 16+ messages in thread
From: Gerd Hoffmann @ 2020-06-22  9:05 UTC (permalink / raw)
  To: Geoffrey McRae; +Cc: qemu-devel

On Sun, Jun 21, 2020 at 02:06:25PM +1000, Geoffrey McRae wrote:
> 
> > Can you stop the stream without closing the connection?
> 
> Not as far as I can tell, it seems the JACK API doesn't allow for this in a
> way that is useful to us.

What happens if you don't feed data to jack?  The cracking you hear on
reboots etc. sounds like jack might reuses the buffers in that case.

So, maybe you can stop sending data to jack when all buffers are already
filled with silence?

take care,
  Gerd



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

* Re: [PATCH 5/6] audio/jack: honour the enable state of the audio device
  2020-06-22  9:05           ` Gerd Hoffmann
@ 2020-06-22  9:27             ` Geoffrey McRae
  0 siblings, 0 replies; 16+ messages in thread
From: Geoffrey McRae @ 2020-06-22  9:27 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: qemu-devel


On 2020-06-22 19:05, Gerd Hoffmann wrote:
> On Sun, Jun 21, 2020 at 02:06:25PM +1000, Geoffrey McRae wrote:
>> 
>> > Can you stop the stream without closing the connection?
>> 
>> Not as far as I can tell, it seems the JACK API doesn't allow for this 
>> in a
>> way that is useful to us.
> 
> What happens if you don't feed data to jack?  The cracking you hear on
> reboots etc. sounds like jack might reuses the buffers in that case.
> 
> So, maybe you can stop sending data to jack when all buffers are 
> already
> filled with silence?

Jack hands us a buffer we have to set, it's uninitialized as it's a ring 
buffer, if we do not zero it then we get repeating samples like your 
sound device has hung.

> 
> take care,
>   Gerd


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

* [PATCH 5/6] audio/jack: honour the enable state of the audio device
@ 2020-06-11 15:20 Geoffrey McRae
  0 siblings, 0 replies; 16+ messages in thread
From: Geoffrey McRae @ 2020-06-11 15:20 UTC (permalink / raw)
  To: qemu-devel; +Cc: kraxel

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>
---
 audio/jackaudio.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 249cbd3265..b2b53985ae 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -56,7 +56,7 @@ typedef struct QJackClient {
     AudiodevJackPerDirectionOptions *opt;
 
     bool out;
-    bool finished;
+    bool enabled;
     bool connect_ports;
     int  packets;
 
@@ -271,9 +271,17 @@ static int qjack_process(jack_nframes_t nframes, void *arg)
     }
 
     if (c->out) {
-        qjack_buffer_read_l(&c->fifo, buffers, nframes);
+        if (likely(c->enabled)) {
+            qjack_buffer_read_l(&c->fifo, buffers, nframes);
+        } else {
+            for(int i = 0; i < c->nchannels; ++i) {
+                memset(buffers[i], 0, nframes * sizeof(float));
+            }
+        }
     } else {
-        qjack_buffer_write_l(&c->fifo, buffers, nframes);
+        if (likely(c->enabled)) {
+            qjack_buffer_write_l(&c->fifo, buffers, nframes);
+        }
     }
 
     return 0;
@@ -314,8 +322,8 @@ static void qjack_client_recover(QJackClient *c)
     if (c->state == QJACK_STATE_DISCONNECTED &&
         c->packets % 100 == 0) {
 
-        /* if not finished then attempt to recover */
-        if (!c->finished) {
+        /* if enabled then attempt to recover */
+        if (c->enabled) {
             dolog("attempting to reconnect to server\n");
             qjack_client_init(c);
         }
@@ -387,7 +395,6 @@ static int qjack_client_init(QJackClient *c)
     char client_name[jack_client_name_size()];
     jack_options_t options = JackNullOption;
 
-    c->finished      = false;
     c->connect_ports = true;
 
     snprintf(client_name, sizeof(client_name), "%s-%s",
@@ -483,8 +490,10 @@ static int qjack_init_out(HWVoiceOut *hw, struct audsettings *as,
     }
 
     jo->c.out       = true;
+    jo->c.enabled   = false;
     jo->c.nchannels = as->nchannels;
     jo->c.opt       = dev->u.jack.out;
+
     int ret = qjack_client_init(&jo->c);
     if (ret != 0) {
         return ret;
@@ -519,8 +528,10 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings *as,
     }
 
     ji->c.out       = false;
+    ji->c.enabled   = false;
     ji->c.nchannels = as->nchannels;
     ji->c.opt       = dev->u.jack.in;
+
     int ret = qjack_client_init(&ji->c);
     if (ret != 0) {
         return ret;
@@ -568,23 +579,25 @@ static void qjack_client_fini(QJackClient *c)
 static void qjack_fini_out(HWVoiceOut *hw)
 {
     QJackOut *jo = (QJackOut *)hw;
-    jo->c.finished = true;
     qjack_client_fini(&jo->c);
 }
 
 static void qjack_fini_in(HWVoiceIn *hw)
 {
     QJackIn *ji = (QJackIn *)hw;
-    ji->c.finished = true;
     qjack_client_fini(&ji->c);
 }
 
 static void qjack_enable_out(HWVoiceOut *hw, bool enable)
 {
+    QJackOut *jo = (QJackOut *)hw;
+    jo->c.enabled = enable;
 }
 
 static void qjack_enable_in(HWVoiceIn *hw, bool enable)
 {
+    QJackIn *ji = (QJackIn *)hw;
+    ji->c.enabled = enable;
 }
 
 static int qjack_thread_creator(jack_native_thread_t *thread,
-- 
2.20.1



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

* [PATCH 5/6] audio/jack: honour the enable state of the audio device
@ 2020-06-11 15:20 Geoffrey McRae
  0 siblings, 0 replies; 16+ messages in thread
From: Geoffrey McRae @ 2020-06-11 15:20 UTC (permalink / raw)
  To: qemu-devel; +Cc: kraxel

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>
---
 audio/jackaudio.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 249cbd3265..b2b53985ae 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -56,7 +56,7 @@ typedef struct QJackClient {
     AudiodevJackPerDirectionOptions *opt;
 
     bool out;
-    bool finished;
+    bool enabled;
     bool connect_ports;
     int  packets;
 
@@ -271,9 +271,17 @@ static int qjack_process(jack_nframes_t nframes, void *arg)
     }
 
     if (c->out) {
-        qjack_buffer_read_l(&c->fifo, buffers, nframes);
+        if (likely(c->enabled)) {
+            qjack_buffer_read_l(&c->fifo, buffers, nframes);
+        } else {
+            for(int i = 0; i < c->nchannels; ++i) {
+                memset(buffers[i], 0, nframes * sizeof(float));
+            }
+        }
     } else {
-        qjack_buffer_write_l(&c->fifo, buffers, nframes);
+        if (likely(c->enabled)) {
+            qjack_buffer_write_l(&c->fifo, buffers, nframes);
+        }
     }
 
     return 0;
@@ -314,8 +322,8 @@ static void qjack_client_recover(QJackClient *c)
     if (c->state == QJACK_STATE_DISCONNECTED &&
         c->packets % 100 == 0) {
 
-        /* if not finished then attempt to recover */
-        if (!c->finished) {
+        /* if enabled then attempt to recover */
+        if (c->enabled) {
             dolog("attempting to reconnect to server\n");
             qjack_client_init(c);
         }
@@ -387,7 +395,6 @@ static int qjack_client_init(QJackClient *c)
     char client_name[jack_client_name_size()];
     jack_options_t options = JackNullOption;
 
-    c->finished      = false;
     c->connect_ports = true;
 
     snprintf(client_name, sizeof(client_name), "%s-%s",
@@ -483,8 +490,10 @@ static int qjack_init_out(HWVoiceOut *hw, struct audsettings *as,
     }
 
     jo->c.out       = true;
+    jo->c.enabled   = false;
     jo->c.nchannels = as->nchannels;
     jo->c.opt       = dev->u.jack.out;
+
     int ret = qjack_client_init(&jo->c);
     if (ret != 0) {
         return ret;
@@ -519,8 +528,10 @@ static int qjack_init_in(HWVoiceIn *hw, struct audsettings *as,
     }
 
     ji->c.out       = false;
+    ji->c.enabled   = false;
     ji->c.nchannels = as->nchannels;
     ji->c.opt       = dev->u.jack.in;
+
     int ret = qjack_client_init(&ji->c);
     if (ret != 0) {
         return ret;
@@ -568,23 +579,25 @@ static void qjack_client_fini(QJackClient *c)
 static void qjack_fini_out(HWVoiceOut *hw)
 {
     QJackOut *jo = (QJackOut *)hw;
-    jo->c.finished = true;
     qjack_client_fini(&jo->c);
 }
 
 static void qjack_fini_in(HWVoiceIn *hw)
 {
     QJackIn *ji = (QJackIn *)hw;
-    ji->c.finished = true;
     qjack_client_fini(&ji->c);
 }
 
 static void qjack_enable_out(HWVoiceOut *hw, bool enable)
 {
+    QJackOut *jo = (QJackOut *)hw;
+    jo->c.enabled = enable;
 }
 
 static void qjack_enable_in(HWVoiceIn *hw, bool enable)
 {
+    QJackIn *ji = (QJackIn *)hw;
+    ji->c.enabled = enable;
 }
 
 static int qjack_thread_creator(jack_native_thread_t *thread,
-- 
2.20.1



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

end of thread, other threads:[~2020-06-22  9:28 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-13  4:05 [PATCH 0/6] audio/jack: fixes to overall jack behaviour Geoffrey McRae
2020-06-13  4:05 ` [PATCH 1/6] audio/jack: fix invalid minimum buffer size check Geoffrey McRae
2020-06-13  4:05 ` [PATCH 2/6] audio/jack: remove unused stopped state Geoffrey McRae
2020-06-13  4:05 ` [PATCH 3/6] audio/jack: remove invalid set of input support bool Geoffrey McRae
2020-06-13  4:05 ` [PATCH 4/6] audio/jack: do not remove ports when finishing Geoffrey McRae
2020-06-13  4:05 ` [PATCH 5/6] audio/jack: honour the enable state of the audio device Geoffrey McRae
2020-06-17 12:44   ` Gerd Hoffmann
2020-06-18  3:11     ` Geoffrey McRae
2020-06-19  9:29       ` Gerd Hoffmann
2020-06-21  4:06         ` Geoffrey McRae
2020-06-22  9:05           ` Gerd Hoffmann
2020-06-22  9:27             ` Geoffrey McRae
2020-06-13  4:05 ` [PATCH 6/6] audio/jack: simplify the re-init code path Geoffrey McRae
2020-06-14  5:07 ` [PATCH 0/6] audio/jack: fixes to overall jack behaviour no-reply
  -- strict thread matches above, loose matches on Subject: below --
2020-06-11 15:20 [PATCH 5/6] audio/jack: honour the enable state of the audio device Geoffrey McRae
2020-06-11 15:20 Geoffrey McRae

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).