<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
  <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
  <meta http-equiv="Content-Style-Type" content="text/css" />
  <meta name="generator" content="pandoc" />
  <title></title>
  <style type="text/css">code{white-space: pre;}</style>
</head>
<body>
<p>Hi Rémi,</p>
<p>On 2017-03-29 20:30, Rémi Denis-Courmont wrote:</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> Le perjantaina 24. maaliskuuta 2017, 3.28.29 EEST Filip Roséen a écrit :</code></pre>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> This refactoring should not only allow for easier maintenance as the
 code size has shrunk, it also implements a few advantages over the
 previous implementation:

  - playlist_preparser_Cancel is now optionally blocking if the
    referred to item is currently being preparsed (required in cases
    where another action would race with the preparser, such as
    playback (as preparsing and playing an entity at the same time can
    lead to duplicate items in the playlist).</code></pre>
</blockquote>
<pre><code> That sounds very prone to deadlock and therefore a bad idea. (Though you could 
 argue that cancel itself is the bad idea.)</code></pre>
</blockquote>
<p>The cancellation is required in order for us to not run into issues where we preparse and entity while it is requested for playback, potentially resulting in duplicate entries in the playlist (as described by #17232).</p>
<p>The mechanism could, of course, be renamed to <code>playlist_preparser_DeleteFromQueueOrWaitForFinish</code> (though something better, though I cannot come up with a good name right now).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code>  - the congestion in terms of interacting with the preparser, and the
    preparsing itself, is lower. Meaning that we will finish a queue of
    items to preparse faster than with the old implementation.

 --

 Changes since previous submission:

  - Adjust to changes in terms of background-worker api
 ---
  src/playlist/preparser.c | 406
 +++++++++++++---------------------------------- 1 file changed, 109
 insertions(+), 297 deletions(-)

 diff --git a/src/playlist/preparser.c b/src/playlist/preparser.c
 index dd25284e81..c6cdab2b2c 100644
 --- a/src/playlist/preparser.c
 +++ b/src/playlist/preparser.c
 @@ -1,12 +1,9 @@
  /**************************************************************************
 *** - * preparser.c: Preparser thread.
 + * preparser.c

 ***************************************************************************
 ** - * Copyright Â© 1999-2009 VLC authors and VideoLAN
 + * Copyright Â© 2017-2017 VLC authors and VideoLAN
   * $Id$
   *
 - * Authors: Samuel Hocevar <sam@zoy.org>
 - *          Clément Stenac <zorglub@videolan.org>
 - *
   * 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 @@
 -21,351 +18,166 @@
   * 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 <assert.h>
 -
  #include <vlc_common.h>

 -#include "fetcher.h"
 -#include "preparser.h"
 +#include "misc/background_worker.h"
  #include "input/input_interface.h"
 -
 -/**************************************************************************
 *** - * Structures/definitions
 -
 ***************************************************************************
 **/ -typedef struct preparser_entry_t preparser_entry_t;
 -
 -struct preparser_entry_t
 -{
 -    input_item_t    *p_item;
 -    input_item_meta_request_option_t i_options;
 -    void            *id;
 -    mtime_t          timeout;
 -};
 +#include "input/input_internal.h"
 +#include "preparser.h"
 +#include "fetcher.h"

  struct playlist_preparser_t
  {
 -    vlc_object_t        *object;
 -    playlist_fetcher_t  *p_fetcher;
 -    mtime_t              default_timeout;
 -
 -    void                *input_id;
 -    enum {
 -        INPUT_RUNNING,
 -        INPUT_STOPPED,
 -        INPUT_CANCELED,
 -    } input_state;
 -
 -    vlc_mutex_t     lock;
 -    vlc_cond_t      wait;
 -    vlc_cond_t      thread_wait;
 -    bool            b_live;
 -    preparser_entry_t  **pp_waiting;
 -    size_t          i_waiting;
 +    vlc_object_t* owner;
 +    playlist_fetcher_t* fetcher;
 +    struct background_worker* worker;
  };

 -static void *Thread( void * );
 -
 -/**************************************************************************
 *** - * Public functions
 -
 ***************************************************************************
 **/ -playlist_preparser_t *playlist_preparser_New( vlc_object_t *parent )
 +static int InputEvent( vlc_object_t* obj, const char* varname,
 +    vlc_value_t old, vlc_value_t cur, void* worker )
  {
 -    playlist_preparser_t *p_preparser = malloc( sizeof(*p_preparser) );
 -    if( !p_preparser )
 -        return NULL;
 +    VLC_UNUSED( obj ); VLC_UNUSED( varname ); VLC_UNUSED( old );

 -    p_preparser->input_id = NULL;
 -    p_preparser->input_state = INPUT_RUNNING;
 -    p_preparser->object = parent;
 -    p_preparser->default_timeout = var_InheritInteger( parent,
 "preparse-timeout" ); -    p_preparser->p_fetcher = playlist_fetcher_New(
 parent );
 -    if( unlikely(p_preparser->p_fetcher == NULL) )
 -        msg_Err( parent, "cannot create fetcher" );
 -
 -    vlc_mutex_init( &p_preparser->lock );
 -    vlc_cond_init( &p_preparser->wait );
 -    vlc_cond_init( &p_preparser->thread_wait );
 -    p_preparser->b_live = false;
 -    p_preparser->i_waiting = 0;
 -    p_preparser->pp_waiting = NULL;
 -
 -    return p_preparser;
 -}
 -
 -void playlist_preparser_Push( playlist_preparser_t *p_preparser,
 input_item_t *p_item, -                             
 input_item_meta_request_option_t i_options, -                             
 int timeout, void *id )
 -{
 -    preparser_entry_t *p_entry = malloc( sizeof(preparser_entry_t) );
 -
 -    if ( !p_entry )
 -        return;
 -    p_entry->p_item = p_item;
 -    p_entry->i_options = i_options;
 -    p_entry->id = id;
 -    p_entry->timeout = (timeout < 0 ? p_preparser->default_timeout :
 timeout) * 1000; -    vlc_gc_incref( p_entry->p_item );
 -
 -    vlc_mutex_lock( &p_preparser->lock );
 -    INSERT_ELEM( p_preparser->pp_waiting, p_preparser->i_waiting,
 -                 p_preparser->i_waiting, p_entry );
 -    if( !p_preparser->b_live )
 -    {
 -        if( vlc_clone_detach( NULL, Thread, p_preparser,
 -                              VLC_THREAD_PRIORITY_LOW ) )
 -            msg_Warn( p_preparser->object, "cannot spawn pre-parser thread"
 ); -        else
 -            p_preparser->b_live = true;
 -    }
 -    vlc_mutex_unlock( &p_preparser->lock );
 -}
 +    if( cur.i_int == INPUT_EVENT_DEAD )
 +        background_worker_RequestProbe( worker );</code></pre>
</blockquote>
<pre><code> It might work, but it is probably either a misnomer, or a hack.</code></pre>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> -void playlist_preparser_fetcher_Push( playlist_preparser_t *p_preparser,
 -             input_item_t *p_item, input_item_meta_request_option_t
 i_options ) -{
 -    if( p_preparser->p_fetcher != NULL )
 -        playlist_fetcher_Push( p_preparser->p_fetcher, p_item, i_options );
 +    return VLC_SUCCESS;
  }

 -void playlist_preparser_Cancel( playlist_preparser_t *p_preparser, void *id
 ) +static int PreparserOpenInput( void* preparser_, void* item_, void** out
 ) {
 -    assert( id != NULL );
 -    vlc_mutex_lock( &p_preparser->lock );
 +    playlist_preparser_t* preparser = preparser_;

 -    /* Remove entries that match with the id */
 -    for( int i = p_preparser->i_waiting - 1; i >= 0; --i )
 +    input_thread_t* input = input_CreatePreparser( preparser->owner, item_
 ); +    if( !input )
      {
 -        preparser_entry_t *p_entry = p_preparser->pp_waiting[i];
 -        if( p_entry->id == id )
 -        {
 -            vlc_gc_decref( p_entry->p_item );
 -            free( p_entry );
 -            REMOVE_ELEM( p_preparser->pp_waiting, p_preparser->i_waiting, i
 ); -        }
 +        input_item_SignalPreparseEnded( item_, ITEM_PREPARSE_FAILED );
 +        return VLC_EGENERIC;
      }

 -    /* Stop the input_thread reading the item (if any) */
 -    if( p_preparser->input_id == id )
 +    var_AddCallback( input, "intf-event", InputEvent, preparser->worker );
 +    if( input_Start( input ) )
      {
 -        p_preparser->input_state = INPUT_CANCELED;
 -        vlc_cond_signal( &p_preparser->thread_wait );
 +        input_Close( input );
 +        var_DelCallback( input, "intf-event", InputEvent, preparser->worker
 ); +        input_item_SignalPreparseEnded( item_, ITEM_PREPARSE_FAILED );
 +        return VLC_EGENERIC;
      }
 -    vlc_mutex_unlock( &p_preparser->lock );
 +
 +    *out = input;
 +    return VLC_SUCCESS;
  }

 -void playlist_preparser_Delete( playlist_preparser_t *p_preparser )
 +static int PreparserProbeInput( void* preparser_, void* input_ )
  {
 -    vlc_mutex_lock( &p_preparser->lock );
 -    /* Remove pending item to speed up preparser thread exit */
 -    while( p_preparser->i_waiting > 0 )
 -    {
 -        preparser_entry_t *p_entry = p_preparser->pp_waiting[0];
 -        vlc_gc_decref( p_entry->p_item );
 -        free( p_entry );
 -        REMOVE_ELEM( p_preparser->pp_waiting, p_preparser->i_waiting, 0 );
 -    }
 +    return input_GetState( input_ ) == END_S;
 +    VLC_UNUSED( preparser_ );
 +}

 -    p_preparser->input_state = INPUT_CANCELED;
 -    vlc_cond_signal( &p_preparser->thread_wait );
 +static void PreparserCloseInput( void* preparser_, void* input_ )
 +{
 +    playlist_preparser_t* preparser = preparser_;
 +    input_thread_t* input = input_;
 +    input_item_t* item = input_priv(input)->p_item;

 -    while( p_preparser->b_live )
 -        vlc_cond_wait( &p_preparser->wait, &p_preparser->lock );
 -    vlc_mutex_unlock( &p_preparser->lock );
 +    var_DelCallback( input, "intf-event", InputEvent, preparser->worker );

 -    /* Destroy the item preparser */
 -    vlc_cond_destroy( &p_preparser->thread_wait );
 -    vlc_cond_destroy( &p_preparser->wait );
 -    vlc_mutex_destroy( &p_preparser->lock );
 +    int status = input_GetState( input ) == END_S
 +        ? ITEM_PREPARSE_DONE : ITEM_PREPARSE_TIMEOUT;

 -    if( p_preparser->p_fetcher != NULL )
 -        playlist_fetcher_Delete( p_preparser->p_fetcher );
 -    free( p_preparser );
 +    input_Stop( input );
 +    input_Close( input );
 +
 +    if( preparser->fetcher )
 +        playlist_fetcher_Push( preparser->fetcher, item, 0 );
 +
 +    input_item_SetPreparsed( item, true );
 +    input_item_SignalPreparseEnded( item, status );
  }

 -/**************************************************************************
 *** - * Privates functions
 -
 ***************************************************************************
 **/ +static void InputItemRelease( void* item ) { input_item_Release( item
 ); } +static void InputItemHold( void* item ) { input_item_Hold( item ); }

 -static int InputEvent( vlc_object_t *obj, const char *varname,
 -                       vlc_value_t old, vlc_value_t cur, void *data )
 +playlist_preparser_t* playlist_preparser_New( vlc_object_t *parent )
  {
 -    playlist_preparser_t *preparser = data;
 -    int event = cur.i_int;
 +    playlist_preparser_t* preparser = malloc( sizeof *preparser );

 -    if( event == INPUT_EVENT_DEAD )
 -    {
 -        vlc_mutex_lock( &preparser->lock );
 +    struct background_worker_config conf = {
 +        .default_timeout = var_InheritInteger( parent, "preparse-timeout"
 ), +        .pf_start = PreparserOpenInput,
 +        .pf_probe = PreparserProbeInput,
 +        .pf_stop = PreparserCloseInput,
 +        .pf_release = InputItemRelease,
 +        .pf_hold = InputItemHold };

 -        preparser->input_state = INPUT_STOPPED;
 -        vlc_cond_signal( &preparser->thread_wait );

 -        vlc_mutex_unlock( &preparser->lock );
 +    if( likely( preparser ) )
 +        preparser->worker = background_worker_New( preparser, &conf );
 +
 +    if( unlikely( !preparser || !preparser->worker ) )
 +    {
 +        free( preparser );
 +        return NULL;
      }

 -    (void) obj; (void) varname; (void) old;
 -    return VLC_SUCCESS;
 +    preparser->owner = parent;
 +    preparser->fetcher = playlist_fetcher_New( parent );
 +
 +    if( unlikely( !preparser->fetcher ) )
 +        msg_Warn( parent, "unable to create art fetcher" );
 +
 +    return preparser;
  }

 -/**
 - * This function preparses an item when needed.
 - */
 -static void Preparse( playlist_preparser_t *preparser,
 -                      preparser_entry_t *p_entry )
 +void playlist_preparser_Push( playlist_preparser_t *preparser,
 +    input_item_t *item, input_item_meta_request_option_t i_options,
 +    int timeout, void *id )
  {
 -    input_item_t *p_item = p_entry->p_item;
 -
 -    vlc_mutex_lock( &p_item->lock );
 -    int i_type = p_item->i_type;
 -    bool b_net = p_item->b_net;
 -    vlc_mutex_unlock( &p_item->lock );
 -
 -    bool b_preparse = false;
 -    switch (i_type) {
 -    case ITEM_TYPE_FILE:
 -    case ITEM_TYPE_DIRECTORY:
 -    case ITEM_TYPE_PLAYLIST:
 -    case ITEM_TYPE_NODE:
 -        if( !b_net || p_entry->i_options &
 META_REQUEST_OPTION_SCOPE_NETWORK ) -            b_preparse = true;
 -        break;
 -    }
 +    vlc_mutex_lock( &item->lock );
 +    int i_type = item->i_type;
 +    int b_net = item->b_net;
 +    vlc_mutex_unlock( &item->lock );

 -    /* Do not preparse if it is already done (like by playing it) */
 -    if( b_preparse && !input_item_IsPreparsed( p_item ) )
 +    switch( i_type )
      {
 -        int status;
 -        input_thread_t *input = input_CreatePreparser( preparser->object,
 p_item ); -        if( input == NULL )
 -        {
 -            input_item_SignalPreparseEnded( p_item, ITEM_PREPARSE_FAILED );
 +        case ITEM_TYPE_NODE:
 +        case ITEM_TYPE_FILE:
 +        case ITEM_TYPE_DIRECTORY:
 +        case ITEM_TYPE_PLAYLIST:
 +            if( !b_net || i_options & META_REQUEST_OPTION_SCOPE_NETWORK )
 +                break;
 +        default:
 +            input_item_SignalPreparseEnded( item, ITEM_PREPARSE_SKIPPED );
              return;
 -        }
 -
 -        var_AddCallback( input, "intf-event", InputEvent, preparser );
 -        if( input_Start( input ) == VLC_SUCCESS )
 -        {
 -            vlc_mutex_lock( &preparser->lock );
 -
 -            if( p_entry->timeout > 0 )
 -            {
 -                mtime_t deadline = mdate() + p_entry->timeout;
 -                while( preparser->input_state == INPUT_RUNNING )
 -                {
 -                    if( vlc_cond_timedwait( &preparser->thread_wait,
 -                                            &preparser->lock, deadline ) )
 -                        preparser->input_state = INPUT_CANCELED; /* timeout
 */ -                }
 -            }
 -            else
 -            {
 -                while( preparser->input_state == INPUT_RUNNING )
 -                    vlc_cond_wait( &preparser->thread_wait,
 &preparser->lock ); -            }
 -            assert( preparser->input_state == INPUT_STOPPED
 -                 || preparser->input_state == INPUT_CANCELED );
 -            status = preparser->input_state == INPUT_STOPPED ?
 -                     ITEM_PREPARSE_DONE : ITEM_PREPARSE_TIMEOUT;
 -
 -            vlc_mutex_unlock( &preparser->lock );
 -        }
 -        else
 -            status = ITEM_PREPARSE_FAILED;
 -
 -        var_DelCallback( input, "intf-event", InputEvent, preparser );
 -        if( status == ITEM_PREPARSE_TIMEOUT )
 -            input_Stop( input );
 -        input_Close( input );
 -
 -        var_SetAddress( preparser->object, "item-change", p_item );
 -        input_item_SetPreparsed( p_item, true );
 -        input_item_SignalPreparseEnded( p_item, status );
      }
 -    else
 -        input_item_SignalPreparseEnded( p_item, ITEM_PREPARSE_SKIPPED );
 +
 +    if( background_worker_Push( preparser->worker, item, id, timeout ) )
 +        input_item_SignalPreparseEnded( item, ITEM_PREPARSE_FAILED );
  }

 -/**
 - * This function ask the fetcher object to fetch the art when needed
 - */
 -static void Art( playlist_preparser_t *p_preparser, input_item_t *p_item )
 +void playlist_preparser_fetcher_Push( playlist_preparser_t *preparser,
 +    input_item_t *item, input_item_meta_request_option_t options )
  {
 -    vlc_object_t *obj = p_preparser->object;
 -    playlist_fetcher_t *p_fetcher = p_preparser->p_fetcher;
 -
 -    bool b_fetch = false;
 -    /* If we haven't retrieved enough meta, add to secondary queue
 -     * which will run the "meta fetchers".
 -     * This only checks for meta, not for art
 -     * \todo don't do this for things we won't get meta for, like vids
 -     */
 -
 -    vlc_mutex_lock( &p_item->lock );
 -    if( p_item->p_meta )
 -    {
 -        const char *psz_arturl = vlc_meta_Get( p_item->p_meta,
 vlc_meta_ArtworkURL ); -        const char *psz_name = vlc_meta_Get(
 p_item->p_meta, vlc_meta_Title ); -
 -        if( !psz_arturl || ( strncasecmp( psz_arturl, "file://", 7 ) &&
 -                             strncasecmp( psz_arturl, "attachment://", 13 )
 ) ) -        {
 -            msg_Dbg( obj, "meta ok for %s, need to fetch art",
 -                     psz_name ? psz_name : "(null)" );
 -            b_fetch = true;
 -        }
 -        else
 -        {
 -            msg_Dbg( obj, "no fetch required for %s (art currently %s)",
 -                     psz_name ? psz_name : "(null)",
 -                     psz_arturl ? psz_arturl : "(null)" );
 -        }
 -    }
 -    vlc_mutex_unlock( &p_item->lock );
 +    if( preparser->fetcher )
 +        playlist_fetcher_Push( preparser->fetcher, item, options );
 +}

 -    if( b_fetch && p_fetcher )
 -        playlist_fetcher_Push( p_fetcher, p_item, 0 );
 +void playlist_preparser_Cancel( playlist_preparser_t *preparser, void *id )
 +{
 +    background_worker_Cancel( preparser->worker, id );
  }

 -/**
 - * This function does the preparsing and issues the art fetching requests
 - */
 -static void *Thread( void *data )
 +void playlist_preparser_Delete( playlist_preparser_t *preparser )
  {
 -    playlist_preparser_t *p_preparser = data;
 +    background_worker_Delete( preparser->worker );

 -    for( ;; )
 -    {
 -        preparser_entry_t *p_entry;
 -
 -        vlc_mutex_lock( &p_preparser->lock );
 -        /* */
 -        p_preparser->input_state = INPUT_RUNNING;
 -        if( p_preparser->i_waiting > 0 )
 -        {
 -            p_entry = p_preparser->pp_waiting[0];
 -            p_preparser->input_id = p_entry->id;
 -            REMOVE_ELEM( p_preparser->pp_waiting, p_preparser->i_waiting, 0
 ); -        }
 -        else
 -        {
 -            p_preparser->b_live = false;
 -            p_preparser->input_id = NULL;
 -            vlc_cond_signal( &p_preparser->wait );
 -            vlc_mutex_unlock( &p_preparser->lock );
 -            break;
 -        }
 -        vlc_mutex_unlock( &p_preparser->lock );
 -        assert( p_entry );
 -
 -        Preparse( p_preparser, p_entry );
 -
 -        Art( p_preparser, p_entry->p_item );
 -        vlc_gc_decref( p_entry->p_item );
 -        free( p_entry );
 -    }
 -    return NULL;
 -}
 +    if( preparser->fetcher )
 +        playlist_fetcher_Delete( preparser->fetcher );

 +    free( preparser );
 +}</code></pre>
</blockquote>
<pre><code> -- 
 雷米‧德尼-库尔蒙
 https://www.remlab.net/

 _______________________________________________
 vlc-devel mailing list
 To unsubscribe or modify your subscription options:
 https://mailman.videolan.org/listinfo/vlc-devel</code></pre>
</blockquote>
</body>
</html>