[PATCH 5/6] audio/jack: honour the enable state of the audio device

[PATCH 5/6] audio/jack: honour the enable state of the audio device

[Geoffrey McRae]

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

Re: [PATCH 5/6] audio/jack: honour the enable state of the audio device

[Geoffrey McRae]

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

Re: [PATCH 5/6] audio/jack: honour the enable state of the audio device

[Gerd Hoffmann]

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

Re: [PATCH 5/6] audio/jack: honour the enable state of the audio device

[Geoffrey McRae]

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

Re: [PATCH 5/6] audio/jack: honour the enable state of the audio device

[Gerd Hoffmann]

  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

Re: [PATCH 5/6] audio/jack: honour the enable state of the audio device

[Geoffrey McRae]

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

Re: [PATCH 5/6] audio/jack: honour the enable state of the audio device

[Gerd Hoffmann]

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

[PATCH 5/6] audio/jack: honour the enable state of the audio device

[Geoffrey McRae]

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

[PATCH 5/6] audio/jack: honour the enable state of the audio device

[Geoffrey McRae]

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