Re: [spice-gtk PATCH v9 4/4] agent: sync guest audio with client values

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Tue, Apr 21, 2015 at 05:36:38PM +0200, Christophe Fergeau wrote:
> On Mon, Apr 20, 2015 at 11:56:18AM +0200, Victor Toso wrote:
> > Functions to sync volume and mute value when necessary. In this patch,
> > only one sync is allowed after agent connect.
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1012868
> > ---
> >  gtk/channel-main.c       | 141 +++++++++++++++++++++++++++++++++++++++++++++++
> >  gtk/spice-session-priv.h |   2 +-
> >  2 files changed, 142 insertions(+), 1 deletion(-)
> > 
> > diff --git a/gtk/channel-main.c b/gtk/channel-main.c
> > index 82169aa..4d16c07 100644
> > --- a/gtk/channel-main.c
> > +++ b/gtk/channel-main.c
> > @@ -30,6 +30,7 @@
> >  #include "spice-util-priv.h"
> >  #include "spice-channel-priv.h"
> >  #include "spice-session-priv.h"
> > +#include "spice-audio-priv.h"
> >  
> >  /**
> >   * SECTION:channel-main
> > @@ -109,6 +110,10 @@ struct _SpiceMainChannelPrivate  {
> >      guint                       migrate_delayed_id;
> >      spice_migrate               *migrate_data;
> >      int                         max_clipboard;
> > +
> > +    bool                        agent_volume_playback_sync;
> > +    bool                        agent_volume_record_sync;
> > +    GCancellable                *cancellable_volume_info;
> >  };
> >  
> >  struct spice_migrate {
> > @@ -187,6 +192,7 @@ static const char *agent_msg_types[] = {
> >      [ VD_AGENT_CLIPBOARD_GRAB          ] = "clipboard grab",
> >      [ VD_AGENT_CLIPBOARD_REQUEST       ] = "clipboard request",
> >      [ VD_AGENT_CLIPBOARD_RELEASE       ] = "clipboard release",
> > +    [ VD_AGENT_AUDIO_VOLUME_SYNC       ] = "volume-sync",
> >  };
> >  
> >  static const char *agent_caps[] = {
> > @@ -201,6 +207,7 @@ static const char *agent_caps[] = {
> >      [ VD_AGENT_CAP_GUEST_LINEEND_LF    ] = "line-end lf",
> >      [ VD_AGENT_CAP_GUEST_LINEEND_CRLF  ] = "line-end crlf",
> >      [ VD_AGENT_CAP_MAX_CLIPBOARD       ] = "max-clipboard",
> > +    [ VD_AGENT_CAP_AUDIO_VOLUME_SYNC   ] = "volume-sync",
> >  };
> >  #define NAME(_a, _i) ((_i) < SPICE_N_ELEMENTS(_a) ? (_a[(_i)] ?: "?") : "?")
> >  
> > @@ -231,6 +238,7 @@ static void spice_main_channel_init(SpiceMainChannel *channel)
> >      c = channel->priv = SPICE_MAIN_CHANNEL_GET_PRIVATE(channel);
> >      c->agent_msg_queue = g_queue_new();
> >      c->file_xfer_tasks = g_hash_table_new(g_direct_hash, g_direct_equal);
> > +    c->cancellable_volume_info = g_cancellable_new();
> >  
> >      spice_main_channel_reset_capabilties(SPICE_CHANNEL(channel));
> >  }
> > @@ -346,6 +354,9 @@ static void spice_main_channel_dispose(GObject *obj)
> >          c->migrate_delayed_id = 0;
> >      }
> >  
> > +    g_cancellable_cancel(c->cancellable_volume_info);
> > +    g_clear_object(&c->cancellable_volume_info);
> > +
> >      if (G_OBJECT_CLASS(spice_main_channel_parent_class)->dispose)
> >          G_OBJECT_CLASS(spice_main_channel_parent_class)->dispose(obj);
> >  }
> > @@ -413,6 +424,9 @@ static void spice_main_channel_reset(SpiceChannel *channel, gboolean migrating)
> >      agent_free_msg_queue(SPICE_MAIN_CHANNEL(channel));
> >      c->agent_msg_queue = g_queue_new();
> >  
> > +    c->agent_volume_playback_sync = false;
> > +    c->agent_volume_record_sync = false;
> > +
> >      set_agent_connected(SPICE_MAIN_CHANNEL(channel), FALSE);
> >  
> >      SPICE_CHANNEL_CLASS(spice_main_channel_parent_class)->channel_reset(channel, migrating);
> > @@ -1092,6 +1106,128 @@ gboolean spice_main_send_monitor_config(SpiceMainChannel *channel)
> >      return TRUE;
> >  }
> >  
> > +static void audio_playback_volume_info_cb(GObject *object, GAsyncResult *res, gpointer user_data)
> > +{
> > +    SpiceMainChannel *main_channel = user_data;
> > +    SpiceSession *session = spice_channel_get_session(SPICE_CHANNEL(main_channel));
> > +    SpiceAudio *audio = spice_audio_get(session, NULL);
> > +    VDAgentAudioVolumeSync *avs;
> > +    guint16 *volume;
> > +    guint8 nchannels;
> > +    gboolean mute, ret;
> > +    gsize array_size;
> > +    GError *error = NULL;
> > +
> > +    ret = SPICE_AUDIO_GET_CLASS(audio)->get_playback_volume_info_finish(audio,
> > +                                                                        res,
> > +                                                                        &mute,
> > +                                                                        &nchannels,
> > +                                                                        &volume,
> > +                                                                        &error);
> > +    if (ret == FALSE || volume == NULL || nchannels == 0) {
> > +        if (error != NULL) {
> > +            spice_warning("Failed to get playback async volume info: %s", error->message);
> > +            g_error_free (error);
> > +        } else {
> > +            SPICE_DEBUG("Failed to get playback async volume info");
> > +        }
> > +        main_channel->priv->agent_volume_playback_sync = false;
> > +        return;
> > +    }
> > +
> > +    array_size = sizeof(uint16_t) * nchannels;
> > +    avs = g_malloc0(sizeof(VDAgentAudioVolumeSync) + array_size);
> > +    avs->is_playback = TRUE;
> > +    avs->mute = mute;
> > +    avs->nchannels = nchannels;
> > +    memcpy(avs->volume, volume, array_size);
> > +
> > +    SPICE_DEBUG ("%s mute=%s nchannels=%u volume[0]=%u",
> > +                 __func__, spice_yes_no(mute), nchannels, volume[0]);
> > +    g_clear_pointer (&volume, g_free);
> 
> g_free(volume); would be enough here.

Sure!

> > +    agent_msg_queue(main_channel, VD_AGENT_AUDIO_VOLUME_SYNC,
> > +                    sizeof(VDAgentAudioVolumeSync) + array_size, avs);
> > +}
> > +
> > +static void agent_sync_audio_playback(SpiceMainChannel *main_channel)
> > +{
> > +    SpiceSession *session = spice_channel_get_session(SPICE_CHANNEL(main_channel));
> > +    SpiceAudio *audio = spice_audio_get(session, NULL);
> > +    SpiceMainChannelPrivate *c = main_channel->priv;
> > +
> > +    if (!test_agent_cap(main_channel, VD_AGENT_CAP_AUDIO_VOLUME_SYNC) ||
> > +        c->agent_volume_playback_sync == true) {
> > +        SPICE_DEBUG("%s - is not going to sync audio with guest", __func__);
> > +        return;
> > +    }
> > +    /* only one per connection */
> > +    g_cancellable_reset(c->cancellable_volume_info);
> > +    c->agent_volume_playback_sync = true;
> > +    SPICE_AUDIO_GET_CLASS(audio)->get_playback_volume_info_async(audio,
> > +            c->cancellable_volume_info, main_channel, audio_playback_volume_info_cb, main_channel);
> 
> I tend to wrap such vfunc calls into more readable
> spice_audio_get_playback_volume_info_async() wrappers which just do the
> vfunc call.
> 

I agree it is more readable to wrap it. I'll fix this as well.

> > +}
> > +
> > +static void audio_record_volume_info_cb(GObject *object, GAsyncResult *res, gpointer user_data)
> > +{
> > +    SpiceMainChannel *main_channel = user_data;
> > +    SpiceSession *session = spice_channel_get_session(SPICE_CHANNEL(main_channel));
> > +    SpiceAudio *audio = spice_audio_get(session, NULL);
> > +    VDAgentAudioVolumeSync *avs;
> > +    guint16 *volume;
> > +    guint8 nchannels;
> > +    gboolean ret, mute;
> > +    gsize array_size;
> > +    GError *error = NULL;
> > +
> > +    ret = SPICE_AUDIO_GET_CLASS(audio)->get_record_volume_info_finish(audio,
> > +                                                                      res,
> > +                                                                      &mute,
> > +                                                                      &nchannels,
> > +                                                                      &volume,
> > +                                                                      &error);
> > +    if (ret == FALSE || volume == NULL || nchannels == 0) {
> > +        if (error != NULL) {
> > +            spice_warning ("Failed to get record async volume info: %s", error->message);
> > +            g_error_free (error);
> > +        } else {
> > +            SPICE_DEBUG("Failed to get record async volume info");
> > +        }
> > +        main_channel->priv->agent_volume_record_sync = false;
> 
> Any reason to use false rather than FALSE? There are a few other
> true/false in this patch.

Not really, I tend to use false for bool and FALSE for gboolean :-)
It is better to keep this as gboolean and use FALSE as well.

Thanks,

> 
> Christophe
> 
> > +        return;
> > +    }
> > +
> > +    array_size = sizeof(uint16_t) * nchannels;
> > +    avs = g_malloc0(sizeof(VDAgentAudioVolumeSync) + array_size);
> > +    avs->is_playback = FALSE;
> > +    avs->mute = mute;
> > +    avs->nchannels = nchannels;
> > +    memcpy(avs->volume, volume, array_size);
> > +
> > +    SPICE_DEBUG ("%s mute=%s nchannels=%u volume[0]=%u",
> > +                 __func__, spice_yes_no(mute), nchannels, volume[0]);
> > +    g_clear_pointer (&volume, g_free);
> > +    agent_msg_queue(main_channel, VD_AGENT_AUDIO_VOLUME_SYNC,
> > +                    sizeof(VDAgentAudioVolumeSync) + array_size, avs);
> > +}
> > +
> > +static void agent_sync_audio_record(SpiceMainChannel *main_channel)
> > +{
> > +    SpiceSession *session = spice_channel_get_session(SPICE_CHANNEL(main_channel));
> > +    SpiceAudio *audio = spice_audio_get(session, NULL);
> > +    SpiceMainChannelPrivate *c = main_channel->priv;
> > +
> > +    if (!test_agent_cap(main_channel, VD_AGENT_CAP_AUDIO_VOLUME_SYNC) ||
> > +        c->agent_volume_record_sync == true) {
> > +        SPICE_DEBUG("%s - is not going to sync audio with guest", __func__);
> > +        return;
> > +    }
> > +    /* only one per connection */
> > +    g_cancellable_reset(c->cancellable_volume_info);
> > +    c->agent_volume_record_sync = true;
> > +    SPICE_AUDIO_GET_CLASS(audio)->get_record_volume_info_async(audio,
> > +            c->cancellable_volume_info, main_channel, audio_record_volume_info_cb, main_channel);
> > +}
> > +
> >  /* any context: the message is not flushed immediately,
> >     you can wakeup() the channel coroutine or send_msg_queue() */
> >  static void agent_display_config(SpiceMainChannel *channel)
> > @@ -1345,6 +1481,8 @@ static void agent_start(SpiceMainChannel *channel)
> >      };
> >      SpiceMsgOut *out;
> >  
> > +    c->agent_volume_playback_sync = false;
> > +    c->agent_volume_record_sync = false;
> >      c->agent_caps_received = false;
> >      set_agent_connected(channel, TRUE);
> >  
> > @@ -1801,6 +1939,9 @@ static void main_agent_handle_msg(SpiceChannel *channel,
> >              c->agent_display_config_sent = true;
> >          }
> >  
> > +        agent_sync_audio_playback(self);
> > +        agent_sync_audio_record(self);
> > +
> >          agent_max_clipboard(self);
> >  
> >          agent_send_msg_queue(self);
> > diff --git a/gtk/spice-session-priv.h b/gtk/spice-session-priv.h
> > index 46938ff..049973a 100644
> > --- a/gtk/spice-session-priv.h
> > +++ b/gtk/spice-session-priv.h
> > @@ -98,7 +98,7 @@ PhodavServer* channel_webdav_server_new(SpiceSession *session);
> >  guint spice_session_get_n_display_channels(SpiceSession *session);
> >  void spice_session_set_main_channel(SpiceSession *session, SpiceChannel *channel);
> >  gboolean spice_session_set_migration_session(SpiceSession *session, SpiceSession *mig_session);
> > -
> > +SpiceAudio *spice_audio_get(SpiceSession *session, GMainContext *context);
> >  G_END_DECLS
> >  
> >  #endif /* __SPICE_CLIENT_SESSION_PRIV_H__ */
> > -- 
> > 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





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]