[vlc-devel] [PATCH 1/7] playlist/background_worker: introduce background-worker utility
Filip Roséen
filip at atch.se
Wed Mar 22 18:43:44 CET 2017
Hi Rémi,
On 2017-03-22 19:27, Rémi Denis-Courmont wrote:
> 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 was originally planning to store the background-worker elsewhere, as
there are other entities that could benefit form the same
implementatation, but as those locations are affected by patches in a
future branch (already written, but it simply moves the files) I
thought I'd first propose them to be under `src/playlist/misc`, and
later move it when there is a "universal" usage.
> I think you should not keep reproducing the same.
Where would you recommend putting them? I was struggling to find a
good place, but I guess `src/misc` is good enough?
> > 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.
The reason there is a *data-member* of type `struct
background_worker_private` was because, inspired by some recent
changes by you (which I really like), I wanted
`background_worker_Init` to be always successful (in order to prevent
the number of error-paths).
It can of course be moved to be *translation-unit* local to
`src/playlist/misc/background_worker.c`.
> > +
> > + 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.
`(1)`
Oh that was an unfortunate side-effect of some heavy rebasing right
before pushing to *vlc-devel*, the signal can of course be lost if the
background-task finishes during the time the lock is released.
I will fix this in an upcoming patch, thank you!
> > + }
> > +
> > + 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().
I was tempted to introduce an alternative to `vlc_array_index_of_item`
that would take a callback so that one could query something other
than the item-address itself (but have the same semantics as
`vlc_array_index_of_item`).
It would simplify the code, but I did not find too many occurances
where such a new function would benefit the codebase as a whole, and
as it wouldn't be used too many times in `background_worker.c`, I was
hesitant to add a *translation-unit* local helper.
>
> > + 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.
A `pf_release` callback that would release the worker in which the
item is part of would be the only case where we would end up in a
reentrent state, and as I thought that did not make sense I didn't go
out to make that a valid usecase.
> > + 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.
Will be clearified when I fix `(1)`.
> > + }
> > + 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.
Will be adjusted as part of `(1)` to make the intent more clear.
> > +}
> > +
> > +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.
My initial conclusion was that since it was a helper with limited
use-case at the current time, it would be safe to not have them
documented; but given your note I will take time to annotate the
functions with relevant documentation to make the implementation more
robust.
> > +
> > + 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
>
Thank you very much for the review! I will return with the necessary
changes as soon as possible (probably in the upcoming 2 hours, or
sooner).
Best Regards,\
Filip
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170322/be48aeed/attachment.html>
More information about the vlc-devel
mailing list