On Fri, Nov 7, 2014 at 8:41 PM, Marc-André Lureau
<marcandre.lureau@xxxxxxxxxx> wrote:
> The audio channels are currently referenced and released on channel
> close events. However, this event may not happen if the channel never
> was connected. Keeping channels alive also prevent session from
> finishing.
>
> By not holding the ref, the channel can go to dispose
> when it is no longer needed, and the session can be disposed too.
> ---
> gtk/spice-gstaudio.c | 81 ++++++++++++++++++++--------------------------------
> gtk/spice-pulse.c | 65 ++++++++++++++++-------------------------
> 2 files changed, 56 insertions(+), 90 deletions(-)
>
> diff --git a/gtk/spice-gstaudio.c b/gtk/spice-gstaudio.c
> index e6cf6a9..d784aa1 100644
> --- a/gtk/spice-gstaudio.c
> +++ b/gtk/spice-gstaudio.c
> @@ -53,9 +53,8 @@ struct _SpiceGstaudioPrivate {
> guint mmtime_id;
> };
>
> -static void channel_event(SpiceChannel *channel, SpiceChannelEvent event,
> - gpointer data);
> static gboolean connect_channel(SpiceAudio *audio, SpiceChannel *channel);
> +static void channel_weak_notified(gpointer data, GObject *where_the_object_was);
>
> static void spice_gstaudio_finalize(GObject *obj)
> {
> @@ -91,27 +90,21 @@ static void spice_gstaudio_dispose(GObject *obj)
> stream_dispose(&p->playback);
> stream_dispose(&p->record);
>
> - if (p->pchannel != NULL) {
> - g_signal_handlers_disconnect_by_func(p->pchannel,
> - channel_event, obj);
> - g_object_unref(p->pchannel);
> - p->pchannel = NULL;
> - }
> + if (p->pchannel)
> + g_object_weak_unref(G_OBJECT(p->pchannel), channel_weak_notified, gstaudio);
> + p->pchannel = NULL;
Please, keep the explicit comparison here.
Using explicit comparisons for everything but booleans help a lot the
understanding when people are quick reading the code.
>
> - if (p->rchannel != NULL) {
> - g_signal_handlers_disconnect_by_func(p->rchannel,
> - channel_event, obj);
> - g_object_unref(p->rchannel);
> - p->rchannel = NULL;
> - }
> + if (p->rchannel)
> + g_object_weak_unref(G_OBJECT(p->rchannel), channel_weak_notified, gstaudio);
> + p->rchannel = NULL;
>
> if (G_OBJECT_CLASS(spice_gstaudio_parent_class)->dispose)
> G_OBJECT_CLASS(spice_gstaudio_parent_class)->dispose(obj);
> }
>
> -static void spice_gstaudio_init(SpiceGstaudio *pulse)
> +static void spice_gstaudio_init(SpiceGstaudio *gstaudio)
> {
> - pulse->priv = SPICE_GSTAUDIO_GET_PRIVATE(pulse);
> + gstaudio->priv = SPICE_GSTAUDIO_GET_PRIVATE(gstaudio);
> }
>
> static void spice_gstaudio_class_init(SpiceGstaudioClass *klass)
> @@ -144,9 +137,8 @@ static void record_new_buffer(GstAppSink *appsink, gpointer data)
> gst_element_post_message(p->record.pipe, msg);
> }
>
> -static void record_stop(SpiceRecordChannel *channel, gpointer data)
> +static void record_stop(SpiceGstaudio *gstaudio)
> {
> - SpiceGstaudio *gstaudio = data;
> SpiceGstaudioPrivate *p = gstaudio->priv;
>
> SPICE_DEBUG("%s", __FUNCTION__);
> @@ -230,7 +222,7 @@ static void record_start(SpiceRecordChannel *channel, gint format, gint channels
> if (p->record.pipe &&
> (p->record.rate != frequency ||
> p->record.channels != channels)) {
> - record_stop(channel, data);
> + record_stop(gstaudio);
> gst_object_unref(p->record.pipe);
> p->record.pipe = NULL;
> }
> @@ -285,31 +277,6 @@ lerr:
> gst_element_set_state(p->record.pipe, GST_STATE_PLAYING);
> }
>
> -static void channel_event(SpiceChannel *channel, SpiceChannelEvent event,
> - gpointer data)
> -{
> - SpiceGstaudio *gstaudio = data;
> - SpiceGstaudioPrivate *p = gstaudio->priv;
> -
> - switch (event) {
> - case SPICE_CHANNEL_OPENED:
> - break;
> - case SPICE_CHANNEL_CLOSED:
> - if (channel == p->pchannel) {
> - p->pchannel = NULL;
> - g_object_unref(channel);
> - } else if (channel == p->rchannel) {
> - record_stop(SPICE_RECORD_CHANNEL(channel), gstaudio);
> - p->rchannel = NULL;
> - g_object_unref(channel);
> - } else /* if (p->pchannel || p->rchannel) */
> - g_warn_if_reached();
> - break;
> - default:
> - break;
> - }
> -}
> -
> static void playback_stop(SpicePlaybackChannel *channel, gpointer data)
> {
> SpiceGstaudio *gstaudio = data;
> @@ -542,6 +509,22 @@ static void record_mute_changed(GObject *object, GParamSpec *pspec, gpointer dat
> g_object_unref(e);
> }
>
> +static void
> +channel_weak_notified(gpointer data,
> + GObject *where_the_object_was)
> +{
> + SpiceGstaudio *gstaudio = SPICE_GSTAUDIO(data);
> + SpiceGstaudioPrivate *p = gstaudio->priv;
> +
> + if (where_the_object_was == (GObject *)p->pchannel) {
> + p->pchannel = NULL;
> + } else if (where_the_object_was == (GObject *)p->rchannel) {
> + SPICE_DEBUG("record closed");
> + record_stop(gstaudio);
> + p->rchannel = NULL;
> + }
> +}
> +
> static gboolean connect_channel(SpiceAudio *audio, SpiceChannel *channel)
> {
> SpiceGstaudio *gstaudio = SPICE_GSTAUDIO(audio);
> @@ -550,15 +533,14 @@ static gboolean connect_channel(SpiceAudio *audio, SpiceChannel *channel)
> if (SPICE_IS_PLAYBACK_CHANNEL(channel)) {
> g_return_val_if_fail(p->pchannel == NULL, FALSE);
>
> - p->pchannel = g_object_ref(channel);
> + p->pchannel = channel;
> + g_object_weak_ref(G_OBJECT(p->pchannel), channel_weak_notified, audio);
> spice_g_signal_connect_object(channel, "playback-start",
> G_CALLBACK(playback_start), gstaudio, 0);
> spice_g_signal_connect_object(channel, "playback-data",
> G_CALLBACK(playback_data), gstaudio, 0);
> spice_g_signal_connect_object(channel, "playback-stop",
> G_CALLBACK(playback_stop), gstaudio, 0);
> - spice_g_signal_connect_object(channel, "channel-event",
> - G_CALLBACK(channel_event), gstaudio, 0);
> spice_g_signal_connect_object(channel, "notify::volume",
> G_CALLBACK(playback_volume_changed), gstaudio, 0);
> spice_g_signal_connect_object(channel, "notify::mute",
> @@ -571,12 +553,11 @@ static gboolean connect_channel(SpiceAudio *audio, SpiceChannel *channel)
> g_return_val_if_fail(p->rchannel == NULL, FALSE);
>
> p->rchannel = g_object_ref(channel);
Do not ref channel here.
> + g_object_weak_ref(G_OBJECT(p->rchannel), channel_weak_notified, audio);
> spice_g_signal_connect_object(channel, "record-start",
> G_CALLBACK(record_start), gstaudio, 0);
> spice_g_signal_connect_object(channel, "record-stop",
> - G_CALLBACK(record_stop), gstaudio, 0);
> - spice_g_signal_connect_object(channel, "channel-event",
> - G_CALLBACK(channel_event), gstaudio, 0);
> + G_CALLBACK(record_stop), gstaudio, G_CONNECT_SWAPPED);
> spice_g_signal_connect_object(channel, "notify::volume",
> G_CALLBACK(record_volume_changed), gstaudio, 0);
> spice_g_signal_connect_object(channel, "notify::mute",
> diff --git a/gtk/spice-pulse.c b/gtk/spice-pulse.c
> index 744ce4d..fc1662e 100644
> --- a/gtk/spice-pulse.c
> +++ b/gtk/spice-pulse.c
> @@ -74,10 +74,9 @@ static const char *context_state_names[] = {
> #define STATE_NAME(array, state) \
> ((state < G_N_ELEMENTS(array)) ? array[state] : NULL)
>
> -static void channel_event(SpiceChannel *channel, SpiceChannelEvent event,
> - gpointer data);
> static void stream_stop(SpicePulse *pulse, struct stream *s);
> static gboolean connect_channel(SpiceAudio *audio, SpiceChannel *channel);
> +static void channel_weak_notified(gpointer data, GObject *where_the_object_was);
>
> static void spice_pulse_finalize(GObject *obj)
> {
> @@ -120,11 +119,11 @@ static void spice_pulse_dispose(GObject *obj)
> p->record.cork_op = NULL;
>
> if (p->pchannel)
> - g_object_unref(p->pchannel);
> + g_object_weak_unref(G_OBJECT(p->pchannel), channel_weak_notified, pulse);
> p->pchannel = NULL;
>
> if (p->rchannel)
> - g_object_unref(p->rchannel);
> + g_object_weak_unref(G_OBJECT(p->rchannel), channel_weak_notified, pulse);
> p->rchannel = NULL;
>
> G_OBJECT_CLASS(spice_pulse_parent_class)->dispose(obj);
> @@ -567,9 +566,8 @@ static void record_start(SpiceRecordChannel *channel, gint format, gint channels
> p->state = state;
> }
>
> -static void record_stop(SpiceRecordChannel *channel, gpointer data)
> +static void record_stop(SpicePulse *pulse)
> {
> - SpicePulse *pulse = data;
> SpicePulsePrivate *p = pulse->priv;
>
> SPICE_DEBUG("%s", __FUNCTION__);
> @@ -581,33 +579,6 @@ static void record_stop(SpiceRecordChannel *channel, gpointer data)
> stream_stop(pulse, &p->record);
> }
>
> -static void channel_event(SpiceChannel *channel, SpiceChannelEvent event,
> - gpointer data)
> -{
> - SpicePulse *pulse = data;
> - SpicePulsePrivate *p = pulse->priv;
> -
> - switch (event) {
> - case SPICE_CHANNEL_OPENED:
> - break;
> - case SPICE_CHANNEL_CLOSED:
> - if (channel == p->pchannel) {
> - SPICE_DEBUG("playback closed");
> - p->pchannel = NULL;
> - g_object_unref(channel);
> - } else if (channel == p->rchannel) {
> - SPICE_DEBUG("record closed");
> - record_stop(SPICE_RECORD_CHANNEL(channel), pulse);
> - p->rchannel = NULL;
> - g_object_unref(channel);
> - } else /* if (p->pchannel || p->rchannel) */
> - g_warn_if_reached();
> - break;
> - default:
> - break;
> - }
> -}
> -
> static void playback_volume_changed(GObject *object, GParamSpec *pspec, gpointer data)
> {
> SpicePulse *pulse = data;
> @@ -748,6 +719,22 @@ static void record_volume_changed(GObject *object, GParamSpec *pspec, gpointer d
> pa_operation_unref(op);
> }
>
> +static void
> +channel_weak_notified(gpointer data,
> + GObject *where_the_object_was)
> +{
> + SpicePulse *pulse = SPICE_PULSE(data);
> + SpicePulsePrivate *p = pulse->priv;
> +
> + if (where_the_object_was == (GObject *)p->pchannel) {
> + p->pchannel = NULL;
> + } else if (where_the_object_was == (GObject *)p->rchannel) {
> + SPICE_DEBUG("record closed");
> + record_stop(pulse);
> + p->rchannel = NULL;
> + }
> +}
> +
> static gboolean connect_channel(SpiceAudio *audio, SpiceChannel *channel)
> {
> SpicePulse *pulse = SPICE_PULSE(audio);
> @@ -756,15 +743,14 @@ static gboolean connect_channel(SpiceAudio *audio, SpiceChannel *channel)
> if (SPICE_IS_PLAYBACK_CHANNEL(channel)) {
> g_return_val_if_fail(p->pchannel == NULL, FALSE);
>
> - p->pchannel = g_object_ref(channel);
> + p->pchannel = channel;
> + g_object_weak_ref(G_OBJECT(p->pchannel), channel_weak_notified, audio);
> spice_g_signal_connect_object(channel, "playback-start",
> G_CALLBACK(playback_start), pulse, 0);
> spice_g_signal_connect_object(channel, "playback-data",
> G_CALLBACK(playback_data), pulse, 0);
> spice_g_signal_connect_object(channel, "playback-stop",
> G_CALLBACK(playback_stop), pulse, 0);
> - spice_g_signal_connect_object(channel, "channel-event",
> - G_CALLBACK(channel_event), pulse, 0);
> spice_g_signal_connect_object(channel, "notify::volume",
> G_CALLBACK(playback_volume_changed), pulse, 0);
> spice_g_signal_connect_object(channel, "notify::mute",
> @@ -778,13 +764,12 @@ static gboolean connect_channel(SpiceAudio *audio, SpiceChannel *channel)
> if (SPICE_IS_RECORD_CHANNEL(channel)) {
> g_return_val_if_fail(p->rchannel == NULL, FALSE);
>
> - p->rchannel = g_object_ref(channel);
> + p->rchannel = channel;
> + g_object_weak_ref(G_OBJECT(p->rchannel), channel_weak_notified, audio);
> spice_g_signal_connect_object(channel, "record-start",
> G_CALLBACK(record_start), pulse, 0);
> spice_g_signal_connect_object(channel, "record-stop",
> - G_CALLBACK(record_stop), pulse, 0);
> - spice_g_signal_connect_object(channel, "channel-event",
> - G_CALLBACK(channel_event), pulse, 0);
> + G_CALLBACK(record_stop), pulse, G_CONNECT_SWAPPED);
> spice_g_signal_connect_object(channel, "notify::volume",
> G_CALLBACK(record_volume_changed), pulse, 0);
> spice_g_signal_connect_object(channel, "notify::mute",
> --
> 1.9.3
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
Looks good. Please, consider the comment about the explicit comparisons.
ACK with ref change.
--
Fabiano Fidêncio
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel