[Spice-devel] [PATCH v2 6/6] sound: don't store client in SndChannel
Jonathon Jongsma
jjongsma at redhat.com
Wed Apr 26 20:55:47 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.
>
> > + 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.
>
> > 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