Patchwork [v4,11/11] audio/rfc: remove PLIVE and PERIOD options

login
register
mail settings
Submitter Marc-André Lureau
Date March 13, 2012, 3:20 p.m.
Message ID <1331652059-10090-12-git-send-email-marcandre.lureau@redhat.com>
Download mbox | patch
Permalink /patch/146432/
State New
Headers show

Comments

Marc-André Lureau - March 13, 2012, 3:20 p.m.
- period seems to be unused now
- plive is very obscure and should either be documented or perhaps removed

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 audio/audio.c          |   35 -----------------------------------
 audio/audio_template.h |   26 --------------------------
 2 files changed, 0 insertions(+), 61 deletions(-)
malc - March 13, 2012, 3:47 p.m.
On Tue, 13 Mar 2012, Marc-Andr? Lureau wrote:

> - period seems to be unused now
> - plive is very obscure and should either be documented or perhaps removed

Plive is obscure because the use case was, in any case it's been years
since i've used it so it can, probably, safely go away.

Period on the other hand is unused because i somehow missed the subtle
change of behavior in one of the patches made by, i think, Gerd.

What i know would like to know is this: Jan, back in the day,
advocated for making the mixeng the default (for musicpal), so, Jan -
can you test this series?

[..snip..]
Gerd Hoffmann - March 14, 2012, 9:22 a.m.
On 03/13/12 16:47, malc wrote:
> On Tue, 13 Mar 2012, Marc-Andr? Lureau wrote:
> 
>> - period seems to be unused now

> Period on the other hand is unused because i somehow missed the subtle
> change of behavior in one of the patches made by, i think, Gerd.

Uhm, which patch?  I think that wasn't intentional, at least I can't
remember intentionally disabling period.  Maybe I missed the subtle
change of behavior too.

I do see the point in having this configurable as this is a cpu overhead
vs. latency tradeoff which one might want to tweak depending on the use
case.

If we keep it we should pass it down to the audio backends I think.
pulseaudio for example uses buffer sizes adjusted for the default period
(see qpa_init_out()), when running with a non-default period pulseaudio
should be able to adjust the buffer sizes accordingly.

cheers,
  Gerd
Marc-André Lureau - March 14, 2012, 11:20 a.m.
Hi

On Wed, Mar 14, 2012 at 10:22 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On 03/13/12 16:47, malc wrote:
>> On Tue, 13 Mar 2012, Marc-Andr? Lureau wrote:
>>
>>> - period seems to be unused now
>
>> Period on the other hand is unused because i somehow missed the subtle
>> change of behavior in one of the patches made by, i think, Gerd.
>
> Uhm, which patch?  I think that wasn't intentional, at least I can't
> remember intentionally disabling period.  Maybe I missed the subtle
> change of behavior too.

Could be:

39deb1e496de81957167daebf5cf5d1fbd5e47c2

> I do see the point in having this configurable as this is a cpu overhead
> vs. latency tradeoff which one might want to tweak depending on the use
> case.

But that would impact a/v sync, or can you report added latency back
to the guest somehow? I imagine audio backend latency should depend on
the configured device buffering/latency, not on an environment tweak.

regards
Gerd Hoffmann - March 14, 2012, 11:49 a.m.
On 03/14/12 12:20, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Mar 14, 2012 at 10:22 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> On 03/13/12 16:47, malc wrote:
>>> On Tue, 13 Mar 2012, Marc-Andr? Lureau wrote:
>>>
>>>> - period seems to be unused now
>>
>>> Period on the other hand is unused because i somehow missed the subtle
>>> change of behavior in one of the patches made by, i think, Gerd.
>>
>> Uhm, which patch?  I think that wasn't intentional, at least I can't
>> remember intentionally disabling period.  Maybe I missed the subtle
>> change of behavior too.
> 
> Could be:
> 
> 39deb1e496de81957167daebf5cf5d1fbd5e47c2

Yes, looks like this one is it.

>> I do see the point in having this configurable as this is a cpu overhead
>> vs. latency tradeoff which one might want to tweak depending on the use
>> case.
> 
> But that would impact a/v sync, or can you report added latency back
> to the guest somehow? I imagine audio backend latency should depend on
> the configured device buffering/latency, not on an environment tweak.

Sure, with lower latency you get better a/v sync too.

Guest interfacing is next to impossible I think, simply because real
hardware has no need for that and thus the interfaces simply don't exist
in the hardware we are emulating.  We can try to fix that with a virtio
soundcard which has such interfaces, but it could be this simply shifts
the issue from the driver/hardware interface to the os-kernel/driver
interface.

cheers,
  Gerd
Marc-André Lureau - March 14, 2012, 12:13 p.m.
Hi

> Guest interfacing is next to impossible I think, simply because real
> hardware has no need for that and thus the interfaces simply don't
> exist
> in the hardware we are emulating.  We can try to fix that with a
> virtio
> soundcard which has such interfaces, but it could be this simply
> shifts
> the issue from the driver/hardware interface to the os-kernel/driver
> interface.

btw, the Xen folks just merged a PulseAudio sink/src implementation a few days ago: http://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/?id=50a7bf1175eaf07521c00bde8eed2f820e64437f

Patch

diff --git a/audio/audio.c b/audio/audio.c
index bb94133..c2e6e15 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -30,7 +30,6 @@ 
 #define AUDIO_CAP "audio"
 #include "audio_int.h"
 
-/* #define DEBUG_PLIVE */
 /* #define DEBUG_LIVE */
 /* #define DEBUG_OUT */
 /* #define DEBUG_CAPTURE */
@@ -62,11 +61,6 @@  struct fixed_settings {
 static struct {
     struct fixed_settings fixed_out;
     struct fixed_settings fixed_in;
-    union {
-        int hertz;
-        int64_t ticks;
-    } period;
-    int plive;
     int log_to_monitor;
     int try_poll_in;
     int try_poll_out;
@@ -96,8 +90,6 @@  static struct {
         }
     },
 
-    .period = { .hertz = 250 },
-    .plive = 0,
     .log_to_monitor = 0,
     .try_poll_in = 1,
     .try_poll_out = 1,
@@ -1453,9 +1445,6 @@  static void audio_run_out (AudioState *s)
             while (sw) {
                 sw1 = sw->entries.le_next;
                 if (!sw->active && !sw->callback.fn) {
-#ifdef DEBUG_PLIVE
-                    dolog ("Finishing with old voice\n");
-#endif
                     audio_close_out (sw);
                 }
                 sw = sw1;
@@ -1642,18 +1631,6 @@  static struct audio_option audio_options[] = {
     },
     /* Misc */
     {
-        .name  = "TIMER_PERIOD",
-        .tag   = AUD_OPT_INT,
-        .valp  = &conf.period.hertz,
-        .descr = "Timer period in HZ (0 - use lowest possible)"
-    },
-    {
-        .name  = "PLIVE",
-        .tag   = AUD_OPT_BOOL,
-        .valp  = &conf.plive,
-        .descr = "(undocumented)"
-    },
-    {
         .name  = "LOG_TO_MONITOR",
         .tag   = AUD_OPT_BOOL,
         .valp  = &conf.log_to_monitor,
@@ -1898,18 +1875,6 @@  static void audio_init (void)
         }
     }
 
-    if (conf.period.hertz <= 0) {
-        if (conf.period.hertz < 0) {
-            dolog ("warning: Timer period is negative - %d "
-                   "treating as zero\n",
-                   conf.period.hertz);
-        }
-        conf.period.ticks = 1;
-    } else {
-        conf.period.ticks =
-            muldiv64 (1, get_ticks_per_sec (), conf.period.hertz);
-    }
-
     e = qemu_add_vm_change_state_handler (audio_vm_change_state_handler, s);
     if (!e) {
         dolog ("warning: Could not register change state handler\n"
diff --git a/audio/audio_template.h b/audio/audio_template.h
index 519432a..4120afb 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -433,29 +433,6 @@  SW *glue (AUD_open_, TYPE) (
         return sw;
     }
 
-#ifdef DAC
-    if (conf.plive && sw && (!sw->active && !sw->empty)) {
-        live = sw->total_hw_samples_mixed;
-
-#ifdef DEBUG_PLIVE
-        dolog ("Replacing voice %s with %d live samples\n", SW_NAME (sw), live);
-        dolog ("Old %s freq %d, bits %d, channels %d\n",
-               SW_NAME (sw), sw->info.freq, sw->info.bits, sw->info.nchannels);
-        dolog ("New %s freq %d, bits %d, channels %d\n",
-               name,
-               as->freq,
-               (as->fmt == AUD_FMT_S16 || as->fmt == AUD_FMT_U16) ? 16 : 8,
-               as->nchannels);
-#endif
-
-        if (live) {
-            old_sw = sw;
-            old_sw->callback.fn = NULL;
-            sw = NULL;
-        }
-    }
-#endif
-
     if (!glue (conf.fixed_, TYPE).enabled && sw) {
         glue (AUD_close_, TYPE) (card, sw);
         sw = NULL;
@@ -495,9 +472,6 @@  SW *glue (AUD_open_, TYPE) (
             * old_sw->info.bytes_per_second
             / sw->info.bytes_per_second;
 
-#ifdef DEBUG_PLIVE
-        dolog ("Silence will be mixed %d\n", mixed);
-#endif
         sw->total_hw_samples_mixed += mixed;
     }
 #endif