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

[Yonit Halperin]

On 05/09/2013 10:46 AM, Marc-André Lureau wrote:
>
>
>
> On Wed, May 1, 2013 at 4:19 PM, Yonit Halperin <yhalperi at redhat.com
> <mailto: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 | 39 +++++++++++++++++++++++++++++++++++++++
>       1 file changed, 39 insertions(+)
>
>     diff --git a/gtk/spice-session.c b/gtk/spice-session.c
>     index f46ac01..1b5a2f2 100644
>     --- a/gtk/spice-session.c
>     +++ b/gtk/spice-session.c
>     @@ -115,11 +115,23 @@ enum {
>       enum {
>           SPICE_SESSION_CHANNEL_NEW,
>           SPICE_SESSION_CHANNEL_DESTROY,
>     +    SPICE_SESSION_MM_TIME_RESET,
>           SPICE_SESSION_LAST_SIGNAL,
>       };
>
>       static guint signals[SPICE_SESSION_LAST_SIGNAL];
>
>     +/* main context */
>     +static void do_emit_main_context(GObject *object, int signum,
>     gpointer params)
>     +{
>     +    switch (signum) {
>     +    case SPICE_SESSION_MM_TIME_RESET:
>     +        g_signal_emit(object, signals[signum], 0);
>     +        break;
>     +    default:
>     +        g_warn_if_reached();
>     +    }
>     +}
>
>       static void update_proxy(SpiceSession *self, const gchar *str)
>       {
>     @@ -1071,6 +1083,23 @@ 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,
>     +                     0, NULL, NULL,
>     +                     g_cclosure_marshal_VOID__VOID,
>     +                     G_TYPE_NONE,
>     +                     0);
>     +
>     +    /**
>            * SpiceSession:read-only:
>            *
>            * Whether this connection is read-only mode.
>     @@ -1927,16 +1956,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 500 // 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) {
>
>
> If time goes backward, I think we should reset mm-time without threshold..
I don't see why it should be different.
>
>     +        SPICE_DEBUG("%s: mm-time-reset, old %u, new %u",
>     __FUNCTION__, old_time, s->mm_time);
>     +        emit_main_context(session, SPICE_SESSION_MM_TIME_RESET);
>
>
> This looks problematic, as this function is called from main and
> coroutine context. We need to handle that, perhaps like I proposed in
> previous review
mmm...I thought this is what you proposed in the previous review, to 
replace g_signal_emit, with emit_main_context. If not, please explain 
the problem.
>
>     +    }
>       }
>
>       G_GNUC_INTERNAL
>     --
>     1.8.1.4
>
>     _______________________________________________
>     Spice-devel mailing list
>     Spice-devel at lists.freedesktop.org
>     <mailto:Spice-devel at lists.freedesktop.org>
>     http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
>
>
> --
> Marc-André Lureau