On Fri, 2017-04-28 at 05:32 -0400, Frediano Ziglio wrote: > > > > On Wed, 2017-04-26 at 06:33 -0400, Frediano Ziglio wrote: > > > > > > > > The base RedChannel already keeps a list of channel clients, so > > > > there's > > > > no need for the SndChannel to also keep track of the client > > > > itself. > > > > > > > > Since the SndChannel only supports a single client (whereas > > > > other > > > > channels may have some partial support for multiple clients), > > > > I've > > > > provided a convenience function for getting the client and > > > > warning > > > > if > > > > there is more than one. > > > > > > > > Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx> > > > > --- > > > > server/sound.c | 74 > > > > +++++++++++++++++++++++++++++++++------------------------- > > > > 1 file changed, 42 insertions(+), 32 deletions(-) > > > > > > > > diff --git a/server/sound.c b/server/sound.c > > > > index 543449a..ca83eae 100644 > > > > --- a/server/sound.c > > > > +++ b/server/sound.c > > > > @@ -166,8 +166,6 @@ GType snd_channel_get_type(void) > > > > G_GNUC_CONST; > > > > struct SndChannel { > > > > RedChannel parent; > > > > > > > > - SndChannelClient *connection; /* Only one client is > > > > supported > > > > */ > > > > - > > > > gboolean active; > > > > SpiceVolumeState volume; > > > > uint32_t frequency; > > > > @@ -240,6 +238,20 @@ static GList *snd_channels; > > > > > > > > static void snd_send(SndChannelClient * client); > > > > > > > > +static SndChannelClient *snd_channel_get_client(SndChannel > > > > *channel) > > > > +{ > > > > + /* sound channels only support a single client */ > > > > + GList *clients = > > > > red_channel_get_clients(RED_CHANNEL(channel)); > > > > + if (clients == NULL) > > > > + return NULL; > > > > + > > > > > > brackets > > > > > > > + if (clients->next != NULL) { > > > > + g_warning("Only one sound client is supported"); > > > > + } > > > > + > > > > > > why we need to enforce this? I think for play should be easy to > > > think > > > about multiple clients, is currently just an implementation > > > limit, > > > and > > > this looks like a regression to me. > > > For recording I have no idea, I see 2 options for multiple > > > clients: > > > - only one can send data; > > > - mix together inputs. > > > > > > I'm not sure whether we need to enforce this or not. I was simply > > trying to maintain the same behavior in this patch series. Perhaps > > in a > > follow-up patch series we can get rid of this requirement, but I > > haven't put much thought into what it would require yet. > > > > The g_warning is a regression. Would you ack the patch without the g_warning here? > > > > > > > > + return clients->data; > > > > +} > > > > + > > > > static RedsState* snd_channel_get_server(SndChannelClient > > > > *client) > > > > { > > > > g_return_val_if_fail(client != NULL, NULL); > > > > @@ -782,10 +794,6 @@ static bool > > > > snd_channel_client_config_socket(RedChannelClient *rcc) > > > > > > > > static void snd_channel_on_disconnect(RedChannelClient *rcc) > > > > { > > > > - 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* > > > > @@ -821,7 +829,7 @@ static void > > > > snd_channel_set_volume(SndChannel > > > > *channel, > > > > uint8_t nchannels, uint16_t > > > > *volume) > > > > { > > > > SpiceVolumeState *st = &channel->volume; > > > > - SndChannelClient *client = channel->connection; > > > > + SndChannelClient *client = > > > > snd_channel_get_client(channel); > > > > > > > > st->volume_nchannels = nchannels; > > > > free(st->volume); > > > > @@ -844,7 +852,7 @@ SPICE_GNUC_VISIBLE void > > > > spice_server_playback_set_volume(SpicePlaybackInstance * > > > > static void snd_channel_set_mute(SndChannel *channel, uint8_t > > > > mute) > > > > { > > > > SpiceVolumeState *st = &channel->volume; > > > > - SndChannelClient *client = channel->connection; > > > > + SndChannelClient *client = > > > > snd_channel_get_client(channel); > > > > > > > > st->mute = mute; > > > > > > > > @@ -879,12 +887,12 @@ SPICE_GNUC_VISIBLE void > > > > spice_server_playback_start(SpicePlaybackInstance *sin) > > > > { > > > > SndChannel *channel = &sin->st->channel; > > > > channel->active = TRUE; > > > > - return snd_playback_start(channel->connection); > > > > + return > > > > snd_playback_start(snd_channel_get_client(channel)); > > > > } > > > > > > > > SPICE_GNUC_VISIBLE void > > > > spice_server_playback_stop(SpicePlaybackInstance > > > > *sin) > > > > { > > > > - SndChannelClient *client = sin->st->channel.connection; > > > > + SndChannelClient *client = snd_channel_get_client(&sin- > > > > >st- > > > > > channel); > > > > > > > > > > > > sin->st->channel.active = FALSE; > > > > if (!client) > > > > @@ -912,7 +920,7 @@ SPICE_GNUC_VISIBLE void > > > > spice_server_playback_stop(SpicePlaybackInstance *sin) > > > > SPICE_GNUC_VISIBLE void > > > > spice_server_playback_get_buffer(SpicePlaybackInstance *sin, > > > > uint3 > > > > 2_t > > > > **frame, > > > > uint3 > > > > 2_t > > > > *num_ > > > > samp > > > > les) > > > > { > > > > - SndChannelClient *client = sin->st->channel.connection; > > > > + SndChannelClient *client = snd_channel_get_client(&sin- > > > > >st- > > > > > channel); > > > > > > > > > > > > *frame = NULL; > > > > *num_samples = 0; > > > > @@ -948,7 +956,7 @@ SPICE_GNUC_VISIBLE void > > > > spice_server_playback_put_samples(SpicePlaybackInstance > > > > } > > > > } > > > > playback_client = frame->client; > > > > - if (!playback_client || sin->st->channel.connection != > > > > SND_CHANNEL_CLIENT(playback_client)) { > > > > + if (!playback_client || snd_channel_get_client(&sin->st- > > > > > channel) != > > > > > > > > SND_CHANNEL_CLIENT(playback_client)) { > > > > /* lost last reference, client has been destroyed > > > > previously */ > > > > spice_debug("audio samples belong to a disconnected > > > > client"); > > > > return; > > > > @@ -970,18 +978,19 @@ void snd_set_playback_latency(RedClient > > > > *client, > > > > uint32_t latency) > > > > > > > > for (l = snd_channels; l != NULL; l = l->next) { > > > > SndChannel *now = l->data; > > > > + SndChannelClient *scc = snd_channel_get_client(now); > > > > 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(RED_CHANNEL_CLIENT(now- > > > > >connection)) > > > > == client) { > > > > + if (type == SPICE_CHANNEL_PLAYBACK && scc && > > > > + red_channel_client_get_client(RED_CHANNEL_CLIENT(s > > > > cc)) > > > > == > > > > client) { > > > > > > > > - if > > > > (red_channel_client_test_remote_cap(RED_CHANNEL_CLIENT(now- > > > > > connection), > > > > > > > > + if > > > > (red_channel_client_test_remote_cap(RED_CHANNEL_CLIENT(scc), > > > > SPICE_PLAYBACK_CAP_LATENCY)) { > > > > - PlaybackChannelClient* playback = > > > > (PlaybackChannelClient*)now->connection; > > > > + PlaybackChannelClient* playback = > > > > (PlaybackChannelClient*)scc; > > > > > > > > playback->latency = latency; > > > > - snd_set_command(now->connection, > > > > SND_PLAYBACK_LATENCY_MASK); > > > > - snd_send(now->connection); > > > > + snd_set_command(scc, > > > > SND_PLAYBACK_LATENCY_MASK); > > > > + snd_send(scc); > > > > } else { > > > > spice_debug("client doesn't not support > > > > SPICE_PLAYBACK_CAP_LATENCY"); > > > > } > > > > @@ -1062,7 +1071,6 @@ > > > > playback_channel_client_constructed(GObject > > > > *object) > > > > spice_debug("playback client %p using mode %s", > > > > playback_client, > > > > spice_audio_data_mode_to_string(playback_clien > > > > t- > > > > > mode)); > > > > > > > > > > > > - channel->connection = scc; > > > > > > When is now this value initialized? I suppose after the > > > constructor. > > > Does this cause no regressions? > > > > You're correct, it does get initialized after the constructor. It > > happens within red_channel_client_initable_init(). > > > > This change does have the potential to introduce regressions. But I > > did > > consider that when writing this patch series. That is actually the > > reason for patch 5/6. Before patch 5, the _constructed() function > > called snd_playback_start(channel) and inside the > > snd_playback_start() > > function, it tried to retrieve the client from the channel in order > > to > > perform some operations on the client. But after patch 5, we simply > > pass the client as an argument to snd_playback_start(). So that > > function no longer relies on the fact that the client is already > > registered with the channel. The same scenario applies to > > snd_record_start(). > > > > I couldn't find any other cases where the code requires the client > > to > > be registered with the channel before the client is initialized. > > But if > > you see anything, please let me know. > > > > You are right, I had a quick view at the set, I was confused by > the fact that I would have expected some function name changes. > > > > > > > > if (!red_client_during_migrate_at_target(red_client)) { > > > > snd_set_command(scc, SND_PLAYBACK_MODE_MASK); > > > > if (channel->volume.volume_nchannels) { > > > > @@ -1080,11 +1088,11 @@ static void snd_set_peer(RedChannel > > > > *red_channel, > > > > RedClient *client, RedsStream > > > > RedChannelCapabilities *caps, GType > > > > type) > > > > { > > > > SndChannel *channel = SND_CHANNEL(red_channel); > > > > - SndChannelClient *snd_client; > > > > + SndChannelClient *snd_client = > > > > snd_channel_get_client(channel);; > > > > > > > > - if (channel->connection) { > > > > - > > > > red_channel_client_disconnect(RED_CHANNEL_CLIENT(channel- > > > > > connection)); > > > > > > > > - channel->connection = NULL; > > > > + /* sound channels currently only support a single client > > > > */ > > > > + if (snd_client) { > > > > + red_channel_client_disconnect(RED_CHANNEL_CLIENT(snd_c > > > > lien > > > > t)); > > > > } > > > > > > > > snd_client = g_initable_new(type, > > > > @@ -1145,12 +1153,12 @@ SPICE_GNUC_VISIBLE void > > > > spice_server_record_start(SpiceRecordInstance *sin) > > > > { > > > > SndChannel *channel = &sin->st->channel; > > > > channel->active = TRUE; > > > > - snd_record_start(channel->connection); > > > > + snd_record_start(snd_channel_get_client(channel)); > > > > } > > > > > > > > SPICE_GNUC_VISIBLE void > > > > spice_server_record_stop(SpiceRecordInstance *sin) > > > > { > > > > - SndChannelClient *client = sin->st->channel.connection; > > > > + SndChannelClient *client = snd_channel_get_client(&sin- > > > > >st- > > > > > channel); > > > > > > > > > > > > sin->st->channel.active = FALSE; > > > > if (!client) > > > > @@ -1168,7 +1176,7 @@ SPICE_GNUC_VISIBLE void > > > > spice_server_record_stop(SpiceRecordInstance *sin) > > > > SPICE_GNUC_VISIBLE uint32_t > > > > spice_server_record_get_samples(SpiceRecordInstance *sin, > > > > ui > > > > nt32 > > > > _t > > > > *s > > > > ampl > > > > es, > > > > ui > > > > nt32 > > > > _t > > > > bu > > > > fsiz > > > > e) > > > > { > > > > - SndChannelClient *client = sin->st->channel.connection; > > > > + SndChannelClient *client = snd_channel_get_client(&sin- > > > > >st- > > > > > channel); > > > > > > > > uint32_t read_pos; > > > > uint32_t now; > > > > uint32_t len; > > > > @@ -1218,7 +1226,8 @@ static void snd_set_rate(SndChannel > > > > *channel, > > > > uint32_t > > > > frequency, uint32_t cap_o > > > > > > > > SPICE_GNUC_VISIBLE uint32_t > > > > spice_server_get_best_playback_rate(SpicePlaybackInstance > > > > *sin) > > > > { > > > > - return snd_get_best_rate(sin ? sin->st->channel.connection > > > > : > > > > NULL, > > > > SPICE_PLAYBACK_CAP_OPUS); > > > > + SndChannelClient *client = sin ? > > > > snd_channel_get_client(&sin->st->channel) : NULL; > > > > + return snd_get_best_rate(client, SPICE_PLAYBACK_CAP_OPUS); > > > > } > > > > > > > > SPICE_GNUC_VISIBLE void > > > > spice_server_set_playback_rate(SpicePlaybackInstance > > > > *sin, uint32_t frequency) > > > > @@ -1228,7 +1237,8 @@ SPICE_GNUC_VISIBLE void > > > > spice_server_set_playback_rate(SpicePlaybackInstance *si > > > > > > > > SPICE_GNUC_VISIBLE uint32_t > > > > spice_server_get_best_record_rate(SpiceRecordInstance *sin) > > > > { > > > > - return snd_get_best_rate(sin ? sin->st->channel.connection > > > > : > > > > NULL, > > > > SPICE_RECORD_CAP_OPUS); > > > > + SndChannelClient *client = sin ? > > > > snd_channel_get_client(&sin->st->channel) : NULL; > > > > + return snd_get_best_rate(client, SPICE_RECORD_CAP_OPUS); > > > > } > > > > > > > > SPICE_GNUC_VISIBLE void > > > > spice_server_set_record_rate(SpiceRecordInstance > > > > *sin, uint32_t frequency) > > > > @@ -1256,7 +1266,6 @@ record_channel_client_constructed(GObject > > > > *object) > > > > > > > > G_OBJECT_CLASS(record_channel_client_parent_class)- > > > > > constructed(object); > > > > > > > > > > > > - channel->connection = scc; > > > > > > same here, can cause a regression > > > > > > > if (channel->volume.volume_nchannels) { > > > > snd_set_command(scc, SND_VOLUME_MUTE_MASK); > > > > } > > > > @@ -1443,10 +1452,11 @@ void snd_set_playback_compression(bool > > > > on) > > > > > > > > for (l = snd_channels; l != NULL; l = l->next) { > > > > SndChannel *now = l->data; > > > > + SndChannelClient *client = > > > > snd_channel_get_client(now); > > > > uint32_t type; > > > > g_object_get(RED_CHANNEL(now), "channel-type", &type, > > > > NULL); > > > > - if (type == SPICE_CHANNEL_PLAYBACK && now->connection) > > > > { > > > > - PlaybackChannelClient* playback = > > > > (PlaybackChannelClient*)now->connection; > > > > + if (type == SPICE_CHANNEL_PLAYBACK && client) { > > > > + PlaybackChannelClient* playback = > > > > PLAYBACK_CHANNEL_CLIENT(client); > > > > RedChannelClient *rcc = > > > > RED_CHANNEL_CLIENT(playback); > > > > bool client_can_celt = > > > > red_channel_client_test_remote_cap(rcc, > > > > SPICE_PLAYBACK_CAP_CELT_0_ > > > > 5_1) > > > > ; > > > > @@ -1456,7 +1466,7 @@ void snd_set_playback_compression(bool > > > > on) > > > > client_c > > > > an_o > > > > pus, > > > > client_c > > > > an_c > > > > elt); > > > > if (playback->mode != desired_mode) { > > > > playback->mode = desired_mode; > > > > - snd_set_command(now->connection, > > > > SND_PLAYBACK_MODE_MASK); > > > > + snd_set_command(client, > > > > SND_PLAYBACK_MODE_MASK); > > > > spice_debug("playback client %p using mode > > > > %s", > > > > playback, > > > > spice_audio_data_mode_to_string(pl > > > > ayba > > > > ck->mode)); > > > > } > > Frediano _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel