[vlc-devel] [PATCH 6/9] preparser: add playlist_preparser_Cancel

[Rémi Denis-Courmont]

On Wednesday 01 June 2016 18:46:33 Thomas Guillem wrote:
> On Wed, Jun 1, 2016, at 16:25, Rémi Denis-Courmont wrote:
> > Le 2016-06-01 13:24, Thomas Guillem a écrit :
> > > ---
> > > 
> > >  src/playlist/preparser.c | 23 +++++++++++++++++++++++
> > >  src/playlist/preparser.h |  8 ++++++++
> > >  2 files changed, 31 insertions(+)
> > > 
> > > diff --git a/src/playlist/preparser.c b/src/playlist/preparser.c
> > > index a5751c5..13d5d3c 100644
> > > --- a/src/playlist/preparser.c
> > > +++ b/src/playlist/preparser.c
> > > @@ -116,6 +116,29 @@ void playlist_preparser_fetcher_Push(
> > > playlist_preparser_t *p_preparser,
> > > 
> > >          playlist_fetcher_Push( p_preparser->p_fetcher, p_item,
> > > 
> > > i_options );
> > > 
> > >  }
> > > 
> > > +void playlist_preparser_Cancel( playlist_preparser_t *p_preparser,
> > > +                                input_item_t *p_item )
> > > +{
> > > +    vlc_mutex_lock( &p_preparser->lock );
> > > +    /* Remove the item from the queue (if present) */
> > > +    for( unsigned int i = 0; i < p_preparser->i_waiting; ++i )
> > > +    {
> > > +        preparser_entry_t *p_entry = p_preparser->pp_waiting[0];
> > > +        if( p_entry->p_item == p_item )
> > > +        {
> > > +            vlc_gc_decref( p_entry->p_item );
> > > +            free( p_entry );
> > > +            REMOVE_ELEM( p_preparser->pp_waiting,
> > > p_preparser->i_waiting, i );
> > > +            break;
> > > +        }
> > > +    }
> > > +    /* Stop the input_thread reading the item (if any) */
> > > +    if( p_preparser->input != NULL
> > > +     && input_GetItem( p_preparser->input ) == p_item )
> > > +        input_Stop( p_preparser->input );
> > > +    vlc_mutex_unlock( &p_preparser->lock );
> > > +}
> > > +
> > 
> > Items are reference counted, so you can have a situation where more
> > than one something wants to preparse the same item. So, you can't just
> > cancel it like that.
> 
> Ah yes, good catch.
> I modified my patches: playlist_preparser_Push() can take a void *. This
> void * can be used as a unique id to identity the caller of the request.
> playlist_preparser_Cancel() cancel all requests done with this id.

I think it´s overkill and fear it will end up worse rather than better.

Of course, it´s tempting to cancel preparsing when the UI is no longer showing 
the item. But then again, if you started preparsing, might as well finish it 
for next time. All the more so considering that items in the playlist cannot 
be truly deleted until the playlist itself is deleted (with the current crappy 
design). You might end up starting, canceling, then starting again then 
canceling again and so on. Better start, finish, and be done with it once and 
for all.

It´s also quite a lot of state to retain and complexity on both side for, 
AFAIK, little gain.

-- 
Rémi Denis-Courmont
http://www.remlab.net/