| Message ID | 20181109142032.1628-1-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] pulseaudio: process audio data in smaller chunks | expand |
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__)) { >
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
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?
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
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.
> > 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
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.
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__)) {
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(-)