This is great! I was debugging the same problem with duplicated keystrokes for Cygwin/XFree86 over two years ago. I traced a single Windows key down message as low as I could in the server, then hit continue and magically two key strokes would appear in an X Client.
I have an easily reproducible case in Cygwin/XFree86 that I use to test this behavior: I run the "Native GDI" engine that Alan Hourihane and I wrote, which only provides the spans routines implemented using GDI calls, thus it is very slow. I open an xterm in this mode, do an 'ls' of /usr/X11R6/bin, and during the 10 seconds it takes to paint the listing I type 'exit'. This comes out at 'exxit' or 'exiit', etc. 9 times out of 10.
I applied your patch and it fixed the problem. I am now distributing this in the default Cygwin/XFree86 distribution.
I didn't notice any immediate adverse effects, but I will let you know if the bug reports start rolling in.
The only suggestion I have is that the function prototypes in include/os.h should follow the conventions of all other prototypes in os.h, using #if NeedFunctionPrototypes:
extern void SetScreenSaverTimer( #if NeedFunctionPrototypes void #endif );
#ifdef DPMSExtension extern void SetDPMSTimers( #if NeedFunctionPrototypes void #endif ); #endif
Thanks for taking the time to debug this properly... I hope that there aren't any major reasons why this isn't the proper way to fix the problem.
Harold
Ivan Pascal wrote:
Hello,
Trying to solve the problem reported in Bugzilla #301 I found that the root of the problem is an order of timeout processing in WaitForSomething subroutine.
The reported problem is that under some conditions Xserver duplicates key press events. For example if one run setxkbmap program it can happen that the Enter key pressed as the end of the command line produces two key press events.
It happens when XKB uses a software emulated autorepeat and a machine is relatively weak or is heavy loaded and so some request performing consumes much time.
When XKB emulates an autorepeat in a software it uses Xserver's timers. each key press sets a timer and a key release cancels the action the timer has to do when it is expired. If no key release event comes before the timer expires a timers callback function emulates a key press event.
The problem is that WaitForSomething checks timers (and runs thir callbacks) twice. Once before it calls select and the second time after a select finishing and a reading of input devices events.
It works correctly when an Xserver is 'almost idle' and sometime processes short (in time) requests. In such case it spends most time sleeping on a select call and even the autorepeat timer expires near the time a real key is released, WaitForSomething code after a select finishing firstly reads keyboard events and cancels uneeded emulated autorepeat events.
But if a server performs a request such as 'load new keyboard map' that consumes much time things become worse. During that time it can happens that the key pressed before the request processing start is already released but the autorepeat timer is already expired too. When the server calls WaitForSomething it firstly checks timers and although the key is already released and a corresponded event is ready to be read the subroutine knows nothing about this event until it calls Select and reads input devices. Therefore it runs callback function that emits autorepeated key press event and when WaitForSomething comes to Select and then to a real key events reading the real key release event can't cancel wrongly emited autorepeat event.
The solution is obvious. WaitForSomething should not run any callbacks before the Select but just check timers and if there are expired timers call select with a zero timeout. Also the code before the select checks screensaver and DPMS timeouts. I don't see reason why those timeouts are processed separately from timers and don't use timers facility. (Strictly speaking to process those timeouts before the select is incorrect too. Because some timeouts can expire and the subroutine can run a screensaver or switch DPMS although some input device events are alreday happened and will be read immediately when WaitForSomteting comes to Select.)
I made a patch that changes the first timers check as I said and moves the screensaver and DPMS timeouts processing into timers and their callbacks. But I don't know why the first timers check was put before the select (except for a making a positive timeout for the select call) and why screensaver and DPMS don't use timers. And I can't guess can my patch cause any problems somewhere else. Please review it.
------------------------------------------------------------------------
--- xc/programs/Xserver/os/WaitFor.c.orig Sat Sep 20 19:07:25 2003
+++ xc/programs/Xserver/os/WaitFor.c Sat Sep 20 19:07:34 2003
@@ -131,17 +131,12 @@
* pClientsReady is an array to store ready client->index values into.
*****************/
-static INT32 timeTilFrob = 0; /* while screen saving */
-
int
WaitForSomething(int *pClientsReady)
{
int i;
struct timeval waittime, *wt;
INT32 timeout = 0;
-#ifdef DPMSExtension
- INT32 standbyTimeout = 0, suspendTimeout = 0, offTimeout = 0;
-#endif
fd_set clientsReadable;
fd_set clientsWritable;
int curclient;
@@ -188,138 +183,17 @@
else
{
#endif
-#ifdef DPMSExtension
- if (ScreenSaverTime > 0 || DPMSEnabled || timers)
-#else
- if (ScreenSaverTime > 0 || timers)
-#endif
- now = GetTimeInMillis();
- wt = NULL;
+ wt = NULL;
if (timers)
- {
- while (timers && (int) (timers->expires - now) <= 0)
- DoTimer(timers, now, &timers);
- if (timers)
- {
- timeout = timers->expires - now;
- waittime.tv_sec = timeout / MILLI_PER_SECOND;
- waittime.tv_usec = (timeout % MILLI_PER_SECOND) *
- (1000000 / MILLI_PER_SECOND);
- wt = &waittime;
- }
- }
- if (ScreenSaverTime > 0
-#ifdef DPMSExtension
- || (DPMSEnabled &&
- (DPMSStandbyTime > 0 || DPMSSuspendTime > 0 || DPMSOffTime > 0))
-#endif
- ) {
-#ifdef DPMSExtension
- if (ScreenSaverTime > 0)
-#endif
- timeout = (ScreenSaverTime -
- (now - lastDeviceEventTime.milliseconds));
-#ifdef DPMSExtension
- if (DPMSStandbyTime > 0)
- standbyTimeout = (DPMSStandbyTime -
- (now - lastDeviceEventTime.milliseconds));
- if (DPMSSuspendTime > 0)
- suspendTimeout = (DPMSSuspendTime -
- (now - lastDeviceEventTime.milliseconds));
- if (DPMSOffTime > 0)
- offTimeout = (DPMSOffTime -
- (now - lastDeviceEventTime.milliseconds));
-#endif /* DPMSExtension */
-
- if (
- timeout <= 0
-#ifdef DPMSExtension
- && ScreenSaverTime > 0
-#endif /* DPMSExtension */
- ) {
- INT32 timeSinceSave;
-
- timeSinceSave = -timeout;
- if (timeSinceSave >= timeTilFrob && timeTilFrob >= 0)
- {
- ResetOsBuffers(); /* not ideal, but better than nothing */
- SaveScreens(SCREEN_SAVER_ON, ScreenSaverActive);
-#ifdef DPMSExtension
- if (ScreenSaverInterval > 0 &&
- DPMSPowerLevel == DPMSModeOn)
-#else
- if (ScreenSaverInterval)
-#endif /* DPMSExtension */
- /* round up to the next ScreenSaverInterval */
- timeTilFrob = ScreenSaverInterval *
- ((timeSinceSave + ScreenSaverInterval) /
- ScreenSaverInterval);
- else
- timeTilFrob = -1;
- }
- timeout = timeTilFrob - timeSinceSave;
- }
- else
- {
- if (ScreenSaverTime > 0 && timeout > ScreenSaverTime)
- timeout = ScreenSaverTime;
- timeTilFrob = 0;
- }
-#ifdef DPMSExtension
- if (DPMSEnabled)
- {
- if (standbyTimeout > 0 - && (timeout <= 0 || timeout > standbyTimeout))
- timeout = standbyTimeout;
- if (suspendTimeout > 0 - && (timeout <= 0 || timeout > suspendTimeout))
- timeout = suspendTimeout;
- if (offTimeout > 0 - && (timeout <= 0 || timeout > offTimeout))
- timeout = offTimeout;
- }
-#endif
- if (timeout > 0 && (!wt || timeout < (int) (timers->expires - now)))
- {
- waittime.tv_sec = timeout / MILLI_PER_SECOND;
- waittime.tv_usec = (timeout % MILLI_PER_SECOND) *
- (1000000 / MILLI_PER_SECOND);
- wt = &waittime;
- }
-#ifdef DPMSExtension
- /* don't bother unless it's switched on */
- if (DPMSEnabled) {
- /*
- * If this mode's enabled, and if the time's come
- * and if we're still at a lesser mode, do it now.
- */
- if (DPMSStandbyTime > 0) {
- if (standbyTimeout <= 0) {
- if (DPMSPowerLevel < DPMSModeStandby) {
- DPMSSet(DPMSModeStandby);
- }
- }
- }
- /*
- * and ditto. Note that since these modes can have the
- * same timeouts, they can happen at the same time.
- */
- if (DPMSSuspendTime > 0) {
- if (suspendTimeout <= 0) {
- if (DPMSPowerLevel < DPMSModeSuspend) {
- DPMSSet(DPMSModeSuspend);
- }
- }
- }
- if (DPMSOffTime > 0) {
- if (offTimeout <= 0) {
- if (DPMSPowerLevel < DPMSModeOff) {
- DPMSSet(DPMSModeOff);
- }
- }
- }
- }
-#endif
+ {
+ now = GetTimeInMillis();
+ timeout = timers->expires - now;
+ if (timeout < 0)
+ timeout = 0;
+ waittime.tv_sec = timeout / MILLI_PER_SECOND;
+ waittime.tv_usec = (timeout % MILLI_PER_SECOND) *
+ (1000000 / MILLI_PER_SECOND);
+ wt = &waittime;
}
XFD_COPYSET(&AllSockets, &LastSelectMask);
#ifdef SMART_SCHEDULE
@@ -646,3 +520,108 @@
xfree(timer);
}
}
+
+static CARD32
+ScreenSaverTimeoutExpire(OsTimerPtr timer,CARD32 now,pointer arg)
+{
+ INT32 timeout = now - lastDeviceEventTime.milliseconds;
+
+ if (timeout < ScreenSaverTime) {
+ return ScreenSaverTime - timeout;
+ }
+
+ ResetOsBuffers(); /* not ideal, but better than nothing */
+ SaveScreens(SCREEN_SAVER_ON, ScreenSaverActive);
+
+#ifdef DPMSExtension
+ if (ScreenSaverInterval > 0 && DPMSPowerLevel == DPMSModeOn)
+#else
+ if (ScreenSaverInterval > 0)
+#endif /* DPMSExtension */
+ return ScreenSaverInterval;
+
+ return 0;
+}
+
+static OsTimerPtr ScreenSaverTimer = NULL;
+
+void
+SetScreenSaverTimer(void)
+{
+ if (ScreenSaverTime > 0) {
+ ScreenSaverTimer = TimerSet(ScreenSaverTimer, 0, ScreenSaverTime,
+ ScreenSaverTimeoutExpire, NULL);
+ } else if (ScreenSaverTimer) {
+ TimerFree(ScreenSaverTimer);
+ ScreenSaverTimer = NULL;
+ }
+}
+
+#ifdef DPMSExtension
+
+static OsTimerPtr DPMSStandbyTimer = NULL;
+static OsTimerPtr DPMSSuspendTimer = NULL;
+static OsTimerPtr DPMSOffTimer = NULL;
+
+static CARD32
+DPMSStandbyTimerExpire(OsTimerPtr timer,CARD32 now,pointer arg)
+{
+ INT32 timeout = now - lastDeviceEventTime.milliseconds;
+
+ if (timeout < DPMSStandbyTime) {
+ return DPMSStandbyTime - timeout;
+ }
+ if (DPMSPowerLevel < DPMSModeStandby) {
+ DPMSSet(DPMSModeStandby);
+ }
+ return DPMSStandbyTime;
+}
+
+static CARD32
+DPMSSuspendTimerExpire(OsTimerPtr timer,CARD32 now,pointer arg)
+{
+ INT32 timeout = now - lastDeviceEventTime.milliseconds;
+
+ if (timeout < DPMSSuspendTime) {
+ return DPMSSuspendTime - timeout;
+ }
+ if (DPMSPowerLevel < DPMSModeSuspend) {
+ DPMSSet(DPMSModeSuspend);
+ }
+ return DPMSSuspendTime;
+}
+
+static CARD32
+DPMSOffTimerExpire(OsTimerPtr timer,CARD32 now,pointer arg)
+{
+ INT32 timeout = now - lastDeviceEventTime.milliseconds;
+
+ if (timeout < DPMSOffTime) {
+ return DPMSOffTime - timeout;
+ }
+ if (DPMSPowerLevel < DPMSModeOff) {
+ DPMSSet(DPMSModeOff);
+ }
+ return DPMSOffTime;
+}
+
+void
+SetDPMSTimers(void)
+{
+ if (!DPMSEnabled)
+ return;
+
+ if (DPMSStandbyTime > 0) {
+ DPMSStandbyTimer = TimerSet(DPMSStandbyTimer, 0, DPMSStandbyTime,
+ DPMSStandbyTimerExpire, NULL);
+ }
+ if (DPMSSuspendTime > 0) {
+ DPMSStandbyTimer = TimerSet(DPMSSuspendTimer, 0, DPMSSuspendTime,
+ DPMSSuspendTimerExpire, NULL);
+ }
+ if (DPMSOffTime > 0) {
+ DPMSOffTimer = TimerSet(DPMSOffTimer, 0, DPMSOffTime,
+ DPMSOffTimerExpire, NULL);
+ }
+}
+#endif
--- xc/programs/Xserver/include/os.h.orig Sat Sep 20 19:33:39 2003
+++ xc/programs/Xserver/include/os.h Sat Sep 20 19:34:32 2003
@@ -220,6 +220,12 @@
extern void TimerFree(OsTimerPtr /* pTimer */);
+extern void SetScreenSaverTimer(void);
+
+#ifdef DPMSExtension
+extern void SetDPMSTimers(void);
+#endif
+
extern SIGVAL AutoResetServer(int /*sig*/);
extern SIGVAL GiveUp(int /*sig*/);
--- xc/programs/Xserver/dix/main.c.orig Sat Sep 20 19:36:09 2003
+++ xc/programs/Xserver/dix/main.c Sat Sep 20 19:36:31 2003
@@ -420,6 +420,9 @@
InitRootWindow(WindowTable[i]);
DefineInitialRootWindow(WindowTable[0]);
SaveScreens(SCREEN_SAVER_FORCER, ScreenSaverReset);
+#ifdef DPMSExtension
+ SetDPMSTimers();
+#endif
#ifdef PANORAMIX
if (!noPanoramiXExtension) {
--- xc/programs/Xserver/dix/window.c.orig Sat Sep 20 19:36:18 2003
+++ xc/programs/Xserver/dix/window.c Sat Sep 20 19:36:39 2003
@@ -3487,6 +3487,8 @@
}
}
screenIsSaved = what;
+ if (mode == ScreenSaverReset)
+ SetScreenSaverTimer();
}
static Bool
_______________________________________________ Devel mailing list [EMAIL PROTECTED] http://XFree86.Org/mailman/listinfo/devel
