[Spice-devel] [PATCH spice-gtk] spice-pulse: Fix set_sink_input_volume() failed on seamless migration

[Marc-André Lureau]

Hi

> >>> ----- Mensaje original -----
> >>>> On seamless migration "switch-over" the playback_volume_changed callback
> >>>> gets
> >>>> called, and at this time the channel object's nchannels property is 0,
> >>>> leading to a warning like this:
> >>>>
> >>>> (remote-viewer:8726): GSpice-WARNING **: set_sink_input_volume() failed:
> >>>> Invalid argument
> >>>>
> >>>> This patch fixes this.
> >>>
> >>> Why is that? the nchannels property is set when receiving volume. Why
> >>> does
> >>> the server send a volume with 0 channels? Does that make sense? Shouldn't
> >>> the fix be in the server instead?
> >>
> >> A very valid question I tracked this down to a server bug, for which I
> >> just send a patch.
> >>
> >> Still I would like to push the trivial spice-gtk fix to silence the
> >> warning
> >> upstream so, so as to silence it when using a new spice-gtk with an older
> >> spice-server.
> >
> > I disagree, the warning shows a misbehaving server. Without it, we wouldn't
> > be able to spot this kind of bug.
> 
> I disagree with you disagree-ing:
> 
> 1) The server sends an empty audio-volume message, this is not illegal, it
> is not really useful, but not really a bug either
> 

It's a bug.

> 2) The warning shows us calling pa_context_set_sink_input_volume with invalid
> parameters, which is a spice-gtk bug, not a server bug.

Sure, but it also print a warning. If think you could agree it's useful to keep the warning, since you want to fix it. Replace the PA warning with a spice-gtk warning, but don't just "hide" it.

> 3) My patch does not remove the warning, it avoids calling
> pa_context_set_sink_input_volume with invalid parameters, which is something
> we
> should never deliberately do.

It stops printing the warning if channels == 0, doesn't it?

> ATM the server *always* sends us the wrong volume message, but we don't check
> for
> nchannels == 0 when processing this message.

The check should probably be in channel-playback.c actually, not even in pulse backend.

> I think we can agree that we should *never* deliberately call functions with
> invalid parameters. So we need to *explicitly* check for nchannels == 0,
> rather
> then relying on pa_context_set_sink_input_volume to check this for us.

I agree, but keep a warning, a return_if_fail() would do too, in channel-playback.c / channel-record.c

> Now we can do 2 things when this checks triggers, we can simply ignore the
> message silently (an empty volume message is a legal message protocol wise),
> or we can add an explicit warning for this, but then this warning will
> trigger whenever we connect to an older server!

Yes, let's keep warning. Server must be fixed, even older version should be updated, this will be one more reason.