Re: [Qemu-devel] [PATCH V2 1/3] audio/paaudio: fix ignored buffer_length setting

[Philippe Mathieu-Daudé]

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 <address@hidden>
> ---
>  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;
>