| Message ID | 20190314133858.37970-3-martin@schrodt.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes for PulseAudio driver | expand |
On 3/14/19 8:38 AM, Martin Schrodt wrote: > Signed-off-by: Martin Schrodt <martin@schrodt.org> The subject line says "what" (good) but doesn't state everything the patch is doing (you are not only making it configurable, but you are changing the default). The commit body should say "why" (missing), and mention all the related things being done to satisfy the "what". Some of that information is already in your cover letter, and merely need to be repeated here (the fact that the previous default of 10ms is too short, but that 15ms worked for you). > --- > audio/paaudio.c | 18 +++++++----------- > qapi/audio.json | 5 ++++- > 2 files changed, 11 insertions(+), 12 deletions(-) > > +++ b/qapi/audio.json > @@ -206,12 +206,15 @@ > # > # @name: name of the sink/source to use > # > +# @latency: latency you want PulseAudio to archieve in microseconds s/archieve/achieve/ > +# (default 15000) > # Since: 4.0 > ## > { 'struct': 'AudiodevPaPerDirectionOptions', > 'base': 'AudiodevPerDirectionOptions', > 'data': { > - '*name': 'str' } } > + '*name': 'str', > + '*latency': 'uint32' } } > > ## > # @AudiodevPaOptions: >
Hi, > r = pa_stream_connect_playback (stream, dev, attr, > PA_STREAM_INTERPOLATE_TIMING > -#ifdef PA_STREAM_ADJUST_LATENCY > |PA_STREAM_ADJUST_LATENCY > -#endif Unrelated change, separate commit please. Which pulse version added this define, and how old is it? (should be answered in the commit message). > - /* > - * qemu audio tick runs at 100 Hz (by default), so processing > - * data chunks worth 10 ms of sound should be a good fit. > - */ > - ba.tlength = pa_usec_to_bytes (10 * 1000, &ss); > - ba.minreq = pa_usec_to_bytes (5 * 1000, &ss); > + ba.tlength = pa_usec_to_bytes (ppdo->latency, &ss); > + ba.minreq = -1; Please explain this in the commit message. > - pdo->buffer_length = dev->timer_period * 4; > + pdo->buffer_length = dev->timer_period * 9 / 2; Likewise. > +# @latency: latency you want PulseAudio to archieve in microseconds > +# (default 15000) Is there a good reason to make this configurable? cheers, Gerd
Hi, On 3/15/19 8:53 AM, Gerd Hoffmann wrote: > Hi, > >> r = pa_stream_connect_playback (stream, dev, attr, >> PA_STREAM_INTERPOLATE_TIMING >> -#ifdef PA_STREAM_ADJUST_LATENCY >> |PA_STREAM_ADJUST_LATENCY >> -#endif > > Unrelated change, separate commit please. > > Which pulse version added this define, and how old is it? > (should be answered in the commit message). I'll just put it in again. Flag was added in PA 0.9.11 (December 2014), so we can probably assume that it's there anyway, and if some poor chap decides to compile against prehistoric PA, it won't break if we leave it in. > >> - /* >> - * qemu audio tick runs at 100 Hz (by default), so processing >> - * data chunks worth 10 ms of sound should be a good fit. >> - */ >> - ba.tlength = pa_usec_to_bytes (10 * 1000, &ss); >> - ba.minreq = pa_usec_to_bytes (5 * 1000, &ss); >> + ba.tlength = pa_usec_to_bytes (ppdo->latency, &ss); >> + ba.minreq = -1; > > Please explain this in the commit message. Commit messages have been revamped in V2 and V3 of the patch. > >> - pdo->buffer_length = dev->timer_period * 4; >> + pdo->buffer_length = dev->timer_period * 9 / 2; > > Likewise. Will clean this up. > >> +# @latency: latency you want PulseAudio to archieve in microseconds >> +# (default 15000) > > Is there a good reason to make this configurable? Yes, to squeeze out some milliseconds of latency, where the emulation is tuned to do so (properly isolate emulation threads, propably lower timer-period...) > > cheers, > Gerd >
diff --git a/audio/paaudio.c b/audio/paaudio.c index 1a799ca3e7..c9007fdb01 100644 --- a/audio/paaudio.c +++ b/audio/paaudio.c @@ -500,16 +500,12 @@ static pa_stream *qpa_simple_new ( if (dir == PA_STREAM_PLAYBACK) { r = pa_stream_connect_playback (stream, dev, attr, PA_STREAM_INTERPOLATE_TIMING -#ifdef PA_STREAM_ADJUST_LATENCY |PA_STREAM_ADJUST_LATENCY -#endif |PA_STREAM_AUTO_TIMING_UPDATE, NULL, NULL); } else { r = pa_stream_connect_record (stream, dev, attr, PA_STREAM_INTERPOLATE_TIMING -#ifdef PA_STREAM_ADJUST_LATENCY |PA_STREAM_ADJUST_LATENCY -#endif |PA_STREAM_AUTO_TIMING_UPDATE); } @@ -549,12 +545,8 @@ static int qpa_init_out(HWVoiceOut *hw, struct audsettings *as, ss.channels = as->nchannels; ss.rate = as->freq; - /* - * qemu audio tick runs at 100 Hz (by default), so processing - * data chunks worth 10 ms of sound should be a good fit. - */ - ba.tlength = pa_usec_to_bytes (10 * 1000, &ss); - ba.minreq = pa_usec_to_bytes (5 * 1000, &ss); + ba.tlength = pa_usec_to_bytes (ppdo->latency, &ss); + ba.minreq = -1; ba.maxlength = -1; ba.prebuf = -1; @@ -813,7 +805,11 @@ static int qpa_validate_per_direction_opts (Audiodev *dev, AudiodevPaPerDirectio { if (!pdo->has_buffer_length) { pdo->has_buffer_length = true; - pdo->buffer_length = dev->timer_period * 4; + pdo->buffer_length = dev->timer_period * 9 / 2; + } + if (!pdo->has_latency) { + pdo->has_latency = true; + pdo->latency = 15000; } return 1; } diff --git a/qapi/audio.json b/qapi/audio.json index 97aee37288..3717d552cd 100644 --- a/qapi/audio.json +++ b/qapi/audio.json @@ -206,12 +206,15 @@ # # @name: name of the sink/source to use # +# @latency: latency you want PulseAudio to archieve in microseconds +# (default 15000) # Since: 4.0 ## { 'struct': 'AudiodevPaPerDirectionOptions', 'base': 'AudiodevPerDirectionOptions', 'data': { - '*name': 'str' } } + '*name': 'str', + '*latency': 'uint32' } } ## # @AudiodevPaOptions:
Signed-off-by: Martin Schrodt <martin@schrodt.org> --- audio/paaudio.c | 18 +++++++----------- qapi/audio.json | 5 ++++- 2 files changed, 11 insertions(+), 12 deletions(-)