[Spice-devel] [spice-server] sound: Don't mute recording when client reconnects

[Frediano Ziglio]

> 
> When a new record channel is added, the code relies on a snd_send() call
> in record_channel_client_constructed() to send RECORD_START to the
> client. However, at this point, snd_send() is non-functional because
> the red_channel_client_pipe_add() call it makes is a no-op because
> prepare_pipe_add() makes a connection check through
> red_channel_client_is_connected() queueing the item. This connection

The patch looks surely good, but why red_channel_client_is_connected
returns a wrong value? The channel is connected but asking if is
connected returns false... Is currently implemented as: is in the
channel list so is connected, otherwise is not which during construction
is wrong as still not in the list.
Won't be better to check if really connected?

> check returns FALSE at ::constructed() time as the channel client will
> only become connected towards the end of
> red_channel_client_initable_init() which runs after the object
> instantiation is complete.
> 

For me a RCC is connected if there is a client connected at socket
level, being into a list of RCCs is not a really a great definition
for connected.

Maybe removing the check for red_channel_client_is_connected on
prepare_pipe_add would solve the issue?

> This causes a bug where starting recording and then
> disconnecting/reconnecting the client does not successfully reenable
> recording. This is a regression introduced by commit d8dc09
> 'sound: Convert SndChannelClient to RedChannelClient'
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1549132
> 
> Signed-off-by: Christophe Fergeau <cfergeau at redhat.com>
> ---
>  server/sound.c | 37 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/server/sound.c b/server/sound.c
> index e3891d2cc..d461b9272 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -105,7 +105,11 @@ typedef struct SndChannelClientClass {
>      RedChannelClientClass parent_class;
>  } SndChannelClientClass;
>  
> -G_DEFINE_TYPE(SndChannelClient, snd_channel_client, RED_TYPE_CHANNEL_CLIENT)
> +static void snd_channel_client_initable_interface_init(GInitableIface
> *iface);
> +static GInitableIface *snd_channel_client_parent_initable_iface;
> +
> +G_DEFINE_TYPE_WITH_CODE(SndChannelClient, snd_channel_client,
> RED_TYPE_CHANNEL_CLIENT,
> +                        G_IMPLEMENT_INTERFACE(G_TYPE_INITABLE,
> snd_channel_client_initable_interface_init))
>  
>  
>  enum {
> @@ -1083,7 +1087,6 @@ playback_channel_client_constructed(GObject *object)
>      if (channel->active) {
>          playback_channel_client_start(scc);
>      }
> -    snd_send(scc);
>  }
>  
>  static void snd_set_peer(RedChannel *red_channel, RedClient *client,
>  RedStream *stream,
> @@ -1269,7 +1272,6 @@ record_channel_client_constructed(GObject *object)
>      if (channel->active) {
>          record_channel_client_start(scc);
>      }
> -    snd_send(scc);
>  }
>  
>  
> @@ -1480,6 +1482,35 @@ snd_channel_client_init(SndChannelClient *self)
>  {
>  }
>  
> +static gboolean snd_channel_client_initable_init(GInitable *initable,
> +                                                 GCancellable *cancellable,
> +                                                 GError **error)
> +{
> +    gboolean success;
> +
> +    success = snd_channel_client_parent_initable_iface->init(initable,
> cancellable, error);
> +    if (!success) {
> +        return FALSE;
> +    }
> +    /* Wait until the very end of the initialization as the channel client
> +     * becomes connected (red_channel_client_is_connected() starts returning
> +     * TRUE) only very late in red_channel_client_initable_init()
> +     * Before that, red_channel_client_pipe_add() is a no-op because a
> connection check is done in
> +     * prepare_pipe_add()
> +     */
> +    snd_send(SND_CHANNEL_CLIENT(initable));
> +
> +    return TRUE;
> +}
> +
> +static void snd_channel_client_initable_interface_init(GInitableIface
> *iface)
> +{
> +    snd_channel_client_parent_initable_iface = g_type_interface_peek_parent
> (iface);
> +
> +    iface->init = snd_channel_client_initable_init;
> +}
> +
> +
>  static void
>  playback_channel_client_class_init(PlaybackChannelClientClass *klass)
>  {

Frediano