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

[Marc-André Lureau <marcandre.lureau@xxxxxxxxx>]

On Thu, May 9, 2013 at 5:06 PM, Yonit Halperin <yhalperi@xxxxxxxxxx> wrote:

On 05/09/2013 10:46 AM, Marc-André Lureau wrote:

On Wed, May 1, 2013 at 4:19 PM, Yonit Halperin <yhalperi@xxxxxxxxxx

<mailto:yhalperi@xxxxxxxxxx>> 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

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel