On Fri, 2016-11-04 at 13:16 +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 ad4b709..ada1e37 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,
> @@ -852,6 +852,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);
Instead of '4u', can we use 'sizeof(playback->samples[0])' or
something?
> + 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);
> + }
> + }
When testing a replay with audio, the sound doesn't seem to be
perfectly synchronized to the video. I suspect it has to do with the
fact that you're doing this nanosleep call here to try to time the
audio. But nothing is being done to time the video/display commands. So
when there's no sound samples coming in, the replay proceeds at full-
throttle speed, but when sound starts playing, it slows everything down
to approximately normal speed. But qxl commands that are processed
between each audio sample command will still proceed at this fast pace,
so the video will always get ahead of the audio.
Also, this approach doesn't really cooperate very well with the "-s, --
slow" option for spice-server-replay. The --slow command adds a given
timeout to every single command processed by the replay tool, including
the audio commands. So if the --slow timeout is large enough, the audio
sample messages will come too late and the audio will stutter. It seems
to me that we should have a single approach to handling timing of
replay commands. Or if not, perhaps the --audio option and the --slow
option should be mutually exclusive.
> +
> + uint32_t got_samples = size / 4u;
same question about 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);
4u again
> + 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;
I don't understand why you reset the delta here.
> + 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;
>
or here
> + 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;
I find the name 'record' to be a little bit confusing in this file
since the name 'record' could also imply the 'RecordChannel' (the
partner of PlaybackChannel)
> };
>
> 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);
It's not clear to me why this part is necessary for this patch. Can you
explain?
> }
>
> 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;
Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel