[Spice-devel] [PATCH v2 6/6] sound: don't store client in SndChannel
Jonathon Jongsma
jjongsma at redhat.com
Fri Apr 28 20:39:15 UTC 2017
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 at redhat.com>
> > > > ---
> > > > 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
More information about the Spice-devel
mailing list