[Spice-devel] [spice-server v2] 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
> 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.
> 
> This commit solves this issue by making PlaybackChannelClient and
> RecordChannelClient implement GInitable, and move the code interacting
> with the client in their _initable_init() function, as at this point the
> objects will be able to send data.
> 
> 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>
> ---
> Hey,
> 
> Another variation of my fix of that recording bug, probably can be seen
> as 'more correct' than the previous one, even though both fix the bug in
> practice.
> 
> 
>  server/sound.c | 119 +++++++++++++++++++++++++++++++++----------------
>  1 file changed, 81 insertions(+), 38 deletions(-)
> 
> diff --git a/server/sound.c b/server/sound.c
> index e3891d2cc..c1058e95c 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -105,6 +105,11 @@ typedef struct SndChannelClientClass {
>      RedChannelClientClass parent_class;
>  } SndChannelClientClass;
>  
> +static void playback_channel_client_initable_interface_init(GInitableIface
> *iface);
> +static void record_channel_client_initable_interface_init(GInitableIface
> *iface);
> +static GInitableIface *playback_channel_client_parent_initable_iface;
> +static GInitableIface *record_channel_client_parent_initable_iface;
> +
>  G_DEFINE_TYPE(SndChannelClient, snd_channel_client, RED_TYPE_CHANNEL_CLIENT)
>  
>  
> @@ -151,8 +156,8 @@ typedef struct PlaybackChannelClientClass {
>      SndChannelClientClass parent_class;
>  } PlaybackChannelClientClass;
>  
> -G_DEFINE_TYPE(PlaybackChannelClient, playback_channel_client,
> TYPE_SND_CHANNEL_CLIENT)
> -
> +G_DEFINE_TYPE_WITH_CODE(PlaybackChannelClient, playback_channel_client,
> TYPE_SND_CHANNEL_CLIENT,
> +                        G_IMPLEMENT_INTERFACE(G_TYPE_INITABLE,
> playback_channel_client_initable_interface_init))
>  

style, line too long.

>  typedef struct SpiceVolumeState {
>      uint16_t *volume;
> @@ -232,7 +237,8 @@ typedef struct RecordChannelClientClass {
>      SndChannelClientClass parent_class;
>  } RecordChannelClientClass;
>  
> -G_DEFINE_TYPE(RecordChannelClient, record_channel_client,
> TYPE_SND_CHANNEL_CLIENT)
> +G_DEFINE_TYPE_WITH_CODE(RecordChannelClient, record_channel_client,
> TYPE_SND_CHANNEL_CLIENT,
> +                        G_IMPLEMENT_INTERFACE(G_TYPE_INITABLE,
> record_channel_client_initable_interface_init))
>  

same

>  
>  /* A list of all Spice{Playback,Record}State objects */
> @@ -1046,7 +1052,6 @@ playback_channel_client_constructed(GObject *object)
>      RedChannelClient *rcc = RED_CHANNEL_CLIENT(playback_client);
>      RedChannel *red_channel = red_channel_client_get_channel(rcc);
>      SndChannel *channel = SND_CHANNEL(red_channel);
> -    RedClient *red_client = red_channel_client_get_client(rcc);
>      SndChannelClient *scc = SND_CHANNEL_CLIENT(playback_client);
>  
>      G_OBJECT_CLASS(playback_channel_client_parent_class)->constructed(object);
> @@ -1072,18 +1077,6 @@ playback_channel_client_constructed(GObject *object)
>  
>      spice_debug("playback client %p using mode %s", playback_client,
>                  spice_audio_data_mode_to_string(playback_client->mode));
> -
> -    if (!red_client_during_migrate_at_target(red_client)) {
> -        snd_set_command(scc, SND_PLAYBACK_MODE_MASK);
> -        if (channel->volume.volume_nchannels) {
> -            snd_set_command(scc, SND_VOLUME_MUTE_MASK);
> -        }
> -    }
> -
> -    if (channel->active) {
> -        playback_channel_client_start(scc);
> -    }
> -    snd_send(scc);
>  }
>  
>  static void snd_set_peer(RedChannel *red_channel, RedClient *client,
>  RedStream *stream,
> @@ -1252,27 +1245,6 @@ record_channel_client_finalize(GObject *object)
>      G_OBJECT_CLASS(record_channel_client_parent_class)->finalize(object);
>  }
>  
> -static void
> -record_channel_client_constructed(GObject *object)
> -{
> -    RecordChannelClient *record_client = RECORD_CHANNEL_CLIENT(object);
> -    RedChannel *red_channel =
> red_channel_client_get_channel(RED_CHANNEL_CLIENT(record_client));
> -    SndChannel *channel = SND_CHANNEL(red_channel);
> -    SndChannelClient *scc = SND_CHANNEL_CLIENT(record_client);
> -
> -    G_OBJECT_CLASS(record_channel_client_parent_class)->constructed(object);
> -
> -    if (channel->volume.volume_nchannels) {
> -        snd_set_command(scc, SND_VOLUME_MUTE_MASK);
> -    }
> -
> -    if (channel->active) {
> -        record_channel_client_start(scc);
> -    }
> -    snd_send(scc);
> -}
> -
> -
>  static void snd_set_record_peer(RedChannel *red_channel, RedClient *client,
>  RedStream *stream,
>                                  G_GNUC_UNUSED int migration,
>                                  RedChannelCapabilities *caps)
> @@ -1480,6 +1452,43 @@ snd_channel_client_init(SndChannelClient *self)
>  {
>  }
>  
> +static gboolean playback_channel_client_initable_init(GInitable *initable,
> +                                                      GCancellable
> *cancellable,
> +                                                      GError **error)
> +{
> +    gboolean success;
> +    RedClient *red_client =
> red_channel_client_get_client(RED_CHANNEL_CLIENT(initable));
> +    SndChannelClient *scc = SND_CHANNEL_CLIENT(initable);
> +    RedChannel *red_channel =
> red_channel_client_get_channel(RED_CHANNEL_CLIENT(initable));
> +    SndChannel *channel = SND_CHANNEL(red_channel);
> +
> +    success = playback_channel_client_parent_initable_iface->init(initable,
> cancellable, error);
> +    if (!success) {

I think is short and easier to read doing the if directly.

> +        return FALSE;
> +    }
> +
> +    if (!red_client_during_migrate_at_target(red_client)) {
> +        snd_set_command(scc, SND_PLAYBACK_MODE_MASK);
> +        if (channel->volume.volume_nchannels) {
> +            snd_set_command(scc, SND_VOLUME_MUTE_MASK);
> +        }
> +    }
> +
> +    if (channel->active) {
> +        playback_channel_client_start(scc);
> +    }
> +    snd_send(SND_CHANNEL_CLIENT(initable));
> +
> +    return TRUE;
> +}
> +
> +static void playback_channel_client_initable_interface_init(GInitableIface
> *iface)
> +{
> +    playback_channel_client_parent_initable_iface =
> g_type_interface_peek_parent (iface);
> +

style: space before parenthesis.

> +    iface->init = playback_channel_client_initable_init;
> +}
> +
>  static void
>  playback_channel_client_class_init(PlaybackChannelClientClass *klass)
>  {
> @@ -1507,11 +1516,45 @@ playback_channel_client_init(PlaybackChannelClient
> *playback)
>      snd_playback_alloc_frames(playback);
>  }
>  
> +static gboolean record_channel_client_initable_init(GInitable *initable,
> +                                                    GCancellable
> *cancellable,
> +                                                    GError **error)
> +{
> +    gboolean success;
> +    RecordChannelClient *record_client = RECORD_CHANNEL_CLIENT(initable);
> +    RedChannel *red_channel =
> red_channel_client_get_channel(RED_CHANNEL_CLIENT(record_client));
> +    SndChannel *channel = SND_CHANNEL(red_channel);
> +    SndChannelClient *scc = SND_CHANNEL_CLIENT(record_client);
> +
> +    success = record_channel_client_parent_initable_iface->init(initable,
> cancellable, error);
> +    if (!success) {
> +        return FALSE;
> +    }
> +
> +    if (channel->volume.volume_nchannels) {
> +        snd_set_command(scc, SND_VOLUME_MUTE_MASK);
> +    }
> +
> +    if (channel->active) {
> +        record_channel_client_start(scc);
> +    }
> +    snd_send(SND_CHANNEL_CLIENT(initable));
> +
> +    return TRUE;
> +}
> +
> +static void record_channel_client_initable_interface_init(GInitableIface
> *iface)
> +{
> +    record_channel_client_parent_initable_iface =
> g_type_interface_peek_parent (iface);
> +

style

> +    iface->init = record_channel_client_initable_init;
> +}
> +
> +
>  static void
>  record_channel_client_class_init(RecordChannelClientClass *klass)
>  {
>      GObjectClass *object_class = G_OBJECT_CLASS(klass);
> -    object_class->constructed = record_channel_client_constructed;
>      object_class->finalize = record_channel_client_finalize;
>  }
>  

Is it only me or che code looks a lot duplicated?
Reusing constructed function instead of removing and create new
function in different places won't reduce the diff?

Frediano