I've been trying to review this for quite a while now. It's rather
dense and difficult to make sure I understand all of the potential
minor changes, etc.
In general, it looks good, but I have a couple of questions/comments
below.
On Tue, 2016-12-20 at 17:44 +0000, Frediano Ziglio wrote:
> This patch is quite huge but have some benefits:
> - reduce dependency (DummyChannel* and some RedChannelClient
> internals);
> - reuse RedChannelClient instead of reading from the RedsStream
> directly.
>
> You can see that SndChannelClient has much less field
> as the code to read/write from/to client is reused from
> RedChannelClient instead of creating a fake RedChannelClient
> just to make the system happy.
>
> One of the different between the old sound code and all other
> RedChannelClient objects was that the sound channel don't use
> a queue while RedChannelClient use RedPipeItem object. This was
> the main reason why RedChannelClient was not used. To implement
Perhaps it's just my brain turning to mush as I try to fully understand
all of these changes, but can you explain *why* we can't just use a
queue like the other channels?
> the old behaviour a "persistent_pipe_item" is used. This RedPipeItem
> will be queued to RedChannelClient (only one!) so signal code we
> have data to send. The {playback,record}_channel_send_item will
> then send the messages to the client using RedChannelClient
> functions.
> For this reason snd_reset_send_data is replaced by a call to
> red_channel_client_init_send_data and snd_begin_send_message is
> replaced by red_channel_client_begin_send_message.
>
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
> server/sound.c | 944 +++++++++++++++++++++------------------------
> -----
> 1 file changed, 413 insertions(+), 531 deletions(-)
>
> diff --git a/server/sound.c b/server/sound.c
> index 310ff6e..a645b60 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -32,14 +32,10 @@
>
> #include "spice.h"
> #include "red-common.h"
> -#include "dummy-channel.h"
> -#include "dummy-channel-client.h"
> #include "main-channel.h"
> #include "reds.h"
> #include "red-qxl.h"
> #include "red-channel-client.h"
> -/* FIXME: for now, allow sound channel access to private
> RedChannelClient data */
> -#include "red-channel-client-private.h"
> #include "red-client.h"
> #include "sound.h"
> #include "demarshallers.h"
> @@ -56,6 +52,7 @@ enum SndCommand {
> SND_MIGRATE,
> SND_CTRL,
> SND_VOLUME,
> + SND_MUTE,
> SND_END_COMMAND,
> };
>
> @@ -68,6 +65,8 @@ enum PlaybackCommand {
> #define SND_MIGRATE_MASK (1 << SND_MIGRATE)
> #define SND_CTRL_MASK (1 << SND_CTRL)
> #define SND_VOLUME_MASK (1 << SND_VOLUME)
> +#define SND_MUTE_MASK (1 << SND_MUTE)
> +#define SND_VOLUME_MUTE_MASK (SND_VOLUME_MASK|SND_MUTE_MASK)
>
> #define SND_PLAYBACK_MODE_MASK (1 << SND_PLAYBACK_MODE)
> #define SND_PLAYBACK_PCM_MASK (1 << SND_PLAYBACK_PCM)
> @@ -82,49 +81,43 @@ typedef struct AudioFrameContainer
> AudioFrameContainer;
> typedef struct SpicePlaybackState PlaybackChannel;
> typedef struct SpiceRecordState RecordChannel;
>
> -typedef void (*snd_channel_send_messages_proc)(void *in_channel);
> -typedef int (*snd_channel_handle_message_proc)(SndChannelClient
> *client, size_t size, uint32_t type, void *message);
> typedef void (*snd_channel_on_message_done_proc)(SndChannelClient
> *client);
> -typedef void (*snd_channel_cleanup_channel_proc)(SndChannelClient
> *client);
>
> -#define SND_CHANNEL_CLIENT(obj) (&(obj)->base)
> +
> +#define TYPE_SND_CHANNEL_CLIENT snd_channel_client_get_type()
> +#define SND_CHANNEL_CLIENT(obj) \
> + (G_TYPE_CHECK_INSTANCE_CAST((obj), TYPE_SND_CHANNEL_CLIENT,
> SndChannelClient))
> +GType snd_channel_client_get_type(void) G_GNUC_CONST;
>
> /* Connects an audio client to a Spice client */
> struct SndChannelClient {
> - RedsStream *stream;
> - SndChannel *channel;
> - spice_parse_channel_func_t parser;
> - int refs;
> -
> - RedChannelClient *channel_client;
> + RedChannelClient parent;
>
> int active;
> int client_active;
> - int blocked;
>
> uint32_t command;
>
> - struct {
> - uint64_t serial;
> - SpiceMarshaller *marshaller;
> - uint32_t size;
> - uint32_t pos;
> - } send_data;
> -
> - struct {
> - uint8_t buf[SND_RECEIVE_BUF_SIZE];
> - uint8_t *message_start;
> - uint8_t *now;
> - uint8_t *end;
> - } receive_data;
> -
> - snd_channel_send_messages_proc send_messages;
> - snd_channel_handle_message_proc handle_message;
> + /* we don't expect very big messages so don't allocate too much
> + * bytes, data will be cached in RecordChannelClient::samples */
> + uint8_t receive_buf[SND_CODEC_MAX_FRAME_BYTES + 64];
> + RedPipeItem persistent_pipe_item;
> +
> snd_channel_on_message_done_proc on_message_done;
> - snd_channel_cleanup_channel_proc cleanup;
In theory, on_message_done() seems like more of a class-level (virtual)
function, no?
> };
>
> -typedef struct AudioFrame AudioFrame;
> +typedef struct SndChannelClientClass {
> + RedChannelClientClass parent_class;
> +} SndChannelClientClass;
> +
> +G_DEFINE_TYPE(SndChannelClient, snd_channel_client,
> RED_TYPE_CHANNEL_CLIENT)
> +
> +
> +enum {
> + RED_PIPE_ITEM_PERSISTENT = RED_PIPE_ITEM_TYPE_CHANNEL_BASE,
> +};
> +
> +
> struct AudioFrame {
> uint32_t time;
> uint32_t samples[SND_CODEC_MAX_FRAME_SIZE];
> @@ -141,8 +134,13 @@ struct AudioFrameContainer
> AudioFrame items[NUM_AUDIO_FRAMES];
> };
>
> +#define TYPE_PLAYBACK_CHANNEL_CLIENT
> playback_channel_client_get_type()
> +#define PLAYBACK_CHANNEL_CLIENT(obj) \
> + (G_TYPE_CHECK_INSTANCE_CAST((obj), TYPE_PLAYBACK_CHANNEL_CLIENT,
> PlaybackChannelClient))
> +GType playback_channel_client_get_type(void) G_GNUC_CONST;
> +
> struct PlaybackChannelClient {
> - SndChannelClient base;
> + SndChannelClient parent;
>
> AudioFrameContainer *frames;
> AudioFrame *free_frames;
> @@ -154,6 +152,13 @@ struct PlaybackChannelClient {
> uint8_t encode_buf[SND_CODEC_MAX_COMPRESSED_BYTES];
> };
>
> +typedef struct PlaybackChannelClientClass {
> + SndChannelClientClass parent_class;
> +} PlaybackChannelClientClass;
> +
> +G_DEFINE_TYPE(PlaybackChannelClient, playback_channel_client,
> TYPE_SND_CHANNEL_CLIENT)
> +
> +
> typedef struct SpiceVolumeState {
> uint16_t *volume;
> uint8_t volume_nchannels;
> @@ -214,8 +219,13 @@ typedef struct RecordChannelClass {
> G_DEFINE_TYPE(RecordChannel, record_channel, TYPE_SND_CHANNEL)
>
>
> +#define TYPE_RECORD_CHANNEL_CLIENT record_channel_client_get_type()
> +#define RECORD_CHANNEL_CLIENT(obj) \
> + (G_TYPE_CHECK_INSTANCE_CAST((obj), TYPE_RECORD_CHANNEL_CLIENT,
> RecordChannelClient))
> +GType record_channel_client_get_type(void) G_GNUC_CONST;
> +
> struct RecordChannelClient {
> - SndChannelClient base;
> + SndChannelClient parent;
> uint32_t samples[RECORD_SAMPLES_SIZE];
> uint32_t write_pos;
> uint32_t read_pos;
> @@ -226,57 +236,24 @@ struct RecordChannelClient {
> uint8_t decode_buf[SND_CODEC_MAX_FRAME_BYTES];
> };
>
> +typedef struct RecordChannelClientClass {
> + SndChannelClientClass parent_class;
> +} RecordChannelClientClass;
> +
> +G_DEFINE_TYPE(RecordChannelClient, record_channel_client,
> TYPE_SND_CHANNEL_CLIENT)
> +
> +
> /* A list of all Spice{Playback,Record}State objects */
> static SndChannel *snd_channels;
>
> -static void snd_receive(SndChannelClient *client);
> static void snd_playback_start(SndChannel *channel);
> static void snd_record_start(SndChannel *channel);
> -static void snd_playback_alloc_frames(PlaybackChannelClient
> *playback);
> -
> -static SndChannelClient *snd_channel_unref(SndChannelClient *client)
> -{
> - if (!--client->refs) {
> - spice_printerr("SndChannelClient=%p freed", client);
> - free(client);
> - return NULL;
> - }
> - return client;
> -}
> +static void snd_send(SndChannelClient * client);
>
> static RedsState* snd_channel_get_server(SndChannelClient *client)
> {
> g_return_val_if_fail(client != NULL, NULL);
> - return red_channel_get_server(RED_CHANNEL(client->channel));
> -}
> -
> -static void snd_disconnect_channel(SndChannelClient *client)
> -{
> - SndChannel *channel;
> - RedsState *reds;
> - RedChannel *red_channel;
> - uint32_t type;
> -
> - if (!client || !client->stream) {
> - spice_debug("not connected");
> - return;
> - }
> - red_channel = red_channel_client_get_channel(client-
> >channel_client);
> - reds = snd_channel_get_server(client);
> - g_object_get(red_channel, "channel-type", &type, NULL);
> - spice_debug("SndChannelClient=%p rcc=%p type=%d",
> - client, client->channel_client, type);
> - channel = client->channel;
> - client->cleanup(client);
> - red_channel_client_disconnect(channel->connection-
> >channel_client);
> - channel->connection->channel_client = NULL;
> - reds_core_watch_remove(reds, client->stream->watch);
> - client->stream->watch = NULL;
> - reds_stream_free(client->stream);
> - client->stream = NULL;
> - spice_marshaller_destroy(client->send_data.marshaller);
> - snd_channel_unref(client);
> - channel->connection = NULL;
> + return
> red_channel_get_server(red_channel_client_get_channel(RED_CHANNEL_CLI
> ENT(client)));
> }
>
> static void snd_playback_free_frame(PlaybackChannelClient
> *playback_client, AudioFrame *frame)
> @@ -294,69 +271,11 @@ static void
> snd_playback_on_message_done(SndChannelClient *client)
> playback_client->in_progress = NULL;
> if (playback_client->pending_frame) {
> client->command |= SND_PLAYBACK_PCM_MASK;
> + snd_send(client);
> }
I'm afraid that it's not entirely clear to me why this change was
necessary.
> }
> }
>
> -static void snd_record_on_message_done(SndChannelClient *client)
> -{
> -}
> -
> -static int snd_send_data(SndChannelClient *client)
> -{
> - uint32_t n;
> -
> - if (!client) {
> - return FALSE;
> - }
> -
> - if (!(n = client->send_data.size - client->send_data.pos)) {
> - return TRUE;
> - }
> -
> - RedsState *reds = snd_channel_get_server(client);
> - for (;;) {
> - struct iovec vec[IOV_MAX];
> - int vec_size;
> -
> - if (!n) {
> - client->on_message_done(client);
> -
> - if (client->blocked) {
> - client->blocked = FALSE;
> - reds_core_watch_update_mask(reds, client->stream-
> >watch, SPICE_WATCH_EVENT_READ);
> - }
> - break;
> - }
> -
> - vec_size = spice_marshaller_fill_iovec(client-
> >send_data.marshaller,
> - vec, IOV_MAX, client-
> >send_data.pos);
> - n = reds_stream_writev(client->stream, vec, vec_size);
> - if (n == -1) {
> - switch (errno) {
> - case EAGAIN:
> - client->blocked = TRUE;
> - reds_core_watch_update_mask(reds, client->stream-
> >watch, SPICE_WATCH_EVENT_READ |
> - SPI
> CE_WATCH_EVENT_WRITE);
> - return FALSE;
> - case EINTR:
> - break;
> - case EPIPE:
> - snd_disconnect_channel(client);
> - return FALSE;
> - default:
> - spice_printerr("%s", strerror(errno));
> - snd_disconnect_channel(client);
> - return FALSE;
> - }
> - } else {
> - client->send_data.pos += n;
> - }
> - n = client->send_data.size - client->send_data.pos;
> - }
> - return TRUE;
> -}
> -
> static int snd_record_handle_write(RecordChannelClient
> *record_client, size_t size, void *message)
> {
> SpiceMsgcRecordPacket *packet;
> @@ -402,35 +321,29 @@ static int
> snd_record_handle_write(RecordChannelClient *record_client, size_t si
> return TRUE;
> }
>
> -static int snd_playback_handle_message(SndChannelClient *client,
> size_t size, uint32_t type, void *message)
> +static int
> +playback_channel_handle_parsed(RedChannelClient *rcc, uint32_t size,
> uint16_t type, void *message)
> {
> - if (!client) {
> - return FALSE;
> - }
> -
> switch (type) {
> case SPICE_MSGC_DISCONNECTING:
> break;
> default:
> - spice_printerr("invalid message type %u", type);
> - return FALSE;
> + return red_channel_client_handle_message(rcc, size, type,
> message);
> }
> return TRUE;
> }
>
> -static int snd_record_handle_message(SndChannelClient *client,
> size_t size, uint32_t type, void *message)
> +static int
> +record_channel_handle_parsed(RedChannelClient *rcc, uint32_t size,
> uint16_t type, void *message)
> {
> - RecordChannelClient *record_client = (RecordChannelClient
> *)client;
> + RecordChannelClient *record_client = RECORD_CHANNEL_CLIENT(rcc);
>
> - if (!client) {
> - return FALSE;
> - }
> switch (type) {
> case SPICE_MSGC_RECORD_DATA:
> return snd_record_handle_write(record_client, size,
> message);
> case SPICE_MSGC_RECORD_MODE: {
> SpiceMsgcRecordMode *mode = (SpiceMsgcRecordMode *)message;
> - SndChannel *channel = client->channel;
> + SndChannel *channel =
> SND_CHANNEL(red_channel_client_get_channel(rcc));
> record_client->mode_time = mode->time;
> if (mode->mode != SPICE_AUDIO_DATA_MODE_RAW) {
> if (snd_codec_is_capable(mode->mode, channel-
> >frequency)) {
> @@ -460,153 +373,23 @@ static int
> snd_record_handle_message(SndChannelClient *client, size_t size, uint
> case SPICE_MSGC_DISCONNECTING:
> break;
> default:
> - spice_printerr("invalid message type %u", type);
> - return FALSE;
> + return red_channel_client_handle_message(rcc, size, type,
> message);
> }
> return TRUE;
> }
>
> -static void snd_receive(SndChannelClient *client)
> -{
> - SpiceDataHeaderOpaque *header;
> -
> - if (!client) {
> - return;
> - }
> -
> - header = &client->channel_client->incoming.header;
> -
> - for (;;) {
> - ssize_t n;
> - n = client->receive_data.end - client->receive_data.now;
> - spice_warn_if_fail(n > 0);
> - n = reds_stream_read(client->stream, client-
> >receive_data.now, n);
> - if (n <= 0) {
> - if (n == 0) {
> - snd_disconnect_channel(client);
> - return;
> - }
> - spice_assert(n == -1);
> - switch (errno) {
> - case EAGAIN:
> - return;
> - case EINTR:
> - break;
> - case EPIPE:
> - snd_disconnect_channel(client);
> - return;
> - default:
> - spice_printerr("%s", strerror(errno));
> - snd_disconnect_channel(client);
> - return;
> - }
> - } else {
> - client->receive_data.now += n;
> - for (;;) {
> - uint8_t *msg_start = client-
> >receive_data.message_start;
> - uint8_t *data = msg_start + header->header_size;
> - size_t parsed_size;
> - uint8_t *parsed;
> - message_destructor_t parsed_free;
> -
> - header->data = msg_start;
> - n = client->receive_data.now - msg_start;
> -
> - if (n < header->header_size ||
> - n < header->header_size + header-
> >get_msg_size(header)) {
> - break;
> - }
> - parsed = client->parser((void *)data, data + header-
> >get_msg_size(header),
> - header-
> >get_msg_type(header),
> - SPICE_VERSION_MINOR,
> &parsed_size, &parsed_free);
> - if (parsed == NULL) {
> - spice_printerr("failed to parse message type
> %d", header->get_msg_type(header));
> - snd_disconnect_channel(client);
> - return;
> - }
> - if (!client->handle_message(client, parsed_size,
> - header-
> >get_msg_type(header), parsed)) {
> - free(parsed);
> - snd_disconnect_channel(client);
> - return;
> - }
> - parsed_free(parsed);
> - client->receive_data.message_start = msg_start +
> header->header_size +
> - header-
> >get_msg_size(header);
> - }
> - if (client->receive_data.now == client-
> >receive_data.message_start) {
> - client->receive_data.now = client->receive_data.buf;
> - client->receive_data.message_start = client-
> >receive_data.buf;
> - } else if (client->receive_data.now == client-
> >receive_data.end) {
> - memcpy(client->receive_data.buf, client-
> >receive_data.message_start, n);
> - client->receive_data.now = client->receive_data.buf
> + n;
> - client->receive_data.message_start = client-
> >receive_data.buf;
> - }
> - }
> - }
> -}
> -
> -static void snd_event(int fd, int event, void *data)
> -{
> - SndChannelClient *client = data;
> -
> - if (event & SPICE_WATCH_EVENT_READ) {
> - snd_receive(client);
> - }
> - if (event & SPICE_WATCH_EVENT_WRITE) {
> - client->send_messages(client);
> - }
> -}
> -
> -static inline int snd_reset_send_data(SndChannelClient *client,
> uint16_t verb)
> -{
> - SpiceDataHeaderOpaque *header;
> -
> - if (!client) {
> - return FALSE;
> - }
> -
> - header = &client->channel_client->priv->send_data.header;
> - spice_marshaller_reset(client->send_data.marshaller);
> - header->data = spice_marshaller_reserve_space(client-
> >send_data.marshaller,
> - header-
> >header_size);
> - spice_marshaller_set_base(client->send_data.marshaller,
> - header->header_size);
> - client->send_data.pos = 0;
> - header->set_msg_size(header, 0);
> - header->set_msg_type(header, verb);
> - client->send_data.serial++;
> - if (!client->channel_client->priv->is_mini_header) {
> - header->set_msg_serial(header, client->send_data.serial);
> - header->set_msg_sub_list(header, 0);
> - }
> -
> - return TRUE;
> -}
> -
> -static int snd_begin_send_message(SndChannelClient *client)
> -{
> - SpiceDataHeaderOpaque *header = &client->channel_client->priv-
> >send_data.header;
> -
> - spice_marshaller_flush(client->send_data.marshaller);
> - client->send_data.size = spice_marshaller_get_total_size(client-
> >send_data.marshaller);
> - header->set_msg_size(header, client->send_data.size - header-
> >header_size);
> - return snd_send_data(client);
> -}
> -
> static int snd_channel_send_migrate(SndChannelClient *client)
> {
> - SpiceMarshaller *m = client->send_data.marshaller;
> + RedChannelClient *rcc = RED_CHANNEL_CLIENT(client);
> + SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
> SpiceMsgMigrate migrate;
>
> - if (!snd_reset_send_data(client, SPICE_MSG_MIGRATE)) {
> - return FALSE;
> - }
> - spice_debug(NULL);
> + red_channel_client_init_send_data(rcc, SPICE_MSG_MIGRATE);
> migrate.flags = 0;
> spice_marshall_msg_migrate(m, &migrate);
>
> - return snd_begin_send_message(client);
> + red_channel_client_begin_send_message(rcc);
> + return FALSE;
Why return FALSE here (and the following functions)? The previous code
returned FALSE to indicate an error. I would think you'd want to return
TRUE from all of these functions, no?
> }
>
> static int snd_playback_send_migrate(PlaybackChannelClient *client)
> @@ -618,25 +401,26 @@ static int snd_send_volume(SndChannelClient
> *client, uint32_t cap, int msg)
> {
> SpiceMsgAudioVolume *vol;
> uint8_t c;
> - SpiceVolumeState *st = &client->channel->volume;
> - SpiceMarshaller *m = client->send_data.marshaller;
> + RedChannelClient *rcc = RED_CHANNEL_CLIENT(client);
> + SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
> + SndChannel *channel =
> SND_CHANNEL(red_channel_client_get_channel(rcc));
> + SpiceVolumeState *st = &channel->volume;
>
> - if (!red_channel_client_test_remote_cap(client->channel_client,
> cap)) {
> + if (!red_channel_client_test_remote_cap(rcc, cap)) {
> return TRUE;
> }
>
> vol = alloca(sizeof (SpiceMsgAudioVolume) +
> st->volume_nchannels * sizeof (uint16_t));
> - if (!snd_reset_send_data(client, msg)) {
> - return FALSE;
> - }
> + red_channel_client_init_send_data(rcc, msg);
> vol->nchannels = st->volume_nchannels;
> for (c = 0; c < st->volume_nchannels; ++c) {
> vol->volume[c] = st->volume[c];
> }
> spice_marshall_SpiceMsgAudioVolume(m, vol);
>
> - return snd_begin_send_message(client);
> + red_channel_client_begin_send_message(rcc);
> + return FALSE;
> }
>
> static int snd_playback_send_volume(PlaybackChannelClient
> *playback_client)
> @@ -648,20 +432,21 @@ static int
> snd_playback_send_volume(PlaybackChannelClient *playback_client)
> static int snd_send_mute(SndChannelClient *client, uint32_t cap, int
> msg)
> {
> SpiceMsgAudioMute mute;
> - SpiceVolumeState *st = &client->channel->volume;
> - SpiceMarshaller *m = client->send_data.marshaller;
> + RedChannelClient *rcc = RED_CHANNEL_CLIENT(client);
> + SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
> + SndChannel *channel =
> SND_CHANNEL(red_channel_client_get_channel(rcc));
> + SpiceVolumeState *st = &channel->volume;
>
> - if (!red_channel_client_test_remote_cap(client->channel_client,
> cap)) {
> + if (!red_channel_client_test_remote_cap(rcc, cap)) {
> return TRUE;
> }
>
> - if (!snd_reset_send_data(client, msg)) {
> - return FALSE;
> - }
> + red_channel_client_init_send_data(rcc, msg);
> mute.mute = st->mute;
> spice_marshall_SpiceMsgAudioMute(m, &mute);
>
> - return snd_begin_send_message(client);
> + red_channel_client_begin_send_message(rcc);
> + return FALSE;
> }
>
> static int snd_playback_send_mute(PlaybackChannelClient
> *playback_client)
> @@ -672,48 +457,45 @@ static int
> snd_playback_send_mute(PlaybackChannelClient *playback_client)
>
> static int snd_playback_send_latency(PlaybackChannelClient
> *playback_client)
> {
> - SndChannelClient *client = SND_CHANNEL_CLIENT(playback_client);
> - SpiceMarshaller *m = client->send_data.marshaller;
> + RedChannelClient *rcc = RED_CHANNEL_CLIENT(playback_client);
> + SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
> SpiceMsgPlaybackLatency latency_msg;
>
> spice_debug("latency %u", playback_client->latency);
> - if (!snd_reset_send_data(client, SPICE_MSG_PLAYBACK_LATENCY)) {
> - return FALSE;
> - }
> + red_channel_client_init_send_data(rcc,
> SPICE_MSG_PLAYBACK_LATENCY);
> latency_msg.latency_ms = playback_client->latency;
> spice_marshall_msg_playback_latency(m, &latency_msg);
>
> - return snd_begin_send_message(client);
> + red_channel_client_begin_send_message(rcc);
> + return FALSE;
> }
> +
> static int snd_playback_send_start(PlaybackChannelClient
> *playback_client)
> {
> - SndChannelClient *client = (SndChannelClient *)playback_client;
> - SpiceMarshaller *m = client->send_data.marshaller;
> + RedChannelClient *rcc = RED_CHANNEL_CLIENT(playback_client);
> + SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
> SpiceMsgPlaybackStart start;
>
> - if (!snd_reset_send_data(client, SPICE_MSG_PLAYBACK_START)) {
> - return FALSE;
> - }
> -
> + red_channel_client_init_send_data(rcc,
> SPICE_MSG_PLAYBACK_START);
> start.channels = SPICE_INTERFACE_PLAYBACK_CHAN;
> - start.frequency = client->channel->frequency;
> + start.frequency =
> SND_CHANNEL(red_channel_client_get_channel(rcc))->frequency;
> spice_assert(SPICE_INTERFACE_PLAYBACK_FMT ==
> SPICE_INTERFACE_AUDIO_FMT_S16);
> start.format = SPICE_AUDIO_FMT_S16;
> start.time = reds_get_mm_time();
> spice_marshall_msg_playback_start(m, &start);
>
> - return snd_begin_send_message(client);
> + red_channel_client_begin_send_message(rcc);
> + return FALSE;
> }
>
> static int snd_playback_send_stop(PlaybackChannelClient
> *playback_client)
> {
> - SndChannelClient *client = (SndChannelClient *)playback_client;
> + RedChannelClient *rcc = RED_CHANNEL_CLIENT(playback_client);
>
> - if (!snd_reset_send_data(client, SPICE_MSG_PLAYBACK_STOP)) {
> - return FALSE;
> - }
> + red_channel_client_init_send_data(rcc, SPICE_MSG_PLAYBACK_STOP);
>
> - return snd_begin_send_message(client);
> + red_channel_client_begin_send_message(rcc);
> + return FALSE;
> }
>
> static int snd_playback_send_ctl(PlaybackChannelClient
> *playback_client)
> @@ -729,32 +511,30 @@ static int
> snd_playback_send_ctl(PlaybackChannelClient *playback_client)
>
> static int snd_record_send_start(RecordChannelClient *record_client)
> {
> - SndChannelClient *client = (SndChannelClient *)record_client;
> - SpiceMarshaller *m = client->send_data.marshaller;
> + RedChannelClient *rcc = RED_CHANNEL_CLIENT(record_client);
> + SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
> SpiceMsgRecordStart start;
>
> - if (!snd_reset_send_data(client, SPICE_MSG_RECORD_START)) {
> - return FALSE;
> - }
> + red_channel_client_init_send_data(rcc, SPICE_MSG_RECORD_START);
>
> start.channels = SPICE_INTERFACE_RECORD_CHAN;
> - start.frequency = client->channel->frequency;
> + start.frequency =
> SND_CHANNEL(red_channel_client_get_channel(rcc))->frequency;
> spice_assert(SPICE_INTERFACE_RECORD_FMT ==
> SPICE_INTERFACE_AUDIO_FMT_S16);
> start.format = SPICE_AUDIO_FMT_S16;
> spice_marshall_msg_record_start(m, &start);
>
> - return snd_begin_send_message(client);
> + red_channel_client_begin_send_message(rcc);
> + return FALSE;
> }
>
> static int snd_record_send_stop(RecordChannelClient *record_client)
> {
> - SndChannelClient *client = (SndChannelClient *)record_client;
> + RedChannelClient *rcc = RED_CHANNEL_CLIENT(record_client);
>
> - if (!snd_reset_send_data(client, SPICE_MSG_RECORD_STOP)) {
> - return FALSE;
> - }
> + red_channel_client_init_send_data(rcc, SPICE_MSG_RECORD_STOP);
>
> - return snd_begin_send_message(client);
> + red_channel_client_begin_send_message(rcc);
> + return FALSE;
> }
>
> static int snd_record_send_ctl(RecordChannelClient *record_client)
> @@ -791,14 +571,13 @@ static int
> snd_record_send_migrate(RecordChannelClient *record_client)
>
> static int snd_playback_send_write(PlaybackChannelClient
> *playback_client)
> {
> - SndChannelClient *client = (SndChannelClient *)playback_client;
> - SpiceMarshaller *m = client->send_data.marshaller;
> + RedChannelClient *rcc = RED_CHANNEL_CLIENT(playback_client);
> + SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
> AudioFrame *frame;
> SpiceMsgPlaybackPacket msg;
> + RedPipeItem *pipe_item = &SND_CHANNEL_CLIENT(playback_client)-
> >persistent_pipe_item;
>
> - if (!snd_reset_send_data(client, SPICE_MSG_PLAYBACK_DATA)) {
> - return FALSE;
> - }
> + red_channel_client_init_send_data(rcc, SPICE_MSG_PLAYBACK_DATA);
>
> frame = playback_client->in_progress;
> msg.time = frame->time;
> @@ -806,9 +585,10 @@ static int
> snd_playback_send_write(PlaybackChannelClient *playback_client)
> spice_marshall_msg_playback_data(m, &msg);
>
> if (playback_client->mode == SPICE_AUDIO_DATA_MODE_RAW) {
> - spice_marshaller_add_by_ref(m, (uint8_t *)frame->samples,
> - snd_codec_frame_size(playback_cl
> ient->codec) *
> - sizeof(frame->samples[0]));
> + spice_marshaller_add_by_ref_full(m, (uint8_t *)frame-
> >samples,
> + snd_codec_frame_size(playba
> ck_client->codec) *
> + sizeof(frame->samples[0]),
> + marshaller_unref_pipe_item,
> pipe_item);
> }
> else {
> int n = sizeof(playback_client->encode_buf);
> @@ -816,46 +596,71 @@ static int
> snd_playback_send_write(PlaybackChannelClient *playback_client)
> snd_codec_frame_size(playback_cl
> ient->codec) * sizeof(frame->samples[0]),
> playback_client->encode_buf, &n)
> != SND_CODEC_OK) {
> spice_printerr("encode failed");
> - snd_disconnect_channel(client);
> - return FALSE;
> + red_channel_client_disconnect(rcc);
> + return TRUE;
Hmm, I see here that switching success from TRUE to FALSE was
apparently intentional? Why?
> }
> - spice_marshaller_add_by_ref(m, playback_client->encode_buf,
> n);
> + spice_marshaller_add_by_ref_full(m, playback_client-
> >encode_buf, n,
> + marshaller_unref_pipe_item,
> pipe_item);
> }
>
> - return snd_begin_send_message(client);
> + red_channel_client_begin_send_message(rcc);
> + return FALSE;
> }
>
> static int playback_send_mode(PlaybackChannelClient
> *playback_client)
> {
> - SndChannelClient *client = (SndChannelClient *)playback_client;
> - SpiceMarshaller *m = client->send_data.marshaller;
> + RedChannelClient *rcc = RED_CHANNEL_CLIENT(playback_client);
> + SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
> SpiceMsgPlaybackMode mode;
>
> - if (!snd_reset_send_data(client, SPICE_MSG_PLAYBACK_MODE)) {
> - return FALSE;
> - }
> + red_channel_client_init_send_data(rcc, SPICE_MSG_PLAYBACK_MODE);
> mode.time = reds_get_mm_time();
> mode.mode = playback_client->mode;
> spice_marshall_msg_playback_mode(m, &mode);
>
> - return snd_begin_send_message(client);
> + red_channel_client_begin_send_message(rcc);
> + return FALSE;
> }
>
> -static void snd_playback_send(void* data)
> +static void snd_persistent_pipe_item_free(struct RedPipeItem *item)
> {
> - PlaybackChannelClient *playback_client =
> (PlaybackChannelClient*)data;
> - SndChannelClient *client = SND_CHANNEL_CLIENT(playback_client);
> + SndChannelClient *client = SPICE_CONTAINEROF(item,
> SndChannelClient, persistent_pipe_item);
> +
> + red_pipe_item_init_full(item, RED_PIPE_ITEM_PERSISTENT,
> + snd_persistent_pipe_item_free);
>
> - if (!playback_client || !snd_send_data(data)) {
> + if (client->on_message_done) {
> + client->on_message_done(client);
> + }
> +}
This is a pretty odd function and I think it deserves some explanation.
So, essentially, we're using the "freeing" of the pipe item to signal
that a message is done being sent. Then we (potentially) kick off
another message if there is an audio frame pending when
on_message_done() is called.
But the first thing that came to my mind was: is there any way to
simply tie into the existing channel callbacks (on_out_msg_done())
instead of using this 'fake' free function to signal that the message
is done? Maybe that would be even more 'odd'
> +
> +static void snd_send(SndChannelClient * client)
> +{
> + RedChannelClient *rcc = RED_CHANNEL_CLIENT(client);
> +
> + if (!client || !red_channel_client_pipe_is_empty(rcc) ||
> !client->command) {
> return;
> }
> + // just append a dummy item and push!
> + red_pipe_item_init_full(&client->persistent_pipe_item,
> RED_PIPE_ITEM_PERSISTENT,
> + snd_persistent_pipe_item_free);
> + red_channel_client_pipe_add_push(rcc, &client-
> >persistent_pipe_item);
> +}
>
> +static void playback_channel_send_item(RedChannelClient *rcc,
> RedPipeItem *base)
Why is the RedPipeItem argument named "base"? Seems something like
'item' would be more appropriate
> +{
> + PlaybackChannelClient *playback_client =
> PLAYBACK_CHANNEL_CLIENT(rcc);
> + SndChannelClient *client = SND_CHANNEL_CLIENT(rcc);
> +
> + client->command &= SND_PLAYBACK_MODE_MASK|SND_PLAYBACK_PCM_MASK|
> + SND_CTRL_MASK|SND_VOLUME_MUTE_MASK|
> + SND_MIGRATE_MASK|SND_PLAYBACK_LATENCY_MASK;
> while (client->command) {
> if (client->command & SND_PLAYBACK_MODE_MASK) {
> + client->command &= ~SND_PLAYBACK_MODE_MASK;
> if (!playback_send_mode(playback_client)) {
> - return;
> + break;
> }
> - client->command &= ~SND_PLAYBACK_MODE_MASK;
So, previously, if playback_send_mode() failed, we didn't clear the
MODE_MASK bit and presumably tried to send this message again next
time. Now we clear it regardless of success or failure. In addition,
when a send succeeded, it continued on to send the next command. In
this version, you break out of the loop and only send a single command.
Not sure that it matters much, but it is a behavior change.
> }
> if (client->command & SND_PLAYBACK_PCM_MASK) {
> spice_assert(!playback_client->in_progress &&
> playback_client->pending_frame);
> @@ -863,95 +668,94 @@ static void snd_playback_send(void* data)
> playback_client->pending_frame = NULL;
> client->command &= ~SND_PLAYBACK_PCM_MASK;
> if (!snd_playback_send_write(playback_client)) {
> - spice_printerr("snd_send_playback_write failed");
> - return;
> + break;
> }
> + spice_printerr("snd_send_playback_write failed");
> }
> if (client->command & SND_CTRL_MASK) {
> + client->command &= ~SND_CTRL_MASK;
> if (!snd_playback_send_ctl(playback_client)) {
> - return;
> + break;
> }
> - client->command &= ~SND_CTRL_MASK;
> }
> if (client->command & SND_VOLUME_MASK) {
> - if (!snd_playback_send_volume(playback_client) ||
> - !snd_playback_send_mute(playback_client)) {
> - return;
> - }
> client->command &= ~SND_VOLUME_MASK;
> + if (!snd_playback_send_volume(playback_client)) {
> + break;
> + }
> + }
> + if (client->command & SND_MUTE_MASK) {
> + client->command &= ~SND_MUTE_MASK;
> + if (!snd_playback_send_mute(playback_client)) {
> + break;
> + }
> }
> if (client->command & SND_MIGRATE_MASK) {
> + client->command &= ~SND_MIGRATE_MASK;
> if (!snd_playback_send_migrate(playback_client)) {
> - return;
> + break;
> }
> - client->command &= ~SND_MIGRATE_MASK;
> }
> if (client->command & SND_PLAYBACK_LATENCY_MASK) {
> + client->command &= ~SND_PLAYBACK_LATENCY_MASK;
> if (!snd_playback_send_latency(playback_client)) {
> - return;
> + break;
> }
> - client->command &= ~SND_PLAYBACK_LATENCY_MASK;
> }
> }
> + snd_send(client);
> }
>
> -static void snd_record_send(void* data)
> +static void record_channel_send_item(RedChannelClient *rcc,
> RedPipeItem *base)
same comments as above
> {
> - RecordChannelClient *record_client = (RecordChannelClient*)data;
> - SndChannelClient *client = SND_CHANNEL_CLIENT(record_client);
> -
> - if (!record_client || !snd_send_data(data)) {
> - return;
> - }
> + RecordChannelClient *record_client = RECORD_CHANNEL_CLIENT(rcc);
> + SndChannelClient *client = SND_CHANNEL_CLIENT(rcc);
>
> + client->command &=
> SND_CTRL_MASK|SND_VOLUME_MUTE_MASK|SND_MIGRATE_MASK;
> while (client->command) {
> if (client->command & SND_CTRL_MASK) {
> + client->command &= ~SND_CTRL_MASK;
> if (!snd_record_send_ctl(record_client)) {
> - return;
> + break;
> }
> - client->command &= ~SND_CTRL_MASK;
> }
> if (client->command & SND_VOLUME_MASK) {
> - if (!snd_record_send_volume(record_client) ||
> - !snd_record_send_mute(record_client)) {
> - return;
> - }
> client->command &= ~SND_VOLUME_MASK;
> + if (!snd_record_send_volume(record_client)) {
> + break;
> + }
> + }
> + if (client->command & SND_MUTE_MASK) {
> + client->command &= ~SND_MUTE_MASK;
> + if (!snd_record_send_mute(record_client)) {
> + break;
> + }
> }
> if (client->command & SND_MIGRATE_MASK) {
> + client->command &= ~SND_MIGRATE_MASK;
> if (!snd_record_send_migrate(record_client)) {
> - return;
> + break;
> }
> - client->command &= ~SND_MIGRATE_MASK;
> }
> }
> + snd_send(client);
> }
>
> -static SndChannelClient *__new_channel(SndChannel *channel, int
> size, uint32_t channel_id,
> - RedClient *red_client,
> - RedsStream *stream,
> - int migrate,
> - snd_channel_send_messages_pro
> c send_messages,
> - snd_channel_handle_message_pr
> oc handle_message,
> - snd_channel_on_message_done_p
> roc on_message_done,
> - snd_channel_cleanup_channel_p
> roc cleanup,
> - uint32_t *common_caps, int
> num_common_caps,
> - uint32_t *caps, int num_caps)
> +static int snd_channel_config_socket(RedChannelClient *rcc)
> {
> - SndChannelClient *client;
> int delay_val;
> int flags;
> #ifdef SO_PRIORITY
> int priority;
> #endif
> int tos;
> + RedsStream *stream = red_channel_client_get_stream(rcc);
> + RedClient *red_client = red_channel_client_get_client(rcc);
> MainChannelClient *mcc = red_client_get_main(red_client);
> - RedsState *reds = red_channel_get_server(RED_CHANNEL(channel));
>
> - spice_assert(stream);
> if ((flags = fcntl(stream->socket, F_GETFL)) == -1) {
> spice_printerr("accept failed, %s", strerror(errno));
> - goto error1;
> + return FALSE;
> }
>
> #ifdef SO_PRIORITY
> @@ -980,69 +784,39 @@ static SndChannelClient
> *__new_channel(SndChannel *channel, int size, uint32_t c
>
> if (fcntl(stream->socket, F_SETFL, flags | O_NONBLOCK) == -1) {
> spice_printerr("accept failed, %s", strerror(errno));
> - goto error1;
> - }
> -
> - spice_assert(size >= sizeof(*client));
> - client = spice_malloc0(size);
> - client->refs = 1;
> - client->parser = spice_get_client_channel_parser(channel_id,
> NULL);
> - client->stream = stream;
> - client->channel = channel;
> - client->receive_data.message_start = client->receive_data.buf;
> - client->receive_data.now = client->receive_data.buf;
> - client->receive_data.end = client->receive_data.buf +
> sizeof(client->receive_data.buf);
> - client->send_data.marshaller = spice_marshaller_new();
> -
> - stream->watch = reds_core_watch_add(reds, stream->socket,
> SPICE_WATCH_EVENT_READ,
> - snd_event, client);
> - if (stream->watch == NULL) {
> - spice_printerr("watch_add failed, %s", strerror(errno));
> - goto error2;
> - }
> -
> - client->send_messages = send_messages;
> - client->handle_message = handle_message;
> - client->on_message_done = on_message_done;
> - client->cleanup = cleanup;
> -
> - client->channel_client =
> - dummy_channel_client_create(RED_CHANNEL(channel),
> red_client,
> - num_common_caps, common_caps,
> num_caps, caps);
> - if (!client->channel_client) {
> - goto error2;
> + return FALSE;
> }
> - return client;
> -
> -error2:
> - free(client);
> -
> -error1:
> - reds_stream_free(stream);
> - return NULL;
> -}
>
> -static int snd_channel_config_socket(RedChannelClient *rcc)
> -{
> - g_assert_not_reached();
> + return TRUE;
> }
>
> static void snd_channel_on_disconnect(RedChannelClient *rcc)
> {
> - g_assert_not_reached();
> + SndChannel *channel =
> SND_CHANNEL(red_channel_client_get_channel(rcc));
> + if (channel->connection && rcc == RED_CHANNEL_CLIENT(channel-
> >connection)) {
> + channel->connection = NULL;
> + }
> }
>
> static uint8_t*
> snd_channel_client_alloc_recv_buf(RedChannelClient *rcc, uint16_t
> type, uint32_t size)
> {
> - g_assert_not_reached();
> + SndChannelClient *client = SND_CHANNEL_CLIENT(rcc);
> + // If message is to big allocate one, this should never happen
to -> too?
> + if (size > sizeof(client->receive_buf)) {
> + return spice_malloc(size);
> + }
> + return client->receive_buf;
> }
>
> static void
> snd_channel_client_release_recv_buf(RedChannelClient *rcc, uint16_t
> type, uint32_t size,
> uint8_t *msg)
> {
> - g_assert_not_reached();
> + SndChannelClient *client = SND_CHANNEL_CLIENT(rcc);
> + if (msg != client->receive_buf) {
> + free(msg);
> + }
> }
>
> static void snd_disconnect_channel_client(RedChannelClient *rcc)
> @@ -1057,8 +831,8 @@ static void
> snd_disconnect_channel_client(RedChannelClient *rcc)
>
> spice_debug("channel-type=%d", type);
> if (channel->connection) {
> - spice_assert(channel->connection->channel_client == rcc);
> - snd_disconnect_channel(channel->connection);
> + spice_assert(RED_CHANNEL_CLIENT(channel->connection) ==
> rcc);
> + red_channel_client_disconnect(rcc);
> }
> }
>
> @@ -1076,7 +850,6 @@ SPICE_GNUC_VISIBLE void
> spice_server_playback_set_volume(SpicePlaybackInstance *
> {
> SpiceVolumeState *st = &sin->st->channel.volume;
> SndChannelClient *client = sin->st->channel.connection;
> - PlaybackChannelClient *playback_client =
> SPICE_CONTAINEROF(client, PlaybackChannelClient, base);
>
> st->volume_nchannels = nchannels;
> free(st->volume);
> @@ -1085,21 +858,22 @@ SPICE_GNUC_VISIBLE void
> spice_server_playback_set_volume(SpicePlaybackInstance *
> if (!client || nchannels == 0)
> return;
>
> - snd_playback_send_volume(playback_client);
> + snd_set_command(client, SND_VOLUME_MUTE_MASK);
> + snd_send(client);
> }
>
> SPICE_GNUC_VISIBLE void
> spice_server_playback_set_mute(SpicePlaybackInstance *sin, uint8_t
> mute)
> {
> SpiceVolumeState *st = &sin->st->channel.volume;
> SndChannelClient *client = sin->st->channel.connection;
> - PlaybackChannelClient *playback_client =
> SPICE_CONTAINEROF(client, PlaybackChannelClient, base);
>
> st->mute = mute;
>
> if (!client)
> return;
>
> - snd_playback_send_mute(playback_client);
> + snd_set_command(client, SND_VOLUME_MUTE_MASK);
> + snd_send(client);
> }
>
> static void snd_playback_start(SndChannel *channel)
> @@ -1114,7 +888,7 @@ static void snd_playback_start(SndChannel
> *channel)
> client->active = TRUE;
> if (!client->client_active) {
> snd_set_command(client, SND_CTRL_MASK);
> - snd_playback_send(client);
> + snd_send(client);
> } else {
> client->command &= ~SND_CTRL_MASK;
> }
> @@ -1128,17 +902,17 @@ SPICE_GNUC_VISIBLE void
> spice_server_playback_start(SpicePlaybackInstance *sin)
> SPICE_GNUC_VISIBLE void
> spice_server_playback_stop(SpicePlaybackInstance *sin)
> {
> SndChannelClient *client = sin->st->channel.connection;
> - PlaybackChannelClient *playback_client =
> SPICE_CONTAINEROF(client, PlaybackChannelClient, base);
>
> sin->st->channel.active = 0;
> if (!client)
> return;
> + PlaybackChannelClient *playback_client =
> PLAYBACK_CHANNEL_CLIENT(client);
> spice_assert(client->active);
> reds_enable_mm_time(snd_channel_get_server(client));
> client->active = FALSE;
> if (client->client_active) {
> snd_set_command(client, SND_CTRL_MASK);
> - snd_playback_send(client);
> + snd_send(client);
> } else {
> client->command &= ~SND_CTRL_MASK;
> client->command &= ~SND_PLAYBACK_PCM_MASK;
> @@ -1156,11 +930,14 @@ SPICE_GNUC_VISIBLE void
> spice_server_playback_get_buffer(SpicePlaybackInstance *
> uint32_t
> **frame, uint32_t *num_samples)
> {
> SndChannelClient *client = sin->st->channel.connection;
> - PlaybackChannelClient *playback_client =
> SPICE_CONTAINEROF(client, PlaybackChannelClient, base);
>
> - if (!client || !playback_client->free_frames) {
> - *frame = NULL;
> - *num_samples = 0;
> + *frame = NULL;
> + *num_samples = 0;
> + if (!client) {
> + return;
> + }
> + PlaybackChannelClient *playback_client =
> PLAYBACK_CHANNEL_CLIENT(client);
> + if (!playback_client->free_frames) {
> return;
> }
> spice_assert(client->active);
> @@ -1201,7 +978,7 @@ SPICE_GNUC_VISIBLE void
> spice_server_playback_put_samples(SpicePlaybackInstance
> frame->time = reds_get_mm_time();
> playback_client->pending_frame = frame;
> snd_set_command(SND_CHANNEL_CLIENT(playback_client),
> SND_PLAYBACK_PCM_MASK);
> - snd_playback_send(SND_CHANNEL_CLIENT(playback_client));
> + snd_send(SND_CHANNEL_CLIENT(playback_client));
> }
>
> void snd_set_playback_latency(RedClient *client, uint32_t latency)
> @@ -1212,15 +989,15 @@ void snd_set_playback_latency(RedClient
> *client, uint32_t latency)
> uint32_t type;
> g_object_get(RED_CHANNEL(now), "channel-type", &type, NULL);
> if (type == SPICE_CHANNEL_PLAYBACK && now->connection &&
> - red_channel_client_get_client(now->connection-
> >channel_client) == client) {
> + red_channel_client_get_client(RED_CHANNEL_CLIENT(now-
> >connection)) == client) {
>
> - if (red_channel_client_test_remote_cap(now->connection-
> >channel_client,
> + if
> (red_channel_client_test_remote_cap(RED_CHANNEL_CLIENT(now-
> >connection),
> SPICE_PLAYBACK_CAP_LATENCY)) {
> PlaybackChannelClient* playback =
> (PlaybackChannelClient*)now->connection;
>
> playback->latency = latency;
> snd_set_command(now->connection,
> SND_PLAYBACK_LATENCY_MASK);
> - snd_playback_send(now->connection);
> + snd_send(now->connection);
> } else {
> spice_debug("client doesn't not support
> SPICE_PLAYBACK_CAP_LATENCY");
> }
> @@ -1255,17 +1032,19 @@ static void
> on_new_playback_channel(SndChannel *channel, SndChannelClient *snd_c
> snd_set_command(snd_channel, SND_CTRL_MASK);
> }
> if (channel->volume.volume_nchannels) {
> - snd_set_command(snd_channel, SND_VOLUME_MASK);
> + snd_set_command(snd_channel, SND_VOLUME_MUTE_MASK);
> }
> if (snd_channel->active) {
> reds_disable_mm_time(reds);
> }
> }
>
> -static void snd_playback_cleanup(SndChannelClient *client)
> +static void
> +playback_channel_client_finalize(GObject *object)
> {
> - PlaybackChannelClient *playback_client =
> SPICE_CONTAINEROF(client, PlaybackChannelClient, base);
> int i;
> + PlaybackChannelClient *playback_client =
> PLAYBACK_CHANNEL_CLIENT(object);
> + SndChannelClient *client = SND_CHANNEL_CLIENT(playback_client);
>
> // free frames, unref them
> for (i = 0; i < NUM_AUDIO_FRAMES; ++i) {
> @@ -1280,43 +1059,31 @@ static void
> snd_playback_cleanup(SndChannelClient *client)
> }
>
> snd_codec_destroy(&playback_client->codec);
> +
> + G_OBJECT_CLASS(playback_channel_client_parent_class)-
> >finalize(object);
> }
>
> -static void snd_set_playback_peer(RedChannel *red_channel, RedClient
> *client, RedsStream *stream,
> - int migration, int
> num_common_caps, uint32_t *common_caps,
> - int num_caps, uint32_t *caps)
> +static void
> +playback_channel_client_constructed(GObject *object)
> {
> + PlaybackChannelClient *playback_client =
> PLAYBACK_CHANNEL_CLIENT(object);
> + RedChannel *red_channel =
> red_channel_client_get_channel(RED_CHANNEL_CLIENT(playback_client));
> + RedClient *client =
> red_channel_client_get_client(RED_CHANNEL_CLIENT(playback_client));
> SndChannel *channel = SND_CHANNEL(red_channel);
> - PlaybackChannelClient *playback_client;
>
> - snd_disconnect_channel(channel->connection);
> -
> - if (!(playback_client = (PlaybackChannelClient
> *)__new_channel(channel,
> - s
> izeof(*playback_client),
> - S
> PICE_CHANNEL_PLAYBACK,
> - c
> lient,
> - s
> tream,
> - m
> igration,
> - s
> nd_playback_send,
> - s
> nd_playback_handle_message,
> - s
> nd_playback_on_message_done,
> - s
> nd_playback_cleanup,
> - c
> ommon_caps, num_common_caps,
> - c
> aps, num_caps))) {
> - return;
> - }
> + G_OBJECT_CLASS(playback_channel_client_parent_class)-
> >constructed(object);
>
> - snd_playback_alloc_frames(playback_client);
> + SND_CHANNEL_CLIENT(playback_client)->on_message_done =
> snd_playback_on_message_done;
>
> - int client_can_celt =
> red_channel_client_test_remote_cap(playback_client-
> >base.channel_client,
> + RedChannelClient *rcc = RED_CHANNEL_CLIENT(playback_client);
> + int client_can_celt = red_channel_client_test_remote_cap(rcc,
> SPICE_PLAYBACK_CAP_CELT_0_
> 5_1);
> - int client_can_opus =
> red_channel_client_test_remote_cap(playback_client-
> >base.channel_client,
> + int client_can_opus = red_channel_client_test_remote_cap(rcc,
> SPICE_PLAYBACK_CAP_OPUS);
> int playback_compression =
> reds_config_get_playback_compression(red_channel_get_server(
> red_channel));
> int desired_mode = snd_desired_audio_mode(playback_compression,
> channel->frequency,
> client_can_celt,
> client_can_opus);
> - playback_client->mode = SPICE_AUDIO_DATA_MODE_RAW;
> if (desired_mode != SPICE_AUDIO_DATA_MODE_RAW) {
> if (snd_codec_create(&playback_client->codec, desired_mode,
> channel->frequency,
> SND_CODEC_ENCODE) == SND_CODEC_OK) {
> @@ -1333,7 +1100,46 @@ static void snd_set_playback_peer(RedChannel
> *red_channel, RedClient *client, Re
> if (channel->active) {
> snd_playback_start(channel);
> }
> - snd_playback_send(channel->connection);
> + snd_send(SND_CHANNEL_CLIENT(playback_client));
> +}
> +
> +static void snd_set_playback_peer(RedChannel *red_channel, RedClient
> *client, RedsStream *stream,
> + int migration, int
> num_common_caps, uint32_t *common_caps,
> + int num_caps, uint32_t *caps)
> +{
> + SndChannel *channel = SND_CHANNEL(red_channel);
> + GArray *common_caps_array = NULL, *caps_array = NULL;
> +
> + if (channel->connection) {
> + red_channel_client_disconnect(RED_CHANNEL_CLIENT(channel-
> >connection));
> + channel->connection = NULL;
> + }
> +
> + if (common_caps) {
> + common_caps_array = g_array_sized_new(FALSE, FALSE, sizeof
> (*common_caps),
> + num_common_caps);
> + g_array_append_vals(common_caps_array, common_caps,
> num_common_caps);
> + }
> + if (caps) {
> + caps_array = g_array_sized_new(FALSE, FALSE, sizeof (*caps),
> num_caps);
> + g_array_append_vals(caps_array, caps, num_caps);
> + }
> +
> + g_initable_new(TYPE_PLAYBACK_CHANNEL_CLIENT,
> + NULL, NULL,
> + "channel", channel,
> + "client", client,
> + "stream", stream,
> + "caps", caps_array,
> + "common-caps", common_caps_array,
> + NULL);
> +
> + if (caps_array) {
> + g_array_unref(caps_array);
> + }
> + if (common_caps_array) {
> + g_array_unref(common_caps_array);
> + }
> }
>
> static void snd_record_migrate_channel_client(RedChannelClient *rcc)
> @@ -1345,9 +1151,9 @@ static void
> snd_record_migrate_channel_client(RedChannelClient *rcc)
> spice_assert(channel);
>
> if (channel->connection) {
> - spice_assert(channel->connection->channel_client == rcc);
> + spice_assert(RED_CHANNEL_CLIENT(channel->connection) ==
> rcc);
> snd_set_command(channel->connection, SND_MIGRATE_MASK);
> - snd_record_send(channel->connection);
> + snd_send(channel->connection);
> }
> }
>
> @@ -1357,7 +1163,6 @@ SPICE_GNUC_VISIBLE void
> spice_server_record_set_volume(SpiceRecordInstance *sin,
> {
> SpiceVolumeState *st = &sin->st->channel.volume;
> SndChannelClient *client = sin->st->channel.connection;
> - RecordChannelClient *record_client = SPICE_CONTAINEROF(client,
> RecordChannelClient, base);
>
> st->volume_nchannels = nchannels;
> free(st->volume);
> @@ -1366,38 +1171,40 @@ SPICE_GNUC_VISIBLE void
> spice_server_record_set_volume(SpiceRecordInstance *sin,
> if (!client || nchannels == 0)
> return;
>
> - snd_record_send_volume(record_client);
> + snd_set_command(client, SND_VOLUME_MUTE_MASK);
> + snd_send(client);
> }
>
> SPICE_GNUC_VISIBLE void
> spice_server_record_set_mute(SpiceRecordInstance *sin, uint8_t mute)
> {
> SpiceVolumeState *st = &sin->st->channel.volume;
> SndChannelClient *client = sin->st->channel.connection;
> - RecordChannelClient *record_client = SPICE_CONTAINEROF(client,
> RecordChannelClient, base);
>
> st->mute = mute;
>
> if (!client)
> return;
>
> - snd_record_send_mute(record_client);
> + snd_set_command(client, SND_VOLUME_MUTE_MASK);
> + snd_send(client);
> }
>
> static void snd_record_start(SndChannel *channel)
> {
> SndChannelClient *client = channel->connection;
> - RecordChannelClient *record_client = SPICE_CONTAINEROF(client,
> RecordChannelClient, base);
>
> channel->active = 1;
> - if (!client)
> + if (!client) {
> return;
> + }
> + RecordChannelClient *record_client =
> RECORD_CHANNEL_CLIENT(client);
> spice_assert(!client->active);
> record_client->read_pos = record_client->write_pos =
> 0; //todo: improve by
> //stre
> am generation
> client->active = TRUE;
> if (!client->client_active) {
> snd_set_command(client, SND_CTRL_MASK);
> - snd_record_send(client);
> + snd_send(client);
> } else {
> client->command &= ~SND_CTRL_MASK;
> }
> @@ -1419,7 +1226,7 @@ SPICE_GNUC_VISIBLE void
> spice_server_record_stop(SpiceRecordInstance *sin)
> client->active = FALSE;
> if (client->client_active) {
> snd_set_command(client, SND_CTRL_MASK);
> - snd_record_send(client);
> + snd_send(client);
> } else {
> client->command &= ~SND_CTRL_MASK;
> }
> @@ -1429,13 +1236,13 @@ SPICE_GNUC_VISIBLE uint32_t
> spice_server_record_get_samples(SpiceRecordInstance
> uint32_t
> *samples, uint32_t bufsize)
> {
> SndChannelClient *client = sin->st->channel.connection;
> - RecordChannelClient *record_client = SPICE_CONTAINEROF(client,
> RecordChannelClient, base);
> uint32_t read_pos;
> uint32_t now;
> uint32_t len;
>
> if (!client)
> return 0;
> + RecordChannelClient *record_client =
> RECORD_CHANNEL_CLIENT(client);
> spice_assert(client->active);
>
> if (record_client->write_pos < RECORD_SAMPLES_SIZE / 2) {
> @@ -1444,14 +1251,17 @@ SPICE_GNUC_VISIBLE uint32_t
> spice_server_record_get_samples(SpiceRecordInstance
>
> len = MIN(record_client->write_pos - record_client->read_pos,
> bufsize);
>
> + // TODO why ?? stream already call if got data
> +#if 0
???
> if (len < bufsize) {
> - SndChannel *channel = client->channel;
> + SndChannel *channel =
> SND_CHANNEL(red_channel_client_get_channel(RED_CHANNEL_CLIENT(client)
> ));
> snd_receive(client);
> if (!channel->connection) {
> return 0;
> }
> len = MIN(record_client->write_pos - record_client-
> >read_pos, bufsize);
> }
> +#endif
>
> read_pos = record_client->read_pos % RECORD_SAMPLES_SIZE;
> record_client->read_pos += len;
> @@ -1467,7 +1277,7 @@ static uint32_t
> snd_get_best_rate(SndChannelClient *client, uint32_t cap_opus)
> {
> int client_can_opus = TRUE;
> if (client) {
> - client_can_opus = red_channel_client_test_remote_cap(client-
> >channel_client, cap_opus);
> + client_can_opus =
> red_channel_client_test_remote_cap(RED_CHANNEL_CLIENT(client),
> cap_opus);
> }
>
> if (client_can_opus &&
> snd_codec_is_capable(SPICE_AUDIO_DATA_MODE_OPUS,
> SND_CODEC_ANY_FREQUENCY))
> @@ -1509,52 +1319,79 @@ static void on_new_record_channel(SndChannel
> *channel, SndChannelClient *snd_cha
> {
> spice_assert(snd_channel);
>
> - channel->connection = snd_channel ;
> + channel->connection = snd_channel;
> if (channel->volume.volume_nchannels) {
> - snd_set_command(snd_channel, SND_VOLUME_MASK);
> + snd_set_command(snd_channel, SND_VOLUME_MUTE_MASK);
> }
> if (snd_channel->active) {
> snd_set_command(snd_channel, SND_CTRL_MASK);
> }
> }
>
> -static void snd_record_cleanup(SndChannelClient *client)
> +static void
> +record_channel_client_finalize(GObject *object)
> {
> - RecordChannelClient *record_client = SPICE_CONTAINEROF(client,
> RecordChannelClient, base);
> + RecordChannelClient *record_client =
> RECORD_CHANNEL_CLIENT(object);
> +
> snd_codec_destroy(&record_client->codec);
> +
> + G_OBJECT_CLASS(record_channel_client_parent_class)-
> >finalize(object);
> +}
> +
> +static void
> +record_channel_client_constructed(GObject *object)
> +{
> + RecordChannelClient *record_client =
> RECORD_CHANNEL_CLIENT(object);
> + RedChannel *red_channel =
> red_channel_client_get_channel(RED_CHANNEL_CLIENT(record_client));
> + SndChannel *channel = SND_CHANNEL(red_channel);
> +
> + G_OBJECT_CLASS(record_channel_client_parent_class)-
> >constructed(object);
> +
> + on_new_record_channel(channel,
> SND_CHANNEL_CLIENT(record_client));
> + if (channel->active) {
> + snd_record_start(channel);
> + }
> + snd_send(SND_CHANNEL_CLIENT(record_client));
> }
>
> +
> static void snd_set_record_peer(RedChannel *red_channel, RedClient
> *client, RedsStream *stream,
> int migration, int num_common_caps,
> uint32_t *common_caps,
> int num_caps, uint32_t *caps)
> {
> SndChannel *channel = SND_CHANNEL(red_channel);
> - RecordChannelClient *record_client;
> -
> - snd_disconnect_channel(channel->connection);
> -
> - if (!(record_client = (RecordChannelClient
> *)__new_channel(channel,
> - sizeo
> f(*record_client),
> - SPICE
> _CHANNEL_RECORD,
> - clien
> t,
> - strea
> m,
> - migra
> tion,
> - snd_r
> ecord_send,
> - snd_r
> ecord_handle_message,
> - snd_r
> ecord_on_message_done,
> - snd_r
> ecord_cleanup,
> - commo
> n_caps, num_common_caps,
> - caps,
> num_caps))) {
> - return;
> + GArray *common_caps_array = NULL, *caps_array = NULL;
> +
> + if (channel->connection) {
> + red_channel_client_disconnect(RED_CHANNEL_CLIENT(channel-
> >connection));
> + channel->connection = NULL;
> }
>
> - record_client->mode = SPICE_AUDIO_DATA_MODE_RAW;
> + if (common_caps) {
> + common_caps_array = g_array_sized_new(FALSE, FALSE, sizeof
> (*common_caps),
> + num_common_caps);
> + g_array_append_vals(common_caps_array, common_caps,
> num_common_caps);
> + }
> + if (caps) {
> + caps_array = g_array_sized_new(FALSE, FALSE, sizeof (*caps),
> num_caps);
> + g_array_append_vals(caps_array, caps, num_caps);
> + }
>
> - on_new_record_channel(channel,
> SND_CHANNEL_CLIENT(record_client));
> - if (channel->active) {
> - snd_record_start(channel);
> + g_initable_new(TYPE_RECORD_CHANNEL_CLIENT,
> + NULL, NULL,
> + "channel", channel,
> + "client", client,
> + "stream", stream,
> + "caps", caps_array,
> + "common-caps", common_caps_array,
> + NULL);
> +
> + if (caps_array) {
> + g_array_unref(caps_array);
> + }
> + if (common_caps_array) {
> + g_array_unref(common_caps_array);
> }
> - snd_record_send(channel->connection);
> }
>
> static void snd_playback_migrate_channel_client(RedChannelClient
> *rcc)
> @@ -1567,9 +1404,9 @@ static void
> snd_playback_migrate_channel_client(RedChannelClient *rcc)
> spice_debug(NULL);
>
> if (channel->connection) {
> - spice_assert(channel->connection->channel_client == rcc);
> + spice_assert(RED_CHANNEL_CLIENT(channel->connection) ==
> rcc);
> snd_set_command(channel->connection, SND_MIGRATE_MASK);
> - snd_playback_send(channel->connection);
> + snd_send(channel->connection);
> }
> }
>
> @@ -1641,8 +1478,13 @@ static void
> playback_channel_class_init(PlaybackChannelClass *klass)
> {
> GObjectClass *object_class = G_OBJECT_CLASS(klass);
> + RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);
>
> object_class->constructed = playback_channel_constructed;
> +
> + channel_class->parser =
> spice_get_client_channel_parser(SPICE_CHANNEL_PLAYBACK, NULL);
> + channel_class->handle_parsed = playback_channel_handle_parsed;
> + channel_class->send_item = playback_channel_send_item;
> }
>
> void snd_attach_playback(RedsState *reds, SpicePlaybackInstance
> *sin)
> @@ -1687,8 +1529,13 @@ static void
> record_channel_class_init(RecordChannelClass *klass)
> {
> GObjectClass *object_class = G_OBJECT_CLASS(klass);
> + RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);
>
> object_class->constructed = record_channel_constructed;
> +
> + channel_class->parser =
> spice_get_client_channel_parser(SPICE_CHANNEL_RECORD, NULL);
> + channel_class->handle_parsed = record_channel_handle_parsed;
> + channel_class->send_item = record_channel_send_item;
> }
>
> void snd_attach_record(RedsState *reds, SpiceRecordInstance *sin)
> @@ -1709,7 +1556,6 @@ static void snd_detach_common(SndChannel
> *channel)
> RedsState *reds = red_channel_get_server(RED_CHANNEL(channel));
>
> remove_channel(channel);
> - snd_disconnect_channel(channel->connection);
> reds_unregister_channel(reds, RED_CHANNEL(channel));
> free(channel->volume.volume);
> channel->volume.volume = NULL;
> @@ -1735,9 +1581,10 @@ void snd_set_playback_compression(int on)
> g_object_get(RED_CHANNEL(now), "channel-type", &type, NULL);
> if (type == SPICE_CHANNEL_PLAYBACK && now->connection) {
> PlaybackChannelClient* playback =
> (PlaybackChannelClient*)now->connection;
> - int client_can_celt =
> red_channel_client_test_remote_cap(playback->base.channel_client,
> + RedChannelClient *rcc = RED_CHANNEL_CLIENT(playback);
> + int client_can_celt =
> red_channel_client_test_remote_cap(rcc,
> SPICE_PLAYBACK_CAP_CELT_0_5_1);
> - int client_can_opus =
> red_channel_client_test_remote_cap(playback->base.channel_client,
> + int client_can_opus =
> red_channel_client_test_remote_cap(rcc,
> SPICE_PLAYBACK_CAP_OPUS);
> int desired_mode = snd_desired_audio_mode(on, now-
> >frequency,
> client_can_opu
> s, client_can_celt);
> @@ -1749,10 +1596,31 @@ void snd_set_playback_compression(int on)
> }
> }
>
> -static void snd_playback_alloc_frames(PlaybackChannelClient
> *playback)
> +static void
> +snd_channel_client_class_init(SndChannelClientClass *self)
> +{
> +}
> +
> +static void
> +snd_channel_client_init(SndChannelClient *self)
> +{
> +}
> +
> +static void
> +playback_channel_client_class_init(PlaybackChannelClientClass
> *klass)
> +{
> + GObjectClass *object_class = G_OBJECT_CLASS(klass);
> + object_class->constructed = playback_channel_client_constructed;
> + object_class->finalize = playback_channel_client_finalize;
> +}
> +
> +static void
> +playback_channel_client_init(PlaybackChannelClient *playback)
> {
> int i;
>
> + playback->mode = SPICE_AUDIO_DATA_MODE_RAW;
> +
> playback->frames = spice_new0(AudioFrameContainer, 1);
> playback->frames->refs = 1;
> for (i = 0; i < NUM_AUDIO_FRAMES; ++i) {
> @@ -1760,3 +1628,17 @@ static void
> snd_playback_alloc_frames(PlaybackChannelClient *playback)
> snd_playback_free_frame(playback, &playback->frames-
> >items[i]);
> }
> }
> +
> +static void
> +record_channel_client_class_init(RecordChannelClientClass *klass)
> +{
> + GObjectClass *object_class = G_OBJECT_CLASS(klass);
> + object_class->constructed = record_channel_client_constructed;
> + object_class->finalize = record_channel_client_finalize;
> +}
> +
> +static void
> +record_channel_client_init(RecordChannelClient *record)
> +{
> + record->mode = SPICE_AUDIO_DATA_MODE_RAW;
> +}
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel