[Spice-devel] [PATCH v2 6/6] sound: don't store client in SndChannel
Frediano Ziglio
fziglio at redhat.com
Fri Apr 28 09:32:36 UTC 2017
>
> 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.
> >
> > > + 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,
> > > uint32_t
> > > **frame,
> > > uint32_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(scc))
> > > ==
> > > 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_client-
> > > >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_clien
> > > 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,
> > > uint32
> > > _t
> > > *sampl
> > > es,
> > > uint32
> > > _t
> > > bufsiz
> > > 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_can_o
> > > pus,
> > > client_can_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(playba
> > > ck->mode));
> > > }
> >
Frediano
More information about the Spice-devel
mailing list