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

[Marc-André Lureau]

On Thu, May 9, 2013 at 5:06 PM, Yonit Halperin <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>> 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.


>>     +        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...

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