Hi, thanks for the review! On Tue, Apr 21, 2015 at 05:26:06PM +0200, Christophe Fergeau wrote: > On Mon, Apr 20, 2015 at 11:56:16AM +0200, Victor Toso wrote: > > 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: https://bugzilla.redhat.com/show_bug.cgi?id=1012868 > > --- > > gtk/spice-pulse.c | 484 +++++++++++++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 475 insertions(+), 9 deletions(-) > > > > diff --git a/gtk/spice-pulse.c b/gtk/spice-pulse.c > > index c583032..8c53475 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->playback.name); > > + g_free(p->record.name); > > You should set these 2 variables to NULL after freeing them as we are in > _dispose. > Sure, g_clear_pointer then; > > + > > 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,37 @@ 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) > > + goto context_fail; > > + 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 +908,416 @@ SpicePulse *spice_pulse_new(SpiceSession *session, GMainContext *context, > > goto error; > > } > > > > + p->playback.name = g_strconcat("sink-input-by-application-name:", > > + g_get_application_name(), NULL); > > + p->record.name = 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->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; > > + > > + 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; > > + g_return_if_fail(task != NULL); > > + > > +#if GLIB_CHECK_VERSION(2,40,0) > > + free_async_task(task); > > +#else > > + /* This must be done now otherwise pulseaudio may return to a > > + * cancelled task operation before free_async_task is called */ > > + if (task->pa_op != NULL) { > > + pa_operation_cancel(task->pa_op); > > + pa_operation_unref(task->pa_op); > > + task->pa_op = NULL; > > + } > > + > > + /* Clear the pending_restore_task reference to avoid triggering a > > + * pa_operation when context state is in READY state */ > > + if (task->pulse->priv->pending_restore_task == task) { > > + task->pulse->priv->pending_restore_task = NULL; > > + } > > + > > + /* FIXME: https://bugzilla.gnome.org/show_bug.cgi?id=705395 > > + * 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) || > > For what it's worth, I generally tend to avoid comparing a boolean to > TRUE/FALSE (especially TRUE) but I tend to favor if (foo) if (!foo) as > there is no real boolean in C, but rather 0, and != 0. In this case, as > you set the is_playback value yourself, it's fine to do the comparison. > Okay > > + (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; > > + > > + g_return_if_fail(task != NULL); > > + 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; > > + GList *it; > > + > > + g_return_if_fail(pulse != NULL); > > + p = pulse->priv; > > + > > + /* Complete all tasks in list */ > > + for(it = p->results; it != NULL; it = it->next) { > > + struct async_task *task = it->data; > > + complete_task(pulse, task, err_msg); > > + free_async_task(task); > > + } > > + g_list_free(p->results); > > + p->results = NULL; > > + 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)) { > > + /* 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->playback.name) == 0) { > > + pstream = &p->playback; > > + } else if (g_strcmp0(info->name, p->record.name) == 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->info.name = 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->info.name = 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->info.name = pstream->name; > > + pstream->info.mute = info->mute; > > + pstream->info.channel_map = info->channel_map; > > + pstream->info.volume = info->volume; > > +} > > It seems source_output_info_cb/source_output_info_cb could be combined > with just > struct stream *pstream = (is_playback) ? &p->playback : &p->record; > as the difference as is down elsewhere in that patch. > Indeed but as they are callbacks I thought it would be better to have each function with your callback; also, pa_sink_input_info not equal to pa_source_output_info. I can have a function to avoid duplication code as I did in the other part of the patch as well, do you think it is better? > > + > > +/* 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 we already have retrieved volume-info from Pulse database then it is > > + * safe to return the volume-info we already have in <stream>info */ > > + > > + 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->playback.info.name != NULL || > > + p->record.info.name != NULL) { > > + /* If the pstream->info.name 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; > > Could also be !!pstream->info.mute, your way is probably more readable. > > > + } > > + > > + 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]); > > + } > > + } > > I don't think changes are needed right now, but when APIs use out > args and return newly alloc'ed memory, it's in my opinion better to make > sure this alloc'ed out arg is always set to NULL in case the caller > unconditionnally wants to call g_free() on it. Here if there is an > error, the returned 'volume' will not be set, so it cannot be blindly > g_free'ed > Okay, I'll fix this as well. Thanks! > Patch looks good otherwise. > > Christophe > Many thanks, > > + > > + 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 > > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > > http://lists.freedesktop.org/mailman/listinfo/spice-devel _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel