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

Rémi Denis-Courmont remi at remlab.net
Wed Mar 22 18:27:54 CET 2017


Le keskiviikkona 22. maaliskuuta 2017, 17.58.41 EET Filip Roséen a écrit :
> The primary reason for this helper utility is to allow for the
> preparser and {meta,art}-fetcher to share a common backbone as they
> both deal with a queue of entities that are to be processed one-by-one
> in the background.
> 
> By using struct background_worker in the preparser and fetcher we will
> prevent code duplication, while also allowing for easier maintenence in
> the future (such as allowing for concurrently running tasks within a
> single queue).
> ---
>  src/Makefile.am                       |   2 +
>  src/playlist/misc/background_worker.c | 192
> ++++++++++++++++++++++++++++++++++ src/playlist/misc/background_worker.h | 
> 62 +++++++++++
>  3 files changed, 256 insertions(+)
>  create mode 100644 src/playlist/misc/background_worker.c
>  create mode 100644 src/playlist/misc/background_worker.h
> 
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 1e0e70fa86..2aa8891026 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -220,6 +220,8 @@ libvlccore_la_SOURCES = \
>  	playlist/fetcher.h \
>  	playlist/sort.c \
>  	playlist/loadsave.c \
> +	playlist/misc/background_worker.c \
> +	playlist/misc/background_worker.h \

Preparsing and meta fetching are in the playlist directory because of 
hysterical raisins. I remove the dependency on the playlist because LibVLC 
needs them too.

I think you should not keep reproducing the same.

>  	playlist/preparser.c \
>  	playlist/preparser.h \
>  	playlist/tree.c \
> diff --git a/src/playlist/misc/background_worker.c
> b/src/playlist/misc/background_worker.c new file mode 100644
> index 0000000000..0c6041e6ef
> --- /dev/null
> +++ b/src/playlist/misc/background_worker.c
> @@ -0,0 +1,192 @@
> +/**************************************************************************
> *** + * 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"
> +
> +static void* Thread( void* data )
> +{
> +    struct background_worker* worker = data;
> +    struct background_worker_private* priv = &worker->p;

I don´t see the point in doing that there. You have all the disadvantage of 
insulated private data, and none of the advantages, since the private data is 
still exposed to the owner.

> +
> +    vlc_mutex_lock( &priv->lock );
> +    for( ;; )
> +    {
> +        vlc_cond_broadcast( &priv->wait );
> +
> +        if( priv->current.handle == NULL )
> +        {
> +            if( vlc_array_count( &priv->queue ) == 0 )
> +                break;
> +
> +            struct background_worker_item* item =
> +                vlc_array_item_at_index( &priv->queue, 0 );
> +
> +            void* handle;
> +            if( !worker->pf_start( priv->owner, item->entity, &handle ) )
> +            {
> +                priv->current.item = item;
> +                priv->current.handle = handle;
> +                priv->current.deadline = item->timeout > VLC_TS_INVALID
> +                    ? mdate() + item->timeout : INT64_MAX;
> +            }
> +
> +            vlc_array_remove( &priv->queue, 0 );
> +            continue;
> +        }
> +
> +        for( ;; )
> +        {
> +            bool const b_timeout = priv->current.deadline < mdate();
> +
> +            vlc_mutex_unlock( &priv->lock );
> +
> +            if( b_timeout ||
> +                worker->pf_probe( priv->owner, priv->current.handle ) )
> +            {
> +                worker->pf_stop( priv->owner, priv->current.handle );
> +                worker->pf_release( priv->current.item->entity );
> +
> +                vlc_mutex_lock( &priv->lock );
> +                break;
> +            }
> +
> +            vlc_mutex_lock( &priv->lock );
> +            vlc_cond_timedwait( &priv->wait, &priv->lock,
> +                                 priv->current.deadline );

This sequence of two instructions can´t be correct, AFAIK.

> +        }
> +
> +        priv->current.item = NULL;
> +        priv->current.handle = NULL;
> +    }
> +
> +    priv->active = false;
> +    vlc_mutex_unlock( &priv->lock );
> +    return NULL;
> +}
> +
> +static void BackgroundWorkerCancel( struct background_worker* worker, void*
> id) +{
> +    struct background_worker_private* priv = &worker->p;
> +
> +    vlc_mutex_lock( &priv->lock );
> +    for( size_t i = 0; i < vlc_array_count( &priv->queue ); )
> +    {
> +        struct background_worker_item* item =
> +            vlc_array_item_at_index( &priv->queue, i );
> +
> +        if( id == NULL || item->id == id )
> +        {
> +            vlc_array_remove( &priv->queue, i );

Looks like reinventing vlc_array_index_of_item().

> +            worker->pf_release( item->entity );

This seems to assume that the callbacks are reentrant. It might be true. But 
it seems like a dangerous design assumption.

> +            free( item );
> +            continue;
> +        }
> +
> +        ++i;
> +    }
> +
> +    if( ( id && priv->current.item && priv->current.item->id == id ) ||
> +        ( id == NULL && priv->active ) )
> +    {
> +        priv->current.deadline = VLC_TS_0;
> +        vlc_cond_broadcast( &priv->wait );
> +
> +        while( ( id && priv->current.item && priv->current.item->id == id )
> || +               ( id == NULL && priv->active ) )
> +        {
> +            vlc_cond_wait( &priv->wait, &priv->lock );
> +        }

This sequence cannot be correct.

> +    }
> +    vlc_mutex_unlock( &priv->lock );
> +}
> +
> +void background_worker_Init( struct background_worker* worker, void* owner,
> +                             int timeout )
> +{
> +    worker->p.default_timeout = timeout > 0 ? timeout*1000 :
> VLC_TS_INVALID; +    worker->p.current.handle = NULL;
> +    worker->p.current.item = NULL;
> +    worker->p.active = false;
> +    worker->p.owner = owner;
> +
> +    vlc_array_init( &worker->p.queue );
> +    vlc_mutex_init( &worker->p.lock );
> +    vlc_cond_init( &worker->p.wait );
> +}
> +
> +int background_worker_Push( struct background_worker* worker, void* entity,
> +                        void* id, int timeout )
> +{
> +    struct background_worker_item* item = malloc( sizeof( *item ) );
> +    struct background_worker_private* priv = &worker->p;
> +
> +    if( unlikely( !item ) )
> +        return VLC_EGENERIC;
> +
> +    item->id = id;
> +    item->entity = entity;
> +    item->timeout = timeout > 0 ? timeout*1000 : worker->p.default_timeout;
> +
> +    vlc_mutex_lock( &worker->p.lock );
> +    vlc_array_append( &worker->p.queue, item );
> +
> +    if( !worker->p.active &&
> +        vlc_clone_detach( NULL, Thread, worker, VLC_THREAD_PRIORITY_LOW ) )
> +    {
> +        vlc_mutex_unlock( &priv->lock );
> +        return VLC_EGENERIC;
> +    }
> +
> +    worker->p.active = true;
> +    worker->pf_hold( item->entity );
> +    vlc_mutex_unlock( &priv->lock );
> +
> +    return VLC_SUCCESS;
> +}
> +
> +void background_worker_Cancel( struct background_worker* worker, void* id )
> +{
> +    BackgroundWorkerCancel( worker, id );
> +}
> +
> +void background_worker_Signal( struct background_worker* worker )
> +{
> +    struct background_worker_private* priv = &worker->p;
> +
> +    vlc_mutex_lock( &priv->lock );
> +    vlc_cond_broadcast( &priv->wait );
> +    vlc_mutex_unlock( &priv->lock );

This sequence cannot be right.

> +}
> +
> +void background_worker_Clean( struct background_worker* worker )
> +{
> +    BackgroundWorkerCancel( worker, NULL );
> +
> +    vlc_array_clear( &worker->p.queue );
> +    vlc_cond_destroy( &worker->p.wait );
> +    vlc_mutex_destroy( &worker->p.lock );
> +}
> diff --git a/src/playlist/misc/background_worker.h
> b/src/playlist/misc/background_worker.h new file mode 100644
> index 0000000000..3d78d63468
> --- /dev/null
> +++ b/src/playlist/misc/background_worker.h
> @@ -0,0 +1,62 @@
> +/**************************************************************************
> *** + * 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. +
> ***************************************************************************
> **/ +
> +#ifndef BACKGROUND_WORKER_H__
> +#define BACKGROUND_WORKER_H__
> +
> +struct background_worker_item {
> +    void* id;
> +    void* entity;
> +    int timeout;
> +};
> +
> +struct background_worker_private {
> +    void* owner;
> +    int default_timeout;
> +
> +    vlc_mutex_t lock;
> +    vlc_cond_t wait;
> +
> +    struct {
> +        void* handle;
> +        mtime_t deadline;
> +        struct background_worker_item* item;
> +    } current;
> +
> +    bool active;
> +    vlc_array_t queue;
> +};
> +
> +struct background_worker {
> +    void( *pf_release )( void* );
> +    void( *pf_hold )( void* );
> +
> +    int( *pf_start )( void* owner, void* item, void** out );
> +    int( *pf_probe )( void* owner, void* handle );
> +    void( *pf_stop )( void* owner, void* handle );

The callback names are rather obscure, and it does not help that they are not 
documented.

> +
> +    struct background_worker_private p;
> +};
> +
> +void background_worker_Init( struct background_worker* worker, void* owner,
> int timeout ); +void background_worker_Cancel( struct background_worker*
> worker, void* id ); +void background_worker_Signal( struct
> background_worker* worker ); +void background_worker_Clean( struct
> background_worker* worker ); +int background_worker_Push( struct
> background_worker* worker, void* entity, void* id, int timeout ); +
> +#endif


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



More information about the vlc-devel mailing list