[pulseaudio-discuss] [PATCH 4/5] rtpoll: Implement pa_mainloop_api support
Tanu Kaskinen
tanuk at iki.fi
Tue Feb 9 11:42:52 CET 2016
On Tue, 2016-02-09 at 15:47 +0530, Arun Raghavan wrote:
> On Fri, 2015-01-02 at 15:04 +0200, Tanu Kaskinen wrote:
> > The new tunnel sink and source use libpulse to talk to the remote
> > server, and libpulse requires a pa_mainloop_api implementation for
> > interacting with the event loop. The tunnel sink and source have so
> > far been using pa_mainloop, but there are some modules that assume
> > that all sinks and sources use pa_rtpoll in their IO threads, and
> > trying to use those modules together with the pa_mainloop based
> > tunnel sinks and sources will result in crashes (see [1]).
> >
> > I will switch the event loop implementation in the tunnel modules
> > to pa_rtpoll, but that requires a pa_mainloop_api implementation in
> > pa_rtpoll first - that's implemented here.
> >
> > pa_rtpoll_run() is changed so that it processes all defer events
> > first, and then all expired time events. The rest of pa_rtpoll_run()
> > works as before, except the poll timeout calculation now has to also
> > take the time events into account. IO events use the existing
> > pa_rtpoll_item interface.
> >
> > The time events are handled separately from the old timer
> > functionality, which is somewhat ugly. It might be a good idea to
> > remove the old timer functionality and only use the time events.
> > I didn't attempt to do that at this time, because I feared that
> > adapting the pa_rtpoll users to the new system would be difficult.
> >
> > [1] https://bugs.freedesktop.org/show_bug.cgi?id=73429
> > ---
>
> As a first comment, I'd like to ask that there be a test for this. It
> should be quite easy to refactor mainloop-test to run the same set of
> tests on different mainloop implementations.
Ok.
> > src/pulsecore/rtpoll.c | 504
> > ++++++++++++++++++++++++++++++++++++++++++++++++-
> > src/pulsecore/rtpoll.h | 4 +
> > 2 files changed, 500 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/pulsecore/rtpoll.c b/src/pulsecore/rtpoll.c
> > index 5f3ca8b..2e1907c 100644
> > --- a/src/pulsecore/rtpoll.c
> > +++ b/src/pulsecore/rtpoll.c
> > @@ -32,6 +32,7 @@
> > #include
> > #include
> >
> > +#include
> > #include
> > #include
> > #include
> > @@ -65,6 +66,18 @@ struct pa_rtpoll {
> > #endif
> >
> > PA_LLIST_HEAD(pa_rtpoll_item, items);
> > +
> > + pa_mainloop_api mainloop_api;
> > +
> > + pa_dynarray *io_events;
> > +
> > + pa_dynarray *time_events;
> > + pa_dynarray *enabled_time_events;
> > + pa_dynarray *expired_time_events;
> > + pa_time_event *cached_next_time_event;
> > +
> > + pa_dynarray *defer_events;
> > + pa_dynarray *enabled_defer_events;
> > };
> >
> > struct pa_rtpoll_item {
> > @@ -84,8 +97,321 @@ struct pa_rtpoll_item {
> > PA_LLIST_FIELDS(pa_rtpoll_item);
> > };
> >
> > +struct pa_io_event {
> > + pa_rtpoll *rtpoll;
> > + pa_rtpoll_item *rtpoll_item;
> > + pa_io_event_flags_t events;
> > + pa_io_event_cb_t callback;
> > + pa_io_event_destroy_cb_t destroy_callback;
> > + void *userdata;
> > +};
> > +
> > +static void io_event_enable(pa_io_event *event, pa_io_event_flags_t
> > events);
> > +
> > +struct pa_time_event {
> > + pa_rtpoll *rtpoll;
> > + pa_usec_t time;
> > + bool use_rtclock;
> > + bool enabled;
> > + pa_time_event_cb_t callback;
> > + pa_time_event_destroy_cb_t destroy_callback;
> > + void *userdata;
> > +};
> > +
> > +static void time_event_restart(pa_time_event *event, const struct
> > timeval *tv);
> > +
> > +struct pa_defer_event {
> > + pa_rtpoll *rtpoll;
> > + bool enabled;
> > + pa_defer_event_cb_t callback;
> > + pa_defer_event_destroy_cb_t destroy_callback;
> > + void *userdata;
> > +};
> > +
> > +static void defer_event_enable(pa_defer_event *event, int enable);
> > +
> > PA_STATIC_FLIST_DECLARE(items, 0, pa_xfree);
> >
> > +static short map_flags_to_libc(pa_io_event_flags_t flags) {
> > + return (short)
> > + ((flags & PA_IO_EVENT_INPUT ? POLLIN : 0) |
> > + (flags & PA_IO_EVENT_OUTPUT ? POLLOUT : 0) |
> > + (flags & PA_IO_EVENT_ERROR ? POLLERR : 0) |
> > + (flags & PA_IO_EVENT_HANGUP ? POLLHUP : 0));
> > +}
> > +
> > +static pa_io_event_flags_t map_flags_from_libc(short flags) {
> > + return
> > + (flags & POLLIN ? PA_IO_EVENT_INPUT : 0) |
> > + (flags & POLLOUT ? PA_IO_EVENT_OUTPUT : 0) |
> > + (flags & POLLERR ? PA_IO_EVENT_ERROR : 0) |
> > + (flags & POLLHUP ? PA_IO_EVENT_HANGUP : 0);
> > +}
>
> Can we dump these two in a util.c and have this reused with mainloop.c?
Ok.
> > +static int io_event_work_cb(pa_rtpoll_item *item) {
> > + pa_io_event *event;
> > + struct pollfd *pollfd;
> > +
> > + pa_assert(item);
> > +
> > + event = pa_rtpoll_item_get_userdata(item);
> > + pollfd = pa_rtpoll_item_get_pollfd(item, NULL);
> > + event->callback(&event->rtpoll->mainloop_api, event, pollfd->fd,
> > map_flags_from_libc(pollfd->revents), event->userdata);
> > +
> > + return 0;
> > +}
> > +
> > +static pa_io_event* io_event_new(pa_mainloop_api *api, int fd,
> > pa_io_event_flags_t events, pa_io_event_cb_t callback,
> > + void *userdata) {
> > + pa_rtpoll *rtpoll;
> > + pa_io_event *event;
> > + struct pollfd *pollfd;
> > +
> > + pa_assert(api);
> > + pa_assert(api->userdata);
> > + pa_assert(fd >= 0);
> > + pa_assert(callback);
> > +
> > + rtpoll = api->userdata;
> > + pa_assert(api == &rtpoll->mainloop_api);
> > +
> > + event = pa_xnew0(pa_io_event, 1);
> > + event->rtpoll = rtpoll;
> > + event->rtpoll_item = pa_rtpoll_item_new(rtpoll,
> > PA_RTPOLL_NORMAL, 1);
> > + pa_rtpoll_item_set_work_callback(event->rtpoll_item,
> > io_event_work_cb);
> > + pa_rtpoll_item_set_userdata(event->rtpoll_item, event);
> > + pollfd = pa_rtpoll_item_get_pollfd(event->rtpoll_item, NULL);
> > + pollfd->fd = fd;
> > + event->callback = callback;
> > + event->userdata = userdata;
> > +
> > + pa_dynarray_append(rtpoll->io_events, event);
> > + io_event_enable(event, events);
> > +
> > + return event;
> > +}
> > +
> > +static void io_event_free(pa_io_event *event) {
> > + pa_assert(event);
> > +
> > + pa_dynarray_remove_by_data(event->rtpoll->io_events, event);
> > +
> > + if (event->destroy_callback)
> > + event->destroy_callback(&event->rtpoll->mainloop_api, event,
> > event->userdata);
> > +
> > + if (event->rtpoll_item)
> > + pa_rtpoll_item_free(event->rtpoll_item);
> > +
> > + pa_xfree(event);
> > +}
> > +
> > +static void io_event_enable(pa_io_event *event, pa_io_event_flags_t
> > events) {
> > + struct pollfd *pollfd;
> > +
> > + pa_assert(event);
> > +
> > + if (events == event->events)
> > + return;
> > +
> > + event->events = events;
> > +
> > + pollfd = pa_rtpoll_item_get_pollfd(event->rtpoll_item, NULL);
> > + pollfd->events = map_flags_to_libc(events);
> > +}
> > +
> > +static void io_event_set_destroy(pa_io_event *event,
> > pa_io_event_destroy_cb_t callback) {
> > + pa_assert(event);
> > +
> > + event->destroy_callback = callback;
> > +}
> > +
> > +static pa_usec_t make_rt(const struct timeval *tv, bool
> > *use_rtclock) {
> > + struct timeval ttv;
> > +
> > + if (!tv) {
> > + *use_rtclock = false;
> > + return PA_USEC_INVALID;
> > + }
> > +
> > + ttv = *tv;
> > + *use_rtclock = !!(ttv.tv_usec & PA_TIMEVAL_RTCLOCK);
> > +
> > + if (*use_rtclock)
> > + ttv.tv_usec &= ~PA_TIMEVAL_RTCLOCK;
> > + else
> > + pa_rtclock_from_wallclock(&ttv);
> > +
> > + return pa_timeval_load(&ttv);
> > +}
>
> Same comment for code reuse about this.
Ok.
> > +static pa_time_event* time_event_new(pa_mainloop_api *api, const
> > struct timeval *tv, pa_time_event_cb_t callback,
> > + void *userdata) {
> > + pa_rtpoll *rtpoll;
> > + pa_time_event *event;
> > +
> > + pa_assert(api);
> > + pa_assert(api->userdata);
> > + pa_assert(callback);
> > +
> > + rtpoll = api->userdata;
> > + pa_assert(api == &rtpoll->mainloop_api);
> > +
> > + event = pa_xnew0(pa_time_event, 1);
> > + event->rtpoll = rtpoll;
> > + event->time = PA_USEC_INVALID;
> > + event->callback = callback;
> > + event->userdata = userdata;
> > +
> > + pa_dynarray_append(rtpoll->time_events, event);
> > + time_event_restart(event, tv);
> > +
> > + return event;
> > +}
> > +
> > +static void time_event_free(pa_time_event *event) {
> > + pa_assert(event);
> > +
> > + time_event_restart(event, NULL);
> > + pa_dynarray_remove_by_data(event->rtpoll->time_events, event);
> > +
> > + if (event->destroy_callback)
> > + event->destroy_callback(&event->rtpoll->mainloop_api, event,
> > event->userdata);
> > +
> > + pa_xfree(event);
> > +}
> > +
> > +static void time_event_restart(pa_time_event *event, const struct
> > timeval *tv) {
> > + pa_usec_t t;
> > + bool use_rtclock;
> > + bool enabled;
> > + bool old_enabled;
> > +
> > + pa_assert(event);
> > +
> > + t = make_rt(tv, &use_rtclock);
> > + enabled = (t != PA_USEC_INVALID);
> > + old_enabled = event->enabled;
> > +
> > + /* We return early only if the event stays disabled. If the
> > event stays
> > + * enabled, we can't return early, because the event time may
> > change. */
> > + if (!enabled && !old_enabled)
> > + return;
> > +
> > + event->enabled = enabled;
> > + event->time = t;
> > + event->use_rtclock = use_rtclock;
> > +
> > + if (enabled && !old_enabled)
> > + pa_dynarray_append(event->rtpoll->enabled_time_events,
> > event);
> > + else if (!enabled) {
> > + pa_dynarray_remove_by_data(event->rtpoll-
> > > enabled_time_events, event);
> > + pa_dynarray_remove_by_data(event->rtpoll-
> > > expired_time_events, event);
> > + }
> > +
> > + if (event->rtpoll->cached_next_time_event == event)
> > + event->rtpoll->cached_next_time_event = NULL;
> > +
> > + if (event->rtpoll->cached_next_time_event && enabled) {
> > + pa_assert(event->rtpoll->cached_next_time_event->enabled);
> > +
> > + if (t < event->rtpoll->cached_next_time_event->time)
> > + event->rtpoll->cached_next_time_event = event;
> > + }
> > +}
> > +
> > +static void time_event_set_destroy(pa_time_event *event,
> > pa_time_event_destroy_cb_t callback) {
> > + pa_assert(event);
> > +
> > + event->destroy_callback = callback;
> > +}
> > +
> > +static pa_defer_event* defer_event_new(pa_mainloop_api *api,
> > pa_defer_event_cb_t callback, void *userdata) {
> > + pa_rtpoll *rtpoll;
> > + pa_defer_event *event;
> > +
> > + pa_assert(api);
> > + pa_assert(api->userdata);
> > + pa_assert(callback);
> > +
> > + rtpoll = api->userdata;
> > + pa_assert(api == &rtpoll->mainloop_api);
> > +
> > + event = pa_xnew0(pa_defer_event, 1);
> > + event->rtpoll = rtpoll;
> > + event->callback = callback;
> > + event->userdata = userdata;
> > +
> > + pa_dynarray_append(rtpoll->defer_events, event);
> > + defer_event_enable(event, true);
> > +
> > + return event;
> > +}
> > +
> > +static void defer_event_free(pa_defer_event *event) {
> > + pa_assert(event);
> > +
> > + defer_event_enable(event, false);
> > + pa_dynarray_remove_by_data(event->rtpoll->defer_events, event);
> > +
> > + if (event->destroy_callback)
> > + event->destroy_callback(&event->rtpoll->mainloop_api, event,
> > event->userdata);
> > +
> > + pa_xfree(event);
> > +}
> > +
> > +static void defer_event_enable(pa_defer_event *event, int enable) {
> > + pa_assert(event);
> > +
> > + if (enable == event->enabled)
> > + return;
> > +
> > + event->enabled = enable;
> > +
> > + if (enable)
> > + pa_dynarray_append(event->rtpoll->enabled_defer_events,
> > event);
> > + else
> > + pa_dynarray_remove_by_data(event->rtpoll-
> > > enabled_defer_events, event);
> > +}
> > +
> > +static void defer_event_set_destroy(pa_defer_event *event,
> > pa_defer_event_destroy_cb_t callback) {
> > + pa_assert(event);
> > +
> > + event->destroy_callback = callback;
> > +}
> > +
> > +static void mainloop_api_quit(pa_mainloop_api *api, int retval) {
> > + pa_rtpoll *rtpoll;
> > +
> > + pa_assert(api);
> > + pa_assert(api->userdata);
> > +
> > + rtpoll = api->userdata;
> > + pa_assert(api == &rtpoll->mainloop_api);
> > +
> > + pa_rtpoll_quit(rtpoll);
> > +}
> > +
> > +static const pa_mainloop_api vtable = {
> > + .userdata = NULL,
> > +
> > + .io_new = io_event_new,
> > + .io_enable = io_event_enable,
> > + .io_free = io_event_free,
> > + .io_set_destroy = io_event_set_destroy,
> > +
> > + .time_new = time_event_new,
> > + .time_restart = time_event_restart,
> > + .time_free = time_event_free,
> > + .time_set_destroy = time_event_set_destroy,
> > +
> > + .defer_new = defer_event_new,
> > + .defer_enable = defer_event_enable,
> > + .defer_free = defer_event_free,
> > + .defer_set_destroy = defer_event_set_destroy,
> > +
> > + .quit = mainloop_api_quit,
> > +};
> > +
> > pa_rtpoll *pa_rtpoll_new(void) {
> > pa_rtpoll *p;
> >
> > @@ -99,6 +425,15 @@ pa_rtpoll *pa_rtpoll_new(void) {
> > p->timestamp = pa_rtclock_now();
> > #endif
> >
> > + p->mainloop_api = vtable;
> > + p->mainloop_api.userdata = p;
> > + p->io_events = pa_dynarray_new(NULL);
> > + p->time_events = pa_dynarray_new(NULL);
> > + p->enabled_time_events = pa_dynarray_new(NULL);
> > + p->expired_time_events = pa_dynarray_new(NULL);
> > + p->defer_events = pa_dynarray_new(NULL);
> > + p->enabled_defer_events = pa_dynarray_new(NULL);
> > +
> > return p;
> > }
> >
> > @@ -167,15 +502,108 @@ static void rtpoll_item_destroy(pa_rtpoll_item
> > *i) {
> > void pa_rtpoll_free(pa_rtpoll *p) {
> > pa_assert(p);
> >
> > + if (p->defer_events) {
> > + pa_defer_event *event;
> > +
> > + while ((event = pa_dynarray_last(p->defer_events)))
> > + defer_event_free(event);
> > + }
> > +
> > + if (p->time_events) {
> > + pa_time_event *event;
> > +
> > + while ((event = pa_dynarray_last(p->time_events)))
> > + time_event_free(event);
> > + }
> > +
> > + if (p->io_events) {
> > + pa_io_event *event;
> > +
> > + while ((event = pa_dynarray_last(p->io_events)))
> > + io_event_free(event);
> > + }
> > +
> > while (p->items)
> > rtpoll_item_destroy(p->items);
> >
> > + if (p->enabled_defer_events) {
> > + pa_assert(pa_dynarray_size(p->enabled_defer_events) == 0);
>
> These asserts seem kind of useless given that we have a loop cleaning
> up the dynarray prior to this.
I can remove the asserts if you don't like them. Your criticism applies
to all asserts, though: it's totally expected that there's code that
ensures that the assertion holds. If such code didn't exist, there
would be a bug.
> > + pa_dynarray_free(p->enabled_defer_events);
> > + }
> > +
> > + if (p->defer_events) {
> > + pa_assert(pa_dynarray_size(p->defer_events) == 0);
> > + pa_dynarray_free(p->defer_events);
> > + }
> > +
> > + if (p->expired_time_events) {
> > + pa_assert(pa_dynarray_size(p->expired_time_events) == 0);
> > + pa_dynarray_free(p->expired_time_events);
> > + }
> > +
> > + if (p->enabled_time_events) {
> > + pa_assert(pa_dynarray_size(p->enabled_time_events) == 0);
> > + pa_dynarray_free(p->enabled_time_events);
> > + }
> > +
> > + if (p->time_events) {
> > + pa_assert(pa_dynarray_size(p->time_events) == 0);
> > + pa_dynarray_free(p->time_events);
> > + }
> > +
> > + if (p->io_events) {
> > + pa_assert(pa_dynarray_size(p->io_events) == 0);
> > + pa_dynarray_free(p->io_events);
> > + }
> > +
> > pa_xfree(p->pollfd);
> > pa_xfree(p->pollfd2);
> >
> > pa_xfree(p);
> > }
> >
> > +pa_mainloop_api *pa_rtpoll_get_mainloop_api(pa_rtpoll *rtpoll) {
> > + pa_assert(rtpoll);
> > +
> > + return &rtpoll->mainloop_api;
>
> Is there any value to having each rtpoll have its own copy of this,
> rather than just returning the vtable directly here?
pa_mainloop_api.userdata is different for each instance.
> > +}
> > +
> > +static void find_expired_time_events(pa_rtpoll *rtpoll) {
> > + pa_usec_t now;
> > + pa_time_event *event;
> > + unsigned idx;
> > +
> > + pa_assert(rtpoll);
> > + pa_assert(pa_dynarray_size(rtpoll->expired_time_events) == 0);
> > +
> > + now = pa_rtclock_now();
> > +
> > + PA_DYNARRAY_FOREACH(event, rtpoll->enabled_time_events, idx) {
> > + if (event->time <= now)
> > + pa_dynarray_append(rtpoll->expired_time_events, event);
> > + }
> > +}
> > +
> > +static pa_time_event *find_next_time_event(pa_rtpoll *rtpoll) {
> > + pa_time_event *event;
> > + pa_time_event *result = NULL;
> > + unsigned idx;
> > +
> > + pa_assert(rtpoll);
> > +
> > + if (rtpoll->cached_next_time_event)
> > + return rtpoll->cached_next_time_event;
> > +
> > + PA_DYNARRAY_FOREACH(event, rtpoll->enabled_time_events, idx) {
> > + if (!result || event->time < result->time)
> > + result = event;
> > + }
> > +
> > + rtpoll->cached_next_time_event = result;
> > +
> > + return result;
> > +}
> > +
> > static void reset_revents(pa_rtpoll_item *i) {
> > struct pollfd *f;
> > unsigned n;
> > @@ -204,9 +632,14 @@ static void reset_all_revents(pa_rtpoll *p) {
> > }
> >
> > int pa_rtpoll_run(pa_rtpoll *p) {
> > + pa_defer_event *defer_event;
> > + pa_time_event *time_event;
> > pa_rtpoll_item *i;
> > int r = 0;
> > struct timeval timeout;
> > + pa_time_event *next_time_event;
> > + struct timeval next_time_event_elapse;
> > + bool timer_enabled;
> >
> > pa_assert(p);
> > pa_assert(!p->running);
> > @@ -218,7 +651,28 @@ int pa_rtpoll_run(pa_rtpoll *p) {
> > p->running = true;
> > p->timer_elapsed = false;
> >
> > - /* First, let's do some work */
> > + /* Dispatch all enabled defer events. */
> > + while ((defer_event = pa_dynarray_last(p-
> > > enabled_defer_events))) {
> > + if (p->quit)
> > + break;
> > +
> > + defer_event->callback(&p->mainloop_api, defer_event,
> > defer_event->userdata);
> > + }
>
> Am I missing something, or is this an infinite loop if there are any
> enabled defer events?
As discussed in IRC, I did this like this, because pa_mainloop_run() in
practice behaves the same way. However, mainloop-api.h says that each
defer event runs only once per mainloop iteration, so I'll have to
change this.
> > +
> > + /* Dispatch all expired time events. */
> > + find_expired_time_events(p);
> > + while ((time_event = pa_dynarray_last(p->expired_time_events)))
> > {
> > + struct timeval tv;
> > +
> > + if (p->quit)
> > + break;
> > +
> > + time_event_restart(time_event, NULL);
> > + time_event->callback(&p->mainloop_api, time_event,
> > pa_timeval_rtstore(&tv, time_event->time, time_event->use_rtclock),
> > + time_event->userdata);
> > + }
> > +
> > + /* Let's do some work */
> > for (i = p->items; i && i->priority < PA_RTPOLL_NEVER; i = i-
> > > next) {
> > int k;
> >
> > @@ -282,15 +736,40 @@ int pa_rtpoll_run(pa_rtpoll *p) {
> > if (p->rebuild_needed)
> > rtpoll_rebuild(p);
> >
> > + /* Calculate timeout */
> > +
> > pa_zero(timeout);
> >
> > - /* Calculate timeout */
> > - if (!p->quit && p->timer_enabled) {
> > + next_time_event = find_next_time_event(p);
> > + if (next_time_event)
> > + pa_timeval_rtstore(&next_time_event_elapse, next_time_event-
> > > time, next_time_event->use_rtclock);
> > +
> > + /* p->timer_enabled and p->next_elapse are controlled by the
> > rtpoll owner,
> > + * while the time events can be created by anyone through
> > pa_mainloop_api.
> > + * It might be a good idea to merge p->timer_enabled and p-
> > > next_elapse
> > + * with the time events so that we wouldn't need to handle them
> > separately
> > + * here. The reason why they are currently separate is that the
> > + * pa_mainloop_api interface was bolted on pa_rtpoll as an
> > afterthought. */
> > + timer_enabled = p->timer_enabled || next_time_event;
> > +
> > + if (!p->quit && timer_enabled) {
> > + struct timeval *next_elapse;
> > struct timeval now;
> > +
> > + if (p->timer_enabled && next_time_event) {
> > + if (pa_timeval_cmp(&p->next_elapse,
> > &next_time_event_elapse) > 0)
> > + next_elapse = &next_time_event_elapse;
> > + else
> > + next_elapse = &p->next_elapse;
> > + } else if (p->timer_enabled)
> > + next_elapse = &p->next_elapse;
> > + else
> > + next_elapse = &next_time_event_elapse;
> > +
> > pa_rtclock_get(&now);
> >
> > - if (pa_timeval_cmp(&p->next_elapse, &now) > 0)
> > - pa_timeval_add(&timeout, pa_timeval_diff(&p-
> > > next_elapse, &now));
> > + if (pa_timeval_cmp(next_elapse, &now) > 0)
> > + pa_timeval_add(&timeout, pa_timeval_diff(next_elapse,
> > &now));
> > }
> >
> > #ifdef DEBUG_TIMING
> > @@ -298,7 +777,7 @@ int pa_rtpoll_run(pa_rtpoll *p) {
> > pa_usec_t now = pa_rtclock_now();
> > p->awake = now - p->timestamp;
> > p->timestamp = now;
> > - if (!p->quit && p->timer_enabled)
> > + if (!p->quit && timer_enabled)
> > pa_log("poll timeout: %d ms ",(int)
> > ((timeout.tv_sec*1000) + (timeout.tv_usec / 1000)));
> > else if (q->quit)
> > pa_log("poll timeout is ZERO");
> > @@ -313,12 +792,21 @@ int pa_rtpoll_run(pa_rtpoll *p) {
> > struct timespec ts;
> > ts.tv_sec = timeout.tv_sec;
> > ts.tv_nsec = timeout.tv_usec * 1000;
> > - r = ppoll(p->pollfd, p->n_pollfd_used, (p->quit || p-
> > > timer_enabled) ? &ts : NULL, NULL);
> > + r = ppoll(p->pollfd, p->n_pollfd_used, (p->quit ||
> > timer_enabled) ? &ts : NULL, NULL);
> > }
> > #else
> > - r = pa_poll(p->pollfd, p->n_pollfd_used, (p->quit || p-
> > > timer_enabled) ? (int) ((timeout.tv_sec*1000) + (timeout.tv_usec /
> > 1000)) : -1);
> > + r = pa_poll(p->pollfd, p->n_pollfd_used, (p->quit ||
> > timer_enabled) ? (int) ((timeout.tv_sec*1000) + (timeout.tv_usec /
> > 1000)) : -1);
> > #endif
> >
> > + /* FIXME: We don't know whether the pa_rtpoll owner's timer
> > elapsed or one
> > + * of the time events created by others through pa_mainloop_api.
> > The alsa
> > + * sink and source use pa_rtpoll_timer_elapsed() to check
> > whether *their*
> > + * timer elapsed, so this ambiguity is a problem for them in
> > theory.
> > + * However, currently the pa_rtpoll objects of the alsa sink and
> > source are
> > + * not being used through pa_mainloop_api, so in practice
> > there's no
> > + * ambiguity. We could use pa_rtclock_now() to check whether p-
> > > next_elapse
> > + * is in the past, but we don't do that currently, because
> > pa_rtclock_now()
> > + * is somewhat expensive and this ambiguity isn't currently a
> > big issue. */
> > p->timer_elapsed = r == 0;
> >
> > #ifdef DEBUG_TIMING
> > diff --git a/src/pulsecore/rtpoll.h b/src/pulsecore/rtpoll.h
> > index c0a4dda..e2aee73 100644
> > --- a/src/pulsecore/rtpoll.h
> > +++ b/src/pulsecore/rtpoll.h
> > @@ -25,7 +25,9 @@
> > #include
> > #include
> >
> > +#include
> > #include
> > +
> > #include
> > #include
> > #include
> > @@ -56,6 +58,8 @@ typedef enum pa_rtpoll_priority {
> > pa_rtpoll *pa_rtpoll_new(void);
> > void pa_rtpoll_free(pa_rtpoll *p);
> >
> > +pa_mainloop_api *pa_rtpoll_get_mainloop_api(pa_rtpoll *rtpoll);
> > +
> > /* Sleep on the rtpoll until the time event, or any of the fd events
> > * is triggered. Returns negative on error, positive if the loop
> > * should continue to run, 0 when the loop should be terminated
>
> Modulo looking at the test and making sure everything works as expected
> (and we can also make sure it is valgrind clean, manually). This looks
> okay to me.
Thanks for the review!
--
Tanu
More information about the pulseaudio-discuss
mailing list