On Thu, 2016-11-10 at 10:42 +0000, Frediano Ziglio wrote:
> This patch allow to record playback and replay with spice-server-
> replay
> utility using the a new --audio option.
> The main concern I have is the way the sound is reproduced.
> The audio is correctly passed to the client however this is not that
> useful for automated testing.
> Also to allow the audio to be audible the timing is respected but
> this
> is done inside red-replay-qxl.c without the control of spice-server-
> replay.
> Perhaps would be better to change interface and send back audio data
> to
> spice-server-replay (changing spice_replay_next_cmd or adding a new
> API)
> allowing to pass timing and audio information. Passing the timing for
> instance would allow the replay utility to better reproduce timing
> which could be important for streaming code which is bound to timing.
>
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
> server/red-record-qxl.c | 23 ++++++++--
> server/red-record-qxl.h | 10 +++++
> server/red-replay-qxl.c | 113
> ++++++++++++++++++++++++++++++++++++++++++++++-
> server/sound.c | 29 ++++++++++--
> server/spice-replay.h | 1 +
> server/spice-server.syms | 5 +++
> server/tests/replay.c | 19 ++++++++
> 7 files changed, 192 insertions(+), 8 deletions(-)
>
> diff --git a/server/red-record-qxl.c b/server/red-record-qxl.c
> index 5d41bb0..aa3e44c 100644
> --- a/server/red-record-qxl.c
> +++ b/server/red-record-qxl.c
> @@ -105,7 +105,7 @@ RecordEncoderData record_encoder_data = {
> static uint8_t output[1024*1024*4]; // static buffer for encoding,
> 4MB
> #endif
>
> -static void write_binary(FILE *fd, const char *prefix, size_t size,
> const uint8_t *buf)
> +static void write_binary(FILE *fd, const char *prefix, size_t size,
> const void *buf)
> {
> int n;
>
> @@ -113,7 +113,7 @@ static void write_binary(FILE *fd, const char
> *prefix, size_t size, const uint8_
> ZlibEncoder *enc;
> int zlib_size;
>
> - record_encoder_data.buf = buf;
> + record_encoder_data.buf = (uint8_t *) buf;
> record_encoder_data.size = size;
> enc = zlib_encoder_create(&record_encoder_data.base,
> RECORD_ZLIB_DEFAULT_COMPRESSION_LEVEL);
> @@ -708,7 +708,7 @@ static void red_record_message(FILE *fd,
> RedMemSlotInfo *slots, int group_id,
> */
> qxl = (QXLMessage *)memslot_get_virt(slots, addr, sizeof(*qxl),
> group_id,
> &error);
> - write_binary(fd, "message", strlen((char*)qxl->data),
> (uint8_t*)qxl->data);
> + write_binary(fd, "message", strlen((char*)qxl->data), qxl-
> >data);
> }
>
> static void red_record_surface_cmd(FILE *fd, RedMemSlotInfo *slots,
> int group_id,
> @@ -853,6 +853,23 @@ void red_record_qxl_command(RedRecord *record,
> RedMemSlotInfo *slots,
> pthread_mutex_unlock(&record->lock);
> }
>
> +void red_record_playback_enable(RedRecord *record, gboolean enable)
> +{
> + pthread_mutex_lock(&record->lock);
> + red_record_event_start(record, 2, enable ?
> REC_TYPE_PLAYBACK_ENABLE : REC_TYPE_PLAYBACK_DISABLE);
> + pthread_mutex_unlock(&record->lock);
> +}
> +
> +void red_record_playback_put_samples(RedRecord *record,
> + const uint32_t *samples,
> unsigned num_samples)
> +{
> + pthread_mutex_lock(&record->lock);
> + red_record_event_start(record, 2, REC_TYPE_PLAYBACK_SAMPLES);
> +
> + write_binary(record->fd, "samples", num_samples *
> sizeof(*samples), samples);
> + pthread_mutex_unlock(&record->lock);
> +}
> +
> /**
> * Redirects child output to the file specified
> */
> diff --git a/server/red-record-qxl.h b/server/red-record-qxl.h
> index 293e24a..82b3a64 100644
> --- a/server/red-record-qxl.h
> +++ b/server/red-record-qxl.h
> @@ -45,4 +45,14 @@ void red_record_event(RedRecord *record, int what,
> uint32_t type);
> void red_record_qxl_command(RedRecord *record, RedMemSlotInfo
> *slots,
> QXLCommandExt ext_cmd);
>
> +enum {
> + REC_TYPE_PLAYBACK_ENABLE,
> + REC_TYPE_PLAYBACK_DISABLE,
> + REC_TYPE_PLAYBACK_SAMPLES,
> +};
> +
> +void red_record_playback_enable(RedRecord *record, gboolean enable);
> +void red_record_playback_put_samples(RedRecord *record,
> + const uint32_t *samples,
> unsigned num_samples);
> +
> #endif
> diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
> index 79e00e5..47ec066 100644
> --- a/server/red-replay-qxl.c
> +++ b/server/red-replay-qxl.c
> @@ -23,12 +23,19 @@
> #include <inttypes.h>
> #include <zlib.h>
> #include <pthread.h>
> +#include <glib.h>
> +#include <assert.h>
> +#include <setjmp.h>
> +
> +#include <common/marshaller.h>
> +#include <common/snd_codec.h>
> +
> #include "reds.h"
> #include "red-qxl.h"
> #include "red-common.h"
> #include "memslot.h"
> #include "red-parse-qxl.h"
> -#include <glib.h>
> +#include "red-record-qxl.h"
>
> #define QXLPHYSICAL_FROM_PTR(ptr) ((QXLPHYSICAL)(intptr_t)(ptr))
> #define QXLPHYSICAL_TO_PTR(phy) ((void*)(intptr_t)(phy))
> @@ -38,6 +45,12 @@ typedef enum {
> REPLAY_ERROR,
> } replay_t;
>
> +typedef struct RedReplayPlayback {
> + SpicePlaybackInstance *sin;
> + uint64_t delta_time;
> + uint32_t *samples, *samples_pos, *samples_end;
> +} RedReplayPlayback;
> +
> struct SpiceReplay {
> FILE *fd;
> gboolean error;
> @@ -54,6 +67,8 @@ struct SpiceReplay {
>
> pthread_mutex_t mutex;
> pthread_cond_t cond;
> +
> + RedReplayPlayback playback;
> };
>
> static ssize_t replay_fread(SpiceReplay *replay, uint8_t *buf,
> size_t size)
> @@ -1286,6 +1301,94 @@ static void replay_handle_dev_input(QXLWorker
> *worker, SpiceReplay *replay,
> }
> }
>
> +static void
> +playback_send_samples(RedReplayPlayback *playback)
> +{
> + if (!playback->sin || !playback->samples) {
> + return;
> + }
> + memset(playback->samples_pos, 0, (playback->samples_end -
> playback->samples_pos) * 4u);
> + spice_server_playback_put_samples(playback->sin, playback-
> >samples);
> + playback->samples = playback->samples_pos = playback-
> >samples_end = NULL;
> +}
> +
> +static void
> +replay_handle_audio_samples(SpiceReplay *replay, int type, uint64_t
> timestamp)
> +{
> + uint8_t *data = NULL;
> + size_t size;
> + RedReplayPlayback *const playback = &replay->playback;
> +
> + read_binary(replay, "samples", &size, &data, 0);
> + if (!playback->sin) {
> + replay_free(replay, data);
> + return;
> + }
> +
> + uint64_t curr_time = spice_get_monotonic_time_ns();
> + if (!playback->delta_time) {
> + playback->delta_time = curr_time - timestamp;
> + } else {
> + uint64_t time = timestamp + playback->delta_time;
> + if (time > curr_time) {
> + time -= curr_time;
> + struct timespec ts = { time / 1000000000u, time %
> 1000000000u };
> + nanosleep(&ts, NULL);
> + }
> + }
> +
> + uint32_t got_samples = size / 4u;
> + const uint32_t *samples = (const uint32_t *) data;
> + while (got_samples) {
> + if (!playback->samples_pos) {
> + uint32_t num_samples;
> + spice_assert(!playback->samples);
> + spice_server_playback_get_buffer(playback->sin,
> &playback->samples, &num_samples);
> + if (!playback->samples) {
> + break;
> + }
> + spice_assert(num_samples);
> + playback->samples_pos = playback->samples;
> + playback->samples_end = num_samples + playback-
> >samples_pos;
> + }
> + uint32_t copy = MIN(got_samples, playback->samples_end -
> playback->samples_pos);
> + memcpy(playback->samples_pos, samples, copy * 4u);
> + got_samples -= copy;
> + samples += copy;
> + playback->samples_pos += copy;
> + if (playback->samples_pos >= playback->samples_end) {
> + playback_send_samples(playback);
> + }
> + }
> + replay_free(replay, data);
> +}
> +
> +static void
> +replay_handle_audio(SpiceReplay *replay, int type, uint64_t
> timestamp)
> +{
> + RedReplayPlayback *const playback = &replay->playback;
> +
> + switch (type) {
> + case REC_TYPE_PLAYBACK_ENABLE:
> + if (playback->sin) {
> + spice_server_playback_start(playback->sin);
> + }
> + playback->delta_time = 0;
> + break;
> + case REC_TYPE_PLAYBACK_DISABLE:
> + if (playback->sin) {
> + // send last piece of frame if available
> + playback_send_samples(playback);
> + spice_server_playback_stop(playback->sin);
> + }
> + playback->delta_time = 0;
> + break;
> + case REC_TYPE_PLAYBACK_SAMPLES:
> + replay_handle_audio_samples(replay, type, timestamp);
> + break;
> + }
> +}
> +
> /*
> * NOTE: This reads from a saved file and performs all io actions,
> calling the
> * dispatcher, until it sees a command, at which point it returns it
> via the
> @@ -1310,6 +1413,9 @@ SPICE_GNUC_VISIBLE QXLCommandExt*
> spice_replay_next_cmd(SpiceReplay *replay,
> if (what == 1) {
> replay_handle_dev_input(worker, replay, type);
> }
> + if (what == 2) {
> + replay_handle_audio(replay, type, timestamp);
> + }
> }
> cmd = replay_malloc0(replay, sizeof(QXLCommandExt));
> cmd->cmd.type = type;
> @@ -1456,3 +1562,8 @@ SPICE_GNUC_VISIBLE void
> spice_replay_free(SpiceReplay *replay)
> fclose(replay->fd);
> free(replay);
> }
> +
> +SPICE_GNUC_VISIBLE void spice_replay_set_playback(SpiceReplay
> *replay, SpicePlaybackInstance *sin)
> +{
> + replay->playback.sin = sin;
> +}
> diff --git a/server/sound.c b/server/sound.c
> index be9ad5f..5183d27 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -28,6 +28,7 @@
>
> #include <common/marshaller.h>
> #include <common/generated_server_marshallers.h>
> +#include <common/snd_codec.h>
>
> #include "spice.h"
> #include "red-common.h"
> @@ -41,9 +42,9 @@
> #include "red-channel-client-private.h"
> #include "red-client.h"
> #include "sound.h"
> -#include <common/snd_codec.h>
> #include "demarshallers.h"
> #include "main-channel-client.h"
> +#include "red-record-qxl.h"
>
> #ifndef IOV_MAX
> #define IOV_MAX 1024
> @@ -123,6 +124,8 @@ struct SndChannel {
> snd_channel_handle_message_proc handle_message;
> snd_channel_on_message_done_proc on_message_done;
> snd_channel_cleanup_channel_proc cleanup;
> +
> + RedRecord *record;
> };
>
> typedef struct PlaybackChannel PlaybackChannel;
> @@ -130,6 +133,7 @@ typedef struct PlaybackChannel PlaybackChannel;
> typedef struct AudioFrame AudioFrame;
> struct AudioFrame {
> uint32_t time;
> + uint32_t num_samples;
> uint32_t samples[SND_CODEC_MAX_FRAME_SIZE];
> PlaybackChannel *channel;
> AudioFrame *next;
> @@ -203,6 +207,7 @@ static SndChannel *snd_channel_unref(SndChannel
> *channel)
> {
> if (!--channel->refs) {
> spice_printerr("SndChannel=%p freed", channel);
> + red_record_unref(channel->record);
> free(channel);
> return NULL;
> }
> @@ -969,6 +974,7 @@ static SndChannel *__new_channel(SndWorker
> *worker, int size, uint32_t channel_i
> channel->receive_data.now = channel->receive_data.buf;
> channel->receive_data.end = channel->receive_data.buf +
> sizeof(channel->receive_data.buf);
> channel->send_data.marshaller = spice_marshaller_new();
> + channel->record = reds_get_record(reds);
>
> stream->watch = reds_core_watch_add(reds, stream->socket,
> SPICE_WATCH_EVENT_READ,
> snd_event, channel);
> @@ -1064,6 +1070,11 @@ SPICE_GNUC_VISIBLE void
> spice_server_playback_start(SpicePlaybackInstance *sin)
> sin->st->worker.active = 1;
> if (!channel)
> return;
> +
> + if (channel->record) {
> + red_record_playback_enable(channel->record, TRUE);
> + }
> +
> spice_assert(!playback_channel->base.active);
> reds_disable_mm_time(snd_channel_get_server(channel));
> playback_channel->base.active = TRUE;
> @@ -1083,6 +1094,11 @@ SPICE_GNUC_VISIBLE void
> spice_server_playback_stop(SpicePlaybackInstance *sin)
> sin->st->worker.active = 0;
> if (!channel)
> return;
> +
> + if (channel->record) {
> + red_record_playback_enable(channel->record, FALSE);
> + }
> +
> spice_assert(playback_channel->base.active);
> reds_enable_mm_time(snd_channel_get_server(channel));
> playback_channel->base.active = FALSE;
> @@ -1107,6 +1123,7 @@ SPICE_GNUC_VISIBLE void
> spice_server_playback_get_buffer(SpicePlaybackInstance *
> {
> SndChannel *channel = sin->st->worker.connection;
> PlaybackChannel *playback_channel = SPICE_CONTAINEROF(channel,
> PlaybackChannel, base);
> + AudioFrame *audio_frame;
>
> if (!channel || !playback_channel->free_frames) {
> *frame = NULL;
> @@ -1116,9 +1133,10 @@ SPICE_GNUC_VISIBLE void
> spice_server_playback_get_buffer(SpicePlaybackInstance *
> spice_assert(playback_channel->base.active);
> snd_channel_ref(channel);
>
> - *frame = playback_channel->free_frames->samples;
> - playback_channel->free_frames = playback_channel->free_frames-
> >next;
> - *num_samples = snd_codec_frame_size(playback_channel->codec);
> + audio_frame = playback_channel->free_frames;
> + *frame = audio_frame->samples;
> + playback_channel->free_frames = audio_frame->next;
> + *num_samples = audio_frame->num_samples =
> snd_codec_frame_size(playback_channel->codec);
> }
>
> SPICE_GNUC_VISIBLE void
> spice_server_playback_put_samples(SpicePlaybackInstance *sin,
> uint32_t *samples)
> @@ -1129,6 +1147,9 @@ SPICE_GNUC_VISIBLE void
> spice_server_playback_put_samples(SpicePlaybackInstance
> frame = SPICE_CONTAINEROF(samples, AudioFrame, samples[0]);
> playback_channel = frame->channel;
> spice_assert(playback_channel);
> + if (playback_channel->base.record) {
> + red_record_playback_put_samples(playback_channel-
> >base.record, samples, frame->num_samples);
> + }
> if (!snd_channel_unref(&playback_channel->base) ||
> sin->st->worker.connection != &playback_channel->base) {
> /* lost last reference, channel has been destroyed
> previously */
> diff --git a/server/spice-replay.h b/server/spice-replay.h
> index 9a02869..713f428 100644
> --- a/server/spice-replay.h
> +++ b/server/spice-replay.h
> @@ -33,5 +33,6 @@ QXLCommandExt* spice_replay_next_cmd(SpiceReplay
> *replay, QXLWorker *worker);
> void spice_replay_free_cmd(SpiceReplay *replay,
> QXLCommandExt *cmd);
> void spice_replay_free(SpiceReplay *replay);
> SpiceReplay * spice_replay_new(FILE *file, int nsurfaces);
> +void spice_replay_set_playback(SpiceReplay *replay,
> SpicePlaybackInstance *sin);
>
> #endif // SPICE_REPLAY_H_
> diff --git a/server/spice-server.syms b/server/spice-server.syms
> index edf04a4..898fc9b 100644
> --- a/server/spice-server.syms
> +++ b/server/spice-server.syms
> @@ -173,3 +173,8 @@ SPICE_SERVER_0.13.2 {
> global:
> spice_server_set_video_codecs;
> } SPICE_SERVER_0.13.1;
> +
> +SPICE_SERVER_0.13.3 {
> +global:
> + spice_replay_set_playback;
> +} SPICE_SERVER_0.13.2;
> diff --git a/server/tests/replay.c b/server/tests/replay.c
> index 3a0d515..63d6921 100644
> --- a/server/tests/replay.c
> +++ b/server/tests/replay.c
> @@ -45,6 +45,7 @@ static SpiceReplay *replay;
> static QXLWorker *qxl_worker = NULL;
> static gboolean started = FALSE;
> static QXLInstance display_sin = { 0, };
> +struct SpicePlaybackInstance playback_sin;
> static gint slow = 0;
> static gint skip = 0;
> static gboolean print_count = FALSE;
> @@ -264,6 +265,15 @@ static QXLInterface display_sif = {
> .flush_resources = flush_resources,
> };
>
> +static const SpicePlaybackInterface playback_sif = {
> + .base = {
> + .type = SPICE_INTERFACE_PLAYBACK,
> + .description = "playback",
> + .major_version = SPICE_INTERFACE_PLAYBACK_MAJOR,
> + .minor_version = SPICE_INTERFACE_PLAYBACK_MINOR,
> + }
> +};
> +
> static void replay_channel_event(int event, SpiceChannelEventInfo
> *info)
> {
> if (info->type == SPICE_CHANNEL_DISPLAY &&
> @@ -307,6 +317,7 @@ int main(int argc, char **argv)
> gint port = 5000, compression =
> SPICE_IMAGE_COMPRESSION_AUTO_GLZ;
> gint streaming = SPICE_STREAM_VIDEO_FILTER;
> gboolean wait = FALSE;
> + gboolean audio = FALSE;
> FILE *fd;
>
> GOptionEntry entries[] = {
> @@ -319,6 +330,7 @@ int main(int argc, char **argv)
> { "slow", 's', 0, G_OPTION_ARG_INT, &slow, "Slow down
> replay. Delays USEC microseconds before each command", "USEC" },
> { "skip", 0, 0, G_OPTION_ARG_INT, &skip, "Skip 'slow' for
> the first n commands", NULL },
> { "count", 0, 0, G_OPTION_ARG_NONE, &print_count, "Print the
> number of commands processed", NULL },
> + { "audio", 0, 0, G_OPTION_ARG_NONE, &audio, "Enable audio
> support", NULL },
> { G_OPTION_REMAINING, 0, 0, G_OPTION_ARG_FILENAME_ARRAY,
> &file, "replay file", "FILE" },
> { NULL }
> };
> @@ -418,6 +430,13 @@ int main(int argc, char **argv)
> display_sin.base.sif = &display_sif.base;
> spice_server_add_interface(server, &display_sin.base);
>
> + if (audio) {
> + playback_sin.base.sif = &playback_sif.base;
> + spice_server_add_interface(server, &playback_sin.base);
> +
> + spice_replay_set_playback(replay, &playback_sin);
> + }
> +
> if (client) {
> start_client(client, &error);
> wait = TRUE;
Sorry, just replied to the old version of this patch before I saw that
you had sent a new series. But it looks like my comments still apply to
this patch.
Jonathon
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel