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

[Rémi Denis-Courmont]

Le keskiviikkona 22. maaliskuuta 2017, 18.43.44 EET Filip Roséen a écrit :
> > 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?

I don´t particularly like src/misc either, but that still seems saner than 
src/playlist/

(...)
> > 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).

I would just inline the private structure into the public one. Or rather, I 
would design the thing so that there are no public members at all.

You can still use init/deinit instead of new/delete if you want. Only expose 
the structure layout is exposed for the purpose of allocation/storage.

> 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.

Sorry, I am not sure what you are trying to express here.

My point is that pf_release is also used in function calls, and without the 
lock. So it has to be reentrant as things stand. And I´m not sure that´s a 
good idea. At the very least, this would need to be clearly and explicitly 
documented.
 

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