| Message ID | 20190314133858.37970-2-martin@schrodt.org |
|---|---|
| State | New |
| Headers | show |
| Series | Fixes for PulseAudio driver | expand |
Hi, > - qapi_AudiodevPaPerDirectionOptions_base(ppdo), &obt_as, 46440); > + qapi_AudiodevPaPerDirectionOptions_base(ppdo), &obt_as, ppdo->buffer_length); I'd just use ppdo->has_buffer_length ? ppdo->buffer_length : dev->timer_period * 4 here. cheers, Gerd
Hi, On 3/15/19 8:43 AM, Gerd Hoffmann wrote: > Hi, > >> - qapi_AudiodevPaPerDirectionOptions_base(ppdo), &obt_as, 46440); >> + qapi_AudiodevPaPerDirectionOptions_base(ppdo), &obt_as, ppdo->buffer_length); > > I'd just use > > ppdo->has_buffer_length ? ppdo->buffer_length : dev->timer_period * 4 > > here. > > cheers, > Gerd > > I made sure the value is present via the new function static int qpa_validate_per_direction_opts() That way, I can group the setting of all defaults in a single place, which is cleaner from my perspective. Wouldn't you agree? cheers, Martin
On Fri, Mar 15, 2019 at 08:49:06AM +0100, Martin Schrodt wrote: > Hi, > > On 3/15/19 8:43 AM, Gerd Hoffmann wrote: > > Hi, > > > >> - qapi_AudiodevPaPerDirectionOptions_base(ppdo), &obt_as, 46440); > >> + qapi_AudiodevPaPerDirectionOptions_base(ppdo), &obt_as, ppdo->buffer_length); > > > > I'd just use > > > > ppdo->has_buffer_length ? ppdo->buffer_length : dev->timer_period * 4 > > > > here. > > > > cheers, > > Gerd > > > > > > I made sure the value is present via the new function > > static int qpa_validate_per_direction_opts() > > That way, I can group the setting of all defaults in a single place, > which is cleaner from my perspective. But you also set has_buffer_length, so we loose the information whenever the user has specified a buffer length on the command line or not. If you want bundle default calculation in one place I'd suggest adding a get_buffer_length() function where you can place the "if (has_buffer_length) then { ... } else { ... }" logic. A simliar function for the latency can be placed next to it. cheers, Gerd
On 3/15/19 9:01 AM, Gerd Hoffmann wrote: > On Fri, Mar 15, 2019 at 08:49:06AM +0100, Martin Schrodt wrote: >> Hi, >> >> On 3/15/19 8:43 AM, Gerd Hoffmann wrote: >>> Hi, >>> >>>> - qapi_AudiodevPaPerDirectionOptions_base(ppdo), &obt_as, 46440); >>>> + qapi_AudiodevPaPerDirectionOptions_base(ppdo), &obt_as, ppdo->buffer_length); >>> >>> I'd just use >>> >>> ppdo->has_buffer_length ? ppdo->buffer_length : dev->timer_period * 4 >>> >>> here. >>> >>> cheers, >>> Gerd >>> >>> >> >> I made sure the value is present via the new function >> >> static int qpa_validate_per_direction_opts() >> >> That way, I can group the setting of all defaults in a single place, >> which is cleaner from my perspective. > > But you also set has_buffer_length, so we loose the information whenever > the user has specified a buffer length on the command line or not. > > If you want bundle default calculation in one place I'd suggest adding a > get_buffer_length() function where you can place the > "if (has_buffer_length) then { ... } else { ... }" logic. A simliar > function for the latency can be placed next to it. > I did so because Zoltans code in audio.c does it the same way. See audio.c, functions audio_validate_opts() and audio_validate_per_direction_opts(). I mean I can do a separate function for each, but is it really worth it?
diff --git a/audio/paaudio.c b/audio/paaudio.c index 5d410ed73f..1a799ca3e7 100644 --- a/audio/paaudio.c +++ b/audio/paaudio.c @@ -577,7 +577,7 @@ static int qpa_init_out(HWVoiceOut *hw, struct audsettings *as, audio_pcm_init_info (&hw->info, &obt_as); hw->samples = pa->samples = audio_buffer_samples( - qapi_AudiodevPaPerDirectionOptions_base(ppdo), &obt_as, 46440); + qapi_AudiodevPaPerDirectionOptions_base(ppdo), &obt_as, ppdo->buffer_length); pa->pcm_buf = audio_calloc(__func__, hw->samples, 1 << hw->info.shift); pa->rpos = hw->rpos; if (!pa->pcm_buf) { @@ -637,7 +637,7 @@ static int qpa_init_in(HWVoiceIn *hw, struct audsettings *as, void *drv_opaque) audio_pcm_init_info (&hw->info, &obt_as); hw->samples = pa->samples = audio_buffer_samples( - qapi_AudiodevPaPerDirectionOptions_base(ppdo), &obt_as, 46440); + qapi_AudiodevPaPerDirectionOptions_base(ppdo), &obt_as, ppdo->buffer_length); pa->pcm_buf = audio_calloc(__func__, hw->samples, 1 << hw->info.shift); pa->wpos = hw->wpos; if (!pa->pcm_buf) { @@ -809,7 +809,15 @@ static int qpa_ctl_in (HWVoiceIn *hw, int cmd, ...) return 0; } -/* common */ +static int qpa_validate_per_direction_opts (Audiodev *dev, AudiodevPaPerDirectionOptions *pdo) +{ + if (!pdo->has_buffer_length) { + pdo->has_buffer_length = true; + pdo->buffer_length = dev->timer_period * 4; + } + return 1; +} + static void *qpa_audio_init(Audiodev *dev) { paaudio *g; @@ -836,6 +844,13 @@ static void *qpa_audio_init(Audiodev *dev) g = g_malloc(sizeof(paaudio)); server = popts->has_server ? popts->server : NULL; + if (!qpa_validate_per_direction_opts(dev, popts->in)) { + goto fail; + } + if (!qpa_validate_per_direction_opts(dev, popts->out)) { + goto fail; + } + g->dev = dev; g->mainloop = NULL; g->context = NULL;
Signed-off-by: Martin Schrodt <martin@schrodt.org> --- audio/paaudio.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)