[PATCH 4/5] rtpoll: Implement pa_mainloop_api support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux