gnif / LookingGlass

An extremely low latency KVMFR (KVM FrameRelay) implementation for guests with VGA PCI Passthrough.
GNU General Public License v2.0
4.66k stars 257 forks source link

[client] wayland: extra CAPS_LOCK button event sent #974

Closed Leo1998 closed 2 years ago

Leo1998 commented 2 years ago

I recently switched to wayland and came across this really weird bug when using vim inside the Windows VM (Caps Lock is remapped to escape). When typing certain key-combinations there is an extra CAPS_LOCK key event sent when pressing the shift key.

In the following example im pressing: a -> CAPS_LOCK -> SHIFT + a The events in the Windows VM are: vm The events on the linux host: vm2

From debugging i found out that the extra key event is produced in wayland/input.c:278 when app_handleKeyboardLEDs is called. If i remove this call the issue is fixed for me.

General questions:

I confirmed this issue is happening on bleeding edge and B5.0.1

Xyene commented 2 years ago

Why are we calling app_handleKeyboardLEDs when the shift key is pressed?

It was much easier to slam the right LED state into the VM whenever it could possibly have changed, rather than try to remember what the state is within the VM (which would also be susceptible to USB-passthrough keyboards being able to change the LED state without Looking Glass knowing -- making Looking Glass query the LED state is even more complicated).

I think what's happening here is you're confusing the Windows Spice agent with your remap.

If I had to guess, without your remap, when you press CapsLock:

  1. LG gets a wl_keyboard.key event and sends a CapsLock event to the VM. Windows turns the Caps Lock LED on.
  2. LG gets a wl_keyboard.modifier event that says Caps Lock is on, and tells the Spice agent that the Caps Lock LED should be on.
  3. The Spice agent sees the Caps Lock LED is on, and does nothing.

With your remap, when you press CapsLock:

  1. LG gets a wl_keyboard.key event and sends a CapsLock event to the VM. This gets remapped to Escape. The Caps Lock LED does not turn on.
  2. LG gets a wl_keyboard.modifier event that says Caps Lock is on, and tells the Spice agent that the Caps Lock LED should be on.
  3. The Spice agent sees the Caps Lock LED is not on, and turns it on. Vim interprets this as CapsLock having been pressed.

...or some variation of the above.

Does app_handleKeyboardLEDs even do anything? When i only press CAPS_LOCK the method isn't even called. It's only invoked when pressing SHIFT for example.

Certainly, it's called whenever we get a modifier event from the compositor. You can run WAYLAND_DEBUG=1 looking-glass-client ... |& grep keyboard. If you don't see a line resembling

[2142616.407] wl_keyboard@43.modifiers(68298, 2, 0, 2, 0)

when you press CapsLock, your compositor just isn't sending it to us (I'd be surprised if that's the case).

Leo1998 commented 2 years ago

Thank you for the quick answer, i have done some more testing on this. If i disable all the remaps in both vm and host everything is working as intended. The problem comes from remapping Esc to CapsLock in the host. The remap in the vm does not change the issue (when not remapped in the vm CapsLock state switches two times).

The following output is generated from the same key-combination as above (with remap in the host): [ 651331.688] wl_keyboard@44.key(2381, 2559116, 30, 1) [ 651431.792] wl_keyboard@44.key(2382, 2559216, 30, 0) [ 651556.664] wl_keyboard@44.key(2383, 2559341, 58, 1) [ 651647.692] wl_keyboard@44.key(2384, 2559432, 58, 0) [ 651766.700] wl_keyboard@44.key(2385, 2559551, 42, 1) [ 651766.756] wl_keyboard@44.modifiers(2386, 1, 0, 0, 0) [ 651894.739] wl_keyboard@44.key(2387, 2559679, 30, 1) [ 652039.685] wl_keyboard@44.key(2388, 2559824, 42, 0) [ 652039.734] wl_keyboard@44.modifiers(2389, 0, 0, 0, 0) [ 652044.707] wl_keyboard@44.key(2390, 2559829, 30, 0)

Without remap in the host: [1592374.474] wl_keyboard@44.key(4431, 3500159, 30, 1) [1592454.364] wl_keyboard@44.key(4432, 3500238, 30, 0) [1592509.509] wl_keyboard@44.key(4433, 3500293, 58, 1) [1592509.565] wl_keyboard@44.modifiers(4434, 2, 0, 2, 0) [1592588.445] wl_keyboard@44.key(4435, 3500372, 58, 0) [1592588.496] wl_keyboard@44.modifiers(4436, 0, 0, 2, 0) [1592710.505] wl_keyboard@44.key(4437, 3500495, 42, 1) [1592710.559] wl_keyboard@44.modifiers(4438, 1, 0, 2, 0) [1592783.610] wl_keyboard@44.key(4439, 3500567, 30, 1) [1592907.391] wl_keyboard@44.key(4440, 3500691, 42, 0) [1592907.439] wl_keyboard@44.modifiers(4441, 0, 0, 2, 0) [1592928.338] wl_keyboard@44.key(4442, 3500712, 30, 0) [1597906.574] -> zwp_keyboard_shortcuts_inhibitor_v1@5

So the issue is pretty much what you already said. CapsLock does not fire a modifier event because of the remap. The CapsLock key is sent to the vm perfectly fine and changes the modifier state. When pressing Shift the modifier event is fired but then changes CapsLock back off by sending another event to the vm.

Another general question: Why do we even need to handle the LED states in such a complicated way on wayland? Without app_handleKeyboardLEDs the CapsLock button event is sent to the vm anyway and changes the LED perfectly fine. In X11 we also don't explicitly handle this and the behaviour is perfectly fine (for me at least).

I know this is really obscure and i might be the only one to ever come across this, but i really want to look into this because i love using looking-glass and this is problem makes it really annoying to use sometimes. (Sorry if i'm wasting your time or being overly complicated)

Xyene commented 2 years ago

Another general question: Why do we even need to handle the LED states in such a complicated way on wayland?

If you toggle CapsLock while LG is not focused, it is reasonable to expect that when you next focus LG and start typing, you will be typing in caps. This wouldn't happen if LG wasn't handling the wl_keyboard.modifiers event. The same goes for NumLock/ScrlLck.

In X11 we also don't explicitly handle this and the behaviour is perfectly fine (for me at least).

The X11 and Wayland backends are maintained by different people (@gnif for X11; @quantum and myself for Wayland), so oftentimes features will first be available in one backend before the other, but they eventually converge.

I know this is really obscure and i might be the only one to ever come across this

I'm sympathetic to the use-case. I'm not convinced it can be made to work, but let's see what we can do.

Your first log, annotated:

[ 651331.688] wl_keyboard@44.key(2381, 2559116, 30, 1)          PRESS a
[ 651431.792] wl_keyboard@44.key(2382, 2559216, 30, 0)          RELEASE a
[ 651556.664] wl_keyboard@44.key(2383, 2559341, 58, 1)          PRESS CapsLock
[ 651647.692] wl_keyboard@44.key(2384, 2559432, 58, 0)          RELEASE CapsLock
[ 651766.700] wl_keyboard@44.key(2385, 2559551, 42, 1)          PRESS Shift
[ 651766.756] wl_keyboard@44.modifiers(2386, 1, 0, 0, 0)        Shift modifier on, CapsLock modifier off sent to VM
[ 651894.739] wl_keyboard@44.key(2387, 2559679, 30, 1)          PRESS a
[ 652039.685] wl_keyboard@44.key(2388, 2559824, 42, 0)          RELEASE Shift
[ 652039.734] wl_keyboard@44.modifiers(2389, 0, 0, 0, 0)        Shift modifier off, CapsLock modifier off sent to VM
[ 652044.707] wl_keyboard@44.key(2390, 2559829, 30, 0)          RELEASE a

Are you remapping CapsLock both in the guest and the host? This log suggests you're definitely doing it in the host (since we don't see a modifiers event following your CapsLock press), but what gets sent to the VM is a CapsLock event, not an Esc (LG ignores the remap for everything but text fields in the overlay UI, because there's no arbitrary VM text injection protocol.)

Xyene commented 2 years ago

This seems to be the culprit: https://gitlab.freedesktop.org/spice/spice/-/commit/ff9c8d4908e12db65a6cd2d61b4ed7b8174e6d78

I think this code is either broken, or doesn't support this use-case. It seems Spice speculatively assumes that a CapsLock key will turn on the caps modifier, and has some sort of error correction mechanism in case QEMU comes back 50ms later to tell it that it did not, in fact, turn on caps. I'm not sure what's supposed to happen to keys sent within those 50ms.

At any rate, it doesn't look like we can do anything about this in LG.

I'd suggest either:

quantum5 commented 2 years ago

Reported as https://gitlab.freedesktop.org/spice/spice/-/issues/68 to Spice.