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

[Steve Lhomme]

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.