[Spice-devel] [PATCH spice-server 7/8] RFC replay: Record and replay playback audio
Frediano Ziglio
fziglio at redhat.com
Fri Nov 11 12:32:15 UTC 2016
>
> 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 at redhat.com>
> > ---
> > 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?
>
Yes, in this case a (char*) would be better, I'll update the other.
>
> > + 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.
>
I think the record part of this patch is quite in a good shape and
should be split but the replay one is more a RFH (request for help) than
RFC.
The current replay interface allows only Qxl commands (that is cursor and
display) to be returned with no timing information (which are currently
just recorded in the file). On the other way currently is the replay
utility (server/tests/replay.c) and not the replay code in the server
(server/red-replay-qxl.c) that manage the time.
This patch replay the audio to the client and obviously if you don't
want to just ear garbage timing is important. So the current code of
this patch when audio is enabled allows to check audio but as you said
video is quite out of sync.
Perhaps would be better to change the current API to return audio and
qxl commands with timing and let the replay utility manage these data.
For instance instead of sending to the client replay utility could
record the data to a file (.wav?).
> 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.
>
Right.
>
>
> > +
> > + 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.
>
Basically to start syncing again. At this point should be
0 anyway.
> > + 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
>
Here to stop syncing and get to max speed.
> > + 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)
>
Yes, maybe here replay_record would be better and less confusing.
>
> > };
> >
> > 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?
>
The number of samples can be changed so recording on the frame
helps to get the correct information.
It's just a single assignment and the the field is put in a place
so the structure on 64 bit machines do not increase of a single bit.
> > }
> >
> > 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 at redhat.com>
>
>
Frediano
More information about the Spice-devel
mailing list