[Spice-devel,spice-server,v3] sound: Remove sound channel global list

Submitted by Frediano Ziglio on May 9, 2017, 2:34 p.m.

Details

Message ID 20170509143444.22459-1-fziglio@redhat.com
State New
Headers show
Series "sound: Remove sound channel global list" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio May 9, 2017, 2:34 p.m.
Use channel list to iterate all channels.
This remove another global variable still left.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 server/reds.c  |  14 ++++++--
 server/sound.c | 100 +++++++++++++++++++++++----------------------------------
 server/sound.h |  15 +++++++--
 3 files changed, 65 insertions(+), 64 deletions(-)

Changes since v2:
- change completely, using Reds global list of all channels
  (suggested by Christophe Fergeau)

This change is better seen ignoring space changes.

Patch hide | download patch | download mbox

diff --git a/server/reds.c b/server/reds.c
index 2a8f905..bdd7377 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -2614,7 +2614,12 @@  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) {
+            snd_set_playback_channel_latency(channel, client, latency);
+        }
     }
 }
 
@@ -4012,8 +4017,13 @@  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) {
+        snd_set_playback_channel_compression(channel, !!enable);
+    }
     return 0;
 }
 
diff --git a/server/sound.c b/server/sound.c
index be7e607..d8b5153 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -181,6 +181,8 @@  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))
+#define IS_PLAYBACK_CHANNEL(obj) \
+    (G_TYPE_CHECK_INSTANCE_TYPE((obj), TYPE_PLAYBACK_CHANNEL))
 GType playback_channel_get_type(void) G_GNUC_CONST;
 
 struct SpicePlaybackState {
@@ -233,9 +235,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 +975,27 @@  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 snd_set_playback_channel_latency(RedChannel *red_channel, RedClient *client,
+                                      uint32_t latency)
 {
-    GList *l;
+    if (!IS_PLAYBACK_CHANNEL(red_channel)) {
+        return;
+    }
 
-    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) {
+    PlaybackChannel *channel = PLAYBACK_CHANNEL(red_channel);
+    SndChannelClient *scc = snd_channel_get_client(SND_CHANNEL(channel));
+    if (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 +1281,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 +1292,6 @@  snd_channel_finalize(GObject *object)
 {
     SndChannel *channel = SND_CHANNEL(object);
 
-    remove_channel(channel);
-
     free(channel->volume.volume);
     channel->volume.volume = NULL;
 
@@ -1346,7 +1332,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 +1381,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 +1428,28 @@  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_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));
-            }
+void snd_set_playback_channel_compression(RedChannel *red_channel, bool on)
+{
+    if (!IS_PLAYBACK_CHANNEL(red_channel)) {
+        return;
+    }
+
+    PlaybackChannel *channel = PLAYBACK_CHANNEL(red_channel);
+    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..ae021be 100644
--- a/server/sound.h
+++ b/server/sound.h
@@ -28,8 +28,17 @@  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);
-
-void snd_set_playback_latency(struct RedClient *client, uint32_t latency);
+/**
+ * Set compression for a given channel.
+ * Function will check if the given channel has the proper type
+ */
+void snd_set_playback_channel_compression(RedChannel *channel, bool on);
+
+/**
+ * Set latency for a given channel.
+ * Function will check if the given channel has the proper type
+ */
+void snd_set_playback_channel_latency(RedChannel *channel, struct RedClient *client,
+                                      uint32_t latency);
 
 #endif /* SOUND_H_ */

Comments

On Tue, May 09, 2017 at 03:34:44PM +0100, Frediano Ziglio wrote:
> Use channel list to iterate all channels.

Use the channel list from RedState to iterate over all channels

> This remove another global variable still left.

This removes one more global variable

> -void snd_set_playback_latency(RedClient *client, uint32_t latency)
> +void snd_set_playback_channel_latency(RedChannel *red_channel, RedClient *client,
> +                                      uint32_t latency)
>  {
> -    GList *l;
> +    if (!IS_PLAYBACK_CHANNEL(red_channel)) {
> +        return;
> +    }

Having a method which will silently ignore invalid objects is fairly
unusual (in this case, even
snd_set_playback_channel_latency(gtk_window_new(), ...) would "work").
Can the IS_PLAYBACK_CHANNEL() check be moved to the reds.c callers, with
the assumption that red_channel has to be of the appropriate type here?
(and same comment for set_playback_compression).

Apart from this, this looks good.

Christophe