[Spice-devel,spice-server] sound: Store only playback channels in global list

Submitted by Frediano Ziglio on May 3, 2017, 9:47 a.m.

Details

Message ID 20170503094748.24532-1-fziglio@redhat.com
State New
Headers show
Series "sound: Store only playback channels in global list" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio May 3, 2017, 9:47 a.m.
The list is used only to iterate playback channels

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 server/sound.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/sound.c b/server/sound.c
index be7e607..64cd6d4 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -234,7 +234,7 @@  G_DEFINE_TYPE(RecordChannelClient, record_channel_client, TYPE_SND_CHANNEL_CLIEN
 
 
 /* A list of all Spice{Playback,Record}State objects */
-static GList *snd_channels;
+static GList *snd_playback_channels;
 
 static void snd_send(SndChannelClient * client);
 
@@ -980,12 +980,10 @@  void snd_set_playback_latency(RedClient *client, uint32_t latency)
 {
     GList *l;
 
-    for (l = snd_channels; l != NULL; l = l->next) {
+    for (l = snd_playback_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 &&
+        if (scc &&
             red_channel_client_get_client(RED_CHANNEL_CLIENT(scc)) == client) {
 
             if (red_channel_client_test_remote_cap(RED_CHANNEL_CLIENT(scc),
@@ -1285,12 +1283,12 @@  static void snd_set_record_peer(RedChannel *red_channel, RedClient *client, Reds
 
 static void add_channel(SndChannel *channel)
 {
-    snd_channels = g_list_prepend(snd_channels, channel);
+    snd_playback_channels = g_list_prepend(snd_playback_channels, channel);
 }
 
 static void remove_channel(SndChannel *channel)
 {
-    snd_channels = g_list_remove(snd_channels, channel);
+    snd_playback_channels = g_list_remove(snd_playback_channels, channel);
 }
 
 static void
@@ -1396,7 +1394,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));
 }
 
@@ -1448,12 +1445,10 @@  void snd_set_playback_compression(bool on)
 {
     GList *l;
 
-    for (l = snd_channels; l != NULL; l = l->next) {
+    for (l = snd_playback_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) {
+        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,

Comments

On Wed, May 03, 2017 at 10:47:48AM +0100, Frediano Ziglio wrote:
> The list is used only to iterate playback channels

This patch seems only partial?

> 
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  server/sound.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/server/sound.c b/server/sound.c
> index be7e607..64cd6d4 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -234,7 +234,7 @@ G_DEFINE_TYPE(RecordChannelClient, record_channel_client, TYPE_SND_CHANNEL_CLIEN
>  
>  
>  /* A list of all Spice{Playback,Record}State objects */
> -static GList *snd_channels;
> +static GList *snd_playback_channels;
>  
>  static void snd_send(SndChannelClient * client);
>  
> @@ -980,12 +980,10 @@ void snd_set_playback_latency(RedClient *client, uint32_t latency)
>  {
>      GList *l;
>  
> -    for (l = snd_channels; l != NULL; l = l->next) {
> +    for (l = snd_playback_channels; l != NULL; l = l->next) {
>          SndChannel *now = l->data;

The shortlog says l->data should be a SndChannelClient now?

>          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 &&

Removal of the "type" check would deserve a commit of its own in my
opinion.

> +        if (scc &&
>              red_channel_client_get_client(RED_CHANNEL_CLIENT(scc)) == client) {
>  
>              if (red_channel_client_test_remote_cap(RED_CHANNEL_CLIENT(scc),
> @@ -1285,12 +1283,12 @@ static void snd_set_record_peer(RedChannel *red_channel, RedClient *client, Reds
>  
>  static void add_channel(SndChannel *channel)
>  {
> -    snd_channels = g_list_prepend(snd_channels, channel);
> +    snd_playback_channels = g_list_prepend(snd_playback_channels, channel);

This is still appending a SndChannel to the list contrary to what the
name/shortlog is saying?


Christophe
> 
> On Wed, May 03, 2017 at 10:47:48AM +0100, Frediano Ziglio wrote:
> > The list is used only to iterate playback channels
> 
> This patch seems only partial?
> 
> > 
> > Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> > ---
> >  server/sound.c | 19 +++++++------------
> >  1 file changed, 7 insertions(+), 12 deletions(-)
> > 
> > diff --git a/server/sound.c b/server/sound.c
> > index be7e607..64cd6d4 100644
> > --- a/server/sound.c
> > +++ b/server/sound.c
> > @@ -234,7 +234,7 @@ G_DEFINE_TYPE(RecordChannelClient,
> > record_channel_client, TYPE_SND_CHANNEL_CLIEN
> >  
> >  
> >  /* A list of all Spice{Playback,Record}State objects */

OT: this comment needs update too.. and perhaps not so OT.

> > -static GList *snd_channels;
> > +static GList *snd_playback_channels;
> >  
> >  static void snd_send(SndChannelClient * client);
> >  
> > @@ -980,12 +980,10 @@ void snd_set_playback_latency(RedClient *client,
> > uint32_t latency)
> >  {
> >      GList *l;
> >  
> > -    for (l = snd_channels; l != NULL; l = l->next) {
> > +    for (l = snd_playback_channels; l != NULL; l = l->next) {
> >          SndChannel *now = l->data;
> 
> The shortlog says l->data should be a SndChannelClient now?
> 

?? why you expect a client now? Neither in the subject not in commit
log I said client, is and was a channel, not a channel client.

> >          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 &&
> 
> Removal of the "type" check would deserve a commit of its own in my
> opinion.
> 

I'd add a comment in the commit log.

> > +        if (scc &&
> >              red_channel_client_get_client(RED_CHANNEL_CLIENT(scc)) ==
> >              client) {
> >  
> >              if
> >              (red_channel_client_test_remote_cap(RED_CHANNEL_CLIENT(scc),
> > @@ -1285,12 +1283,12 @@ static void snd_set_record_peer(RedChannel
> > *red_channel, RedClient *client, Reds
> >  
> >  static void add_channel(SndChannel *channel)
> >  {
> > -    snd_channels = g_list_prepend(snd_channels, channel);
> > +    snd_playback_channels = g_list_prepend(snd_playback_channels,
> > channel);
> 
> This is still appending a SndChannel to the list contrary to what the
> name/shortlog is saying?
> 

You are right.

> 
> Christophe
> 

Frediano
On Wed, May 03, 2017 at 06:06:21AM -0400, Frediano Ziglio wrote:
> > 
> > On Wed, May 03, 2017 at 10:47:48AM +0100, Frediano Ziglio wrote:
> > > The list is used only to iterate playback channels
> > 
> > This patch seems only partial?
> > 
> > > 
> > > Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> > > ---
> > >  server/sound.c | 19 +++++++------------
> > >  1 file changed, 7 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/server/sound.c b/server/sound.c
> > > index be7e607..64cd6d4 100644
> > > --- a/server/sound.c
> > > +++ b/server/sound.c
> > > @@ -234,7 +234,7 @@ G_DEFINE_TYPE(RecordChannelClient,
> > > record_channel_client, TYPE_SND_CHANNEL_CLIEN
> > >  
> > >  
> > >  /* A list of all Spice{Playback,Record}State objects */
> 
> OT: this comment needs update too.. and perhaps not so OT.
> 
> > > -static GList *snd_channels;
> > > +static GList *snd_playback_channels;
> > >  
> > >  static void snd_send(SndChannelClient * client);
> > >  
> > > @@ -980,12 +980,10 @@ void snd_set_playback_latency(RedClient *client,
> > > uint32_t latency)
> > >  {
> > >      GList *l;
> > >  
> > > -    for (l = snd_channels; l != NULL; l = l->next) {
> > > +    for (l = snd_playback_channels; l != NULL; l = l->next) {
> > >          SndChannel *now = l->data;
> > 
> > The shortlog says l->data should be a SndChannelClient now?
> > 
> 
> ?? why you expect a client now? Neither in the subject not in commit
> log I said client, is and was a channel, not a channel client.

Oh whatever, I read "channel client" rather than "playback channel" in
the log.

/me goes to read the patch again.

Christopeh