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

[Hans de Goede]

Hi,

On 04/01/2013 10:38 PM, Marc-André Lureau wrote:
>
>
> ----- Mensaje original -----
>> Hi,
>>
>> On 04/01/2013 07:58 PM, Marc-André Lureau wrote:
>>>
>>>
>>> ----- 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

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.

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.

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

Normally when we get the initial audio-volume-msg we don't have a playback
stream yet, so we don't call pa_context_set_sink_input_volume.

But on live migration we do have a playback stream so we end up calling
pa_context_set_sink_input_volume with invalid parameters.

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.

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!

I suggest we compromise and add an explicit check for nchannels == 0 and
which logs a debug message when triggered.

Regards,

Hans