| Message ID | 20190314142634.34537-2-martin@schrodt.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes for PulseAudio driver | expand |
Hi Martin, On 3/14/19 3:26 PM, Martin Schrodt wrote: > Audiodev configuration allows to set the length of the buffered data. The setting was ignored and a constant value used instead. This patch makes the code apply the setting properly. > > Signed-off-by: Martin Schrodt <martin@schrodt.org> > --- > audio/paaudio.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > 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 this function is expected to return a boolean value, please use the 'bool' C type and true/false. > +{ > + if (!pdo->has_buffer_length) { > + pdo->has_buffer_length = true; > + pdo->buffer_length = dev->timer_period * 4; buffer_length set: return true. > + } else, also return true? Why return a value then? > + 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; >
On 3/14/19 10:34 PM, Philippe Mathieu-Daudé wrote: >> +static int qpa_validate_per_direction_opts (Audiodev *dev, AudiodevPaPerDirectionOptions *pdo) > > If this function is expected to return a boolean value, please use the > 'bool' C type and true/false. > >> +{ >> + if (!pdo->has_buffer_length) { >> + pdo->has_buffer_length = true; >> + pdo->buffer_length = dev->timer_period * 4; > > buffer_length set: return true. > >> + } > > else, also return true? > > Why return a value then? > >> + return 1; >> +} You're right, in the current state it makes no sense. I did it because I also want to add the possibility that validation fails. For example, a buffer length of 0 is invalid, but the case is not handled at the moment. Since I was not sure how exactly to do the validation (wanted Kraxel to answer first), that case is missing atm.
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;
Audiodev configuration allows to set the length of the buffered data. The setting was ignored and a constant value used instead. This patch makes the code apply the setting properly. Signed-off-by: Martin Schrodt <martin@schrodt.org> --- audio/paaudio.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)