[Spice-devel] [spice-server] sound: Don't mute recording when client reconnects
Frediano Ziglio
fziglio at redhat.com
Wed May 23 17:36:35 UTC 2018
>
> >
> > 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)
> > {
>
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 at redhat.com>
---
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
More information about the Spice-devel
mailing list