wayland: Implement key repetition

[mcoffin]

NOTE: This still needs cleanup. I'm just getting the base implementation out there to get comments before I spend a bunch of time cleaning it up.

This implements key repitition for wayland. I could use some comments on the high-level approach, as there are plenty of cleanup tasks that came from code-churn while debugging.

I wound up having to pass an EventsLoopProxy handle through the wl_seat and wl_keyboard implementations so that the underlying timer event thread would be able to wake up the EventsLoop that's reading from the sink.

Comments appreciated. I'm using this locally with jwilm/alacritty (with a few patches to update glutin to use this version of winit), and it seems to work alright. The threading overhead is a bit of a bummer, but it seems to be fairly minimal.

TODO List

• Audit + fix error handling with internal queues + async wakeups of EventsLoop.
• Cleanup (read: unwrap) the HashMap usage for storing state in the timer thread, since we only track one key repeat at a time (I didn't know this initially until I actually played around with it in Xorg)
• Break up the monolith of nesting that occurs inside KeyboardIData::new
• Think about the currently hard-coded channel capacities to make sure they're sensible.
• Dynamically configure the tokio_timer::Timer implementation based on the current RepeatInfo config. (This will just allow for more efficiency in the timer thread, and potentially more granular repeat intervals. 10ms is a decent preset, but dynamic-configuration is probably more correct).

[mcoffin]

ping @vberger per talking to tomaka on IRC

[elinorbgr]

Thanks a lot for tackling this!

I don't have much time right now, and being unfamiliar with tokio it'll take me some time to correctly review this. So this may have to wait for next week for a proper review.

I gave a quick glance and saw nothing catching my eye negatively. But I want to be sure that @tomaka is okay with the added dependency on tokio?

[tomaka]

I don't mind adding a dependency on tokio, as long as none of the types of tokio or futures are re-exported in the API.

[tomaka]

Please do not randomly bump the version in winit.

[mcoffin]

Oh oops. Forgot I'd committed that in the PR. I'll remove it. I did it when I was working on getting alacritty to use my branch.

[Ralith]

Due to the use of tokio-timer and a dedicated tokio thread, this adds two new background threads. Is that really necessary? Given that every supported platform should support blocking for events with a timeout, this should be feasible to implement with no extraneous threads at all.

[mcoffin]

@Ralith What thread do you suppose we should block while waiting for the timeout event? I think you're going to need one additional thread (the timer thread) regardless.

Now, if we wrote our own timer implementation, we could potentially teach the timer thread to block and not run any timer operations while waiting on the lock for the EventsLoopSink, but that's going to (hopefully not, but potentially) screw up the nice timing of the keyboard events. At least this way, if the system gets bogged down somehow, the timer should emit events in to the buffer channel for the repeated keyboard events, which will get processed (back on the event thread).

[mcoffin]

After talking on IRC, @Ralith is totally right. Here's the chat log if anybody cares.

17:32 mcoffin You around?
17:33 Ralith yep
17:35 mcoffin Saw your comment on my PR for winit. I think you're going to need one thread (the timer thread) regardless
17:36 Ralith why do you think that?
17:37 mcoffin And the reason I chose to have the second tokio processing thread was in case the application event loop ever gets bogged down (low system resources, etc). Then the repeat events will still successfully buffer if the timer thread is operating properly. Now, if we write our own timer implementtion with knowlege of exactly the kind of timers we want, we might be able to get the async processing and timer
17:37 mcoffin thread to be one and the same (but we'd have to implement some kind of notification wakeup for it for when a new event comes in from wayland and we're in a waiting state)
17:38 mcoffin Whereas with that implementation from the PR, we still get semi-well-timed keyboard repeats, even if they're not getting processed immediately
17:39 mcoffin How exactly were you thinking it could be done without a timer thread at all?
17:39 Ralith I'm not sure why a single additional thread would be any more vulnerable to bogging down than two additional threads, notwithstanding the point that no additional threads at all should be needed
17:40 mcoffin If it can be done with 0, then definitely let me know what you're thinking there, because I didn't see a way to do that immediately, and I agree that's the better sol'n
17:41 Ralith the same way it's always done; use the same logic, but replace any sleeps with time-limited calls to wait for events
17:41 Ralith (though the actual complexity of tokio-timer logic is almost certainly unwarranted here)
17:43 mcoffin A bunch of that apparent complexity came about from trying to emulate X11's repeat behavior to the T
17:43 Ralith I'm talking about the complexity of tokio-timer itself, not your code
17:44 Ralith tokio-timer is designed for handling (hundreds of) thousands of concurrent timers, not a handful
17:44 mcoffin Ahh, yes, agreed.
17:44 Ralith tokio has simple threadless timers built-in for normal cases
17:44 Ralith (but tokio is still unnecessary here)
17:44 mcoffin Oh, awesome. I'm an idiot, could you shoot me a link haha?
17:45 Ralith https://docs.rs/tokio-core/0.1.11/tokio_core/reactor/struct.Interval.html https://docs.rs/tokio-core/0.1.11/tokio_core/reactor/struct.Timeout.html
17:45 Ralith in your defense, the tokio docs are very byzantine
17:45 mcoffin ^ Cool. TIL.
17:46 Ralith but, again, even tokio alone is unnecessary here, because you can get the desired behavior by checking keyboard state and time elapsed in the backend's poll_events function, and inserting an appropriate timeout in the blocking call at the heart of run_forever
17:47 mcoffin Oooh, I see what you're saying now.
17:47 Ralith using more or less exactly the same logic the above tokio primitives reduce down to, except with the wayland call to fetch events at the heart of it instead of epoll
17:47 mcoffin But that's then going to move a bunch of that code in to the Wayland EventsLoop implementation, no?
17:48 mcoffin gotcha gotcha...
17:48 mcoffin Too bad that EventsLoop didn't just support those primitives from the beginning ;). I see what you're saying now though. I'll play around with it.
17:49 Ralith \o/
17:49 Ralith and yeah, arguably EventsLoop should be refactored around a fn poll_event(timeout: Duration) -> Option<Event> primitive or something
17:50 Ralith I believe that's blocked by #237
17:52 mcoffin So, I do have one concern with that implementation though. Say we have a running repeat, and some other event comes through. What happens to the scheduled repeat? Do we have to re-block with a different timeout? Also: is it assertable that event processing time is negligible enough that users won't notice the subtle differences is keyboard repeat times?
17:54 Ralith the set of upcoming key repeats is a queue that is maintained within the backend as long-lived state
17:55 mcoffin So how far out do you schedule those? All of them based off the timing of the first? Schedule each one as soon as the last is processed?
17:56 mcoffin totally sees why Wayland left this to the client implementations now.
17:57 Ralith every time you're going to block, you block until the next repeat would happen; if the call times out you yield the repeat, otherwise you yield the returned event
17:57 Ralith when not blocking, just check if the current time is > the time of the earliest scheduled repeat
17:57 Ralith if true, return that repeat immediately
17:58 Ralith you don't really need much in the way of clever scheduling since it's a simple, strictly linear queue
17:58 Ralith with one entry for each key that is currently being held
17:59 Ralith event processing time and so forth are moot, because all events are processed serially regardless
18:00 mcoffin ^ I see. Yea that makes sense
18:01 Ralith if the user takes ten seconds to get around to checking for events, nothing is going to work properly :P
18:01 mcoffin Event processing time could be non-negiligible though, rendering a, say, 210ms delay between each repeat when wayland specified 200ms, but I'd agree that's not worth the extra threading + complexity.
18:02 mcoffin I only mention just because it's infuriating when holding down a key in an application for a length of time doesn't render as many repeats as in a different implementation
18:02 Ralith that doesn't matter regardless
18:02 Ralith whether you use threads or not has absolutely no bearing on that
18:02 Ralith all that the application code sees is a linear sequence of events
18:02 Ralith you do not need threads to come up with such a sequence under any circumstances
18:03 mcoffin I keep forgetting that the application isn't running any responses on the EventsLoop thread
18:04 Ralith I'm not sure what that means
18:06 mcoffin I just wrote a response and talked myself in a circle. Got it now on how that would work too, you just wouldn't block the next time around, and would have to store the time that the repeat was supposed to happen and check with current time if you need to emit another repeat immediately
18:06 mcoffin got it
18:06 mcoffin Thanks for the help haha. I'll see what I can come up with for a re-start on that
18:06 mcoffin Thanks :)
18:07 Ralith no problem, thanks for working on this!

[tomaka]

(just approving my change request about the version bump, so that it's not a blocker)

[elinorbgr]

@mcoffin @Ralith blocking with timeout on the wayland socket is going to be... complicated. The wayland C libs only offer two ways of reading the socket:

• blocking indefinitely
• reading pending data in a non-blocking way

There is no provided way to block with timeout.

However, we can take inspiration of what the weston apps do to handle key repeat info: they use a timerfd and epoll. They register both the timerfd and the wayland fd to an epoll watch, and process each accordingly by respectively dispatching key repeat events or doing an non-blocking read&dispatch of the wayland socket.

I think we can adapt the wayland EventsLoop in winit to do that too, but it'll likely require some serious refactor. (though I believe it'd mostly affect the EventsLoop logic only, not the whole backend).

[Ralith]

epoll+timerfd seems like overkill here; if wayland gives you a fd, then you can just use poll and supply it a timeout directly. That should be simple, and it's not obvious to me why it wouldn't be able to easily substitute in for any existing "block indefinitely" call.

[elinorbgr]

I'm not very familiar with poll mechanisms tbh, but if we can easily block with timeout on a given fd for read-readiness, then yes, that'd absolutely work.

[mcoffin]

This is going to be a complete re-write of this obviously, so you guys are welcome to close this and I can re-open when I get the time to flesh it out.

[embik]

@mcoffin Hey there, are you still working on this feature?

[trimental]

Smithay/client-toolkit@1691329 Wanted to mention that upstream work has been done for this.

[elinorbgr]

This has been superseded by #628