| Re: [AD] user-defined events |
[ Thread Index |
Date Index
| More lists.liballeg.org/allegro-developers Archives
]
On 2008-10-16, Elias Pschernig <elias.pschernig@xxxxxxxxxx> wrote:
> On Fri, 2008-10-17 at 00:06 +1100, Peter Wang wrote:
> >
> > When is an event supposed to be destroyed? al_wait_for_event() returns a
> > copy of the event, probably on the user's stack. Allegro doesn't know
> > when that copy is no longer needed, so the user needs to explicitly call
> > an unref function.
>
> My idea was, Allegro calls the destroy_event() callback whenever it
> loses a (not necessarily the last) reference to an event. Because it
> calls the copy_event() callback whenever a copy is made, it should be
> 100% correct - users can do their own reference counting inside the
> copy/destroy callbacks, or make actual copies of the user data and free
> them. Is it not going to work?
I can't see how it would work. Probably I didn't understand your
description.
> > Implementing reference counting for user events wouldn't actually be
> > hard, but I'm still unsure. If you use atomic instructions then you
> > would at least avoid having an extra mutex per event. Maybe do it as an
> > exercise and see?
> >
>
> Yes, likely to call the callbacks at the correct location, it would be
> as much effort as implementing reference counting - haven't looked at
> the code yet.
>
> > Or use a conservative garbage collector..
> >
>
> I wonder if that would be easy, if I want to only collect my user events
> - seems overkill in any case.
Definitely overkill :)
The following patch implements (optional) reference counting for user
events. al_emit_user_event() takes a destructor argument,
and the user needs to call al_unref_user_event().
Peter
diff --git a/addons/acodec/ogg.c b/addons/acodec/ogg.c
index fa5a949..9f77196 100644
--- a/addons/acodec/ogg.c
+++ b/addons/acodec/ogg.c
@@ -120,7 +120,7 @@ static void ogg_stream_close(ALLEGRO_STREAM *stream)
ALLEGRO_EVENT quit_event;
quit_event.type = _KCM_STREAM_FEEDER_QUIT_EVENT_TYPE;
- al_emit_user_event(*((ALLEGRO_EVENT_SOURCE**)stream), &quit_event);
+ al_emit_user_event(*((ALLEGRO_EVENT_SOURCE**)stream), &quit_event, NULL);
al_join_thread(stream->feed_thread, NULL);
al_destroy_thread(stream->feed_thread);
diff --git a/addons/acodec/wav.c b/addons/acodec/wav.c
index b65c353..cab86f3 100644
--- a/addons/acodec/wav.c
+++ b/addons/acodec/wav.c
@@ -160,7 +160,7 @@ static void _sndfile_stream_close(ALLEGRO_STREAM *stream)
ALLEGRO_EVENT quit_event;
quit_event.type = _KCM_STREAM_FEEDER_QUIT_EVENT_TYPE;
- al_emit_user_event(*((ALLEGRO_EVENT_SOURCE**)stream), &quit_event);
+ al_emit_user_event(*((ALLEGRO_EVENT_SOURCE**)stream), &quit_event, NULL);
al_join_thread(stream->feed_thread, NULL);
al_destroy_thread(stream->feed_thread);
diff --git a/addons/kcm_audio/kcm_stream.c b/addons/kcm_audio/kcm_stream.c
index 6cb01e2..6dd1dba 100644
--- a/addons/kcm_audio/kcm_stream.c
+++ b/addons/kcm_audio/kcm_stream.c
@@ -608,7 +608,7 @@ bool _al_kcm_emit_stream_event(ALLEGRO_STREAM *stream, unsigned long count)
ALLEGRO_EVENT event;
event.user.type = ALLEGRO_EVENT_STREAM_EMPTY_FRAGMENT;
event.user.timestamp = al_current_time();
- al_emit_user_event(stream->spl.es, &event);
+ al_emit_user_event(stream->spl.es, &event, NULL);
}
return true;
diff --git a/examples/CMakeLists.txt b/examples/CMakeLists.txt
index 05c01ca..6039855 100644
--- a/examples/CMakeLists.txt
+++ b/examples/CMakeLists.txt
@@ -66,6 +66,7 @@ example(ex_timedwait NOTHING)
example(ex_get_path NOTHING)
example(ex_path NOTHING)
example(ex_user_events NOTHING)
+example(ex_user_events2 NOTHING)
if(SUPPORT_D3D)
example_cxx(ex_d3d d3dx9)
diff --git a/examples/ex_user_events.c b/examples/ex_user_events.c
index bbd0c52..4b1ecbc 100644
--- a/examples/ex_user_events.c
+++ b/examples/ex_user_events.c
@@ -7,7 +7,38 @@
/* XXX Currently we have no formal way to allocate event type numbers. */
-#define MY_EVENT_TYPE 1025
+#define MY_SIMPLE_EVENT_TYPE 1025
+#define MY_COMPLEX_EVENT_TYPE 1026
+
+
+/* Just some fantasy event, supposedly used in an RPG - it's just to show that
+ * in practice, the 4 user fields we have now never will be enough. */
+typedef struct MY_EVENT
+{
+ int id;
+ int type; /* For example "attack" or "buy". */
+ int x, y, z; /* Position in the game world the event takes place. */
+ int server_time; /* Game time in ticks the event takes place. */
+ int source_unit_id; /* E.g. attacker or seller. */
+ int destination_unit_id; /* E.g. defender of buyer. */
+ int item_id; /* E.g. weapon used or item sold. */
+ int amount; /* Gold the item is sold for. */
+} MY_EVENT;
+
+
+MY_EVENT *new_event(int id)
+{
+ MY_EVENT *event = calloc(1, sizeof *event);
+ event->id = id;
+ return event;
+}
+
+
+void my_event_dtor(ALLEGRO_USER_EVENT *event)
+{
+ printf("my_event_dtor: %p\n", (void *) event->data1);
+ free((void *) event->data1);
+}
int main(void)
@@ -49,21 +80,35 @@ int main(void)
printf("Got timer event %d\n", n);
- user_event.user.type = MY_EVENT_TYPE;
+ user_event.user.type = MY_SIMPLE_EVENT_TYPE;
user_event.user.data1 = n;
- al_emit_user_event(user_src, &user_event);
+ al_emit_user_event(user_src, &user_event, NULL);
+
+ user_event.user.type = MY_COMPLEX_EVENT_TYPE;
+ user_event.user.data1 = (intptr_t)new_event(n);
+ al_emit_user_event(user_src, &user_event, my_event_dtor);
}
- else if (event.type == MY_EVENT_TYPE) {
+ else if (event.type == MY_SIMPLE_EVENT_TYPE) {
int n = (int) event.user.data1;
ASSERT(event.user.source == user_src);
- printf("Got user event %d\n", n);
+ al_unref_user_event(&event.user);
+
+ printf("Got simple user event %d\n", n);
if (n == 5) {
break;
}
}
+ else if (event.type == MY_COMPLEX_EVENT_TYPE) {
+ MY_EVENT *my_event = (void *)event.user.data1;
+ ASSERT(event.user.source == user_src);
+
+ printf("Got complex user event %d\n", my_event->id);
+ al_unref_user_event(&event.user);
+ }
}
+ al_destroy_event_queue(queue);
al_destroy_user_event_source(user_src);
al_uninstall_timer(timer);
diff --git a/examples/ex_user_events2.c b/examples/ex_user_events2.c
new file mode 100644
index 0000000..c55d956
--- /dev/null
+++ b/examples/ex_user_events2.c
@@ -0,0 +1,70 @@
+/*
+ * Example program for the Allegro library.
+ */
+
+#include <stdio.h>
+#include "allegro5/allegro5.h"
+
+
+/* XXX Currently we have no formal way to allocate event type numbers. */
+#define MY_EVENT_TYPE 1025
+
+
+void print_dtor(ALLEGRO_USER_EVENT *event)
+{
+ printf("print_dtor called\n");
+ (void) event;
+}
+
+
+int main(void)
+{
+ ALLEGRO_EVENT_SOURCE *user_src;
+ ALLEGRO_EVENT_QUEUE *queue;
+ ALLEGRO_EVENT user_event;
+ ALLEGRO_EVENT event;
+
+ if (!al_init()) {
+ TRACE("Could not init Allegro.\n");
+ return 1;
+ }
+
+ user_src = al_create_user_event_source();
+ if (!user_src) {
+ TRACE("Could not create user event source.\n");
+ return 1;
+ }
+
+ queue = al_create_event_queue();
+ al_register_event_source(queue, user_src);
+
+ user_event.type = MY_EVENT_TYPE;
+
+ al_emit_user_event(user_src, &user_event, print_dtor);
+ printf("peek: print_dtor should NOT be called\n");
+ al_peek_next_event(queue, &user_event);
+ al_unref_user_event(&user_event.user);
+
+ printf("get: print_dtor should be called \n");
+ al_get_next_event(queue, &user_event);
+ al_unref_user_event(&user_event.user);
+
+ al_emit_user_event(user_src, &user_event, print_dtor);
+ printf("drop: print_dtor should be called\n");
+ al_drop_next_event(queue);
+
+ al_emit_user_event(user_src, &user_event, print_dtor);
+ printf("flush: print_dtor should be called\n");
+ al_flush_event_queue(queue);
+
+ al_emit_user_event(user_src, &user_event, print_dtor);
+ printf("destroy_queue: print_dtor should be called\n");
+ al_destroy_event_queue(queue);
+
+ al_destroy_user_event_source(user_src);
+
+ return 0;
+}
+END_OF_MAIN()
+
+/* vim: set sts=3 sw=3 et: */
diff --git a/include/allegro5/events.h b/include/allegro5/events.h
index 5e03bd0..a9dd166 100644
--- a/include/allegro5/events.h
+++ b/include/allegro5/events.h
@@ -131,6 +131,8 @@ enum
ALLEGRO_EVENT_DISPLAY_SWITCH_OUT = 46
};
+#define ALLEGRO_EVENT_TYPE_IS_USER(t) ((t) >= 1024)
+
/*
@@ -218,6 +220,7 @@ typedef struct ALLEGRO_TIMER_EVENT
typedef struct ALLEGRO_USER_EVENT
{
_AL_EVENT_HEADER(struct ALLEGRO_EVENT_SOURCE)
+ struct ALLEGRO_USER_EVENT_DESCRIPTOR *__internal__descr;
intptr_t data1;
intptr_t data2;
intptr_t data3;
@@ -284,7 +287,9 @@ AL_FUNC(void, al_destroy_user_event_source, (ALLEGRO_EVENT_SOURCE *));
/* The second argument is ALLEGRO_EVENT instead of ALLEGRO_USER_EVENT
* to prevent users passing a pointer to a too-short structure.
*/
-AL_FUNC(bool, al_emit_user_event, (ALLEGRO_EVENT_SOURCE *, ALLEGRO_EVENT *));
+AL_FUNC(bool, al_emit_user_event, (ALLEGRO_EVENT_SOURCE *, ALLEGRO_EVENT *,
+ void (*dtor)(ALLEGRO_USER_EVENT *)));
+AL_FUNC(void, al_unref_user_event, (ALLEGRO_USER_EVENT *));
diff --git a/include/allegro5/internal/aintern_events.h b/include/allegro5/internal/aintern_events.h
index bbfe272..c39b999 100644
--- a/include/allegro5/internal/aintern_events.h
+++ b/include/allegro5/internal/aintern_events.h
@@ -15,6 +15,12 @@ struct ALLEGRO_EVENT_SOURCE
_AL_VECTOR queues;
};
+typedef struct ALLEGRO_USER_EVENT_DESCRIPTOR
+{
+ void (*dtor)(ALLEGRO_USER_EVENT *event);
+ volatile int refcount;
+} ALLEGRO_USER_EVENT_DESCRIPTOR;
+
AL_FUNC(void, _al_event_source_init, (ALLEGRO_EVENT_SOURCE*));
AL_FUNC(void, _al_event_source_free, (ALLEGRO_EVENT_SOURCE*));
diff --git a/src/events.c b/src/events.c
index d7b7c8b..f07f1d2 100644
--- a/src/events.c
+++ b/src/events.c
@@ -48,6 +48,8 @@ struct ALLEGRO_EVENT_QUEUE
static bool do_wait_for_event(ALLEGRO_EVENT_QUEUE *queue,
ALLEGRO_EVENT *ret_event, ALLEGRO_TIMEOUT *timeout);
static void copy_event(ALLEGRO_EVENT *dest, const ALLEGRO_EVENT *src);
+static void ref_if_user_event(ALLEGRO_EVENT *event);
+static void unref_if_user_event(ALLEGRO_EVENT *event);
static void discard_events_of_source(ALLEGRO_EVENT_QUEUE *queue,
const ALLEGRO_EVENT_SOURCE *source);
@@ -186,7 +188,7 @@ bool al_event_queue_is_empty(ALLEGRO_EVENT_QUEUE *queue)
/* circ_array_next:
* Return the next index in a circular array.
*/
-static int circ_array_next(const _AL_VECTOR *vector, int i)
+static unsigned int circ_array_next(const _AL_VECTOR *vector, unsigned int i)
{
return (i + 1) % _al_vector_size(vector);
}
@@ -218,22 +220,24 @@ static ALLEGRO_EVENT *get_next_event_if_any(ALLEGRO_EVENT_QUEUE *queue,
-/* get_peek_or_drop_next_event: [primary thread]
- * Helper function to do the work for al_get_next_event,
- * al_peek_next_event and al_drop_next_event which are all very
- * similar.
+/* Function: al_get_next_event
+ * Take the next event packet out of the event queue specified, and
+ * copy the contents into RET_EVENT, returning true. The original
+ * event packet will be removed from the queue. If the event queue is
+ * empty, return false and the contents of RET_EVENT are unspecified.
*/
-static bool get_peek_or_drop_next_event(ALLEGRO_EVENT_QUEUE *queue,
- ALLEGRO_EVENT *do_copy, bool delete)
+bool al_get_next_event(ALLEGRO_EVENT_QUEUE *queue, ALLEGRO_EVENT *ret_event)
{
ALLEGRO_EVENT *next_event;
+ ASSERT(queue);
+ ASSERT(ret_event);
_al_mutex_lock(&queue->mutex);
- next_event = get_next_event_if_any(queue, delete);
+ next_event = get_next_event_if_any(queue, true);
if (next_event) {
- if (do_copy)
- copy_event(do_copy, next_event);
+ copy_event(ret_event, next_event);
+ /* Don't increment reference count on user events. */
}
_al_mutex_unlock(&queue->mutex);
@@ -243,22 +247,6 @@ static bool get_peek_or_drop_next_event(ALLEGRO_EVENT_QUEUE *queue,
-/* Function: al_get_next_event
- * Take the next event packet out of the event queue specified, and
- * copy the contents into RET_EVENT, returning true. The original
- * event packet will be removed from the queue. If the event queue is
- * empty, return false and the contents of RET_EVENT are unspecified.
- */
-bool al_get_next_event(ALLEGRO_EVENT_QUEUE *queue, ALLEGRO_EVENT *ret_event)
-{
- ASSERT(queue);
- ASSERT(ret_event);
-
- return get_peek_or_drop_next_event(queue, ret_event, true);
-}
-
-
-
/* Function: al_peek_next_event
* Copy the contents of the next event packet in the event queue
* specified into RET_EVENT and return true. The original event
@@ -268,10 +256,21 @@ bool al_get_next_event(ALLEGRO_EVENT_QUEUE *queue, ALLEGRO_EVENT *ret_event)
*/
bool al_peek_next_event(ALLEGRO_EVENT_QUEUE *queue, ALLEGRO_EVENT *ret_event)
{
+ ALLEGRO_EVENT *next_event;
ASSERT(queue);
ASSERT(ret_event);
- return get_peek_or_drop_next_event(queue, ret_event, false);
+ _al_mutex_lock(&queue->mutex);
+
+ next_event = get_next_event_if_any(queue, false);
+ if (next_event) {
+ copy_event(ret_event, next_event);
+ ref_if_user_event(ret_event);
+ }
+
+ _al_mutex_unlock(&queue->mutex);
+
+ return (next_event ? true : false);
}
@@ -282,9 +281,17 @@ bool al_peek_next_event(ALLEGRO_EVENT_QUEUE *queue, ALLEGRO_EVENT *ret_event)
*/
void al_drop_next_event(ALLEGRO_EVENT_QUEUE *queue)
{
+ ALLEGRO_EVENT *next_event;
ASSERT(queue);
- get_peek_or_drop_next_event(queue, NULL, true);
+ _al_mutex_lock(&queue->mutex);
+
+ next_event = get_next_event_if_any(queue, true);
+ if (next_event) {
+ unref_if_user_event(next_event);
+ }
+
+ _al_mutex_unlock(&queue->mutex);
}
@@ -294,9 +301,19 @@ void al_drop_next_event(ALLEGRO_EVENT_QUEUE *queue)
*/
void al_flush_event_queue(ALLEGRO_EVENT_QUEUE *queue)
{
+ unsigned int i;
ASSERT(queue);
_al_mutex_lock(&queue->mutex);
+
+ /* Decrement reference counts on all user events. */
+ i = queue->events_tail;
+ while (i != queue->events_head) {
+ ALLEGRO_EVENT *old_ev = _al_vector_ref(&queue->events, i);
+ unref_if_user_event(old_ev);
+ i = circ_array_next(&queue->events, i);
+ }
+
queue->events_head = queue->events_tail = 0;
_al_mutex_unlock(&queue->mutex);
}
@@ -471,6 +488,33 @@ static void copy_event(ALLEGRO_EVENT *dest, const ALLEGRO_EVENT *src)
+/* Increment a user event's reference count, if the event passed is a user
+ * event and requires it.
+ */
+static void ref_if_user_event(ALLEGRO_EVENT *event)
+{
+ if (ALLEGRO_EVENT_TYPE_IS_USER(event->type)) {
+ ALLEGRO_USER_EVENT_DESCRIPTOR *descr = event->user.__internal__descr;
+ if (descr) {
+ __sync_fetch_and_add(&descr->refcount, 1);
+ }
+ }
+}
+
+
+
+/* Decrement a user event's reference count, if the event passed is a user
+ * event and requires it.
+ */
+static void unref_if_user_event(ALLEGRO_EVENT *event)
+{
+ if (ALLEGRO_EVENT_TYPE_IS_USER(event->type)) {
+ al_unref_user_event(&event->user);
+ }
+}
+
+
+
/* Internal function: _al_event_queue_push_event
* Event sources call this function when they have something to add to
* the queue. If a queue cannot accept the event, the event's
@@ -490,6 +534,7 @@ void _al_event_queue_push_event(ALLEGRO_EVENT_QUEUE *queue,
{
new_event = alloc_event(queue);
copy_event(new_event, orig_event);
+ ref_if_user_event(new_event);
/* Wake up threads that are waiting for an event to be placed in
* the queue.
@@ -564,6 +609,9 @@ static void discard_events_of_source(ALLEGRO_EVENT_QUEUE *queue,
new_event = _al_vector_alloc_back(&queue->events);
copy_event(new_event, old_event);
}
+ else {
+ unref_if_user_event(old_event);
+ }
i = circ_array_next(&old_events, i);
}
@@ -582,6 +630,32 @@ static void discard_events_of_source(ALLEGRO_EVENT_QUEUE *queue,
+/* Function: al_unref_user_event
+ * Unreference a user-defined event. This must be called on any user event
+ * that you get from <al_get_next_event>, <al_peek_next_event>,
+ * <al_wait_for_event>, etc. which is reference counted.
+ * This function does nothing if the event is not reference counted.
+ *
+ * See also: <al_emit_user_event>.
+ */
+void al_unref_user_event(ALLEGRO_USER_EVENT *event)
+{
+ ALLEGRO_USER_EVENT_DESCRIPTOR *descr;
+ ASSERT(event);
+
+ descr = event->__internal__descr;
+ if (descr) {
+ ASSERT(descr->refcount > 0);
+ /* XXX uses gcc extension */
+ if (__sync_sub_and_fetch(&descr->refcount, 1) == 0) {
+ (descr->dtor)(event);
+ _AL_FREE(descr);
+ }
+ }
+}
+
+
+
/*
* Local Variables:
* c-basic-offset: 3
diff --git a/src/evtsrc.c b/src/evtsrc.c
index 691c2ef..4ba0c8f 100644
--- a/src/evtsrc.c
+++ b/src/evtsrc.c
@@ -202,27 +202,36 @@ void al_destroy_user_event_source(ALLEGRO_EVENT_SOURCE *src)
* Returns `false' if the event source isn't registered with any queues,
* hence the event wouldn't have been delivered into any queues.
*
- * It is not recommended to have user events that hold unique references to
- * dynamically allocated memory blocks. If you choose to do that, it is your
- * responsibility to manage that memory. Event structures are *copied* into
- * event queues. If a user event is emitted by an event source registered
- * with multiple queues, there would be multiple events pointing to the same
- * piece of dynamically allocated memory. In some cases, such as when an
- * event queue is destroyed, when an event source is removed from a queue, or
- * if you call <al_flush_event_queue>, events may be dropped without further
- * warning. If one of those events held a unique pointer to a memory block
- * then that would result in a memory leak.
+ * Reference counting will be performed on the event if `dtor' is non-NULL.
+ * When the reference count drops to zero `dtor' will be called with a copy of
+ * the event as an argument. It should free the resources associated with
+ * the event. If `dtor' is NULL then reference counting will not be
+ * performed.
+ *
+ * You need to call <al_unref_user_event> when you are done with a reference
+ * counted user event that you have gotten from <al_get_next_event>,
+ * <al_peek_next_event>, <al_wait_for_event>, etc. You may, but do not need
+ * to, call <al_unref_user_event> on non-reference counted user events.
*/
bool al_emit_user_event(ALLEGRO_EVENT_SOURCE *src,
- ALLEGRO_EVENT *event)
+ ALLEGRO_EVENT *event, void (*dtor)(ALLEGRO_USER_EVENT *))
{
size_t num_queues;
bool rc;
ASSERT(src);
ASSERT(event);
- /* We could check if it's not a builtin event type. */
- ASSERT(event->type != 0);
+ ASSERT(ALLEGRO_EVENT_TYPE_IS_USER(event->any.type));
+
+ if (dtor) {
+ ALLEGRO_USER_EVENT_DESCRIPTOR *descr = _AL_MALLOC(sizeof(*descr));
+ descr->refcount = 0;
+ descr->dtor = dtor;
+ event->user.__internal__descr = descr;
+ }
+ else {
+ event->user.__internal__descr = NULL;
+ }
_al_event_source_lock(src);
{
@@ -238,6 +247,11 @@ bool al_emit_user_event(ALLEGRO_EVENT_SOURCE *src,
}
_al_event_source_unlock(src);
+ if (dtor && !rc) {
+ dtor(&event->user);
+ _AL_FREE(event->user.__internal__descr);
+ }
+
return rc;
}