[spice-server] sound: do not change volume or mute state on migration

Submitted by Victor Toso on Nov. 16, 2017, 3:17 p.m.

Details

Message ID 20171116151701.24760-1-victortoso@redhat.com
State New
Headers show
Series "sound: do not change volume or mute state on migration" ( rev: 2 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso Nov. 16, 2017, 3:17 p.m.
From: Victor Toso <me@victortoso.com>

On migration, we are resending the current volume and mute state in
the Guest. If the client user has change its master volume in the
guest it might change the client application volume too and the volume
jump (increase or decrease) might happen on migration.

This patch is a complement of f10de4bc084fcc - Here, volume was
jumping regardless of guest's volume value.

Resolves: rhbz#1425443
Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 server/sound.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Patch hide | download patch | download mbox

diff --git a/server/sound.c b/server/sound.c
index b1bfaaaa..c6c8bdc8 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -414,6 +414,12 @@  static bool snd_send_volume(SndChannelClient *client, uint32_t cap, int msg)
         return false;
     }
 
+    /* Never changes volume or mute state on migration */
+    if (red_client_during_migrate_at_target(red_channel_client_get_client(rcc))) {
+        spice_debug("Do not change volume during migration");
+        return FALSE;
+    }
+
     vol = alloca(sizeof (SpiceMsgAudioVolume) +
                  st->volume_nchannels * sizeof (uint16_t));
     red_channel_client_init_send_data(rcc, msg);
@@ -445,6 +451,12 @@  static bool snd_send_mute(SndChannelClient *client, uint32_t cap, int msg)
         return false;
     }
 
+   /* Never changes volume or mute state on migration */
+   if (red_client_during_migrate_at_target(red_channel_client_get_client(rcc))) {
+       spice_debug("Do not change mute during migration");
+       return FALSE;
+   }
+
     red_channel_client_init_send_data(rcc, msg);
     mute.mute = st->mute;
     spice_marshall_SpiceMsgAudioMute(m, &mute);

Comments

> 
> From: Victor Toso <me@victortoso.com>
> 
> On migration, we are resending the current volume and mute state in
> the Guest.

Maybe is just me but this sentence is confusing. By "we" I think you
mean spice-server but the confusion came from the fact that is Qemu
that on the new machine try to send the volume to spice-server causing
a potential volume/mute message to be sent to the client.
Another part that is not clear is the "state in the Guest" I think you
are meaning we are sending the "current Guest state value".
I'll try to rephrase:
"On migration, Qemu notify spice-server with the current Guest volume
and mute state values which currently is handled forwarding these values
to the client."

>             If the client user has change its master volume in the
> guest it might change the client application volume too and the volume
> jump (increase or decrease) might happen on migration.
> 
> This patch is a complement of f10de4bc084fcc - Here, volume was
> jumping regardless of guest's volume value.
> 
> Resolves: rhbz#1425443
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  server/sound.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/server/sound.c b/server/sound.c
> index b1bfaaaa..c6c8bdc8 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -414,6 +414,12 @@ static bool snd_send_volume(SndChannelClient *client,
> uint32_t cap, int msg)
>          return false;
>      }
>  
> +    /* Never changes volume or mute state on migration */
> +    if
> (red_client_during_migrate_at_target(red_channel_client_get_client(rcc))) {
> +        spice_debug("Do not change volume during migration");
> +        return FALSE;
> +    }
> +
>      vol = alloca(sizeof (SpiceMsgAudioVolume) +
>                   st->volume_nchannels * sizeof (uint16_t));
>      red_channel_client_init_send_data(rcc, msg);
> @@ -445,6 +451,12 @@ static bool snd_send_mute(SndChannelClient *client,
> uint32_t cap, int msg)
>          return false;
>      }
>  
> +   /* Never changes volume or mute state on migration */
> +   if
> (red_client_during_migrate_at_target(red_channel_client_get_client(rcc))) {
> +       spice_debug("Do not change mute during migration");
> +       return FALSE;
> +   }
> +
>      red_channel_client_init_send_data(rcc, msg);
>      mute.mute = st->mute;
>      spice_marshall_SpiceMsgAudioMute(m, &mute);

Frediano
Hi,

On Thu, Nov 16, 2017 at 10:58:03AM -0500, Frediano Ziglio wrote:
> > 
> > From: Victor Toso <me@victortoso.com>
> > 
> > On migration, we are resending the current volume and mute state in
> > the Guest.
> 
> Maybe is just me but this sentence is confusing. By "we" I think you
> mean spice-server but the confusion came from the fact that is Qemu
> that on the new machine try to send the volume to spice-server causing
> a potential volume/mute message to be sent to the client.
> Another part that is not clear is the "state in the Guest" I think you
> are meaning we are sending the "current Guest state value".
> I'll try to rephrase:
> "On migration, Qemu notify spice-server with the current Guest volume
> and mute state values which currently is handled forwarding these values
> to the client."

Yes, that's correct ~ sorry for the confusion.

Is the rest of commit log clear enough? Spice should mimic what happens
in the guest but a volume/mute change on migration is not happening per
user request which can cause odd volume/mute changes in the client.

Thanks,
    Victor

> 
> >             If the client user has change its master volume in the
> > guest it might change the client application volume too and the volume
> > jump (increase or decrease) might happen on migration.
> > 
> > This patch is a complement of f10de4bc084fcc - Here, volume was
> > jumping regardless of guest's volume value.
> > 
> > Resolves: rhbz#1425443
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > ---
> >  server/sound.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/server/sound.c b/server/sound.c
> > index b1bfaaaa..c6c8bdc8 100644
> > --- a/server/sound.c
> > +++ b/server/sound.c
> > @@ -414,6 +414,12 @@ static bool snd_send_volume(SndChannelClient *client,
> > uint32_t cap, int msg)
> >          return false;
> >      }
> >  
> > +    /* Never changes volume or mute state on migration */
> > +    if
> > (red_client_during_migrate_at_target(red_channel_client_get_client(rcc))) {
> > +        spice_debug("Do not change volume during migration");
> > +        return FALSE;
> > +    }
> > +
> >      vol = alloca(sizeof (SpiceMsgAudioVolume) +
> >                   st->volume_nchannels * sizeof (uint16_t));
> >      red_channel_client_init_send_data(rcc, msg);
> > @@ -445,6 +451,12 @@ static bool snd_send_mute(SndChannelClient *client,
> > uint32_t cap, int msg)
> >          return false;
> >      }
> >  
> > +   /* Never changes volume or mute state on migration */
> > +   if
> > (red_client_during_migrate_at_target(red_channel_client_get_client(rcc))) {
> > +       spice_debug("Do not change mute during migration");
> > +       return FALSE;
> > +   }
> > +
> >      red_channel_client_init_send_data(rcc, msg);
> >      mute.mute = st->mute;
> >      spice_marshall_SpiceMsgAudioMute(m, &mute);
> 
> Frediano
> 
> Hi,
> 
> On Thu, Nov 16, 2017 at 10:58:03AM -0500, Frediano Ziglio wrote:
> > > 
> > > From: Victor Toso <me@victortoso.com>
> > > 
> > > On migration, we are resending the current volume and mute state in
> > > the Guest.
> > 
> > Maybe is just me but this sentence is confusing. By "we" I think you
> > mean spice-server but the confusion came from the fact that is Qemu
> > that on the new machine try to send the volume to spice-server causing
> > a potential volume/mute message to be sent to the client.
> > Another part that is not clear is the "state in the Guest" I think you
> > are meaning we are sending the "current Guest state value".
> > I'll try to rephrase:
> > "On migration, Qemu notify spice-server with the current Guest volume
> > and mute state values which currently is handled forwarding these values
> > to the client."
> 
> Yes, that's correct ~ sorry for the confusion.
> 
> Is the rest of commit log clear enough? Spice should mimic what happens
> in the guest but a volume/mute change on migration is not happening per
> user request which can cause odd volume/mute changes in the client.
> 
> Thanks,
>     Victor
> 

Was looking at these settings, maybe the fix would be better in
snd_channel_set_mute and snd_channel_set_volume (not setting the
mask) ?
What about record_channel_client_constructed? There is a similar
check (for migration) in playback_channel_client_constructed but
not in record_channel_client_constructed.

> > 
> > >             If the client user has change its master volume in the
> > > guest it might change the client application volume too and the volume
> > > jump (increase or decrease) might happen on migration.
> > > 
> > > This patch is a complement of f10de4bc084fcc - Here, volume was
> > > jumping regardless of guest's volume value.
> > > 
> > > Resolves: rhbz#1425443
> > > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > > ---
> > >  server/sound.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/server/sound.c b/server/sound.c
> > > index b1bfaaaa..c6c8bdc8 100644
> > > --- a/server/sound.c
> > > +++ b/server/sound.c
> > > @@ -414,6 +414,12 @@ static bool snd_send_volume(SndChannelClient
> > > *client,
> > > uint32_t cap, int msg)
> > >          return false;
> > >      }
> > >  
> > > +    /* Never changes volume or mute state on migration */
> > > +    if
> > > (red_client_during_migrate_at_target(red_channel_client_get_client(rcc)))
> > > {
> > > +        spice_debug("Do not change volume during migration");
> > > +        return FALSE;
> > > +    }
> > > +
> > >      vol = alloca(sizeof (SpiceMsgAudioVolume) +
> > >                   st->volume_nchannels * sizeof (uint16_t));
> > >      red_channel_client_init_send_data(rcc, msg);
> > > @@ -445,6 +451,12 @@ static bool snd_send_mute(SndChannelClient *client,
> > > uint32_t cap, int msg)
> > >          return false;
> > >      }
> > >  
> > > +   /* Never changes volume or mute state on migration */
> > > +   if
> > > (red_client_during_migrate_at_target(red_channel_client_get_client(rcc)))
> > > {
> > > +       spice_debug("Do not change mute during migration");
> > > +       return FALSE;
> > > +   }
> > > +
> > >      red_channel_client_init_send_data(rcc, msg);
> > >      mute.mute = st->mute;
> > >      spice_marshall_SpiceMsgAudioMute(m, &mute);
> > 
> > Frediano
>
Hi,

On Thu, Nov 16, 2017 at 11:57:12AM -0500, Frediano Ziglio wrote:
> > 
> > Hi,
> > 
> > On Thu, Nov 16, 2017 at 10:58:03AM -0500, Frediano Ziglio wrote:
> > > > 
> > > > From: Victor Toso <me@victortoso.com>
> > > > 
> > > > On migration, we are resending the current volume and mute state in
> > > > the Guest.
> > > 
> > > Maybe is just me but this sentence is confusing. By "we" I think you
> > > mean spice-server but the confusion came from the fact that is Qemu
> > > that on the new machine try to send the volume to spice-server causing
> > > a potential volume/mute message to be sent to the client.
> > > Another part that is not clear is the "state in the Guest" I think you
> > > are meaning we are sending the "current Guest state value".
> > > I'll try to rephrase:
> > > "On migration, Qemu notify spice-server with the current Guest volume
> > > and mute state values which currently is handled forwarding these values
> > > to the client."
> > 
> > Yes, that's correct ~ sorry for the confusion.
> > 
> > Is the rest of commit log clear enough? Spice should mimic what happens
> > in the guest but a volume/mute change on migration is not happening per
> > user request which can cause odd volume/mute changes in the client.
> > 
> > Thanks,
> >     Victor
> > 
> 
> Was looking at these settings, maybe the fix would be better in
> snd_channel_set_mute and snd_channel_set_volume (not setting the
> mask) ?

Agreed, I'll send a v2

> What about record_channel_client_constructed? There is a similar
> check (for migration) in playback_channel_client_constructed but
> not in record_channel_client_constructed.

Okay, I did a quick look at it and its the same code introduced by
Marc-André at f10de4bc084fcc (in this patch's commit log) but it was
moved a bit during the refactor, see:

Current changes are from removing "on_new_playback_channel_client"
function from:

f37629996c1877 sound: Remove on_new_playback_channel_client()

The check was moved to that function at:

258c2234babed9 playback: Don't lose client connection after migration

OT: This change actually fixed
https://bugs.freedesktop.org/show_bug.cgi?id=100136

And before that, it was when the check was introduced:

f10de4bc084fcc sound: do not modify client state on migration

Anyway, I recall discussing this with Marc-André in the past. Both mine
and his fixes worked at the time but it might be that we are dealing
with different issues - It should be safe not to change audio while
migration IMHO.

> > > >             If the client user has change its master volume in the
> > > > guest it might change the client application volume too and the volume
> > > > jump (increase or decrease) might happen on migration.
> > > > 
> > > > This patch is a complement of f10de4bc084fcc - Here, volume was
> > > > jumping regardless of guest's volume value.
> > > > 
> > > > Resolves: rhbz#1425443
> > > > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > > > ---
> > > >  server/sound.c | 12 ++++++++++++
> > > >  1 file changed, 12 insertions(+)
> > > > 
> > > > diff --git a/server/sound.c b/server/sound.c
> > > > index b1bfaaaa..c6c8bdc8 100644
> > > > --- a/server/sound.c
> > > > +++ b/server/sound.c
> > > > @@ -414,6 +414,12 @@ static bool snd_send_volume(SndChannelClient
> > > > *client,
> > > > uint32_t cap, int msg)
> > > >          return false;
> > > >      }
> > > >  
> > > > +    /* Never changes volume or mute state on migration */
> > > > +    if
> > > > (red_client_during_migrate_at_target(red_channel_client_get_client(rcc)))
> > > > {
> > > > +        spice_debug("Do not change volume during migration");
> > > > +        return FALSE;
> > > > +    }
> > > > +
> > > >      vol = alloca(sizeof (SpiceMsgAudioVolume) +
> > > >                   st->volume_nchannels * sizeof (uint16_t));
> > > >      red_channel_client_init_send_data(rcc, msg);
> > > > @@ -445,6 +451,12 @@ static bool snd_send_mute(SndChannelClient *client,
> > > > uint32_t cap, int msg)
> > > >          return false;
> > > >      }
> > > >  
> > > > +   /* Never changes volume or mute state on migration */
> > > > +   if
> > > > (red_client_during_migrate_at_target(red_channel_client_get_client(rcc)))
> > > > {
> > > > +       spice_debug("Do not change mute during migration");
> > > > +       return FALSE;
> > > > +   }
> > > > +
> > > >      red_channel_client_init_send_data(rcc, msg);
> > > >      mute.mute = st->mute;
> > > >      spice_marshall_SpiceMsgAudioMute(m, &mute);
> > > 
> > > Frediano
> >