| [AD] review timed-wait changes |
[ Thread Index |
Date Index
| More lists.liballeg.org/allegro-developers Archives
]
Hi,
This patch changes two things.
1. split al_wait_for_event() into two functions: al_wait_for_event (no
timeout) and al_wait_for_event_timed. al_wait_for_event_timed takes a
timeout in seconds rather than milliseconds, for consistency with
al_current_time() and al_rest().
2. change the interface of _al_cond_timedwait() to take an absolute
timeout rather than a relative one. There's no really compelling reason
for this but the old interface made the pthread implementation a bit
convoluted, converting structured time representations to floats then
back again. The new interface makes the Win32 implementation a little
convoluted, but hopefully less so. Could somebody apply the patch and
check that exnew_timedwait works in Windows? Also check if the code to
handle timeGetTime() overflows makes sense.
Question: should al_wait_for_event_timed() take an absolute timeout
specification, or should we provide both? Relative timeouts are
convenient for one off calls, but absolute timeouts would be much better
when waiting for multiple events in a loop, e.g.
float abst = al_current_time() + X;
while (al_wait_for_event_timed(queue, &ev, abst)) {
handle ev
}
/* time's up */
Peter
--- include/allegro5/events.h (revision 11469)
+++ include/allegro5/events.h (local)
@@ -263,8 +263,6 @@ typedef struct ALLEGRO_EVENT_SOURCE ALLE
typedef struct ALLEGRO_EVENT_QUEUE ALLEGRO_EVENT_QUEUE;
-#define ALLEGRO_WAIT_FOREVER (-1)
-
AL_FUNC(ALLEGRO_EVENT_QUEUE*, al_create_event_queue, (void));
AL_FUNC(void, al_destroy_event_queue, (ALLEGRO_EVENT_QUEUE*));
AL_FUNC(void, al_register_event_source, (ALLEGRO_EVENT_QUEUE*, ALLEGRO_EVENT_SOURCE*));
@@ -274,9 +272,11 @@ AL_FUNC(bool, al_get_next_event, (ALLEGR
AL_FUNC(bool, al_peek_next_event, (ALLEGRO_EVENT_QUEUE*, ALLEGRO_EVENT *ret_event));
AL_FUNC(void, al_drop_next_event, (ALLEGRO_EVENT_QUEUE*));
AL_FUNC(void, al_flush_event_queue, (ALLEGRO_EVENT_QUEUE*));
-AL_FUNC(bool, al_wait_for_event, (ALLEGRO_EVENT_QUEUE*,
- ALLEGRO_EVENT *ret_event,
- long msecs));
+AL_FUNC(void, al_wait_for_event, (ALLEGRO_EVENT_QUEUE*,
+ ALLEGRO_EVENT *ret_event));
+AL_FUNC(bool, al_wait_for_event_timed, (ALLEGRO_EVENT_QUEUE*,
+ ALLEGRO_EVENT *ret_event,
+ float msecs));
--- include/allegro5/internal/aintern_thread.h (revision 11469)
+++ include/allegro5/internal/aintern_thread.h (local)
@@ -11,6 +11,7 @@ AL_BEGIN_EXTERN_C
typedef struct _AL_THREAD _AL_THREAD;
typedef struct _AL_MUTEX _AL_MUTEX;
typedef struct _AL_COND _AL_COND;
+typedef struct _AL_COND_TIMEOUT _AL_COND_TIMEOUT;
AL_FUNC(void, _al_thread_create, (_AL_THREAD*,
@@ -30,7 +31,8 @@ AL_FUNC(void, _al_mutex_destroy, (_AL_MU
AL_FUNC(void, _al_cond_init, (_AL_COND*));
AL_FUNC(void, _al_cond_destroy, (_AL_COND*));
AL_FUNC(void, _al_cond_wait, (_AL_COND*, _AL_MUTEX*));
-AL_FUNC(int, _al_cond_timedwait, (_AL_COND*, _AL_MUTEX*, unsigned long abstime));
+AL_FUNC(void, _al_cond_timeout_init, (_AL_COND_TIMEOUT*, unsigned int rel_msecs));
+AL_FUNC(int, _al_cond_timedwait, (_AL_COND*, _AL_MUTEX*, const _AL_COND_TIMEOUT *timeout));
AL_FUNC(void, _al_cond_broadcast, (_AL_COND*));
AL_FUNC(void, _al_cond_signal, (_AL_COND*));
--- include/allegro5/platform/aintuthr.h (revision 11469)
+++ include/allegro5/platform/aintuthr.h (local)
@@ -34,6 +34,11 @@ struct _AL_COND
pthread_cond_t cond;
};
+struct _AL_COND_TIMEOUT
+{
+ struct timespec abstime;
+};
+
AL_INLINE(bool, _al_thread_should_stop, (struct _AL_THREAD *t),
{
--- include/allegro5/platform/aintwthr.h (revision 11469)
+++ include/allegro5/platform/aintwthr.h (local)
@@ -37,6 +37,11 @@ struct _AL_COND
CRITICAL_SECTION mtxUnblockLock;
};
+struct _AL_COND_TIMEOUT
+{
+ DWORD abstime;
+};
+
AL_INLINE(bool, _al_thread_should_stop, (struct _AL_THREAD *t),
{
--- src/compat/cokeybd.c (revision 11469)
+++ src/compat/cokeybd.c (local)
@@ -479,7 +479,7 @@ static void cokeybd_thread_func(_AL_THRE
/* wait for an event; wait up every so often to check if we
* should quit
*/
- if (!al_wait_for_event(cokeybd_event_queue, &event, 250))
+ if (!al_wait_for_event_timed(cokeybd_event_queue, &event, 0.250))
continue;
_al_mutex_lock(&key_buffers_lock);
--- src/compat/comouse.c (revision 11469)
+++ src/compat/comouse.c (local)
@@ -1079,7 +1079,7 @@ static void comouse_thread_func(_AL_THRE
/* wait for an event; wait up every so often to check if we
* should quit
*/
- if (!al_wait_for_event(comouse_event_queue, &event, 250))
+ if (!al_wait_for_event_timed(comouse_event_queue, &event, 0.250))
continue;
_al_mutex_lock(&comouse_mutex);
--- src/compat/cotimer.c (revision 11469)
+++ src/compat/cotimer.c (local)
@@ -149,7 +149,7 @@ static void timer_thread_func(_AL_THREAD
while (!_al_thread_should_stop(self)) {
ALLEGRO_EVENT event;
- if (!al_wait_for_event(event_queue, &event, 50))
+ if (!al_wait_for_event_timed(event_queue, &event, 0.050))
continue;
if ((ALLEGRO_TIMER *)event.any.source == retrace_timer) {
--- src/events.c (revision 11469)
+++ src/events.c (local)
@@ -298,14 +298,20 @@ void al_flush_event_queue(ALLEGRO_EVENT_
-/* wait_on_queue_forever: [primary thread]
- * Helper for al_wait_for_event. The caller must lock the queue
- * before calling.
+/* [primary thread]
+ *
+ * Function: al_wait_for_event
+ * Wait until the event queue specified is non-empty. If RET_EVENT
+ * is not NULL, the first event packet in the queue will be copied
+ * into RET_EVENT and removed from the queue. If RET_EVENT is NULL
+ * the first event packet is left at the head of the queue.
*/
-static void wait_on_queue_forever(ALLEGRO_EVENT_QUEUE *queue, ALLEGRO_EVENT *ret_event)
+void al_wait_for_event(ALLEGRO_EVENT_QUEUE *queue, ALLEGRO_EVENT *ret_event)
{
ALLEGRO_EVENT *next_event = NULL;
+ ASSERT(queue);
+
_al_mutex_lock(&queue->mutex);
{
while (_al_vector_is_empty(&queue->events))
@@ -326,16 +332,30 @@ static void wait_on_queue_forever(ALLEGR
-/* wait_on_queue_timed: [primary thread]
- * Helper for al_wait_for_event. The caller must lock the queue
- * before calling.
+/* [primary thread]
+ *
+ * Function: al_wait_for_event_timed
+ * Wait until the event queue specified is non-empty. If RET_EVENT
+ * is not NULL, the first event packet in the queue will be copied
+ * into RET_EVENT and removed from the queue. If RET_EVENT is NULL
+ * the first event packet is left at the head of the queue.
+ *
+ * TIMEOUT_MSECS determines approximately how many seconds to
+ * wait. If the call times out, false is returned. Otherwise true is
+ * returned.
*/
-static bool wait_on_queue_timed(ALLEGRO_EVENT_QUEUE *queue, ALLEGRO_EVENT *ret_event, long msecs)
+bool al_wait_for_event_timed(ALLEGRO_EVENT_QUEUE *queue,
+ ALLEGRO_EVENT *ret_event, float secs)
{
- unsigned long timeout = ALLEGRO_SECS_TO_MSECS(al_current_time()) + msecs;
+ _AL_COND_TIMEOUT timeout;
bool timed_out = false;
ALLEGRO_EVENT *next_event = NULL;
+ ASSERT(queue);
+ ASSERT(secs >= 0);
+
+ _al_cond_timeout_init(&timeout, (unsigned int) 1000.0 * secs);
+
_al_mutex_lock(&queue->mutex);
{
int result = 0;
@@ -344,8 +364,9 @@ static bool wait_on_queue_timed(ALLEGRO_
* variable, which will be signaled when an event is placed into
* the queue.
*/
- while (_al_vector_is_empty(&queue->events) && (result != -1))
- result = _al_cond_timedwait(&queue->cond, &queue->mutex, timeout);
+ while (_al_vector_is_empty(&queue->events) && (result != -1)) {
+ result = _al_cond_timedwait(&queue->cond, &queue->mutex, &timeout);
+ }
if (result == -1)
timed_out = true;
@@ -369,32 +390,6 @@ static bool wait_on_queue_timed(ALLEGRO_
-/* Function: al_wait_for_event
- * Wait until the event queue specified is non-empty. If RET_EVENT
- * is not NULL, the first event packet in the queue will be copied
- * into RET_EVENT and removed from the queue. If RET_EVENT is NULL
- * the first event packet is left at the head of the queue.
- *
- * TIMEOUT_MSECS determines approximately how many milliseconds to
- * wait. If it is ALLEGRO_WAIT_FOREVER, the call will wait indefinitely. If the
- * call times out, false is returned. Otherwise true is returned.
- */
-bool al_wait_for_event(ALLEGRO_EVENT_QUEUE *queue, ALLEGRO_EVENT *ret_event, long msecs)
-{
- ASSERT(queue);
- ASSERT(msecs == ALLEGRO_WAIT_FOREVER || msecs >= 0);
-
- if (msecs == ALLEGRO_WAIT_FOREVER) {
- wait_on_queue_forever(queue, ret_event);
- return true;
- }
- else {
- return wait_on_queue_timed(queue, ret_event, msecs);
- }
-}
-
-
-
/*----------------------------------------------------------------------*/
--- src/unix/uxthread.c (revision 11469)
+++ src/unix/uxthread.c (local)
@@ -116,30 +116,25 @@ void _al_mutex_destroy(_AL_MUTEX *mutex)
/* condition variables */
/* most of the condition variable implementation is actually inline */
-int _al_cond_timedwait(_AL_COND *cond, _AL_MUTEX *mutex, unsigned long abstime)
+void _al_cond_timeout_init(_AL_COND_TIMEOUT *timeout, unsigned int rel_msecs)
{
- long msecs;
struct timeval now;
- struct timespec timeout;
- int retcode = 0;
-
- /* FIXME: It is rather stupid that abstime is referenced to
- * al_current_time() base. A different interface is needed, which
- * takes into account that win32 uses relative timeouts for
- * WaitFor*, but relative timeouts are bad for _al_cond_timedwait().
- */
-
- msecs = abstime - ALLEGRO_SECS_TO_MSECS(al_current_time());
- if (msecs < 0)
- return -1;
gettimeofday(&now, NULL);
- timeout.tv_sec = now.tv_sec + (msecs / 1000);
- timeout.tv_nsec = (now.tv_usec + (msecs % 1000) * 1000) * 1000;
- timeout.tv_sec += timeout.tv_nsec / 1000000000L;
- timeout.tv_nsec = timeout.tv_nsec % 1000000000L;
+ timeout->abstime.tv_sec = now.tv_sec + (rel_msecs / 1000);
+ timeout->abstime.tv_nsec = (now.tv_usec + (rel_msecs % 1000) * 1000) * 1000;
+ timeout->abstime.tv_sec += timeout->abstime.tv_nsec / 1000000000L;
+ timeout->abstime.tv_nsec = timeout->abstime.tv_nsec % 1000000000L;
+}
+
+
+int _al_cond_timedwait(_AL_COND *cond, _AL_MUTEX *mutex,
+ const _AL_COND_TIMEOUT *timeout)
+{
+ int retcode;
- retcode = pthread_cond_timedwait(&cond->cond, &mutex->mutex, &timeout);
+ retcode = pthread_cond_timedwait(&cond->cond, &mutex->mutex,
+ &timeout->abstime);
return (retcode == ETIMEDOUT) ? -1 : 0;
}
--- src/win/wtime.c (revision 11469)
+++ src/win/wtime.c (local)
@@ -81,7 +81,7 @@ static double high_res_current_time(void
double al_current_time(void)
{
- return (*real_current_time_func)();
+ return (*real_current_time_func)();
}
--- src/win/wxthread.c (revision 11469)
+++ src/win/wxthread.c (local)
@@ -288,29 +288,52 @@ static int cond_wait(_AL_COND *cond, _AL
void _al_cond_wait(_AL_COND *cond, _AL_MUTEX *mtxExternal)
{
+ int result;
+
ASSERT(cond);
ASSERT(mtxExternal);
- {
- int result = cond_wait(cond, mtxExternal, INFINITE);
- ASSERT(result != -1);
- (void)result;
+ result = cond_wait(cond, mtxExternal, INFINITE);
+ ASSERT(result != -1);
+ (void)result;
+}
+
+
+void _al_cond_timeout_init(_AL_COND_TIMEOUT *timeout, unsigned int rel_msecs)
+{
+ /* Don't create a timeout structure with an absolute time before the current
+ * time. We rely on this to deal with timeGetTime() wrapping.
+ */
+ if (rel_msecs < 0) {
+ timeout->abstime = timeGetTime();
+ }
+ else {
+ timeout->abstime = timeGetTime() + rel_msecs;
}
}
-int _al_cond_timedwait(_AL_COND *cond, _AL_MUTEX *mtxExternal, unsigned long abstime)
+int _al_cond_timedwait(_AL_COND *cond, _AL_MUTEX *mtxExternal,
+ const _AL_COND_TIMEOUT *timeout)
{
+ DWORD now;
+ signed int rel_msecs;
+
ASSERT(cond);
ASSERT(mtxExternal);
- {
- int reltime = abstime - ALLEGRO_SECS_TO_MSECS(al_current_time());
- if (reltime < 0)
- return -1;
+ now = timeGetTime();
+ rel_msecs = (signed int)(timeout->abstime - now);
- return cond_wait(cond, mtxExternal, reltime);
+ /* timeGetTime() wraps approximately every 49.71 days. */
+ if (rel_msecs < 0) {
+ rel_msecs += 0xffffffff;
}
+ if (rel_msecs == INFINITE) {
+ rel_msecs--;
+ }
+
+ return cond_wait(cond, mtxExternal, rel_msecs);
}