[Qemu-devel] [PATCH v2] pulseaudio: process audio data in smaller chunks

[Qemu-devel] [PATCH v2] pulseaudio: process audio data in smaller chunks

[Gerd Hoffmann]

The rate of pulseaudio absorbing the audio stream is used to control the
the rate of the guests audio stream.  When the emulated hardware uses
small chunks (like intel-hda does) we need small chunks on the audio
backend side too, otherwise that feedback loop doesn't work very well.

Cc: Max Ehrlich <maxehr@umiacs.umd.edu>
Cc: Martin Schrodt <martin@schrodt.org>
Buglink: https://bugs.launchpad.net/bugs/1795527
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 audio/paaudio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/audio/paaudio.c b/audio/paaudio.c
index 949769774d..4c100bc318 100644
--- a/audio/paaudio.c
+++ b/audio/paaudio.c
@@ -227,7 +227,7 @@ static void *qpa_thread_out (void *arg)
             }
         }
 
-        decr = to_mix = audio_MIN (pa->live, pa->g->conf.samples >> 2);
+        decr = to_mix = audio_MIN(pa->live, pa->g->conf.samples >> 5);
         rpos = pa->rpos;
 
         if (audio_pt_unlock(&pa->pt, __func__)) {
@@ -319,7 +319,7 @@ static void *qpa_thread_in (void *arg)
             }
         }
 
-        incr = to_grab = audio_MIN (pa->dead, pa->g->conf.samples >> 2);
+        incr = to_grab = audio_MIN(pa->dead, pa->g->conf.samples >> 5);
         wpos = pa->wpos;
 
         if (audio_pt_unlock(&pa->pt, __func__)) {
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2] pulseaudio: process audio data in smaller chunks

[Philippe Mathieu-Daudé]

Hi Gerd,

On 11/9/18 3:20 PM, Gerd Hoffmann wrote:
> The rate of pulseaudio absorbing the audio stream is used to control the
> the rate of the guests audio stream.  When the emulated hardware uses
> small chunks (like intel-hda does) we need small chunks on the audio
> backend side too, otherwise that feedback loop doesn't work very well.

Shouldn't this be user-configurable?

> 
> Cc: Max Ehrlich <maxehr@umiacs.umd.edu>
> Cc: Martin Schrodt <martin@schrodt.org>
> Buglink: https://bugs.launchpad.net/bugs/1795527
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  audio/paaudio.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/audio/paaudio.c b/audio/paaudio.c
> index 949769774d..4c100bc318 100644
> --- a/audio/paaudio.c
> +++ b/audio/paaudio.c
> @@ -227,7 +227,7 @@ static void *qpa_thread_out (void *arg)
>              }
>          }
>  
> -        decr = to_mix = audio_MIN (pa->live, pa->g->conf.samples >> 2);
> +        decr = to_mix = audio_MIN(pa->live, pa->g->conf.samples >> 5);
>          rpos = pa->rpos;
>  
>          if (audio_pt_unlock(&pa->pt, __func__)) {
> @@ -319,7 +319,7 @@ static void *qpa_thread_in (void *arg)
>              }
>          }
>  
> -        incr = to_grab = audio_MIN (pa->dead, pa->g->conf.samples >> 2);
> +        incr = to_grab = audio_MIN(pa->dead, pa->g->conf.samples >> 5);
>          wpos = pa->wpos;
>  
>          if (audio_pt_unlock(&pa->pt, __func__)) {
> 

Re: [Qemu-devel] [PATCH v2] pulseaudio: process audio data in smaller chunks

[Gerd Hoffmann]

On Sat, Nov 10, 2018 at 08:36:39PM +0100, Philippe Mathieu-Daudé wrote:
> Hi Gerd,
> 
> On 11/9/18 3:20 PM, Gerd Hoffmann wrote:
> > The rate of pulseaudio absorbing the audio stream is used to control the
> > the rate of the guests audio stream.  When the emulated hardware uses
> > small chunks (like intel-hda does) we need small chunks on the audio
> > backend side too, otherwise that feedback loop doesn't work very well.
> 
> Shouldn't this be user-configurable?

Why?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2] pulseaudio: process audio data in smaller chunks

[Philippe Mathieu-Daudé]

On 12/11/18 10:12, Gerd Hoffmann wrote:
> On Sat, Nov 10, 2018 at 08:36:39PM +0100, Philippe Mathieu-Daudé wrote:
>> Hi Gerd,
>>
>> On 11/9/18 3:20 PM, Gerd Hoffmann wrote:
>>> The rate of pulseaudio absorbing the audio stream is used to control the
>>> the rate of the guests audio stream.  When the emulated hardware uses
>>> small chunks (like intel-hda does) we need small chunks on the audio
>>> backend side too, otherwise that feedback loop doesn't work very well.
>>
>> Shouldn't this be user-configurable?
> 
> Why?

When emulated hardware is not intel-hda?

Re: [Qemu-devel] [PATCH v2] pulseaudio: process audio data in smaller chunks

[Gerd Hoffmann]

On Mon, Nov 12, 2018 at 10:51:36AM +0100, Philippe Mathieu-Daudé wrote:
> On 12/11/18 10:12, Gerd Hoffmann wrote:
> > On Sat, Nov 10, 2018 at 08:36:39PM +0100, Philippe Mathieu-Daudé wrote:
> > > Hi Gerd,
> > > 
> > > On 11/9/18 3:20 PM, Gerd Hoffmann wrote:
> > > > The rate of pulseaudio absorbing the audio stream is used to control the
> > > > the rate of the guests audio stream.  When the emulated hardware uses
> > > > small chunks (like intel-hda does) we need small chunks on the audio
> > > > backend side too, otherwise that feedback loop doesn't work very well.
> > > 
> > > Shouldn't this be user-configurable?
> > 
> > Why?
> 
> When emulated hardware is not intel-hda?

Ok, maybe it is not required then, but it also doesn't hurt.

Also, when making chunk size configurable we should not leave that
to the confused user but pick a working value automatically, probably
depending on the emulated device.  I can't see what the benefit would
be though, especially given that intel-hda is probably used in most
configurations these days.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2] pulseaudio: process audio data in smaller chunks

[Philippe Mathieu-Daudé]

On 12/11/18 12:28, Gerd Hoffmann wrote:
> On Mon, Nov 12, 2018 at 10:51:36AM +0100, Philippe Mathieu-Daudé wrote:
>> On 12/11/18 10:12, Gerd Hoffmann wrote:
>>> On Sat, Nov 10, 2018 at 08:36:39PM +0100, Philippe Mathieu-Daudé wrote:
>>>> Hi Gerd,
>>>>
>>>> On 11/9/18 3:20 PM, Gerd Hoffmann wrote:
>>>>> The rate of pulseaudio absorbing the audio stream is used to control the
>>>>> the rate of the guests audio stream.  When the emulated hardware uses
>>>>> small chunks (like intel-hda does) we need small chunks on the audio
>>>>> backend side too, otherwise that feedback loop doesn't work very well.
>>>>
>>>> Shouldn't this be user-configurable?
>>>
>>> Why?
>>
>> When emulated hardware is not intel-hda?
> 
> Ok, maybe it is not required then, but it also doesn't hurt.
> 
> Also, when making chunk size configurable we should not leave that
> to the confused user but pick a working value automatically, probably
> depending on the emulated device.  I can't see what the benefit would
> be though, especially given that intel-hda is probably used in most
> configurations these days.

For nowadays uses I agree with your patch.
For retrocomputing and other weird stuffs, if we don't have this 
user-configurable, I think we need a comment in the code about this 
change, else it might be hard for other people to figure this change in 
the git history.

With a such comment:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Regards,

Phil.

Re: [Qemu-devel] [PATCH v2] pulseaudio: process audio data in smaller chunks

[Gerd Hoffmann]

> > Also, when making chunk size configurable we should not leave that
> > to the confused user but pick a working value automatically, probably
> > depending on the emulated device.  I can't see what the benefit would
> > be though, especially given that intel-hda is probably used in most
> > configurations these days.
> 
> For nowadays uses I agree with your patch.
> For retrocomputing and other weird stuffs, if we don't have this
> user-configurable, I think we need a comment in the code about this change,
> else it might be hard for other people to figure this change in the git
> history.

Again, why?  The pulse audio threads might wake up more often than
needed.  This should have no negative effect on the functionality of
the emulated sound cards though.  Worst case is we need a few more cpu
cycles due to the higher wakeup rate (didn't measure that though).

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2] pulseaudio: process audio data in smaller chunks

[Philippe Mathieu-Daudé]

On Mon, Nov 12, 2018 at 12:58 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> > > Also, when making chunk size configurable we should not leave that
> > > to the confused user but pick a working value automatically, probably
> > > depending on the emulated device.  I can't see what the benefit would
> > > be though, especially given that intel-hda is probably used in most
> > > configurations these days.
> >
> > For nowadays uses I agree with your patch.
> > For retrocomputing and other weird stuffs, if we don't have this
> > user-configurable, I think we need a comment in the code about this change,
> > else it might be hard for other people to figure this change in the git
> > history.
>
> Again, why?  The pulse audio threads might wake up more often than
> needed.  This should have no negative effect on the functionality of
> the emulated sound cards though.  Worst case is we need a few more cpu
> cycles due to the higher wakeup rate (didn't measure that though).

OK, fine then!

Thanks,

Phil.