[Spice-devel] [PATCH spice-server v4] sound: Remove sound channel global list
Jonathon Jongsma
jjongsma at redhat.com
Thu May 11 15:19:57 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.
> 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) {
RedChannel *channel = red_channel_client_get_channel(rcc);
if (IS_PLAYBACK_CHANNEL(channel) {
playback_channel_set_latency(channel, 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);
}
}
}
?
> }
> }
>
> @@ -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) {
> + 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_ */
More information about the Spice-devel
mailing list