<!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>I will let Hugo answer your remarks in detail as the patch might have been written by me, but the rationale for it is his (including the first part of the commit message).</p>
<p>However, I will reply with a few of my personal opinions on the matter.</p>
<p>On 2017-03-22 19:33, 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> Nit: postpone takes no dash.
Le keskiviikkona 22. maaliskuuta 2017, 17.58.46 EET 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> Since one can query an art fetching</code></pre>
</blockquote>
<pre><code> Nit: Not sure what that sentence means. Maybe you mean "request fetching art".</code></pre>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> through libvlc_media_parse_with_options,
one would expect the event originating from this request to be sent upon
all requested operations completion.</code></pre>
</blockquote>
<pre><code> Nit: completion of all requested ops.</code></pre>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> The alternative would be to monitor
the libvlc_MediaMetaChanged, hoping for an artork url change, but this
can't account for error nor timeout.</code></pre>
</blockquote>
<pre><code> Nit: artwork.</code></pre>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> --
The changes introduced were written after a discussion with Hugo
Beauzée-Luyssen where he expressed that he would, if possible, be able to
post-pone the preparsing events until the art fetching is complete.</code></pre>
</blockquote>
<pre><code> AFAIK, there is a reason why they are separate. Preparsing only accesses the
media itself, while meta fetch goes to the network.</code></pre>
</blockquote>
<p>The meta-fetchers does not necessarily have to go to the network, as an example <code>modules/meta_engine/folder.c</code> is an art-finder that only requires local access (but I do agree with the overall opinion in your note).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> In many cases, the later is orders of magnitude slower. So it makes sense to
have the preparsed meta-data available separately.</code></pre>
</blockquote>
<p>Definitely agreed, but as one can use <code>libvlc_MetadataRequest</code> to not request network-metadata, it could be viewed as intended by design. It is however debatable if that is actually what most/all libvlc users expect (as this fix is mostly for them).</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> Post-poning the event fixes issues that are currently reproducible where
medialibrary is used, and art is missing (because it listens to the preparse
event in order to generate the database contents).
It can be viewed as a follow-up to the below rejected patch (with the
same goal in mind):
- https://patches.videolan.org/patch/15810/
---
src/playlist/fetcher.c | 42 ++++++++++++++++++++++++++++++++++--------
src/playlist/fetcher.h | 4 ++--
src/playlist/preparser.c | 7 +++++--
3 files changed, 41 insertions(+), 12 deletions(-)
diff --git a/src/playlist/fetcher.c b/src/playlist/fetcher.c
index ab4b3989c4..0172906146 100644
--- a/src/playlist/fetcher.c
+++ b/src/playlist/fetcher.c
@@ -50,6 +50,7 @@ struct playlist_fetcher_t {
struct fetcher_request {
input_item_t* item;
atomic_uint refs;
+ int preparse_status;
int options;
};
@@ -194,13 +195,23 @@ static int SearchByScope( playlist_fetcher_t* fetcher,
! SearchArt( fetcher, item, scope ) )
{
AddAlbumCache( fetcher, req->item, false );
- background_worker_Push( &fetcher->downloader, req, NULL, 0 );
- return VLC_SUCCESS;
+
+ if( !background_worker_Push( &fetcher->downloader, req, NULL, 0 ) )
+ return VLC_SUCCESS;
}
return VLC_EGENERIC;
}
+static void SetPreparsed( struct fetcher_request* req )
+{
+ if( req->preparse_status != -1 )
+ {
+ input_item_SignalPreparseEnded( req->item, req->preparse_status );
+ input_item_SetPreparsed( req->item, true );</code></pre>
</blockquote>
<pre><code> This order looks race-prone. Normally, one changes the state, then signals the
change.</code></pre>
</blockquote>
<p>Nice catch, my only excuse is that the patch was written in a hurry while I was actually focusing on the previous patches in this serious.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> With that said, I think I don´t really understand what this patch does, or
that I agree with it.</code></pre>
</blockquote>
<p>The gist of the patch is to postpone the events denoting preparsing to after all work requested by <code>libvlc_MetadataRequest</code> has finished.</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> + }
+}
+
static void Downloader( playlist_fetcher_t* fetcher,
struct fetcher_request* req )
{
@@ -259,6 +270,7 @@ out:
}
free( psz_arturl );
+ SetPreparsed( req );
return;
error:
@@ -274,15 +286,25 @@ static void SearchLocal( playlist_fetcher_t* fetcher,
struct fetcher_request* re if( var_InheritBool( fetcher->owner,
"metadata-network-access" ) || req->options &
META_REQUEST_OPTION_SCOPE_NETWORK )
{
- background_worker_Push( &fetcher->network, req, NULL, 0 );
+ if( background_worker_Push( &fetcher->network, req, NULL, 0 ) )
+ SetPreparsed( req );
+
+ return;
+ }
+ else
+ {
+ input_item_SetArtNotFound( req->item, true );
+ SetPreparsed( req );
}
- else input_item_SetArtNotFound( req->item, true );
}
static void SearchNetwork( playlist_fetcher_t* fetcher, struct
fetcher_request* req ) {
if( SearchByScope( fetcher, req, FETCHER_SCOPE_NETWORK ) )
+ {
input_item_SetArtNotFound( req->item, true );
+ SetPreparsed( req );
+ }
}
static void RequestRelease( void* req_ )
@@ -399,22 +421,26 @@ playlist_fetcher_t* playlist_fetcher_New(
vlc_object_t* owner ) return fetcher;
}
-void playlist_fetcher_Push( playlist_fetcher_t* fetcher, input_item_t*
item, - input_item_meta_request_option_t options )
+int playlist_fetcher_Push( playlist_fetcher_t* fetcher, input_item_t* item,
+ input_item_meta_request_option_t options, int preparse_status ) {
struct fetcher_request* req = malloc( sizeof *req );
if( unlikely( !req ) )
- return;
+ return VLC_ENOMEM;
req->item = item;
req->options = options;
+ req->preparse_status = preparse_status;
atomic_init( &req->refs, 1 );
input_item_Hold( item );
- background_worker_Push( &fetcher->local, req, NULL, 0 );
+ if( background_worker_Push( &fetcher->local, req, NULL, 0 ) )
+ SetPreparsed( req );
+
RequestRelease( req );
+ return VLC_SUCCESS;
}
void playlist_fetcher_Delete( playlist_fetcher_t* fetcher )
diff --git a/src/playlist/fetcher.h b/src/playlist/fetcher.h
index c5d9a3ddbb..06718acf77 100644
--- a/src/playlist/fetcher.h
+++ b/src/playlist/fetcher.h
@@ -46,8 +46,8 @@ playlist_fetcher_t *playlist_fetcher_New( vlc_object_t *
); * The input item is retained until the art fetching is done or until the
* fetcher object is destroyed.
*/
-void playlist_fetcher_Push( playlist_fetcher_t *, input_item_t *,
- input_item_meta_request_option_t );
+int playlist_fetcher_Push( playlist_fetcher_t *, input_item_t *,
+ input_item_meta_request_option_t, int );
/**
* This function destroys the fetcher object and thread.
diff --git a/src/playlist/preparser.c b/src/playlist/preparser.c
index 98a05b858c..f513abd8d3 100644
--- a/src/playlist/preparser.c
+++ b/src/playlist/preparser.c
@@ -95,7 +95,10 @@ static void PreparserCloseInput( void* preparser_, void*
input_ ) input_Close( input );
if( preparser->fetcher )
- playlist_fetcher_Push( preparser->fetcher, item, 0 );
+ {
+ if( !playlist_fetcher_Push( preparser->fetcher, item, 0, status ) )
+ return;
+ }
input_item_SetPreparsed( item, true );
input_item_SignalPreparseEnded( item, status );
@@ -163,7 +166,7 @@ void playlist_preparser_fetcher_Push(
playlist_preparser_t *preparser, input_item_t *item,
input_item_meta_request_option_t options ) {
if( preparser->fetcher )
- playlist_fetcher_Push( preparser->fetcher, item, options );
+ playlist_fetcher_Push( preparser->fetcher, item, options, -1 );
}
void playlist_preparser_Cancel( playlist_preparser_t *preparser, void *id )</code></pre>
</blockquote>
</blockquote>
<p>Thank you,<br />
Filip Roséen</p>
</body>
</html>