[Spice-devel] [PATCH spice-server v4] sound: Remove sound channel global list
Frediano Ziglio
fziglio at redhat.com
Mon Aug 21 15:15:26 UTC 2017
>
> On Tue, 2017-05-09 at 19:50 +0100, Frediano Ziglio wrote:
> > Use the channel list from RedState to iterate over all channels.
>
> By the way, this should be RedsState rather than RedState.
>
Fixed.
>
> > This removes one more global variable.
> >
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> > server/reds.c | 18 +++++++++--
> > server/sound.c | 96 ++++++++++++++++++----------------------------
> > ------------
> > server/sound.h | 19 ++++++++++--
> > 3 files changed, 63 insertions(+), 70 deletions(-)
> >
> > Changes since v3:
> > - check class type in RedsState;
> > - change function names.
> >
> > diff --git a/server/reds.c b/server/reds.c
> > index 2a8f905..5d7f226 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -2614,7 +2614,14 @@ void reds_set_client_mm_time_latency(RedsState
> > *reds, RedClient *client, uint32_
> > latency, reds->mm_time_latency);
> > }
> > } else {
> > - snd_set_playback_latency(client, latency);
> > + GListIter it;
> > + RedChannel *channel;
> > +
> > + GLIST_FOREACH(reds->channels, it, RedChannel, channel) {
> > + if (IS_PLAYBACK_CHANNEL(channel)) {
> > + playback_channel_set_latency(PLAYBACK_CHANNEL(channe
> > l), client, latency);
> > + }
> > + }
>
> It seems a bit strange to me that we would call
> func(channel, client, latency)
>
> when we don't know for sure whether client is actually related to
> channel or not. To me it would make more sense to iterate through the
> client's channels rather than the RedsState's channel list. Something
> like:
>
> GList *channel_clients = red_client_get_channel_clients(client);
> GLIST_FOREACH(channel_clients, it, RedChannelClient, rcc) {
This list requires a lock to be held in order to scan it.
You could write a function in RedClient that accepts a callback
and scan the list but potentially you hold the lock for a not
really quantified time.
> RedChannel *channel = red_channel_client_get_channel(rcc);
> if (IS_PLAYBACK_CHANNEL(channel) {
> playback_channel_set_latency(channel, latency);
This however assumes that you can have only a client attached
to the channel. Currently true but not a great assumption.
And if you don't assume it you have to specify the client as
parameter and we get back to func(channel, client, latency).
> }
> }
>
> Note that the function red_client_get_channel_clients() does not exist
> at the moment. But it would be pretty simple to add. (Also note that
> playback_channel_set_latency() would not require both a 'channel' and a
> 'client' argument in this scenario.)
>
> Alternately, instead of adding a new method to retrieve the channel
> clients from RedClient, we could simply add a red_client_set_latency()
> function which would forward the request on to any playback channels it
> owns. For example
>
> void reds_set_client_mm_time_latency(RedsState *reds, RedClient
> *client, uint32_t latency) {
> ...
> red_client_set_latency(client, latency);
> ...
> }
>
> ...
>
>
> red_client_set_latency(RedClient *client, uint32_t latency) {
> GListIter it;
> GLIST_FOREACH(client->channels, it, RedChannelClient, rcc) {
> RedChannel *channel = red_channel_client_get_channel(rcc);
> if (IS_PLAYBACK_CHANNEL(channel) {
> playback_channel_set_latency(channel, latency);
> }
> }
> }
>
> ?
This solution still have the issue of the lock (although here
is easier to lock) and the single client assumption and you
also add the dependency from RedClient to PlaybackChannel which
IMO is pretty wrong.
>
>
> > }
> > }
> >
> > @@ -4012,8 +4019,15 @@ static void reds_set_video_codecs(RedsState
> > *reds, GArray *video_codecs)
> >
> > SPICE_GNUC_VISIBLE int
> > spice_server_set_playback_compression(SpiceServer *reds, int enable)
> > {
> > + GListIter it;
> > + RedChannel *channel;
> > +
> > reds->config->playback_compression = !!enable;
> > - snd_set_playback_compression(enable);
> > + GLIST_FOREACH(reds->channels, it, RedChannel, channel) {
The "it" argument was removed today, rest of the patch is still fine.
> > + if (IS_PLAYBACK_CHANNEL(channel)) {
> > + playback_channel_set_compression(PLAYBACK_CHANNEL(channe
> > l), !!enable);
> > + }
> > + }
> > return 0;
> > }
> >
> > diff --git a/server/sound.c b/server/sound.c
> > index be7e607..7140854 100644
> > --- a/server/sound.c
> > +++ b/server/sound.c
> > @@ -71,7 +71,6 @@ typedef struct PlaybackChannelClient
> > PlaybackChannelClient;
> > typedef struct RecordChannelClient RecordChannelClient;
> > typedef struct AudioFrame AudioFrame;
> > typedef struct AudioFrameContainer AudioFrameContainer;
> > -typedef struct SpicePlaybackState PlaybackChannel;
> > typedef struct SpiceRecordState RecordChannel;
> >
> > typedef void (*snd_channel_on_message_done_proc)(SndChannelClient
> > *client);
> > @@ -178,11 +177,6 @@ typedef struct SndChannelClass {
> > G_DEFINE_TYPE(SndChannel, snd_channel, RED_TYPE_CHANNEL)
> >
> >
> > -#define TYPE_PLAYBACK_CHANNEL playback_channel_get_type()
> > -#define PLAYBACK_CHANNEL(obj) \
> > - (G_TYPE_CHECK_INSTANCE_CAST((obj), TYPE_PLAYBACK_CHANNEL,
> > PlaybackChannel))
> > -GType playback_channel_get_type(void) G_GNUC_CONST;
> > -
> > struct SpicePlaybackState {
> > SndChannel channel;
> > };
> > @@ -233,9 +227,6 @@ typedef struct RecordChannelClientClass {
> > G_DEFINE_TYPE(RecordChannelClient, record_channel_client,
> > TYPE_SND_CHANNEL_CLIENT)
> >
> >
> > -/* A list of all Spice{Playback,Record}State objects */
> > -static GList *snd_channels;
> > -
> > static void snd_send(SndChannelClient * client);
> >
> > /* sound channels only support a single client */
> > @@ -976,28 +967,22 @@ SPICE_GNUC_VISIBLE void
> > spice_server_playback_put_samples(SpicePlaybackInstance
> > snd_send(SND_CHANNEL_CLIENT(playback_client));
> > }
> >
> > -void snd_set_playback_latency(RedClient *client, uint32_t latency)
> > +void playback_channel_set_latency(PlaybackChannel *channel,
> > RedClient *client,
> > + uint32_t latency)
> > {
> > - GList *l;
> > + SndChannelClient *scc =
> > snd_channel_get_client(SND_CHANNEL(channel));
> > + if (scc &&
> > + red_channel_client_get_client(RED_CHANNEL_CLIENT(scc)) ==
> > client) {
> >
> > - 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 && scc &&
> > - red_channel_client_get_client(RED_CHANNEL_CLIENT(scc))
> > == client) {
> > + if
> > (red_channel_client_test_remote_cap(RED_CHANNEL_CLIENT(scc),
> > + SPICE_PLAYBACK_CAP_LATENCY)) {
> > + PlaybackChannelClient* playback =
> > (PlaybackChannelClient*)scc;
> >
> > - if
> > (red_channel_client_test_remote_cap(RED_CHANNEL_CLIENT(scc),
> > - SPICE_PLAYBACK_CAP_LATENCY)) {
> > - PlaybackChannelClient* playback =
> > (PlaybackChannelClient*)scc;
> > -
> > - playback->latency = latency;
> > - snd_set_command(scc, SND_PLAYBACK_LATENCY_MASK);
> > - snd_send(scc);
> > - } else {
> > - spice_debug("client doesn't not support
> > SPICE_PLAYBACK_CAP_LATENCY");
> > - }
> > + playback->latency = latency;
> > + snd_set_command(scc, SND_PLAYBACK_LATENCY_MASK);
> > + snd_send(scc);
> > + } else {
> > + spice_debug("client doesn't not support
> > SPICE_PLAYBACK_CAP_LATENCY");
> > }
> > }
> > }
> > @@ -1283,16 +1268,6 @@ static void snd_set_record_peer(RedChannel
> > *red_channel, RedClient *client, Reds
> > TYPE_RECORD_CHANNEL_CLIENT);
> > }
> >
> > -static void add_channel(SndChannel *channel)
> > -{
> > - snd_channels = g_list_prepend(snd_channels, channel);
> > -}
> > -
> > -static void remove_channel(SndChannel *channel)
> > -{
> > - snd_channels = g_list_remove(snd_channels, channel);
> > -}
> > -
> > static void
> > snd_channel_init(SndChannel *self)
> > {
> > @@ -1304,8 +1279,6 @@ snd_channel_finalize(GObject *object)
> > {
> > SndChannel *channel = SND_CHANNEL(object);
> >
> > - remove_channel(channel);
> > -
> > free(channel->volume.volume);
> > channel->volume.volume = NULL;
> >
> > @@ -1346,7 +1319,6 @@ playback_channel_constructed(GObject *object)
> > }
> > red_channel_set_cap(RED_CHANNEL(self),
> > SPICE_PLAYBACK_CAP_VOLUME);
> >
> > - add_channel(self);
> > reds_register_channel(reds, RED_CHANNEL(self));
> > }
> >
> > @@ -1396,7 +1368,6 @@ record_channel_constructed(GObject *object)
> > }
> > red_channel_set_cap(RED_CHANNEL(self), SPICE_RECORD_CAP_VOLUME);
> >
> > - add_channel(self);
> > reds_register_channel(reds, RED_CHANNEL(self));
> > }
> >
> > @@ -1444,30 +1415,23 @@ void snd_detach_record(SpiceRecordInstance
> > *sin)
> > snd_detach_common(&sin->st->channel);
> > }
> >
> > -void snd_set_playback_compression(bool on)
> > -{
> > - GList *l;
> > -
> > - 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 && 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);
> > - bool 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);
> > - if (playback->mode != desired_mode) {
> > - playback->mode = desired_mode;
> > - snd_set_command(client, SND_PLAYBACK_MODE_MASK);
> > - spice_debug("playback client %p using mode %s",
> > playback,
> > - spice_audio_data_mode_to_string(playback
> > ->mode));
> > - }
> > +void playback_channel_set_compression(PlaybackChannel *channel, bool
> > on)
> > +{
> > + SndChannelClient *client =
> > snd_channel_get_client(SND_CHANNEL(channel));
> > + if (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);
> > + bool client_can_opus =
> > red_channel_client_test_remote_cap(rcc,
> > + SPICE_PLAYBACK_CAP_OPUS);
> > + int desired_mode = snd_desired_audio_mode(on,
> > SND_CHANNEL(channel)->frequency,
> > + client_can_opus,
> > client_can_celt);
> > + if (playback->mode != desired_mode) {
> > + playback->mode = desired_mode;
> > + snd_set_command(client, SND_PLAYBACK_MODE_MASK);
> > + spice_debug("playback client %p using mode %s",
> > playback,
> > + spice_audio_data_mode_to_string(playback-
> > >mode));
> > }
> > }
> > }
> > diff --git a/server/sound.h b/server/sound.h
> > index 2f0a2b9..9794457 100644
> > --- a/server/sound.h
> > +++ b/server/sound.h
> > @@ -22,14 +22,29 @@
> >
> > struct RedClient;
> >
> > +typedef struct SpicePlaybackState PlaybackChannel;
> > +#define TYPE_PLAYBACK_CHANNEL playback_channel_get_type()
> > +#define PLAYBACK_CHANNEL(obj) \
> > + (G_TYPE_CHECK_INSTANCE_CAST((obj), TYPE_PLAYBACK_CHANNEL,
> > PlaybackChannel))
> > +#define IS_PLAYBACK_CHANNEL(obj) \
> > + (G_TYPE_CHECK_INSTANCE_TYPE((obj), TYPE_PLAYBACK_CHANNEL))
> > +GType playback_channel_get_type(void) G_GNUC_CONST;
> > +
> > void snd_attach_playback(RedsState *reds, SpicePlaybackInstance
> > *sin);
> > void snd_detach_playback(SpicePlaybackInstance *sin);
> >
> > void snd_attach_record(RedsState *reds, SpiceRecordInstance *sin);
> > void snd_detach_record(SpiceRecordInstance *sin);
> >
> > -void snd_set_playback_compression(bool on);
> > +/**
> > + * Set compression for a given playback channel.
> > + */
> > +void playback_channel_set_compression(PlaybackChannel *channel, bool
> > on);
> >
> > -void snd_set_playback_latency(struct RedClient *client, uint32_t
> > latency);
> > +/**
> > + * Set latency for a given playback channel.
> > + */
> > +void playback_channel_set_latency(PlaybackChannel *channel, struct
> > RedClient *client,
> > + uint32_t latency);
> >
> > #endif /* SOUND_H_ */
>
Frediano
More information about the Spice-devel
mailing list