Re: [Qemu-devel] [RfC PATCH 11/11] spice: add audio

[malc]

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),
> +};
> 

-- 
mailto:address@hidden