[Spice-devel] [spice-server 11/17] sound: Prefer snd_set_command() over snd_*_send_*()

[Frediano Ziglio]

> 
> On Wed, Jan 11, 2017 at 09:43:48AM -0500, Frediano Ziglio wrote:
> > > 
> > > snd_set_command()/snd_send() are higher level methods which take care of
> > > scheduling calls to the corresponding snd_*_send_*() methods when
> > > appropriate. This commit switches a few direct snd_*_send_*() calls to
> > > snd_set_command()/snd_send().
> > > 
> > > Based on a patch from Frediano Ziglio <fziglio at redhat.com>
> > > 
> > > Signed-off-by: Christophe Fergeau <cfergeau at redhat.com>
> > > ---
> > >  server/sound.c | 12 ++++--------
> > >  1 file changed, 4 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/server/sound.c b/server/sound.c
> > > index d45e4b9..b0a1367 100644
> > > --- a/server/sound.c
> > > +++ b/server/sound.c
> > > @@ -1094,7 +1094,6 @@ SPICE_GNUC_VISIBLE void
> > > spice_server_playback_set_volume(SpicePlaybackInstance *
> > >  {
> > >      SpiceVolumeState *st = &sin->st->channel.volume;
> > >      SndChannelClient *client = sin->st->channel.connection;
> > > -    PlaybackChannelClient *playback_client = SPICE_CONTAINEROF(client,
> > > PlaybackChannelClient, base);
> > >  
> > >      st->volume_nchannels = nchannels;
> > >      free(st->volume);
> > > @@ -1103,21 +1102,20 @@ SPICE_GNUC_VISIBLE void
> > > spice_server_playback_set_volume(SpicePlaybackInstance *
> > >      if (!client || nchannels == 0)
> > >          return;
> > >  
> > > -    snd_playback_send_volume(playback_client);
> > > +    snd_set_command(client, SND_VOLUME_MASK);
> > >  }
> > >  
> > >  SPICE_GNUC_VISIBLE void
> > >  spice_server_playback_set_mute(SpicePlaybackInstance
> > >  *sin, uint8_t mute)
> > >  {
> > >      SpiceVolumeState *st = &sin->st->channel.volume;
> > >      SndChannelClient *client = sin->st->channel.connection;
> > > -    PlaybackChannelClient *playback_client = SPICE_CONTAINEROF(client,
> > > PlaybackChannelClient, base);
> > >  
> > >      st->mute = mute;
> > >  
> > >      if (!client)
> > >          return;
> > >  
> > > -    snd_playback_send_mute(playback_client);
> > > +    snd_set_command(client, SND_MUTE_MASK);
> > 
> > For compatibility you need to send both MUTE and VOLUME so
> > SND_VOLUME_MUTE_MASK.
> 
> Are you sure? snd_playback_send_mute() is/was directly marshalling a
> 'mute' message, it was not going through the VOLUME check which was
> setting both 'volume' and 'mute' messages. So just setting the MUTE
> flag seems ok to me?
> 
> Christophe
> 

You are right. Surprisingly if the network queue was full the old code
just ignored the request not sending/setting any command.
After your code you don't schedule a send any volume/mute changes.
Maybe nothing change as will be send after/before next audio frame.
Maybe this patch should be partially merged to "sound: Use RedChannelClient
to receive/send data" ?
Maybe in the loop volume/mute flags should be checked before the frames?

Frediano