[Spice-devel] [spice-gtk PATCH v9 2/4] audio: spice-pulse implement async volume-info
Christophe Fergeau
cfergeau at redhat.com
Tue Apr 21 08:26:06 PDT 2015
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.
> +
> 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.
> + (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.
> +
> +/* 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
Patch looks good otherwise.
Christophe
> +
> + 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150421/e5a489fe/attachment-0001.sig>
More information about the Spice-devel
mailing list