[pulseaudio-discuss] [PATCH 1/2] thread_mq supports a mainloop as thread/consumer

[Tanu Kaskinen]

Hi Alex,

About the summary: "thread_mq supports a mainloop as thread/consumer" -
the summary should always start with "<label>:", unless the commit
modifies code all over the tree. Usually the label should be the file
name without the file name extension. In this case the label should be
"thread-mq".

On Mon, 2013-07-22 at 01:49 +0200, Alexander Couzens wrote:
> Signed-off-by: Alexander Couzens <lynxis at fe80.eu>
> ---
>  src/pulsecore/thread-mq.c | 92 +++++++++++++++++++++++++++++++++++++++++++----
>  src/pulsecore/thread-mq.h |  5 ++-
>  2 files changed, 89 insertions(+), 8 deletions(-)
> 
> diff --git a/src/pulsecore/thread-mq.c b/src/pulsecore/thread-mq.c
> index dd84c9a..cf35642 100644
> --- a/src/pulsecore/thread-mq.c
> +++ b/src/pulsecore/thread-mq.c
> @@ -30,11 +30,51 @@
>  #include <pulsecore/semaphore.h>
>  #include <pulsecore/macro.h>
>  
> +#include <pulse/mainloop-api.h>
> +
>  #include "thread-mq.h"
>  
>  PA_STATIC_TLS_DECLARE_NO_FREE(thread_mq);
>  
> -static void asyncmsgq_read_cb(pa_mainloop_api*api, pa_io_event* e, int fd, pa_io_event_flags_t events, void *userdata) {
> +static void asyncmsgq_read_inq_cb(pa_mainloop_api*api, pa_io_event* e, int fd, pa_io_event_flags_t events, void *userdata) {

Cosmetic: pointer variables should always be formatted like this:

    "type *variable"

Not "type*variable" and not "type* variable". The bad formatting was in
the original code, but this is a good opportunity fix it.

> +    pa_thread_mq *q = userdata;
> +    pa_asyncmsgq *aq;
> +
> +    pa_assert(pa_asyncmsgq_read_fd(q->inq) == fd);
> +    pa_assert(events == PA_IO_EVENT_INPUT);
> +
> +    pa_asyncmsgq_ref(aq = q->inq);
> +    pa_asyncmsgq_read_after_poll(aq);
> +
> +    for (;;) {
> +        pa_msgobject *object;
> +        int code;
> +        void *data;
> +        int64_t offset;
> +        pa_memchunk chunk;
> +
> +        /* Check whether there is a message for us to process */
> +        while (pa_asyncmsgq_get(aq, &object, &code, &data, &offset, &chunk, 0) >= 0) {
> +            int ret;
> +
> +            if (!object && code == PA_MESSAGE_SHUTDOWN) {
> +                pa_asyncmsgq_done(aq, 0);
> +                api->quit(api, 0);
> +                break;
> +            }
> +
> +            ret = pa_asyncmsgq_dispatch(object, code, data, offset, &chunk);
> +            pa_asyncmsgq_done(aq, ret);
> +        }
> +
> +        if (pa_asyncmsgq_read_before_poll(aq) == 0)
> +            break;
> +    }
> +
> +    pa_asyncmsgq_unref(aq);
> +}

Instead of copying code, please put the shared code in a helper function
that asyncmsgq_read_inq_cb() and asyncmsgq_read_outq_cb() can then call.
The same goes for the write callbacks.

There is currently a difference between the read callbacks: one handles
PA_MESSAGE_SHUTDOWN and one doesn't. It should be safe to always handle
PA_MESSAGE_SHUTDOWN, so you don't need to care about that difference
when writing the helper function. The only readon why the shutdown
message isn't currently handled for outq is that that message is never
sent to the main thread.

> +
> +static void asyncmsgq_read_outq_cb(pa_mainloop_api*api, pa_io_event* e, int fd, pa_io_event_flags_t events, void *userdata) {
>      pa_thread_mq *q = userdata;
>      pa_asyncmsgq *aq;
>  
> @@ -66,7 +106,7 @@ static void asyncmsgq_read_cb(pa_mainloop_api*api, pa_io_event* e, int fd, pa_io
>      pa_asyncmsgq_unref(aq);
>  }
>  
> -static void asyncmsgq_write_cb(pa_mainloop_api*api, pa_io_event* e, int fd, pa_io_event_flags_t events, void *userdata) {
> +static void asyncmsgq_write_inq_cb(pa_mainloop_api*api, pa_io_event* e, int fd, pa_io_event_flags_t events, void *userdata) {
>      pa_thread_mq *q = userdata;
>  
>      pa_assert(pa_asyncmsgq_write_fd(q->inq) == fd);
> @@ -76,6 +116,38 @@ static void asyncmsgq_write_cb(pa_mainloop_api*api, pa_io_event* e, int fd, pa_i
>      pa_asyncmsgq_write_before_poll(q->inq);
>  }
>  
> +static void asyncmsgq_write_outq_cb(pa_mainloop_api*api, pa_io_event* e, int fd, pa_io_event_flags_t events, void *userdata) {
> +    pa_thread_mq *q = userdata;
> +
> +    pa_assert(pa_asyncmsgq_write_fd(q->outq) == fd);
> +    pa_assert(events == PA_IO_EVENT_INPUT);
> +
> +    pa_asyncmsgq_write_after_poll(q->outq);
> +    pa_asyncmsgq_write_before_poll(q->outq);
> +}
> +
> +void pa_thread_mq_init_rtmainloop(pa_thread_mq *q, pa_mainloop_api *mainloop, pa_mainloop_api *rt_mainloop) {

I don't like the naming here. There is no requirement that the thread
has anything to do with realtime scheduling, so "rt" doesn't make sense
as a prefix. I suggest names "main_mainloop" and "thread_mainloop". I
know "main_mainloop" sounds stupid, but I think it's logical - there are
two mainloops, one of which is for the main thread and one is for the IO
thread. I would be OK also with "io_mainloop" instead of
"thread_mainloop", but "main_mainloop" is really the only logical name
that I can think of for the main thread mainloop.

As for the function name, I don't have any good ideas. Hopefully we can
implement pa_mainloop_api on top of pa_rtpoll, then we can have just one
version of initializing the thread_mq. Until then, I guess if you're
going to rename rt_mainloop to thread_mainloop, the function name could
be pa_thread_mq_init_thread_mainloop() or
pa_thread_mq_init_with_thread_mainloop() (both feel pretty clumsy,
though).

Another option would be to merge pa_thread_mq_init() and
pa_thread_mq_init_rtmainloop() so that there would be just one
pa_thread_mq_init() that would handle both cases. The function would
have an argument for both pa_rtpoll and pa_mainloop_api, and callers
would pass NULL for one of them depending on how they implement their IO
thread.

> @@ -106,9 +178,15 @@ void pa_thread_mq_done(pa_thread_mq *q) {
>      if (!pa_asyncmsgq_dispatching(q->outq))
>          pa_asyncmsgq_flush(q->outq, true);
>  
> -    q->mainloop->io_free(q->read_event);
> -    q->mainloop->io_free(q->write_event);
> -    q->read_event = q->write_event = NULL;
> +    q->mainloop->io_free(q->read_main_event);
> +    q->mainloop->io_free(q->write_main_event);
> +    q->read_main_event = q->write_main_event = NULL;
> +
> +    if(q->rtmainloop) {

Missing space after if.

-- 
Tanu