Horrendous keyboard lag in games that compounds with time pressed

[oleksijderkach]

It's as if scrcpy places "key pressed" events in a queue with every other event, which quickly clogs up and doesn't let other events through. E.g.: open any game that supports keyboard and press W for a brief moment and at the same time, try to swipe a screen. It goes through. Then, press W for ten seconds and then try to swipe the screen. Touch input goes through after a few seconds(!). And so on: input lag is proportional to the time spent pressing a keyboard button.

EDIT: Only some games have this issue. From what I have on my phone, Minecraft and Survivalcraft exhibit this problem while Sonic 1 does not. Apologies if this issue is not scrcpy's fault

[rom1v]

Hi,

Thank you for your report.

I just tested a random game (minetest) on a tablet, and indeed I can reproduce (a bit) the problem.

However, I added some logs, which show all key and touch events are transmitted and injected immediately by scrcpy.

After some delay (sometimes a few seconds), the game reacts to the events, but there are no new event injected at that time. So it seems the device/game takes time to consume the events.

Not sure if there are possible workarounds on our side.

[fefo-dev]

Maybe related to the issue, but I get a similar problem randomly sometimes on normal use. scrcpy still inputs everything as expected in the end, backspaces and all, but stuff like parsing a long link can take almost a minute.

For the record, I'm using the latest release with the --prefer-text modifier, but I remember this happening before the text fix.

[rom1v]

I just did another test: inject key events by pressing a key for several seconds in scrcpy, then touch with a finger on the physical device. The touch events consumption is also delayed.

[xeropresence]

After looking into #109 i think the issues are related.

With this small change xeropresence@3d52cd5

games instantly start working correctly, however this has a side-effect where now text apps would not work as expected where if you hold a key down it would start repeating.

Does disabling key-repeating make the most sense?
When would key-repeating be the desired scenario other then testing if keys were being sent?
Perhaps a shortcut combo to toggle between key-repeating and not repeating?

[rom1v]

@xeropresence thank you 👍

By default, letters are sent as key events (instead of text events), unless --prefer-text is passed (see #87), so repeated events must be sent. Instead, maybe we should send letters as text events by default, and add an option --game-mode or similar, where letters are passed as key events and not repeated…

However, this will cause the same issue for other keys like Space and Enter (keeping space pressed should inject several spaces).

I will think about it.

[xeropresence]

I had tried prefer-text before this and character would move but then stop, so i then started looking at what was going on.

Do you know what the actual behavior is on android when holding down a key via a physical keyboard? If my understanding of the current system is correct, isn't the key repeating a hack since we are sending a keydown event, shouldn't that mean it's the responsibility of whatever app to repeat the key?

On the server-side, can you check if the virtual keyboard is open, or even better if a text input is active? If you can then it would make sense to not process the duplicate keydown events when either of those cases was false.

[rom1v]

shouldn't that mean it's the responsibility of whatever app to repeat the key?

That is what I just tested, but it seems it does not (if you disable repeat on the client side, it does not repeat letters or space).

On the server-side, can you check if the virtual keyboard is open, or even better if a text input is active?

No (and it would be too hacky).

Anyway, I think adding a command-line option --no-key-repeat would be acceptable. What do you think?

[xeropresence]

Anyway, I think adding a command-line option --no-key-repeat would be acceptable. What do you think?

My thought was if key repeating did not occur with a physical keyboard, then that would be a big part of if repeating would be enabled by default or not.

I'm not a huge fan of it being only a flag since you may want to switch between repeating mode and non repeating mode and it would be silly to have to restart the program with different flags each time.

That is what I just tested, but it seems it does not (if you disable repeat on the client side, it does not repeat letters or space).

So, I was reading the android documentation,

https://developer.android.com/reference/android/view/KeyEvent

is scrcpy incrementing the repeat counter on events past the first one? It seems like for every event past the initial keydown, repeat should be incremented by one, where as the value for the keyup is always 0.

[rom1v]

On the device side, scrcpy always injects the events with repeat = 0:

scrcpy/server/src/main/java/com/genymobile/scrcpy/Controller.java

Line 134 in 3c0fc8f

return device.injectKeyEvent(action, keycode, 0, metaState);

On the client-side, SDL only reports repeat as a boolean (either 0 or 1). It is only used for ignoring repeated events on shortcuts, and is not forwarded to the device.

It would be interesting to know if passing the actual repeat value would solve the keyboard lag.

[xeropresence]

That's what im wondering, can you store a counter, unordered_map<key,int>, increment every time sdl repeat is true, wipe on repeat is false, and send the counter with the key press event?

[rom1v]

unordered_map<key,int>

(This is C++) 😄

A simple counter should be sufficient (not one per key, in practice only one key may be repeated at the same time).

Do you want to work on that to see if it improves things?

Here is a list of things to do:

• add a int repeat field control_msg.h (in control_msg.inject_key_codes struct) and ControlMessage.java
• adapt control_msg_serialize() and parseInjectKeycode()
• pass the actual repeat value in convert_input_key()

[xeropresence]

I guess testing with one number would be a start, but if that does resolve the issue then it would be best to adhere to the spec and have the repeat value track each individual key. I guess an array would suffice since were working with only c.

I'll give it a shot, doesn't sound too bad.

[rom1v]

I guess an array would suffice since were working with only c.

A single unsigned int is sufficient: if the current event has repeat enabled, increment the counter, otherwise, reset it to 0.

[xeropresence]

I was thinking tracking the count inside input_manager.h, i'm reading thru the code now. I miss protobuff already.

[rom1v]

I miss protobuff already.

#1289 (comment)

[xeropresence]

I guess an array would suffice since were working with only c.

A single unsigned int is sufficient: if the current event has repeat enabled, increment the counter, otherwise, reset it to 0.

https://github.com/Genymobile/scrcpy/blob/master/app/src/input_manager.c#L414
This is looks like it's freshly allocated each time. We need someplace to store the counter then the field inside the struct should get updated, correct?

[rom1v]

Yes, for exemple, you could just add a field unsigned key_repeat_count; in struct input_manager.

Add pass this count to convert_input_key().

[xeropresence]

Setting repeat properly does seem to alleviate the lag.

Looking at the logging, I don't think i completely understand how sending multiple keys down at the same time is handled, figure it be best to report the findings before I delve any deeper. (It's this lack of understanding as to why im suggesting an array in this implementation)

int repeatCounter[AKEYCODE_ALL_APPS];
static bool
convert_input_key(const SDL_KeyboardEvent *from, struct control_msg *to,
                  bool prefer_text, bool repeat) {
    to->type = CONTROL_MSG_TYPE_INJECT_KEYCODE;

    if (!convert_keycode_action(from->type, &to->inject_keycode.action)) {
        return false;
    }

    uint16_t mod = from->keysym.mod;
    if (!convert_keycode(from->keysym.sym, &to->inject_keycode.keycode, mod,
                         prefer_text)) {
        return false;
    }

    if (to->inject_keycode.action == AKEY_EVENT_ACTION_DOWN && repeat)
    {
        repeatCounter[to->inject_keycode.keycode] = repeatCounter[to->inject_keycode.keycode] + 1;
        //LOGW("Setting repeat mode to %d %d",to->inject_keycode.keycode,to->inject_keycode.repeat );
    }
    else
    {
        //LOGW("Clearing repeate for %d",to->inject_keycode.keycode );
        repeatCounter[to->inject_keycode.keycode] = 0;
    }


    to->inject_keycode.repeat = repeatCounter[to->inject_keycode.keycode];
    //LOGW("Current code for %d ======== %d",to->inject_keycode.keycode,to->inject_keycode.repeat );

    to->inject_keycode.metastate = convert_meta_state(mod);

    return true;
}

this was quick and dirty to test and see if it even worked, so what do you think the best way to proceed would be?

I did some testing, and setting repeat to one or zero based on sdl repeat did not resolve the issue, so it seems that the repeat counter incrementing is integral to the issue being resolved.

Also, from the documentation https://developer.android.com/reference/android/view/KeyEvent#getRepeatCount()

Retrieve the repeat count of the event. For key down events, this is the number of times the key has repeated with the first down starting at 0 and counting up from there. For key up events, this is always equal to zero. For multiple key events, this is the number of down/up pairs that have occurred.

Not sure how this should be handled.

[xeropresence]

I don't think this matches the documentation 100% either, as the sdl fires multiple keydown events before repeat is set to true, so the counter is technically off by a few.

[rom1v]

Do you have a branch with your changes?

[xeropresence]

I'll push it now.

[xeropresence]

https://github.com/xeropresence/scrcpy/tree/repeatCounter

[rom1v]

Thank you 👍

So I guess the issue is not related to repeat, but only to the rate of events that the device is not able to inject at the same rate. It's just that repeated events produce a lot of events in a small amount of time.

(IIRC there is also the same problem with multitouch events on a touchscreen.)

I'm not a huge fan of it being only a flag since you may want to switch between repeating mode and non repeating mode and it would be silly to have to restart the program with different flags each time.

If your device lags on successive events, I think it's ok to always pass --no-key-repeat, and never reenable it. This would be the only purpose of the flag.

[xeropresence]

If your device lags on successive events, I think it's ok to always pass --no-key-repeat, and never reenable it. This would be the only purpose of the flag.

Are events being dropped when the repeat counter > 0 and events come in with a higher count?

An issue with this is that there are some keys you would want to repeat, ie backspace,delete, arrow keys,peroid(...), maybe space. I play games, but also send text messages, snapchat, etc where those repeats would be desirable.

If tracking the repeat count resolves the issue, and better matches what the documentation expects, im in favor of that over adding a flag or a shortcut.

[rom1v]

If tracking the repeat count resolves the issue

Yes, but apparently, it does not.

An issue with this is that there are some keys you would want to repeat, ie backspace,delete, arrow keys,peroid(...), maybe space.

Isn't there a lag when you press those keys repeated on your device?

[xeropresence]

With my branch, there is a slightly delay in games when the key is released, but it is only slightly longer then when I was disabling repeats entirely, which is still much faster then using the v1.14
I think that might be due to where multiple keydowns are being sent without sdl setting repeat to true.

Outside of games, ie in snapchat I don't notice any lag.

[rom1v]

which is still much faster then using the v1.14

Oh, so it improves the situation? The difference is reproducible after several tests switching back and forth between v1.14 and this branch?

I think that might be due to where multiple keydowns are being sent without sdl setting repeat to true.

I can't reproduce that.

[rom1v]

Just to clarify my position: in any case, repeat should be correctly initialized for injecting events on the device. So unless it causes problems, this change should be done anyway.

[xeropresence]

Sounds good. I'm currently trying to do some testing to quantify the results of the change using recording.

[xeropresence]

Ok, so i modified my branch to limit a maximum of 40 repeat events to be generated. After 40 events were reached a fake keyup event was sent and all further events were ignored until an actual keyup event occurred.

Repeat with a hardcoded 0 value, v1.14
https://streamable.com/jlhi14

Repeat incrementing
https://streamable.com/d7l6bv

Not sending repeat at all
https://streamable.com/f2f5or

I also did some additional testing with stardew valley. Stardew valley is much less cpu, gpu intensive and displayed no noticeable difference between versions until I went and locked the cpu frequency to its smallest possible value. Once the cpu frequency was limited, there was no difference between the hardcoded 0 value and the incrementing version. Both versions exhibited a noticeable delay when stopping. The version where repeat keys were not sent at all exhibited no delay.

In conclusion:
Incrementing the repeat value is either beneficial in scenarios where the cpu is somewhat bound or when the app ignores incoming repeat values

Not sending repeat keys offers the best performance, but will inhibit scenarios where the user does want the keys to actually be repeated, ie (delete,backspace, arrow keys, etc)

I think a flag is good, but a shortcut is better so that you can disable repeats, play games, then when you swap to sending messages you don't need to restart the program.

[rom1v]

OK, so first let's inject the correct repeat value. :)

I think a flag is good, but a shortcut is better so that you can disable repeats, play games, then when you swap to sending messages you don't need to restart the program.

The issue is that a stateful toggle flag without visual feedback is confusing. That's why I haven't added one for --prefer-text.

Oh, also, please implement on dev (the development branch), not master: https://github.com/Genymobile/scrcpy/blob/master/BUILD.md#branches

[xeropresence]

I've rebased my branch onto dev.

Sounds like a OSD could be useful for displaying information about state changes.

[rom1v]

Sounds like a OSD could be useful for displaying information about state changes.

(see #1484)

But even with that, I'm not sure I want to consume a shortcut for such an advanced feature (which is only necessary on some devices, as a workaround to avoid sending too many events).

[xeropresence]

What other changes would you like to see, just using a single unsigned int for tracking the count?

[rom1v]

just using a single unsigned int for tracking the count?

Yes, and resetting it if and only if the current event has repeat set to false (otherwise, increment it).
Please also rebase on current dev and open a PR :)

Thank you 😉

[rom1v]

So using v1.15 with --no-key-repeat should fix this issue. I'm closing.