[Spice-devel] [PATCH v2 6/6] sound: don't store client in SndChannel

[Frediano Ziglio]

> 
> 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 at redhat.com>
> > > > > ---
> > > > >  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?
> 

Sure.

Frediano