<div dir="ltr">Hi <div class="gmail_extra"> </div><div class="gmail_extra">A few comments below. </div><div class="gmail_extra"><div class="gmail_quote">On Thu, Apr 16, 2015 at 4:42 PM, Victor Toso <<a href="mailto:victortoso@redhat.com" target="_blank">victortoso@redhat.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class=""><div class="h5">In case of volume-sync between client and guest, we request volume-info from the availables streams and if the stream is not available we rely on ext-stream-restore. By using ext-stream-restore we can get the last stream data of the application that is stored by PulseAudio. Related: <a href="https://bugzilla.redhat.com/show_bug.cgi?id=1012868" target="_blank">https://bugzilla.redhat.com/show_bug.cgi?id=1012868</a> --- gtk/spice-pulse.c | 476 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 467 insertions(+), 9 deletions(-) diff --git a/gtk/spice-pulse.c b/gtk/spice-pulse.c index c583032..c2be1b3 100644 --- a/gtk/spice-pulse.c +++ b/gtk/spice-pulse.c @@ -25,18 +25,34 @@ #include <pulse/glib-mainloop.h> #include <pulse/pulseaudio.h> +#include <pulse/ext-stream-restore.h> #define SPICE_PULSE_GET_PRIVATE(obj) \ (G_TYPE_INSTANCE_GET_PRIVATE((obj), SPICE_TYPE_PULSE, SpicePulsePrivate)) +struct async_task { + SpicePulse *pulse; + SpiceMainChannel *main_channel; + GSimpleAsyncResult *res; + GAsyncReadyCallback callback; + gpointer user_data; + gboolean is_playback; + pa_operation *pa_op; + gulong cancel_id; + GCancellable *cancellable; +}; + struct stream { - pa_sample_spec spec; - pa_stream *stream; - int state; - pa_operation *uncork_op; - pa_operation *cork_op; - gboolean started; - guint num_underflow; + pa_sample_spec spec; + pa_stream *stream; + int state; + pa_operation *uncork_op; + pa_operation *cork_op; + gboolean started; + guint num_underflow; + gboolean info_updated; + gchar *name; + pa_ext_stream_restore_info info; }; struct _SpicePulsePrivate { @@ -50,6 +66,8 @@ struct _SpicePulsePrivate { struct stream record; guint last_delay; guint target_delay; + struct async_task *pending_restore_task; + GList *results; }; G_DEFINE_TYPE(SpicePulse, spice_pulse, SPICE_TYPE_AUDIO) @@ -77,6 +95,18 @@ static const char *context_state_names[] = { 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_get_playback_volume_info_async(SpiceAudio *audio, GCancellable *cancellable, + SpiceMainChannel *main_channel, GAsyncReadyCallback callback, gpointer user_data); +static gboolean spice_pulse_get_playback_volume_info_finish(SpiceAudio *audio, GAsyncResult *res, + gboolean *mute, guint8 *nchannels, guint16 **volume, GError **error); +static void spice_pulse_get_record_volume_info_async(SpiceAudio *audio, GCancellable *cancellable, + SpiceMainChannel *main_channel, GAsyncReadyCallback callback, gpointer user_data); +static gboolean spice_pulse_get_record_volume_info_finish(SpiceAudio *audio,GAsyncResult *res, + gboolean *mute, guint8 *nchannels, guint16 **volume, GError **error); +static void stream_restore_read_cb(pa_context *context, + const pa_ext_stream_restore_info *info, int eol, void *userdata); +static void spice_pulse_complete_async_task(struct async_task *task, const gchar *err_msg); +static void spice_pulse_complete_all_async_tasks(SpicePulse *pulse, const gchar *err_msg); static void spice_pulse_finalize(GObject *obj) { @@ -118,6 +148,12 @@ static void spice_pulse_dispose(GObject *obj) pa_operation_unref(p->record.cork_op); p->record.cork_op = NULL; + if (p->results != NULL) + spice_pulse_complete_all_async_tasks(pulse, "PulseAudio is being dispose"); + + g_free(p-><a href="http://playback.name" target="_blank">playback.name</a>); + g_free(p-><a href="http://record.name" target="_blank">record.name</a>); + if (p->pchannel) g_object_weak_unref(G_OBJECT(p->pchannel), channel_weak_notified, pulse); p->pchannel = NULL; @@ -140,6 +176,10 @@ static void spice_pulse_class_init(SpicePulseClass *klass) SpiceAudioClass *audio_class = SPICE_AUDIO_CLASS(klass); audio_class->connect_channel = connect_channel; + audio_class->get_playback_volume_info_async = spice_pulse_get_playback_volume_info_async; + audio_class->get_playback_volume_info_finish = spice_pulse_get_playback_volume_info_finish; + audio_class->get_record_volume_info_async = spice_pulse_get_record_volume_info_async; + audio_class->get_record_volume_info_finish = spice_pulse_get_record_volume_info_finish; gobject_class->finalize = spice_pulse_finalize; gobject_class->dispose = spice_pulse_dispose; @@ -812,18 +852,40 @@ static void context_state_callback(pa_context *c, void *userdata) if (!p->playback.stream && p->playback.started) create_playback(SPICE_PULSE(userdata)); + + if (p->pending_restore_task != NULL && + p->pending_restore_task->pa_op == NULL) { + pa_operation *op = pa_ext_stream_restore_read(p->context, + stream_restore_read_cb, + pulse); + if (!op) { + spice_pulse_complete_all_async_tasks(pulse, + pa_strerror(pa_context_errno(p->context))); + } else { + p->pending_restore_task->pa_op = op; + } + } break; } case PA_CONTEXT_FAILED: g_warning("PulseAudio context failed %s", pa_strerror(pa_context_errno(p->context))); - break; + goto context_fail; case PA_CONTEXT_TERMINATED: default: SPICE_DEBUG("PulseAudio context terminated"); - break; + goto context_fail; + } + + return; + +context_fail: + if (p->pending_restore_task != NULL) { + const gchar *errmsg = pa_strerror(pa_context_errno(p->context)); + errmsg = (errmsg != NULL) ? errmsg : "PulseAudio context terminated"; + spice_pulse_complete_all_async_tasks(pulse, errmsg); } } @@ -849,9 +911,405 @@ SpicePulse *spice_pulse_new(SpiceSession *session, GMainContext *context, goto error; } + p-><a href="http://playback.name" target="_blank">playback.name</a> = g_strconcat("sink-input-by-application-name:", + g_get_application_name(), NULL); + p-><a href="http://record.name" target="_blank">record.name</a> = g_strconcat("source-output-by-application-name:", + g_get_application_name(), NULL); return pulse; error: g_object_unref(pulse); return NULL; } + +static gboolean free_async_task(gpointer user_data) +{ + struct async_task *task = user_data; + + if (task == NULL) + return G_SOURCE_REMOVE; + + if (task->pulse) + g_object_unref(task->pulse); + + if (task->res) + g_object_unref(task->res); + + if (task->main_channel) + g_object_unref(task->main_channel); + + if (task->pa_op != NULL) + pa_operation_unref(task->pa_op); + + if (task->cancel_id != 0) { + g_cancellable_disconnect(task->cancellable, task->cancel_id); + g_clear_object(&task->cancellable); + } + + g_free(task); + return G_SOURCE_REMOVE; +} + +static void cancel_task(GCancellable *cancellable, gpointer user_data) +{ + struct async_task *task = user_data; + + if (task->pa_op != NULL) { + pa_operation_cancel(task->pa_op); + pa_operation_unref(task->pa_op); + task->pa_op = NULL; + } + + if (task->pulse->priv->pending_restore_task == task) { + task->pulse->priv->pending_restore_task = NULL; + } </div></div></blockquote><div> </div><div>better in free_async_task() imho, but perhaps you wanted to clear it before the idle(). In which case, move it close to the idle_add() </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class=""><div class="h5"> + +#if GLIB_CHECK_VERSION(2,40,0) + free_async_task(task); +#else + /* FIXME: <a href="https://bugzilla.gnome.org/show_bug.cgi?id=705395" target="_blank">https://bugzilla.gnome.org/show_bug.cgi?id=705395</a> + * Free the memory in idle */ + g_idle_add(free_async_task, task); +#endif +} + +static void complete_task(SpicePulse *pulse, struct async_task *task, const gchar *err_msg) +{ + SpicePulsePrivate *p = pulse->priv; + + /* If we do have any err_msg, we failed */ + if (err_msg != NULL) { + g_simple_async_result_set_op_res_gboolean(task->res, FALSE); + g_simple_async_result_set_error(task->res, + SPICE_CLIENT_ERROR, + SPICE_CLIENT_ERROR_FAILED, + "restore-info failed due %s", + err_msg); + /* Volume-info does not change if stream is not found */ + } else if ((task->is_playback == TRUE && p->playback.info_updated == FALSE) || + (task->is_playback == FALSE && p->record.info_updated == FALSE)) { + g_simple_async_result_set_op_res_gboolean(task->res, FALSE); + g_simple_async_result_set_error(task->res, + SPICE_CLIENT_ERROR, + SPICE_CLIENT_ERROR_FAILED, + "Stream not found by pulse"); + } else { + g_simple_async_result_set_op_res_gboolean(task->res, TRUE); + } + + /* As all async calls to PulseAudio are done with glib mainloop, it is + * safe to complete the operation synchronously here. */ + g_simple_async_result_complete(task->res); +} + +static void spice_pulse_complete_async_task(struct async_task *task, const gchar *err_msg) +{ + SpicePulsePrivate *p; + + if (task == NULL) + return; </div></div></blockquote><div> </div><div>When is this condition normally reached? g_return_if_fail() instead? </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class=""><div class="h5"> + + p = task->pulse->priv; + complete_task(task->pulse, task, err_msg); + if (p->results != NULL) { + p->results = g_list_remove(p->results, task); + SPICE_DEBUG("Number of async task is %d", g_list_length(p->results)); + } + free_async_task(task); +} + +static void spice_pulse_complete_all_async_tasks(SpicePulse *pulse, const gchar *err_msg) +{ + SpicePulsePrivate *p = pulse->priv; + GList *it; + + /* Complete all tasks in list */ + for(it = p->results; it != NULL; it = it->next) { + struct async_task *task = it->data; + + if (task->pa_op != NULL) { + pa_operation_cancel(task->pa_op); + pa_operation_unref(task->pa_op); + task->pa_op = NULL; + } + complete_task(pulse, task, err_msg); + free_async_task(task); + } + g_list_free(p->results); + p->results = NULL; + p->pending_restore_task = NULL; </div></div></blockquote><div> </div><div>(to be removed once in free_async_task) </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class=""><div class="h5"> + SPICE_DEBUG("All async tasks completed"); +} + +static void stream_restore_read_cb(pa_context *context, + const pa_ext_stream_restore_info *info, + int eol, + void *userdata) +{ + SpicePulsePrivate *p = SPICE_PULSE(userdata)->priv; + struct stream *pstream = NULL; + + if (eol || + (p->playback.info_updated == TRUE && + p->record.info_updated == TRUE)) { </div></div></blockquote><div> </div><div>This might still overwrite values set by info_cb if one of the two is set, I would just skip the code below when it's already set. It's also easier to read. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class=""><div class="h5"> + /* As we only have one pa_operation running the stream-restore-info + * which retrieves volume-info from both Playback and Record channels, + * We can complete all async tasks now that this operation ended. + * (or we already have the volume-info we want) + * Note: the following function cancel the current pa_operation */ + spice_pulse_complete_all_async_tasks(SPICE_PULSE(userdata), NULL); + return; + } + + if (g_strcmp0(info->name, p-><a href="http://playback.name" target="_blank">playback.name</a>) == 0) { + pstream = &p->playback; + } else if (g_strcmp0(info->name, p-><a href="http://record.name" target="_blank">record.name</a>) == 0) { + pstream = &p->record; + } else { + /* This is not the stream you are looking for. */ + return; + } + + if (info->channel_map.channels == 0) { + SPICE_DEBUG("Number of channels stored is zero. Ignore. (%s)", info->name); + return; + } + + pstream->info_updated = TRUE; + pstream-><a href="http://info.name" target="_blank">info.name</a> = pstream->name; + pstream->info.mute = info->mute; + pstream->info.channel_map = info->channel_map; + pstream->info.volume = info->volume; +} + +static void source_output_info_cb(pa_context *context, + const pa_source_output_info *info, + int eol, + void *userdata) +{ + struct async_task *task = userdata; + SpicePulsePrivate *p = task->pulse->priv; + struct stream *pstream = &p->record; + + if (eol) { + spice_pulse_complete_async_task(task, NULL); + return; + } + + pstream->info_updated = TRUE; + pstream-><a href="http://info.name" target="_blank">info.name</a> = pstream->name; + pstream->info.mute = info->mute; + pstream->info.channel_map = info->channel_map; + pstream->info.volume = info->volume; +} + +static void sink_input_info_cb(pa_context *context, + const pa_sink_input_info *info, + int eol, + void *userdata) +{ + struct async_task *task = userdata; + SpicePulsePrivate *p = task->pulse->priv; + struct stream *pstream = &p->playback; + + if (eol) { + spice_pulse_complete_async_task(task, NULL); + return; + } + + pstream->info_updated = TRUE; + pstream-><a href="http://info.name" target="_blank">info.name</a> = pstream->name; + pstream->info.mute = info->mute; + pstream->info.channel_map = info->channel_map; + pstream->info.volume = info->volume; +} + +/* to avoid code duplication */ +static void pulse_stream_restore_info_async(gboolean is_playback, + SpiceAudio *audio, + GCancellable *cancellable, + SpiceMainChannel *main_channel, + GAsyncReadyCallback callback, + gpointer user_data) +{ + SpicePulsePrivate *p = SPICE_PULSE(audio)->priv; + GSimpleAsyncResult *simple; + struct async_task *task = g_malloc0(sizeof(struct async_task)); + pa_operation *op = NULL; + + simple = g_simple_async_result_new(G_OBJECT(audio), + callback, + user_data, + pulse_stream_restore_info_async); + g_simple_async_result_set_check_cancellable (simple, cancellable); + + task->res = simple; + task->pulse = g_object_ref(audio); + task->callback = callback; + task->user_data = user_data; + task->is_playback = is_playback; + task->main_channel = g_object_ref(main_channel); + task->pa_op = NULL; + + if (cancellable) { + task->cancellable = g_object_ref(cancellable); + task->cancel_id = g_cancellable_connect(cancellable, G_CALLBACK(cancel_task), task, NULL); + } + + /* If Playback/Record stream is created we use pulse API to get volume-info + * from those streams directly. If the stream is not created, retrieve last + * volume/mute values from Pulse database using the application name; */ + + if (is_playback == TRUE && + p->playback.stream != NULL && + pa_stream_get_index(p->playback.stream) != PA_INVALID_INDEX) { + SPICE_DEBUG("Playback stream is created - get-sink-input-info"); + p->playback.info_updated = FALSE; + op = pa_context_get_sink_input_info(p->context, + pa_stream_get_index(p->playback.stream), + sink_input_info_cb, + task); + if (!op) + goto fail; + task->pa_op = op; + + } else if (is_playback == FALSE && + p->record.stream != NULL && + pa_stream_get_index(p->record.stream) != PA_INVALID_INDEX) { + SPICE_DEBUG("Record stream is created - get-source-output-info"); + p->record.info_updated = FALSE; + op = pa_context_get_source_output_info(p->context, + pa_stream_get_index(p->record.stream), + source_output_info_cb, + task); + if (!op) + goto fail; + task->pa_op = op; + + } else { + if (p-><a href="http://playback.info.name" target="_blank">playback.info.name</a> != NULL || + p-><a href="http://record.info.name" target="_blank">record.info.name</a> != NULL) { + /* If the pstream-><a href="http://info.name" target="_blank">info.name</a> is set then we already have updated + * volume information. We can complete the request now */ + SPICE_DEBUG("Return the volume-information we already have"); + spice_pulse_complete_async_task(task, NULL); + return; + } + + if (p->results == NULL) { + SPICE_DEBUG("Streams are not created - ext-stream-restore"); + p->playback.info_updated = FALSE; + p->record.info_updated = FALSE; + + if (pa_context_get_state(p->context) == PA_CONTEXT_READY) { + /* Restore value from pulse db */ + op = pa_ext_stream_restore_read(p->context, stream_restore_read_cb, audio); + if (!op) + goto fail; + task->pa_op = op; + } else { + /* It is possible that we want to get volume-info before the + * context is in READY state. In this case, we wait for the + * context state change to READY. */ + p->pending_restore_task = task; + } + } + } + + p->results = g_list_append(p->results, task); + SPICE_DEBUG ("Number of async task is %d", g_list_length(p->results)); + return; + +fail: + if (!op) { + g_simple_async_report_error_in_idle(G_OBJECT(audio), + callback, + user_data, + SPICE_CLIENT_ERROR, + SPICE_CLIENT_ERROR_FAILED, + "Volume-Info failed: %s", + pa_strerror(pa_context_errno(p->context))); + free_async_task(task); + } +} + +/* to avoid code duplication */ +static gboolean pulse_stream_restore_info_finish(gboolean is_playback, + SpiceAudio *audio, + GAsyncResult *res, + gboolean *mute, + guint8 *nchannels, + guint16 **volume, + GError **error) +{ + SpicePulsePrivate *p = SPICE_PULSE(audio)->priv; + struct stream *pstream = (is_playback) ? &p->playback : &p->record; + GSimpleAsyncResult *simple = (GSimpleAsyncResult *) res; + + g_return_val_if_fail(g_simple_async_result_is_valid(res, + G_OBJECT(audio), pulse_stream_restore_info_async), FALSE); + + if (g_simple_async_result_propagate_error(simple, error)) { + return FALSE; + } + + if (mute != NULL) { + *mute = (pstream->info.mute) ? TRUE : FALSE; + } + + if (nchannels != NULL) { + *nchannels = pstream->info.channel_map.channels; + } + + if (volume != NULL) { + gint i; + *volume = g_new(guint16, pstream->info.channel_map.channels); + for (i = 0; i < pstream->info.channel_map.channels; i++) { + (*volume)[i] = MIN(pstream->info.volume.values[i], G_MAXUINT16); + SPICE_DEBUG("(%s) volume at channel %d is %u", + (is_playback) ? "playback" : "record", i, (*volume)[i]); + } + } + + return g_simple_async_result_get_op_res_gboolean(simple); +} + +static void spice_pulse_get_playback_volume_info_async(SpiceAudio *audio, + GCancellable *cancellable, + SpiceMainChannel *main_channel, + GAsyncReadyCallback callback, + gpointer user_data) +{ + pulse_stream_restore_info_async(TRUE, audio, cancellable, main_channel, callback, user_data); +} + +static gboolean spice_pulse_get_playback_volume_info_finish(SpiceAudio *audio, + GAsyncResult *res, + gboolean *mute, + guint8 *nchannels, + guint16 **volume, + GError **error) +{ + return pulse_stream_restore_info_finish(TRUE, audio, res, mute, + nchannels, volume, error); +} + +static void spice_pulse_get_record_volume_info_async(SpiceAudio *audio, + GCancellable *cancellable, + SpiceMainChannel *main_channel, + GAsyncReadyCallback callback, + gpointer user_data) +{ + pulse_stream_restore_info_async(FALSE, audio, cancellable, main_channel, callback, user_data); +} + +static gboolean spice_pulse_get_record_volume_info_finish(SpiceAudio *audio, + GAsyncResult *res, + gboolean *mute, + guint8 *nchannels, + guint16 **volume, + GError **error) +{ + return pulse_stream_restore_info_finish(FALSE, audio, res, mute, + nchannels, volume, error); +} -- 2.1.0 _______________________________________________ Spice-devel mailing list <a href="mailto:Spice-devel@lists.freedesktop.org">Spice-devel@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/spice-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/spice-devel</a> </div></div></blockquote></div> </div><div class="gmail_extra"> </div><div class="gmail_extra"> -- <div class="gmail_signature">Marc-André Lureau</div> </div></div>