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

[Christophe Fergeau]

On Wed, Jan 18, 2017 at 03:25:09AM -0500, Frediano Ziglio wrote:
> > 
> > Hey,
> > 
> > On Fri, Jan 13, 2017 at 05:47:13AM -0500, Frediano Ziglio wrote:
> > > 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?
> > 
> > Hmm, good point that changing this is going to potentially change the
> > order in which the volume/mute/... messages are going to be sent, and
> > delay them a bit. It will also cause less messages to be sent if the
> > user keeps calling spice_server_playback_set_volume() (for example).
> > 
> > I don't think the messages are going to be delayed for a long time
> > though, so this should not really cause an issue (ie we should be fine
> > making that change without trying to get the exact same message order as
> > before)
> > 
> > Christophe
> > 
> 
> I think it's not a bit problem for playback but could be an issue for
> record. While on playback we'll have samples to send with volume with
> record is possible that guest wants to reduce the recording volume
> but if you are not pushing the volume the record will keep to arrive
> at the same volume.
> Note that snd_set_command just set a field.

Indeed, this could be problematic, I'll try moving this change _after_
the introduction of snd_send().

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Frediano Ziglio]

> 
> Hey,
> 
> On Fri, Jan 13, 2017 at 05:47:13AM -0500, Frediano Ziglio wrote:
> > 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?
> 
> Hmm, good point that changing this is going to potentially change the
> order in which the volume/mute/... messages are going to be sent, and
> delay them a bit. It will also cause less messages to be sent if the
> user keeps calling spice_server_playback_set_volume() (for example).
> 
> I don't think the messages are going to be delayed for a long time
> though, so this should not really cause an issue (ie we should be fine
> making that change without trying to get the exact same message order as
> before)
> 
> Christophe
> 

I think it's not a bit problem for playback but could be an issue for
record. While on playback we'll have samples to send with volume with
record is possible that guest wants to reduce the recording volume
but if you are not pushing the volume the record will keep to arrive
at the same volume.
Note that snd_set_command just set a field.

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Christophe Fergeau]

Hey,

On Fri, Jan 13, 2017 at 05:47:13AM -0500, Frediano Ziglio wrote:
> 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?

Hmm, good point that changing this is going to potentially change the
order in which the volume/mute/... messages are going to be sent, and
delay them a bit. It will also cause less messages to be sent if the
user keeps calling spice_server_playback_set_volume() (for example).

I don't think the messages are going to be delayed for a long time
though, so this should not really cause an issue (ie we should be fine
making that change without trying to get the exact same message order as
before)

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [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 
> > > 
> > > Signed-off-by: Christophe Fergeau 
> > > ---
> > >  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 = >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 = >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
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Christophe Fergeau]

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 
> > 
> > Signed-off-by: Christophe Fergeau 
> > ---
> >  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 = >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 = >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


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Frediano Ziglio]

> 
> 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 
> 
> Signed-off-by: Christophe Fergeau 
> ---
>  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 = >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 = >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.

>  }
>  
>  static void snd_playback_start(SndChannel *channel)
> @@ -1378,7 +1376,6 @@ SPICE_GNUC_VISIBLE void
> spice_server_record_set_volume(SpiceRecordInstance *sin,
>  {
>  SpiceVolumeState *st = >st->channel.volume;
>  SndChannelClient *client = sin->st->channel.connection;
> -RecordChannelClient *record_client = SPICE_CONTAINEROF(client,
> RecordChannelClient, base);
>  
>  st->volume_nchannels = nchannels;
>  free(st->volume);
> @@ -1387,21 +1384,20 @@ SPICE_GNUC_VISIBLE void
> spice_server_record_set_volume(SpiceRecordInstance *sin,
>  if (!client || nchannels == 0)
>  return;
>  
> -snd_record_send_volume(record_client);
> +snd_set_command(client, SND_VOLUME_MASK);
>  }
>  
>  SPICE_GNUC_VISIBLE void spice_server_record_set_mute(SpiceRecordInstance
>  *sin, uint8_t mute)
>  {
>  SpiceVolumeState *st = >st->channel.volume;
>  SndChannelClient *client = sin->st->channel.connection;
> -RecordChannelClient *record_client = SPICE_CONTAINEROF(client,
> RecordChannelClient, base);
>  
>  st->mute = mute;
>  
>  if (!client)
>  return;
>  
> -snd_record_send_mute(record_client);
> +snd_set_command(client, SND_MUTE_MASK);
>  }
>  
>  static void snd_record_start(SndChannel *channel)

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Christophe Fergeau]

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 

Signed-off-by: Christophe Fergeau 
---
 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 = >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 = >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);
 }
 
 static void snd_playback_start(SndChannel *channel)
@@ -1378,7 +1376,6 @@ SPICE_GNUC_VISIBLE void 
spice_server_record_set_volume(SpiceRecordInstance *sin,
 {
 SpiceVolumeState *st = >st->channel.volume;
 SndChannelClient *client = sin->st->channel.connection;
-RecordChannelClient *record_client = SPICE_CONTAINEROF(client, 
RecordChannelClient, base);
 
 st->volume_nchannels = nchannels;
 free(st->volume);
@@ -1387,21 +1384,20 @@ SPICE_GNUC_VISIBLE void 
spice_server_record_set_volume(SpiceRecordInstance *sin,
 if (!client || nchannels == 0)
 return;
 
-snd_record_send_volume(record_client);
+snd_set_command(client, SND_VOLUME_MASK);
 }
 
 SPICE_GNUC_VISIBLE void spice_server_record_set_mute(SpiceRecordInstance *sin, 
uint8_t mute)
 {
 SpiceVolumeState *st = >st->channel.volume;
 SndChannelClient *client = sin->st->channel.connection;
-RecordChannelClient *record_client = SPICE_CONTAINEROF(client, 
RecordChannelClient, base);
 
 st->mute = mute;
 
 if (!client)
 return;
 
-snd_record_send_mute(record_client);
+snd_set_command(client, SND_MUTE_MASK);
 }
 
 static void snd_record_start(SndChannel *channel)
-- 
2.9.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel