<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Thu, May 9, 2013 at 5:06 PM, Yonit Halperin <<a href="mailto:yhalperi@redhat.com" target="_blank">yhalperi@redhat.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im">On 05/09/2013 10:46 AM, Marc-André Lureau wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im"> On Wed, May 1, 2013 at 4:19 PM, Yonit Halperin <<a href="mailto:yhalperi@redhat.com" target="_blank">yhalperi@redhat.com</a> </div><div><div class="h5"> <mailto:<a href="mailto:yhalperi@redhat.com" target="_blank">yhalperi@redhat.com</a>>> 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.. </div></div></blockquote> I don't see why it should be different.<div class="im"> </div></blockquote><div> </div><div>The threshold is a simple heuristic to detect timer discontinuity, in my view. </div><div>But when time goes backward although it is supposed to be monotonic, there isn't any need for a threshold. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + 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 </blockquote></div> 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. </blockquote><div> 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... </div></div> -- Marc-André Lureau </div></div>