Fix various problems with the timer code.

* SDL_timer_running wasn't always updated correctly.
* Fixed occasional crash in SDL_SetTimer() when clearing threaded timers
* It was possible to get both the timer thread and event thread running
* Other misc. cleanup

--HG--
extra : convert_revision : svn%3Ac70aab31-4412-0410-b14c-859654838e24/trunk%401029

include/SDL_timer.h

@@ -81,6 +81,8 @@ typedef Uint32 (SDLCALL *SDL_TimerCallback)(Uint32 interval);
  * in the same program, as it is implemented using setitimer().  You also
  * should not use this function in multi-threaded applications as signals
  * to multi-threaded apps have undefined behavior in some implementations.
+ *
+ * This function returns 0 if successful, or -1 if there was an error.
  */
 extern DECLSPEC int SDLCALL SDL_SetTimer(Uint32 interval, SDL_TimerCallback callback);
 

src/events/SDL_events.c

@@ -91,7 +91,6 @@ void SDL_Unlock_EventThread(void)
 
 static int SDL_GobbleEvents(void *unused)
 {
-	SDL_SetTimerThreaded(2);
 	event_thread = SDL_ThreadID();
 	while ( SDL_EventQ.active ) {
 		SDL_VideoDevice *video = current_video;
@@ -162,6 +161,8 @@ static int SDL_StartEventThread(Uint32 flags)
 		}
 		SDL_EventLock.safe = 0;
 
+		/* The event thread will handle timers too */
+		SDL_SetTimerThreaded(2);
 		SDL_EventThread = SDL_CreateThread(SDL_GobbleEvents, NULL);
 		if ( SDL_EventThread == NULL ) {
 			return(-1);

src/timer/SDL_timer.c

@@ -45,8 +45,6 @@ int SDL_timer_running = 0;
 Uint32 SDL_alarm_interval = 0;
 SDL_TimerCallback SDL_alarm_callback;
 
-static volatile SDL_bool list_changed = SDL_FALSE;
-
 /* Data used for a thread-based timer */
 static int SDL_timer_threaded = 0;
 
@@ -59,8 +57,8 @@ struct _SDL_TimerID {
 };
 
 static SDL_TimerID SDL_timers = NULL;
-static Uint32 num_timers = 0;
 static SDL_mutex *SDL_timer_mutex;
+static volatile SDL_bool list_changed = SDL_FALSE;
 
 /* Set whether or not the timer should use a thread.
    This should not be called while the timer subsystem is running.
@@ -83,16 +81,19 @@ int SDL_TimerInit(void)
 {
 	int retval;
 
-	SDL_timer_running = 0;
-	SDL_SetTimer(0, NULL);
 	retval = 0;
+	if ( SDL_timer_started ) {
+		SDL_TimerQuit();
+	}
 	if ( ! SDL_timer_threaded ) {
 		retval = SDL_SYS_TimerInit();
 	}
 	if ( SDL_timer_threaded ) {
 		SDL_timer_mutex = SDL_CreateMutex();
 	}
+	if ( retval == 0 ) {
 		SDL_timer_started = 1;
+	}
 	return(retval);
 }
 
@@ -113,43 +114,41 @@ void SDL_ThreadedTimerCheck(void)
 {
 	Uint32 now, ms;
 	SDL_TimerID t, prev, next;
-	int removed;
+	SDL_bool removed;
-	SDL_NewTimerCallback callback;
-	Uint32 interval;
-	void *param;
-
-	now = SDL_GetTicks();
 
 	SDL_mutexP(SDL_timer_mutex);
+	list_changed = SDL_FALSE;
+	now = SDL_GetTicks();
 	for ( prev = NULL, t = SDL_timers; t; t = next ) {
-		removed = 0;
+		removed = SDL_FALSE;
 		ms = t->interval - SDL_TIMESLICE;
 		next = t->next;
-		if ( (t->last_alarm < now) && ((now - t->last_alarm) > ms) ) {
+		if ( (int)(now - t->last_alarm) > (int)ms ) {
+			struct _SDL_TimerID timer;
+
 			if ( (now - t->last_alarm) < t->interval ) {
 				t->last_alarm += t->interval;
 			} else {
 				t->last_alarm = now;
 			}
-			list_changed = SDL_FALSE;
 #ifdef DEBUG_TIMERS
 			printf("Executing timer %p (thread = %d)\n",
 				t, SDL_ThreadID());
 #endif
-			callback = t->cb;
+			timer = *t;
-			interval = t->interval;
-			param = t->param;
 			SDL_mutexV(SDL_timer_mutex);
-			ms = callback(interval, param);
+			ms = timer.cb(timer.interval, timer.param);
 			SDL_mutexP(SDL_timer_mutex);
 			if ( list_changed ) {
-				/* Abort, list of timers has been modified */
+				/* Abort, list of timers modified */
+				/* FIXME: what if ms was changed? */
 				break;
 			}
 			if ( ms != t->interval ) {
 				if ( ms ) {
 					t->interval = ROUND_RESOLUTION(ms);
-				} else { /* Remove the timer from the linked list */
+				} else {
+					/* Remove timer from the list */
 #ifdef DEBUG_TIMERS
 					printf("SDL: Removing timer %p\n", t);
 #endif
@@ -159,8 +158,8 @@ void SDL_ThreadedTimerCheck(void)
 						SDL_timers = next;
 					}
 					free(t);
-					-- num_timers;
+					--SDL_timer_running;
-					removed = 1;
+					removed = SDL_TRUE;
 				}
 			}
 		}
@@ -172,6 +171,26 @@ void SDL_ThreadedTimerCheck(void)
 	SDL_mutexV(SDL_timer_mutex);
 }
 
+static SDL_TimerID SDL_AddTimerInternal(Uint32 interval, SDL_NewTimerCallback callback, void *param)
+{
+	SDL_TimerID t;
+	t = (SDL_TimerID) malloc(sizeof(struct _SDL_TimerID));
+	if ( t ) {
+		t->interval = ROUND_RESOLUTION(interval);
+		t->cb = callback;
+		t->param = param;
+		t->last_alarm = SDL_GetTicks();
+		t->next = SDL_timers;
+		SDL_timers = t;
+		++SDL_timer_running;
+		list_changed = SDL_TRUE;
+	}
+#ifdef DEBUG_TIMERS
+	printf("SDL_AddTimer(%d) = %08x num_timers = %d\n", interval, (Uint32)t, SDL_timer_running);
+#endif
+	return t;
+}
+
 SDL_TimerID SDL_AddTimer(Uint32 interval, SDL_NewTimerCallback callback, void *param)
 {
 	SDL_TimerID t;
@@ -188,21 +207,7 @@ SDL_TimerID SDL_AddTimer(Uint32 interval, SDL_NewTimerCallback callback, void *p
 		return NULL;
 	}
 	SDL_mutexP(SDL_timer_mutex);
-	t = (SDL_TimerID) malloc(sizeof(struct _SDL_TimerID));
+	t = SDL_AddTimerInternal(interval, callback, param);
-	if ( t ) {
-		t->interval = ROUND_RESOLUTION(interval);
-		t->cb = callback;
-		t->param = param;
-		t->last_alarm = SDL_GetTicks();
-		t->next = SDL_timers;
-		SDL_timers = t;
-		++ num_timers;
-		list_changed = SDL_TRUE;
-		SDL_timer_running = 1;
-	}
-#ifdef DEBUG_TIMERS
-	printf("SDL_AddTimer(%d) = %08x num_timers = %d\n", interval, (Uint32)t, num_timers);
-#endif
 	SDL_mutexV(SDL_timer_mutex);
 	return t;
 }
@@ -223,34 +228,19 @@ SDL_bool SDL_RemoveTimer(SDL_TimerID id)
 				SDL_timers = t->next;
 			}
 			free(t);
-			-- num_timers;
+			--SDL_timer_running;
 			removed = SDL_TRUE;
 			list_changed = SDL_TRUE;
 			break;
 		}
 	}
 #ifdef DEBUG_TIMERS
-	printf("SDL_RemoveTimer(%08x) = %d num_timers = %d thread = %d\n", (Uint32)id, removed, num_timers, SDL_ThreadID());
+	printf("SDL_RemoveTimer(%08x) = %d num_timers = %d thread = %d\n", (Uint32)id, removed, SDL_timer_running, SDL_ThreadID());
 #endif
 	SDL_mutexV(SDL_timer_mutex);
 	return removed;
 }
 
-static void SDL_RemoveAllTimers(SDL_TimerID t)
-{
-	SDL_TimerID freeme;
-
-	/* Changed to non-recursive implementation.
-	   The recursive implementation is elegant, but subject to 
-	   stack overflow if there are lots and lots of timers.
-	 */
-	while ( t ) {
-		freeme = t;
-		t = t->next;
-		free(freeme);
-	}
-}
-
 /* Old style callback functions are wrapped through this */
 static Uint32 callback_wrapper(Uint32 ms, void *param)
 {
@@ -266,21 +256,29 @@ int SDL_SetTimer(Uint32 ms, SDL_TimerCallback callback)
 	printf("SDL_SetTimer(%d)\n", ms);
 #endif
 	retval = 0;
-	if ( SDL_timer_running ) {	/* Stop any currently running timer */
+
-		SDL_timer_running = 0;
 	if ( SDL_timer_threaded ) {
 		SDL_mutexP(SDL_timer_mutex);
-			SDL_RemoveAllTimers(SDL_timers);
+	}
-			SDL_timers = NULL;
+	if ( SDL_timer_running ) {	/* Stop any currently running timer */
-			SDL_mutexV(SDL_timer_mutex);
+		if ( SDL_timer_threaded ) {
+			while ( SDL_timers ) {
+				SDL_TimerID freeme = SDL_timers;
+				SDL_timers = SDL_timers->next;
+				free(freeme);
+			}
+			SDL_timer_running = 0;
+			list_changed = SDL_TRUE;
 		} else {
 			SDL_SYS_StopTimer();
+			SDL_timer_running = 0;
 		}
 	}
 	if ( ms ) {
 		if ( SDL_timer_threaded ) {
-			retval = (SDL_AddTimer(ms, callback_wrapper,
+			if ( SDL_AddTimerInternal(ms, callback_wrapper, (void *)callback) == NULL ) {
-					       (void *)callback) != NULL);
+				retval = -1;
+			}
 		} else {
 			SDL_timer_running = 1;
 			SDL_alarm_interval = ms;
@@ -288,5 +286,9 @@ int SDL_SetTimer(Uint32 ms, SDL_TimerCallback callback)
 			retval = SDL_SYS_StartTimer();
 		}
 	}
+	if ( SDL_timer_threaded ) {
+		SDL_mutexP(SDL_timer_mutex);
+	}
+
 	return retval;
 }