[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 11:16 AM, Marc-André Lureau wrote:
>
>
>
> On Thu, May 9, 2013 at 5:06 PM, Yonit Halperin <yhalperi at redhat.com
> <mailto:yhalperi at redhat.com>> wrote:
>
>     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>
>         <mailto: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.
>
>
> The threshold is a simple heuristic to detect timer discontinuity, in my
> view.
>
> But when time goes backward although it is supposed to be monotonic,
> there isn't any need for a threshold.
>
I understand you point. Practically, for the lipsync, it shouldn't 
matter much. But I don't mind changing it to have no threshold.
>
>              +        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.
>
>
>
> There is no facility today to send signal on main/leader (g_signal_emit)
> or coroutine (emit_main_context) depending on the context automatically.
> So far, we have avoided sending signaling in both contexts. Using
> emit_main_context() from main context will at least produce some
> warnings, and the delayed emission might have undesired result depending
> on coroutine backend.
>
> One solution would be to check if the current context is main/leader or
> if it has a caller coroutine ("coroutine context"). Perhaps the easier
> would be to augment emit_main_context() macro to handle this...
I'll do it. Does it matter that with pulse backend, updating the mm-time 
is actually done from pulse audio main loop?
>
> --
> Marc-André Lureau