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

[Victor Toso]

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 at 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 at 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
> > 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20171204/66bb3cfa/attachment.sig>