[RfC,11/11] spice: add audio

---
 Makefile.objs      |    1 +
 audio/audio.c      |    3 +
 audio/audio_int.h  |    1 +
 audio/spiceaudio.c |  315 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 320 insertions(+), 0 deletions(-)
 create mode 100644 audio/spiceaudio.c

On Wed, 14 Apr 2010, Gerd Hoffmann wrote:

The code does not follow neither audio(which is passable should it be 
internally consistent) nor general QEMU code style (braces missing)

> ---
>  Makefile.objs      |    1 +
>  audio/audio.c      |    3 +
>  audio/audio_int.h  |    1 +
>  audio/spiceaudio.c |  315 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 320 insertions(+), 0 deletions(-)
>  create mode 100644 audio/spiceaudio.c
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index ab1af88..b11db4c 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -84,6 +84,7 @@ common-obj-$(CONFIG_POSIX) += migration-exec.o migration-unix.o migration-fd.o
>  audio-obj-y = audio.o noaudio.o wavaudio.o mixeng.o
>  audio-obj-$(CONFIG_SDL) += sdlaudio.o
>  audio-obj-$(CONFIG_OSS) += ossaudio.o
> +audio-obj-$(CONFIG_SPICE) += spiceaudio.o
>  audio-obj-$(CONFIG_COREAUDIO) += coreaudio.o
>  audio-obj-$(CONFIG_ALSA) += alsaaudio.o
>  audio-obj-$(CONFIG_DSOUND) += dsoundaudio.o
> diff --git a/audio/audio.c b/audio/audio.c
> index dbf0b96..67fc1d3 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -45,6 +45,9 @@
>  */
>  static struct audio_driver *drvtab[] = {
>      CONFIG_AUDIO_DRIVERS
> +#ifdef CONFIG_SPICE
> +    &spice_audio_driver,
> +#endif
>      &no_audio_driver,
>      &wav_audio_driver
>  };
> diff --git a/audio/audio_int.h b/audio/audio_int.h
> index 06e313f..d1f6c2d 100644
> --- a/audio/audio_int.h
> +++ b/audio/audio_int.h
> @@ -209,6 +209,7 @@ extern struct audio_driver coreaudio_audio_driver;
>  extern struct audio_driver dsound_audio_driver;
>  extern struct audio_driver esd_audio_driver;
>  extern struct audio_driver pa_audio_driver;
> +extern struct audio_driver spice_audio_driver;
>  extern struct audio_driver winwave_audio_driver;
>  extern struct mixeng_volume nominal_volume;
>  
> diff --git a/audio/spiceaudio.c b/audio/spiceaudio.c
> new file mode 100644
> index 0000000..0e18f2f
> --- /dev/null
> +++ b/audio/spiceaudio.c
> @@ -0,0 +1,315 @@
> +#include "hw/hw.h"
> +#include "qemu-timer.h"
> +#include "qemu-spice.h"
> +
> +#define AUDIO_CAP "spice"
> +#include "audio.h"
> +#include "audio_int.h"
> +
> +#define LINE_IN_SAMPLES 1024
> +#define LINE_OUT_SAMPLES 1024
> +
> +typedef struct SpiceVoiceOut {
> +    HWVoiceOut            hw;
> +    SpicePlaybackInstance sin;
> +    uint32_t              *frame;
> +    uint32_t              *fpos;
> +    uint32_t              fsize;
> +    uint64_t              prev_ticks;
> +    int                   active;
> +} SpiceVoiceOut;
> +
> +typedef struct SpiceVoiceIn {
> +    HWVoiceIn             hw;
> +    SpiceRecordInstance   sin;
> +    uint64_t              prev_ticks;
> +    int                   active;
> +    uint32_t              samples[LINE_IN_SAMPLES];
> +} SpiceVoiceIn;
> +
> +static SpicePlaybackInterface playback_sif = {
> +    .base.type          = SPICE_INTERFACE_PLAYBACK,
> +    .base.description   = "playback",
> +    .base.major_version = SPICE_INTERFACE_PLAYBACK_MAJOR,
> +    .base.minor_version = SPICE_INTERFACE_PLAYBACK_MINOR,
> +};
> +
> +static SpiceRecordInterface record_sif = {
> +    .base.type          = SPICE_INTERFACE_RECORD,
> +    .base.description   = "record",
> +    .base.major_version = SPICE_INTERFACE_RECORD_MAJOR,
> +    .base.minor_version = SPICE_INTERFACE_RECORD_MINOR,
> +};
> +
> +static void *spice_audio_init(void)
> +{
> +    if (!using_spice)
> +        return NULL;
> +    return qemu_malloc(42);

Eh? The HGttG references should at least be given an explanation in
the comments.

> +}
> +
> +static void spice_audio_fini(void *opaque)
> +{
> +    qemu_free(opaque);
> +}
> +
> +static uint64_t get_monotonic_time(void)
> +{
> +    struct timespec time_space;
> +    clock_gettime(CLOCK_MONOTONIC, &time_space);

a. The presence of monotonic clock is not guranteed
b. The call can fail yet the result value is not checked
c. I have a really hard time following what rt clock (regardless
   of monotonicity is doing here at all)

> +    return (uint64_t)time_space.tv_sec * 1000 * 1000 * 1000 + time_space.tv_nsec;
> +}
> +
> +/* playback */
> +
> +static int line_out_init(HWVoiceOut *hw, struct audsettings *as)
> +{
> +    SpiceVoiceOut *out = container_of(hw, SpiceVoiceOut, hw);
> +    struct audsettings settings;
> +
> +    settings.freq       = SPICE_INTERFACE_PLAYBACK_FREQ;
> +    settings.nchannels  = SPICE_INTERFACE_PLAYBACK_CHAN;
> +    settings.fmt        = AUD_FMT_S16;
> +    settings.endianness = AUDIO_HOST_ENDIANNESS;
> +
> +    audio_pcm_init_info(&hw->info, &settings);
> +    hw->samples = LINE_OUT_SAMPLES;
> +    out->active = 0;
> +
> +    out->sin.base.sif = &playback_sif.base;
> +    spice_server_add_interface(spice_server, &out->sin.base);
> +    return 0;
> +}
> +
> +static void line_out_fini(HWVoiceOut *hw)
> +{
> +    SpiceVoiceOut *out = container_of(hw, SpiceVoiceOut, hw);
> +
> +    spice_server_remove_interface(&out->sin.base);
> +}
> +
> +static int line_out_run(HWVoiceOut *hw, int live)
> +{
> +    SpiceVoiceOut *out = container_of(hw, SpiceVoiceOut, hw);
> +    int rpos, decr;
> +    int samples;
> +    uint64_t now;
> +    uint64_t ticks;
> +    uint64_t bytes;
> +
> +    if (!live) {
> +        return 0;
> +    }
> +
> +    now = get_monotonic_time();
> +    ticks = now - out->prev_ticks;
> +    bytes = (ticks * hw->info.bytes_per_second) / (1000 * 1000 * 1000);
> +    out->prev_ticks = now;
> +
> +    decr = (bytes > INT_MAX)
> +        ? INT_MAX >> hw->info.shift
> +        : (bytes + (1 << (hw->info.shift - 1))) >> hw->info.shift;
> +    decr = audio_MIN(live, decr);
> +
> +    samples = decr;
> +    rpos = hw->rpos;
> +    while (samples) {
> +        int left_till_end_samples = hw->samples - rpos;
> +        int len = audio_MIN(samples, left_till_end_samples);
> +
> +        if (!out->frame) {
> +            spice_server_playback_get_buffer(&out->sin, &out->frame, &out->fsize);
> +            out->fpos = out->frame;
> +        }
> +        if (out->frame) {
> +            len = audio_MIN(len, out->fsize);
> +            hw->clip(out->fpos, hw->mix_buf + rpos, len);
> +            out->fsize -= len;
> +            out->fpos  += len;
> +            if (out->fsize == 0) {
> +                spice_server_playback_put_samples(&out->sin, out->frame);
> +                out->frame = out->fpos = NULL;
> +            }
> +        }
> +        rpos = (rpos + len) % hw->samples;
> +        samples -= len;
> +    }
> +    hw->rpos = rpos;
> +    return decr;
> +}
> +
> +static int line_out_write(SWVoiceOut *sw, void *buf, int len)
> +{
> +    return audio_pcm_sw_write(sw, buf, len);
> +}
> +
> +static int line_out_ctl(HWVoiceOut *hw, int cmd, ...)
> +{
> +    SpiceVoiceOut *out = container_of(hw, SpiceVoiceOut, hw);
> +
> +    switch (cmd) {
> +    case VOICE_ENABLE:
> +        if (out->active) {
> +            break;
> +        }
> +        out->active = 1;
> +        out->prev_ticks = get_monotonic_time();
> +        spice_server_playback_start(&out->sin);
> +        break;
> +    case VOICE_DISABLE:
> +        if (!out->active) {
> +            break;
> +        }
> +        out->active = 0;
> +        if (out->frame) {
> +            memset(out->fpos, 0, out->fsize << 2);
> +            spice_server_playback_put_samples(&out->sin, out->frame);
> +            out->frame = out->fpos = NULL;
> +            spice_server_playback_stop(&out->sin);
> +        }
> +        break;
> +    }
> +    return 0;
> +}
> +
> +/* record */
> +
> +static int line_in_init(HWVoiceIn *hw, struct audsettings *as)
> +{
> +    SpiceVoiceIn *in = container_of(hw, SpiceVoiceIn, hw);
> +    struct audsettings settings;
> +
> +    settings.freq       = SPICE_INTERFACE_RECORD_FREQ;
> +    settings.nchannels  = SPICE_INTERFACE_RECORD_CHAN;
> +    settings.fmt        = AUD_FMT_S16;
> +    settings.endianness = AUDIO_HOST_ENDIANNESS;
> +
> +    audio_pcm_init_info(&hw->info, &settings);
> +    hw->samples = LINE_IN_SAMPLES;
> +    in->active = 0;
> +
> +    in->sin.base.sif = &record_sif.base;
> +    spice_server_add_interface(spice_server, &in->sin.base);
> +    return 0;
> +}
> +
> +static void line_in_fini(HWVoiceIn *hw)
> +{
> +    SpiceVoiceIn *in = container_of(hw, SpiceVoiceIn, hw);
> +
> +    spice_server_remove_interface(&in->sin.base);
> +}
> +
> +static int line_in_run(HWVoiceIn *hw)
> +{
> +    SpiceVoiceIn *in = container_of(hw, SpiceVoiceIn, hw);
> +    int num_samples;
> +    int ready;
> +    int len[2];
> +    uint64_t now;
> +    uint64_t ticks;
> +    uint64_t delta_samp;
> +    uint32_t *samples;
> +
> +    if (!(num_samples = hw->samples - audio_pcm_hw_get_live_in(hw))) {
> +        return 0;
> +    }
> +
> +    now = get_monotonic_time();
> +    ticks = now - in->prev_ticks;
> +    in->prev_ticks = now;
> +    delta_samp = (ticks * hw->info.bytes_per_second) / (1000 * 1000 * 1000);
> +    delta_samp = (delta_samp + (1 << (hw->info.shift - 1))) >> hw->info.shift;
> +
> +    num_samples = audio_MIN(num_samples, delta_samp);
> +
> +    ready = spice_server_record_get_samples(&in->sin, in->samples, num_samples);
> +    samples = in->samples;
> +    if (ready == 0) {
> +        static uint32_t silence[LINE_IN_SAMPLES];
> +        samples = silence;
> +        ready = LINE_IN_SAMPLES;
> +    }
> +
> +    num_samples = audio_MIN(ready, num_samples);
> +
> +    if (hw->wpos + num_samples > hw->samples) {
> +        len[0] = hw->samples - hw->wpos;
> +        len[1] = num_samples - len[0];
> +    } else {
> +        len[0] = num_samples;
> +        len[1] = 0;
> +    }
> +
> +    hw->conv(hw->conv_buf + hw->wpos, samples, len[0], &nominal_volume);
> +
> +    if (len[1]) {
> +        hw->conv(hw->conv_buf, samples + len[0], len[1],
> +                 &nominal_volume);
> +    }
> +
> +    hw->wpos = (hw->wpos + num_samples) % hw->samples;
> +
> +    return num_samples;
> +}
> +
> +static int line_in_read(SWVoiceIn *sw, void *buf, int size)
> +{
> +    return audio_pcm_sw_read(sw, buf, size);
> +}
> +
> +static int line_in_ctl(HWVoiceIn *hw, int cmd, ...)
> +{
> +    SpiceVoiceIn *in = container_of(hw, SpiceVoiceIn, hw);
> +
> +    switch (cmd) {
> +    case VOICE_ENABLE:
> +        if (in->active) {
> +            break;
> +        }
> +        in->active = 1;
> +        in->prev_ticks = get_monotonic_time();
> +        spice_server_record_start(&in->sin);
> +        break;
> +    case VOICE_DISABLE:
> +        if (!in->active) {
> +            break;
> +        }
> +        in->active = 0;
> +        spice_server_record_stop(&in->sin);
> +        break;
> +    }
> +    return 0;
> +}
> +
> +static struct audio_option audio_options[] = {
> +    { /* end of list */ },
> +};
> +
> +static struct audio_pcm_ops audio_callbacks = {
> +    .init_out = line_out_init,
> +    .fini_out = line_out_fini,
> +    .run_out  = line_out_run,
> +    .write    = line_out_write,
> +    .ctl_out  = line_out_ctl,
> +
> +    .init_in  = line_in_init,
> +    .fini_in  = line_in_fini,
> +    .run_in   = line_in_run,
> +    .read     = line_in_read,
> +    .ctl_in   = line_in_ctl,
> +};
> +
> +struct audio_driver spice_audio_driver = {
> +    .name           = "spice",
> +    .descr          = "spice audio driver",
> +    .options        = audio_options,
> +    .init           = spice_audio_init,
> +    .fini           = spice_audio_fini,
> +    .pcm_ops        = &audio_callbacks,
> +    .can_be_default = 1,
> +    .max_voices_out = 1,
> +    .max_voices_in  = 1,
> +    .voice_size_out = sizeof(SpiceVoiceOut),
> +    .voice_size_in  = sizeof(SpiceVoiceIn),
> +};
>

On 04/14/2010 10:51 PM, malc wrote:
>> >  +
>> >  +static uint64_t get_monotonic_time(void)
>> >  +{
>> >  +    struct timespec time_space;
>> >  +    clock_gettime(CLOCK_MONOTONIC,&time_space);
> a. The presence of monotonic clock is not guranteed

There is qemu_get_clock_ns(rt_clock).

Paolo

On Thu, 15 Apr 2010, Paolo Bonzini wrote:

> On 04/14/2010 10:51 PM, malc wrote:
> > > >  +
> > > >  +static uint64_t get_monotonic_time(void)
> > > >  +{
> > > >  +    struct timespec time_space;
> > > >  +    clock_gettime(CLOCK_MONOTONIC,&time_space);
> > a. The presence of monotonic clock is not guranteed
> 
> There is qemu_get_clock_ns(rt_clock).

Sorry, but what does it have to do with anything?

> 
> Paolo
>

On 04/15/2010 02:13 AM, malc wrote:
>>>>> >  >  >  >    +static uint64_t get_monotonic_time(void)
>>>>> >  >  >  >    +{
>>>>> >  >  >  >    +    struct timespec time_space;
>>>>> >  >  >  >    +    clock_gettime(CLOCK_MONOTONIC,&time_space);
>>> >  >  a. The presence of monotonic clock is not guranteed
>> >
>> >  There is qemu_get_clock_ns(rt_clock).
> Sorry, but what does it have to do with anything?

It is exactly the same as Gerd's function plus a fallback if no 
monotonic clock is available.

Paolo

On Thu, 15 Apr 2010, Paolo Bonzini wrote:

> On 04/15/2010 02:13 AM, malc wrote:
> > > > > > >  >  >  >    +static uint64_t get_monotonic_time(void)
> > > > > > >  >  >  >    +{
> > > > > > >  >  >  >    +    struct timespec time_space;
> > > > > > >  >  >  >    +    clock_gettime(CLOCK_MONOTONIC,&time_space);
> > > > >  >  a. The presence of monotonic clock is not guranteed
> > > >
> > > >  There is qemu_get_clock_ns(rt_clock).
> > Sorry, but what does it have to do with anything?
> 
> It is exactly the same as Gerd's function plus a fallback if no monotonic
> clock is available.
> 

Ah get_clock which is as broken as snippet above.

On 04/14/10 22:51, malc wrote:
> On Wed, 14 Apr 2010, Gerd Hoffmann wrote:
>
> The code does not follow neither audio(which is passable should it be
> internally consistent) nor general QEMU code style (braces missing)

Will add the missing braces.

>> +static void *spice_audio_init(void)
>> +{
>> +    if (!using_spice)
>> +        return NULL;
>> +    return qemu_malloc(42);
>
> Eh? The HGttG references should at least be given an explanation in
> the comments.

Just need return something non-NULL here to indicate success.
Also wanted to check how carefully the reviewers are looking ;)

> c. I have a really hard time following what rt clock (regardless
>     of monotonicity is doing here at all)

Accept audio data with the correct rate.  When sending directly to the 
audio device the host hardware controls this.  Spice sends the audio 
data off to the network, so this doesn't work.  The math used by spice 
here looks like a old version of the noaudio code for rate control (/me 
inherited that code so I don't know for sure), which makes sense to me.

cheers,
   Gerd

Hi,

>> c. I have a really hard time following what rt clock (regardless
>> of monotonicity is doing here at all)
>
> Accept audio data with the correct rate. When sending directly to the
> audio device the host hardware controls this. Spice sends the audio data
> off to the network, so this doesn't work. The math used by spice here
> looks like a old version of the noaudio code for rate control (/me
> inherited that code so I don't know for sure), which makes sense to me.

malc pointed out in irc simliar discussions came up for esd support.
Thread starts here:
   http://www.mail-archive.com/qemu-devel@nongnu.org/msg06593.html

Summary: having two clocks should better be avoided (one being vmclock 
and the other esd consuming the data, i.e. indirectly the sound hardware 
actually playing the data).  So instead of using vmtime for rate control 
the esd driver just feeds esd as fast as it can accept data.

Advantage of that approach:

   You'll avoid all clock sync issues such as audible audio blibs
   happening in case one clock is slightly faster as the other.

Problems with that approach:

   General: It adds extra latency.

   Spice: A client may or may not be connected.  In case no client is
   connected nobody consumes the sound stream data and thus there is no
   clock ...

cheers,
   Gerd