From bfa9efd51b642f7a54c89cf33ae40e0a42dbcb07 Mon Sep 17 00:00:00 2001 From: Sam Lantinga Date: Thu, 13 Jan 2005 23:24:56 +0000 Subject: [PATCH] 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 --- include/SDL_timer.h | 2 + src/events/SDL_events.c | 5 +- src/timer/SDL_timer.c | 128 ++++++++++++++++++++-------------------- 3 files changed, 70 insertions(+), 65 deletions(-) diff --git a/include/SDL_timer.h b/include/SDL_timer.h index 2da6d89ef..938479b37 100644 --- a/include/SDL_timer.h +++ b/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); diff --git a/src/events/SDL_events.c b/src/events/SDL_events.c index f80e23404..43ae04d4c 100644 --- a/src/events/SDL_events.c +++ b/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; @@ -114,7 +113,7 @@ static int SDL_GobbleEvents(void *unused) /* Give up the CPU for the rest of our timeslice */ SDL_EventLock.safe = 1; - if( SDL_timer_running ) { + if ( SDL_timer_running ) { SDL_ThreadedTimerCheck(); } SDL_Delay(1); @@ -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); diff --git a/src/timer/SDL_timer.c b/src/timer/SDL_timer.c index 0706042d2..88883ccf1 100644 --- a/src/timer/SDL_timer.c +++ b/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(); } - SDL_timer_started = 1; + 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_NewTimerCallback callback; - Uint32 interval; - void *param; - - now = SDL_GetTicks(); + SDL_bool removed; 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()); + t, SDL_ThreadID()); #endif - callback = t->cb; - interval = t->interval; - param = t->param; + timer = *t; 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; - removed = 1; + --SDL_timer_running; + 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)); - 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 + t = SDL_AddTimerInternal(interval, callback, param); 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_threaded ) { + SDL_mutexP(SDL_timer_mutex); + } 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; - SDL_mutexV(SDL_timer_mutex); + 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, - (void *)callback) != NULL); + if ( SDL_AddTimerInternal(ms, callback_wrapper, (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; }