[Spice-devel] [PATCH spice-gtk v2 1/2] spice-session: new signal for notifying on a significant change in mm-time

[Marc-André Lureau]

On Tue, Apr 30, 2013 at 8:24 PM, Yonit Halperin <yhalperi at redhat.com> wrote:

> mm-time can change after migration. This can cause video synchronization
> problems after migration if not handled appropriately (see rhbz#951664).
> ---
>  gtk/spice-session.c | 29 +++++++++++++++++++++++++++++
>  gtk/spice-session.h |  3 ++-
>  2 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/gtk/spice-session.c b/gtk/spice-session.c
> index f46ac01..765fb48 100644
> --- a/gtk/spice-session.c
> +++ b/gtk/spice-session.c
> @@ -115,6 +115,7 @@ enum {
>  enum {
>      SPICE_SESSION_CHANNEL_NEW,
>      SPICE_SESSION_CHANNEL_DESTROY,
> +    SPICE_SESSION_MM_TIME_RESET,
>      SPICE_SESSION_LAST_SIGNAL,
>  };
>
> @@ -1071,6 +1072,24 @@ static void
> spice_session_class_init(SpiceSessionClass *klass)
>                       SPICE_TYPE_CHANNEL);
>
>      /**
> +     * SpiceSession::mm-time-reset:
> +     * @session: the session that emitted the signal
> +     *
> +     * The #SpiceSession::mm-time-reset is emitted when we identify
> discontinuity in mm-time
> +     *
> +     * Since 0.20
> +     **/
> +    signals[SPICE_SESSION_MM_TIME_RESET] =
> +        g_signal_new("mm-time-reset",
> +                     G_OBJECT_CLASS_TYPE(gobject_class),
> +                     G_SIGNAL_RUN_FIRST,
> +                     G_STRUCT_OFFSET(SpiceSessionClass, mm_time_reset),
> +                     NULL, NULL,
> +                     g_cclosure_marshal_VOID__VOID,
> +                     G_TYPE_NONE,
> +                     0);
> +
> +    /**
>       * SpiceSession:read-only:
>       *
>       * Whether this connection is read-only mode.
> @@ -1927,16 +1946,26 @@ guint32 spice_session_get_mm_time(SpiceSession
> *session)
>      return s->mm_time + (g_get_monotonic_time() - s->mm_time_at_clock) /
> 1000;
>  }
>
> +#define MM_TIME_DIFF_RESET_THRESH 5000 // 5 sec
>

5 seconds seems rather large. Why not something much smaller, like 0.5 sec?

+
>  G_GNUC_INTERNAL
>  void spice_session_set_mm_time(SpiceSession *session, guint32 time)
>  {
>      SpiceSessionPrivate *s = SPICE_SESSION_GET_PRIVATE(session);
> +    guint32 old_time;
>
>      g_return_if_fail(s != NULL);
>
> +    old_time = spice_session_get_mm_time(session);
> +
>      s->mm_time = time;
>      s->mm_time_at_clock = g_get_monotonic_time();
>      SPICE_DEBUG("set mm time: %u", spice_session_get_mm_time(session));
> +    if (time > old_time + MM_TIME_DIFF_RESET_THRESH ||
> +        old_time > time + MM_TIME_DIFF_RESET_THRESH) {
>

imho, time < old_time shouldn't need any threshold.


> +        SPICE_DEBUG("%s: mm-time-reset, old %u, new %u", __FUNCTION__,
> old_time, s->mm_time);
> +        g_signal_emit(session, signals[SPICE_SESSION_MM_TIME_RESET], 0);
>

We have to be careful about coroutine context when emiting signals. Since
this function is called from coroutine, we can should check if the current
context is main/leader or if it has a caller / running a coroutine. Perhaps
the easier would be to augment emit_main_context() macro to handle this in
all cases.


> +    }
>  }
>
>  G_GNUC_INTERNAL
> diff --git a/gtk/spice-session.h b/gtk/spice-session.h
> index b07f525..86056e2 100644
> --- a/gtk/spice-session.h
> +++ b/gtk/spice-session.h
> @@ -74,13 +74,14 @@ struct _SpiceSessionClass
>      /* signals */
>      void (*channel_new)(SpiceSession *session, SpiceChannel *channel);
>      void (*channel_destroy)(SpiceSession *session, SpiceChannel *channel);
> +    void (*mm_time_reset)(SpiceSession *session);
>
>
The class handler isn't strictly necessary (it is used for derived class
who want to override the default class handler)
I think we shouldn't add it.


>      /*< private >*/
>      /*
>       * If adding fields to this struct, remove corresponding
>       * amount of padding to avoid changing overall struct size
>       */
> -    gchar _spice_reserved[SPICE_RESERVED_PADDING];
> +    gchar _spice_reserved[SPICE_RESERVED_PADDING - sizeof(void *)];
>  };
>
>  GType spice_session_get_type(void);
> --
> 1.8.1.4
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>



-- 
Marc-André Lureau
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20130501/a2f9c958/attachment.html>