[Spice-devel] [PATCH spice-gtk 3/3] widget: set keypress-delay to 0 on unix socket

[Frediano Ziglio]

> 
> Hi
> 
> ----- Original Message -----
> > Hi
> > 
> > ----- Original Message -----
> > > > 
> > > > From: Marc-André Lureau <marcandre.lureau at redhat.com>
> > > > 
> > > > There is no strong need for keypress-delay on local connection (not
> > > > verified: unless the system is heavily loaded, in which case the VM
> > > > will
> > > > probably be stuck too and may or not repeat the key when running).
> > > > 
> > > > The benefit of removing keypress-delay is that games or interfaces that
> > > > require "realtime" responses, such as FPS, are slightly better without
> > > > the 100ms input delay.
> > > > 
> > > 
> > > The same apply to tcp connection so I would just set this to 0,
> > > not only for unix sockets.
> > > Or perhaps set to a value (like 10?) that causes no problem.
> > > Considering that if network is quite busy the kernel should collapse
> > > tcp packets I would prefer to set this delay to 0.
> > 
> > The keypress-delay and SPICE_INPUTS_CAP_SCANCODE were introduced to fix key
> > repeat that happen quite easily over the network.
> > 
> > We use TCP_NODELAY, not sure it's always a good idea then.
> > 
> > So we can't blindly remove the keypress-delay, and I don't have a better
> > solution to avoid spurious key repeats over the network when typing atm.
> 
> Just to clarify, keypress-delay is not used to delay all key events, but
> rather to wait for the next "key-release" event. As soon as it comes, the
> input event "press-release" is sent.  (see keypress-delay documentation)
> 

I know. The issue with real network is caused by latency, more precisely on
latency variance and not average. This as if latency would be constant the
press/release would be just skewed and key duration would be the same on both
sides (guest and client).
Delaying the press and sending press+release at the same time fix the different
duration caused by latency change in the usually typing case where a key is
released after being press without intermixed pressions/releases.
However this post the press to the release event reducing the duration to
0.
It appears some game (in my experience OpenArena) handle this 0 duration as
no key (at least during the menu).
I'm not saying your patch does not fix the issue, just that it fixes it
partially in the case of unix sockets where the latency and variance are
near to 0.

However I think for game experience 100ms is a too big value for this delay,
remote connection or not.

About TCP_NODELAY this is a long story. It surely could help decreasing
latency but without buffering or any other advice to sockets can increase
quite a lot bandwidth usage. As long as kernel has space in the queue it
keeps sending packets (even for a single byte) to the network stack.
If queues are full it starts collapsing packets and optimizing them
reducing number of packets.

With which game(s) you are experiencing this issue?
Does reducing the default delay helps?

> > 
> > > 
> > > > Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> > > > ---
> > > >  src/spice-widget.c | 25 ++++++++++++++++++++++++-
> > > >  1 file changed, 24 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/src/spice-widget.c b/src/spice-widget.c
> > > > index 46e0e49..67e66b7 100644
> > > > --- a/src/spice-widget.c
> > > > +++ b/src/spice-widget.c
> > > > @@ -97,6 +97,8 @@ enum {
> > > >      SPICE_DISPLAY_LAST_SIGNAL,
> > > >  };
> > > >  
> > > > +#define DEFAULT_KEYPRESS_DELAY 100
> > > > +
> > > >  static guint signals[SPICE_DISPLAY_LAST_SIGNAL];
> > > >  
> > > >  #ifdef G_OS_WIN32
> > > > @@ -2181,7 +2183,7 @@ static void
> > > > spice_display_class_init(SpiceDisplayClass
> > > > *klass)
> > > >          (gobject_class, PROP_KEYPRESS_DELAY,
> > > >           g_param_spec_uint("keypress-delay", "Keypress delay",
> > > >                             "Keypress delay",
> > > > -                           0, G_MAXUINT, 100,
> > > > +                           0, G_MAXUINT, DEFAULT_KEYPRESS_DELAY,
> > > >                             G_PARAM_READWRITE |
> > > >                             G_PARAM_CONSTRUCT |
> > > >                             G_PARAM_STATIC_STRINGS));
> > > > @@ -2658,6 +2660,25 @@ static void cursor_reset(SpiceCursorChannel
> > > > *channel,
> > > > gpointer data)
> > > >      gdk_window_set_cursor(window, NULL);
> > > >  }
> > > >  
> > > > +static void inputs_channel_event(SpiceChannel *channel,
> > > > SpiceChannelEvent
> > > > event,
> > > > +                                 gpointer data)
> > > > +{
> > > > +    SpiceDisplay *display = data;
> > > > +    guint delay = DEFAULT_KEYPRESS_DELAY;
> > > > +    GSocket *sock;
> > > > +
> > > > +    if (event != SPICE_CHANNEL_OPENED)
> > > > +        return;
> > > > +
> > > > +    g_object_get(channel, "socket", &sock, NULL);
> > > > +    if (g_socket_get_family(sock) == G_SOCKET_FAMILY_UNIX) {
> > > > +        delay = 0;

You can probably call spice_display_set_keypress_delay to avoid setting
the delay to the default again.

> > > > +    }
> > > > +    g_object_unref(sock);
> > > > +
> > > > +    spice_display_set_keypress_delay(display, delay);
> > > > +}
> > > > +
> > > >  #ifndef G_OS_WIN32
> > > >  G_GNUC_INTERNAL
> > > >  void spice_display_widget_gl_scanout(SpiceDisplay *display)
> > > > @@ -2795,6 +2816,8 @@ static void channel_new(SpiceSession *s,
> > > > SpiceChannel
> > > > *channel, gpointer data)
> > > >      if (SPICE_IS_INPUTS_CHANNEL(channel)) {
> > > >          d->inputs = SPICE_INPUTS_CHANNEL(channel);
> > > >          spice_channel_connect(channel);
> > > > +        spice_g_signal_connect_object(channel, "channel-event",
> > > > +
> > > > G_CALLBACK(inputs_channel_event),
> > > > display, 0);
> > > >          return;
> > > >      }
> > > >  
> > > 

Frediano