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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

Just to mention that I tested both patches and they work as
expected in the situation around rhbz#1549132

        toso

On Wed, May 23, 2018 at 01:36:35PM -0400, Frediano Ziglio wrote:
> > > 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@xxxxxxxxxx>
> > > ---
> > >  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)
> > >  {
> > 
> 
> Testing this, so far is working correctly:
> 
> 
> 
> Subject: [PATCH spice-server] rcc: Make red_channel_client_is_connected return
>  correct information during initialization
> 
> Use stream->watch as flag to check if connected or not initializing
> it during construction, not during init (after all hierarchy is built).
> 
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
>  server/red-channel-client.c | 33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> index 893a764d..0057108f 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -173,6 +173,7 @@ static void red_channel_client_clear_sent_item(RedChannelClient *rcc);
>  static void red_channel_client_initable_interface_init(GInitableIface *iface);
>  static void red_channel_client_set_message_serial(RedChannelClient *channel, uint64_t);
>  static bool red_channel_client_config_socket(RedChannelClient *rcc);
> +static void red_channel_client_event(int fd, int event, void *data);
>  
>  /*
>   * When an error occurs over a channel, we treat it as a warning
> @@ -293,6 +294,22 @@ red_channel_client_get_property(GObject *object,
>      }
>  }
>  
> +static void
> +red_channel_client_init_watch(RedChannelClient *rcc)
> +{
> +    if (!rcc->priv->stream || rcc->priv->stream->watch || !rcc->priv->channel) {
> +        return;
> +    }
> +
> +    SpiceCoreInterfaceInternal *core = red_channel_get_core_interface(rcc->priv->channel);
> +    red_stream_set_core_interface(rcc->priv->stream, core);
> +    rcc->priv->stream->watch =
> +        core->watch_add(core, rcc->priv->stream->socket,
> +                        SPICE_WATCH_EVENT_READ,
> +                        red_channel_client_event,
> +                        rcc);
> +}
> +
>  static void
>  red_channel_client_set_property(GObject *object,
>                                  guint property_id,
> @@ -305,11 +322,13 @@ red_channel_client_set_property(GObject *object,
>      {
>          case PROP_STREAM:
>              self->priv->stream = g_value_get_pointer(value);
> +            red_channel_client_init_watch(self);
>              break;
>          case PROP_CHANNEL:
>              if (self->priv->channel)
>                  g_object_unref(self->priv->channel);
>              self->priv->channel = g_value_dup_object(value);
> +            red_channel_client_init_watch(self);
>              break;
>          case PROP_CLIENT:
>              self->priv->client = g_value_get_object(value);
> @@ -913,7 +932,6 @@ static gboolean red_channel_client_initable_init(GInitable *initable,
>                                                   GError **error)
>  {
>      GError *local_error = NULL;
> -    SpiceCoreInterfaceInternal *core;
>      RedChannelClient *self = RED_CHANNEL_CLIENT(initable);
>  
>      if (!self->priv->stream) {
> @@ -932,16 +950,10 @@ static gboolean red_channel_client_initable_init(GInitable *initable,
>          goto cleanup;
>      }
>  
> -    core = red_channel_get_core_interface(self->priv->channel);
> -    red_stream_set_core_interface(self->priv->stream, core);
> -    self->priv->stream->watch =
> -        core->watch_add(core, self->priv->stream->socket,
> -                        SPICE_WATCH_EVENT_READ,
> -                        red_channel_client_event,
> -                        self);
> -
>      if (self->priv->monitor_latency
>          && red_stream_get_family(self->priv->stream) != AF_UNIX) {
> +        SpiceCoreInterfaceInternal *core = red_channel_get_core_interface(self->priv->channel);
> +
>          self->priv->latency_monitor.timer =
>              core->timer_add(core, red_channel_client_ping_timer, self);
>  
> @@ -1665,8 +1677,7 @@ bool red_channel_client_is_mini_header(RedChannelClient *rcc)
>  
>  gboolean red_channel_client_is_connected(RedChannelClient *rcc)
>  {
> -    return rcc->priv->channel
> -        && (g_list_find(red_channel_get_clients(rcc->priv->channel), rcc) != NULL);
> +    return rcc->priv->stream->watch != NULL;
>  }
>  
>  static void red_channel_client_clear_sent_item(RedChannelClient *rcc)
> -- 
> 
> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]