[vlc-devel] [PATCH 3/7] playlist/fetcher: refactor

[Rémi Denis-Courmont]

Le perjantaina 24. maaliskuuta 2017, 3.28.30 EEST Filip Roséen a écrit :
> The following changes refactors the fetcher to take advantange of the
> newly introduced background_worker helper. The new implementation
> should not only be easier to maintain, but it also adds some
> advantages over the old implementation:
> 
>  - The implementation has shrunk in size.
> 
>  - A fetch-request can include a timeout.
> 
>  - Given that there now is a background worker associated with each of
>    the different fetcher types (local, network, download):
> 
>     - A slow download does not prevent the network-fetcher from
>       working the queue.
> 
>     - A slow network-fetcher does not prevent further work in regards
>       of pending requests in the local fetcher.
> 
>  - A fetch request can now be properly cancelled (most importantly
>    during VLC close).
> 
>  - We no longer invoke modules with "meta fetcher" capability if the
>    item already has all metadata in terms of title, album, and artist.
> 
>  - We no longer invoke modules with "art finder" capability of the
>    item already has vlc_meta_ArtworkUrl.
> 
> fixes: #18150
> 
> --
> 
> Changes since last submission:
> 
>  - adjust to changes in terms of background-worker api
>  - add missing error-check in terms of metadata-probing
>  - slight refactor (logic is overall the same)
> ---
>  src/playlist/fetcher.c | 770
> +++++++++++++++++++++---------------------------- 1 file changed, 327
> insertions(+), 443 deletions(-)
> 
> diff --git a/src/playlist/fetcher.c b/src/playlist/fetcher.c
> index 1fdb64cee5..7e9f4124a5 100644
> --- a/src/playlist/fetcher.c
> +++ b/src/playlist/fetcher.c
> @@ -1,12 +1,9 @@
>  /**************************************************************************
> *** - * fetcher.c: Art fetcher thread.
> + * fetcher.c
>  
> ***************************************************************************
> ** - * Copyright © 1999-2009 VLC authors and VideoLAN
> + * Copyright © 2017-2017 VLC authors and VideoLAN
>   * $Id$
>   *
> - * Authors: Samuel Hocevar <sam at zoy.org>
> - *          Clément Stenac <zorglub at 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,555 +18,442 @@
>   * 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 <limits.h>
> -#include <assert.h>
> -
>  #include <vlc_common.h>
>  #include <vlc_stream.h>
> -#include <vlc_meta_fetcher.h>
> -#include <vlc_memory.h>
> -#include <vlc_demux.h>
>  #include <vlc_modules.h>
>  #include <vlc_interrupt.h>
> +#include <vlc_arrays.h>
> +#include <vlc_atomic.h>
> +#include <vlc_threads.h>
> +#include <vlc_memstream.h>
> +#include <vlc_meta_fetcher.h>
> 
> -#include "libvlc.h"
>  #include "art.h"
> +#include "libvlc.h"
>  #include "fetcher.h"
>  #include "input/input_interface.h"
> +#include "misc/background_worker.h"
> +#include "misc/interrupt.h"
> 
> -/**************************************************************************
> *** - * Structures/definitions
> -
> ***************************************************************************
> **/ -typedef enum
> -{
> -    PASS1_LOCAL = 0,
> -    PASS2_NETWORK
> -} fetcher_pass_t;
> -#define PASS_COUNT 2
> +struct playlist_fetcher_t {
> +    struct background_worker* local;
> +    struct background_worker* network;
> +    struct background_worker* downloader;
> 
> -typedef struct
> -{
> -    char *psz_artist;
> -    char *psz_album;
> -    char *psz_arturl;
> -    bool b_found;
> -    meta_fetcher_scope_t e_scope; /* max scope */
> -
> -} playlist_album_t;
> -
> -typedef struct fetcher_entry_t fetcher_entry_t;
> +    vlc_dictionary_t album_cache;
> +    vlc_object_t* owner;
> +    vlc_mutex_t lock;
> +};
> 
> -struct fetcher_entry_t
> -{
> -    input_item_t    *p_item;
> -    input_item_meta_request_option_t i_options;
> -    fetcher_entry_t *p_next;
> +struct fetcher_request {
> +    input_item_t* item;
> +    atomic_uint refs;
> +    int options;
>  };
> 
> -struct playlist_fetcher_t
> -{
> -    vlc_object_t   *object;
> -    vlc_mutex_t     lock;
> -    vlc_cond_t      wait;
> -    bool            b_live;
> -    vlc_interrupt_t *interrupt;
> +struct fetcher_thread {
> +    void (*pf_worker)( playlist_fetcher_t*, struct fetcher_request* );
> 
> -    fetcher_entry_t *p_waiting_head[PASS_COUNT];
> -    fetcher_entry_t *p_waiting_tail[PASS_COUNT];
> +    struct background_worker* worker;
> +    struct fetcher_request* req;
> +    playlist_fetcher_t* fetcher;
> 
> -    DECL_ARRAY(playlist_album_t) albums;
> -    meta_fetcher_scope_t e_scope;
> +    vlc_interrupt_t interrupt;
> +    vlc_thread_t thread;
> +    atomic_bool active;

This can´t be right. Either there is a lock and atomicity is not necessary, or 
this will race.

Nack.

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