Re: [PATCH v2 6/6] sound: don't store client in SndChannel

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 2017-04-28 at 05:32 -0400, Frediano Ziglio wrote:
> > 
> > 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@xxxxxxxxxx>
> > > > ---
> > > >  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.
> > 
> 
> The g_warning is a regression.

Would you ack the patch without the g_warning here?


> 
> > > 
> > > > +    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,
> > > >                                                           uint3
> > > > 2_t
> > > > **frame,
> > > >                                                           uint3
> > > > 2_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(s
> > > > cc))
> > > > ==
> > > > 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_clien
> > > > t-
> > > > > 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.
> > 
> 
> You are right, I had a quick view at the set, I was confused by
> the fact that I would have expected some function name changes.
> 
> > > 
> > > >      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_c
> > > > lien
> > > > 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,
> > > >                                                              ui
> > > > nt32
> > > > _t
> > > >                                                              *s
> > > > ampl
> > > > es,
> > > >                                                              ui
> > > > nt32
> > > > _t
> > > >                                                              bu
> > > > fsiz
> > > > 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_c
> > > > an_o
> > > > pus,
> > > >                                                        client_c
> > > > an_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(pl
> > > > ayba
> > > > ck->mode));
> > > >              }
> 
> Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]