[vlc-devel] [PATCH 1/7] playlist/background_worker: introduce background-worker utility

[Rémi Denis-Courmont]

Le perjantaina 24. maaliskuuta 2017, 3.28.28 EEST Filip Roséen a écrit :
> This added utility will make it easier to handle a queue of tasks that
> is to be finished in the order received. It allows for users of the
> utility to focus on the end-goal instead of having to deal with
> synchronization issues in terms of task-queue handling.
> 
> --
> 
> Changes since previous submission:
> 
>  - added documentation
>  - moved from src/playlist/ to src/misc
>  - background-worker are now constructed through a single call
>  - background-worker internals are now fully private
>  - rewrote thread-synchronization logic to be more robust (and easier
>    to prove correct)
> ---
>  src/Makefile.am              |   2 +
>  src/misc/background_worker.c | 235
> +++++++++++++++++++++++++++++++++++++++++++ src/misc/background_worker.h |
> 182 +++++++++++++++++++++++++++++++++ 3 files changed, 419 insertions(+)
>  create mode 100644 src/misc/background_worker.c
>  create mode 100644 src/misc/background_worker.h
> 
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 1e0e70fa86..208128d076 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -310,6 +310,8 @@ libvlccore_la_SOURCES = \
>  	text/filesystem.c \
>  	text/iso_lang.c \
>  	text/iso-639_def.h \
> +	misc/background_worker.c \
> +	misc/background_worker.h \
>  	misc/md5.c \
>  	misc/probe.c \
>  	misc/rand.c \
> diff --git a/src/misc/background_worker.c b/src/misc/background_worker.c
> new file mode 100644
> index 0000000000..527509fb98
> --- /dev/null
> +++ b/src/misc/background_worker.c
> @@ -0,0 +1,235 @@
> +/**************************************************************************
> *** + * Copyright (C) 2017 VLC authors and VideoLAN
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License as published by
> + * the Free Software Foundation; either version 2.1 of the License, or + *
> (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program; if not, write to the Free Software Foundation,
> + * Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301, USA. +
> ***************************************************************************
> **/ +
> +#ifdef HAVE_CONFIG_H
> +#  include "config.h"
> +#endif
> +
> +#include <vlc_common.h>
> +#include <vlc_threads.h>
> +#include <vlc_arrays.h>
> +
> +#include "libvlc.h"
> +#include "background_worker.h"
> +
> +struct bg_queued_item {
> +    void* id; /**< id associated with entity */
> +    void* entity; /**< the entity to process */
> +    int timeout; /**< timeout duration in microseconds */
> +};
> +
> +struct background_worker {
> +    void* owner;
> +    struct background_worker_config conf;
> +
> +    struct {
> +        bool probe_request; /**< true if a probe is requested */

Not clear what this has to do with a supposedly generic framework. In fact, 
not clear what this does at all.

> +        vlc_mutex_t lock; /**< acquire to inspect members that follow */
> +        vlc_cond_t wait; /**< wait for update in terms of head */
> +        mtime_t deadline; /**< deadline of the current task */

Err, likewise. If this is per-item, it wouldn´t be here. And if this is 
internal state of the thread, it should be on the stack.

And if the thread is doing nothing, the whole point of the existing system is 
that the thread should not exist at all.

> +        void* id; /**< id of the current task */

Suspicious.

> +        bool active; /**< true if there is an active thread */
> +    } head;
> +
> +    struct {
> +        vlc_mutex_t lock; /**< acquire to inspect members that follow */
> +        vlc_array_t data; /**< queue of pending entities to process */
> +    } tail;
> +};
> +
> +static void* Thread( void* data )
> +{
> +    struct background_worker* worker = data;
> +
> +    for( ;; )
> +    {
> +        struct bg_queued_item* item = NULL;
> +        void* handle;
> +
> +        vlc_mutex_lock( &worker->tail.lock );
> +        {
> +            if( vlc_array_count( &worker->tail.data ) )
> +            {
> +                item = vlc_array_item_at_index( &worker->tail.data, 0 );
> +                handle = NULL;
> +
> +                vlc_array_remove( &worker->tail.data, 0 );
> +            }
> +
> +            vlc_mutex_lock( &worker->head.lock );
> +            {
> +                worker->head.deadline = INT64_MAX;
> +                worker->head.active = item != NULL;
> +                worker->head.id = item ? item->id : NULL;
> +
> +                if( item && item->timeout > 0 )
> +                    worker->head.deadline = mdate() + item->timeout;
> +            }
> +            vlc_cond_broadcast( &worker->head.wait );

The usage of the CV still does not make sense.

So nack.

-- 
雷米‧德尼-库尔蒙
https://www.remlab.net/