[vlc-devel] [RFC v2 2/2] preparser: use the new executor API

[Alexandre Janniaux]

Hi,

On Wed, Sep 02, 2020 at 12:04:49PM +0200, Steve Lhomme wrote:
> On 2020-09-02 11:06, Romain Vimont wrote:
> > On Wed, Sep 02, 2020 at 08:28:14AM +0200, Steve Lhomme wrote:
> > > On 2020-09-01 18:13, Romain Vimont wrote:
> > > > Replace the background_worker by an executor.
> > > > ---
> > > >    src/preparser/preparser.c | 399 ++++++++++++++++++++++++--------------
> > > >    1 file changed, 251 insertions(+), 148 deletions(-)
> > > >
> > > > diff --git a/src/preparser/preparser.c b/src/preparser/preparser.c
> > > > index 723feb96dc..c78e956985 100644
> > > > --- a/src/preparser/preparser.c
> > > > +++ b/src/preparser/preparser.c
> > > > @@ -24,8 +24,9 @@
> > > >    #include <vlc_common.h>
> > > >    #include <vlc_atomic.h>
> > > > +#include <vlc_executor.h>
> > > > +#include <vlc_tick.h>
> > >
> > > Why do we need this include ? We never needed it before.
> >
> > We don't. It's from an older version of the executor :)
> >
> >
> > > > -#include "misc/background_worker.h"
> > > >    #include "input/input_interface.h"
> > > >    #include "input/input_internal.h"
> > > >    #include "preparser.h"
> > > > @@ -35,222 +36,307 @@ struct input_preparser_t
> > > >    {
> > > >        vlc_object_t* owner;
> > > >        input_fetcher_t* fetcher;
> > > > -    struct background_worker* worker;
> > > > +    vlc_executor_t *executor;
> > > > +    vlc_tick_t default_timeout;
> > > >        atomic_bool deactivated;
> > > > +
> > > > +    vlc_mutex_t lock;
> > > > +    struct vlc_list submitted_tasks; /**< list of struct input_preparser_task */
> > > >    };
> > > > -typedef struct input_preparser_req_t
> > > > +struct input_preparser_task
> > > >    {
> > > > +    input_preparser_t *preparser;
> > > >        input_item_t *item;
> > > >        input_item_meta_request_option_t options;
> > > >        const input_preparser_callbacks_t *cbs;
> > > >        void *userdata;
> > > > -    vlc_atomic_rc_t rc;
> > > > -} input_preparser_req_t;
> > > > +    void *id;
> > > > +    vlc_tick_t timeout;
> > > > -typedef struct input_preparser_task_t
> > > > -{
> > > > -    input_preparser_req_t *req;
> > > > -    input_preparser_t* preparser;
> > > > -    int preparse_status;
> > > >        input_item_parser_id_t *parser;
> > > > -    atomic_int state;
> > > > -    atomic_bool done;
> > > > -} input_preparser_task_t;
> > > > -
> > > > -static input_preparser_req_t *ReqCreate(input_item_t *item,
> > > > -                                        input_item_meta_request_option_t options,
> > > > -                                        const input_preparser_callbacks_t *cbs,
> > > > -                                        void *userdata)
> > > > +
> > > > +    vlc_mutex_t lock;
> > > > +    vlc_cond_t cond_ended;
> > > > +    bool preparse_ended;
> > > > +    int preparse_status;
> > > > +    bool fetch_ended;
> > > > +
> > > > +    atomic_bool interrupted;
> > > > +
> > > > +    struct vlc_runnable runnable; /**< to be passed to the executor */
> > > > +
> > > > +    struct vlc_list node; /**< node of input_preparser_t.submitted_tasks */
> > > > +};
> > > > +
> > > > +static void RunnableRun(void *);
> > > > +
> > > > +static struct input_preparser_task *
> > > > +TaskNew(input_preparser_t *preparser, input_item_t *item,
> > > > +        input_item_meta_request_option_t options,
> > > > +        const input_preparser_callbacks_t *cbs, void *userdata,
> > > > +        void *id, vlc_tick_t timeout)
> > > >    {
> > > > -    input_preparser_req_t *req = malloc(sizeof(*req));
> > > > -    if (unlikely(!req))
> > > > +    assert(timeout >= 0);
> > >
> > > Did you mean VLC_TICK_INVALID ?
> >
> > I keep the meaning exposed to the user:
> >   - -1: use the default timeout (so not possible here)
> >   - 0: infinite timeout
> >   - >0: use the given timeout
>
> That's not how vlc_tick_t is usually used. You should have defines for these
> values.
> An infinite timeout should be INT64_MAX, not VLC_TICK_INVALID.

This seems to be a preexisting issue independant of this patchset, though
I'm not sure of the matching criteria here.

    // From background_worker.h

    /**
     * Default timeout for completing a task
     *
     * If less-than 0 a task can run indefinitely without being killed, whereas
     * a positive value denotes the maximum number of milliseconds a task can
     * run before \ref pf_stop is called to kill it.
     **/
    vlc_tick_t default_timeout;

Regards,
--
Alexandre Janniaux
Videolabs