diff mbox series

[1/3] fix: buffer_length is ignored

Message ID 20190314133858.37970-2-martin@schrodt.org (mailing list archive)
State New, archived
Headers show
Series Fixes for PulseAudio driver | expand

Commit Message

Martin Schrodt March 14, 2019, 1:38 p.m. UTC 
Signed-off-by: Martin Schrodt <martin@schrodt.org>
---
 audio/paaudio.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

Comments

Gerd Hoffmann March 15, 2019, 7:43 a.m. UTC | #1 
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
Martin Schrodt March 15, 2019, 7:49 a.m. UTC | #2 
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
Gerd Hoffmann March 15, 2019, 8:01 a.m. UTC | #3 
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
Martin Schrodt March 15, 2019, 8:28 a.m. UTC | #4 
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 mbox series

Patch

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;