<!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-22 19:27, 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 keskiviikkona 22. maaliskuuta 2017, 17.58.41 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> The primary reason for this helper utility is to allow for the
preparser and {meta,art}-fetcher to share a common backbone as they
both deal with a queue of entities that are to be processed one-by-one
in the background.
By using struct background_worker in the preparser and fetcher we will
prevent code duplication, while also allowing for easier maintenence in
the future (such as allowing for concurrently running tasks within a
single queue).
---
src/Makefile.am | 2 +
src/playlist/misc/background_worker.c | 192
++++++++++++++++++++++++++++++++++ src/playlist/misc/background_worker.h |
62 +++++++++++
3 files changed, 256 insertions(+)
create mode 100644 src/playlist/misc/background_worker.c
create mode 100644 src/playlist/misc/background_worker.h
diff --git a/src/Makefile.am b/src/Makefile.am
index 1e0e70fa86..2aa8891026 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -220,6 +220,8 @@ libvlccore_la_SOURCES = \
playlist/fetcher.h \
playlist/sort.c \
playlist/loadsave.c \
+ playlist/misc/background_worker.c \
+ playlist/misc/background_worker.h \</code></pre>
</blockquote>
<pre><code> Preparsing and meta fetching are in the playlist directory because of
hysterical raisins. I remove the dependency on the playlist because LibVLC
needs them too.</code></pre>
</blockquote>
<p>I was originally planning to store the background-worker elsewhere, as there are other entities that could benefit form the same implementatation, but as those locations are affected by patches in a future branch (already written, but it simply moves the files) I thought I’d first propose them to be under <code>src/playlist/misc</code>, and later move it when there is a “universal” usage.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> I think you should not keep reproducing the same.</code></pre>
</blockquote>
<p>Where would you recommend putting them? I was struggling to find a good place, but I guess <code>src/misc</code> is good enough?</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> playlist/preparser.c \
playlist/preparser.h \
playlist/tree.c \
diff --git a/src/playlist/misc/background_worker.c
b/src/playlist/misc/background_worker.c new file mode 100644
index 0000000000..0c6041e6ef
--- /dev/null
+++ b/src/playlist/misc/background_worker.c
@@ -0,0 +1,192 @@
+/**************************************************************************
*** + * Copyright (C) 2017 VLC authors and VideoLAN
+ *
+ * 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 + *
(at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * 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 <vlc_common.h>
+#include <vlc_threads.h>
+#include <vlc_arrays.h>
+
+#include "libvlc.h"
+#include "background_worker.h"
+
+static void* Thread( void* data )
+{
+ struct background_worker* worker = data;
+ struct background_worker_private* priv = &worker->p;</code></pre>
</blockquote>
<pre><code> 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.</code></pre>
</blockquote>
<p>The reason there is a <em>data-member</em> of type <code>struct background_worker_private</code> was because, inspired by some recent changes by you (which I really like), I wanted <code>background_worker_Init</code> to be always successful (in order to prevent the number of error-paths).</p>
<p>It can of course be moved to be <em>translation-unit</em> local to <code>src/playlist/misc/background_worker.c</code>.</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> +
+ 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 );</code></pre>
</blockquote>
<pre><code> This sequence of two instructions can´t be correct, AFAIK.</code></pre>
</blockquote>
<p><code>(1)</code></p>
<p>Oh that was an unfortunate side-effect of some heavy rebasing right before pushing to <em>vlc-devel</em>, the signal can of course be lost if the background-task finishes during the time the lock is released.</p>
<p>I will fix this in an upcoming patch, thank you!</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> + }
+
+ 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 );</code></pre>
</blockquote>
<pre><code> Looks like reinventing vlc_array_index_of_item().</code></pre>
</blockquote>
<p>I was tempted to introduce an alternative to <code>vlc_array_index_of_item</code> that would take a callback so that one could query something other than the item-address itself (but have the same semantics as <code>vlc_array_index_of_item</code>).</p>
<p>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 <code>background_worker.c</code>, I was hesitant to add a <em>translation-unit</em> local helper.</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> + worker->pf_release( item->entity );</code></pre>
</blockquote>
<pre><code> This seems to assume that the callbacks are reentrant. It might be true. But
it seems like a dangerous design assumption.</code></pre>
</blockquote>
<p>A <code>pf_release</code> 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.</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> + free( item );
+ continue;
+ }
+
+ ++i;
+ }
+
+ if( ( id && priv->current.item && priv->current.item->id == id ) ||
+ ( id == NULL && priv->active ) )
+ {
+ priv->current.deadline = VLC_TS_0;
+ vlc_cond_broadcast( &priv->wait );
+
+ while( ( id && priv->current.item && priv->current.item->id == id )
|| + ( id == NULL && priv->active ) )
+ {
+ vlc_cond_wait( &priv->wait, &priv->lock );
+ }</code></pre>
</blockquote>
<pre><code> This sequence cannot be correct.</code></pre>
</blockquote>
<p>Will be clearified when I fix <code>(1)</code>.</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> + }
+ vlc_mutex_unlock( &priv->lock );
+}
+
+void background_worker_Init( struct background_worker* worker, void* owner,
+ int timeout )
+{
+ worker->p.default_timeout = timeout > 0 ? timeout*1000 :
VLC_TS_INVALID; + worker->p.current.handle = NULL;
+ worker->p.current.item = NULL;
+ worker->p.active = false;
+ worker->p.owner = owner;
+
+ vlc_array_init( &worker->p.queue );
+ vlc_mutex_init( &worker->p.lock );
+ vlc_cond_init( &worker->p.wait );
+}
+
+int background_worker_Push( struct background_worker* worker, void* entity,
+ void* id, int timeout )
+{
+ struct background_worker_item* item = malloc( sizeof( *item ) );
+ struct background_worker_private* priv = &worker->p;
+
+ if( unlikely( !item ) )
+ return VLC_EGENERIC;
+
+ item->id = id;
+ item->entity = entity;
+ item->timeout = timeout > 0 ? timeout*1000 : worker->p.default_timeout;
+
+ vlc_mutex_lock( &worker->p.lock );
+ vlc_array_append( &worker->p.queue, item );
+
+ if( !worker->p.active &&
+ vlc_clone_detach( NULL, Thread, worker, VLC_THREAD_PRIORITY_LOW ) )
+ {
+ vlc_mutex_unlock( &priv->lock );
+ return VLC_EGENERIC;
+ }
+
+ worker->p.active = true;
+ worker->pf_hold( item->entity );
+ vlc_mutex_unlock( &priv->lock );
+
+ return VLC_SUCCESS;
+}
+
+void background_worker_Cancel( struct background_worker* worker, void* id )
+{
+ BackgroundWorkerCancel( worker, id );
+}
+
+void background_worker_Signal( struct background_worker* worker )
+{
+ struct background_worker_private* priv = &worker->p;
+
+ vlc_mutex_lock( &priv->lock );
+ vlc_cond_broadcast( &priv->wait );
+ vlc_mutex_unlock( &priv->lock );</code></pre>
</blockquote>
<pre><code> This sequence cannot be right.</code></pre>
</blockquote>
<p>Will be adjusted as part of <code>(1)</code> to make the intent more clear.</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> +}
+
+void background_worker_Clean( struct background_worker* worker )
+{
+ BackgroundWorkerCancel( worker, NULL );
+
+ vlc_array_clear( &worker->p.queue );
+ vlc_cond_destroy( &worker->p.wait );
+ vlc_mutex_destroy( &worker->p.lock );
+}
diff --git a/src/playlist/misc/background_worker.h
b/src/playlist/misc/background_worker.h new file mode 100644
index 0000000000..3d78d63468
--- /dev/null
+++ b/src/playlist/misc/background_worker.h
@@ -0,0 +1,62 @@
+/**************************************************************************
*** + * Copyright (C) 2017 VLC authors and VideoLAN
+ *
+ * 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 + *
(at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301, USA. +
***************************************************************************
**/ +
+#ifndef BACKGROUND_WORKER_H__
+#define BACKGROUND_WORKER_H__
+
+struct background_worker_item {
+ void* id;
+ void* entity;
+ int timeout;
+};
+
+struct background_worker_private {
+ void* owner;
+ int default_timeout;
+
+ vlc_mutex_t lock;
+ vlc_cond_t wait;
+
+ struct {
+ void* handle;
+ mtime_t deadline;
+ struct background_worker_item* item;
+ } current;
+
+ bool active;
+ vlc_array_t queue;
+};
+
+struct background_worker {
+ void( *pf_release )( void* );
+ void( *pf_hold )( void* );
+
+ int( *pf_start )( void* owner, void* item, void** out );
+ int( *pf_probe )( void* owner, void* handle );
+ void( *pf_stop )( void* owner, void* handle );</code></pre>
</blockquote>
<pre><code> The callback names are rather obscure, and it does not help that they are not
documented.</code></pre>
</blockquote>
<p>My initial conclusion was that since it was a helper with limited use-case at the current time, it would be safe to not have them documented; but given your note I will take time to annotate the functions with relevant documentation to make the implementation more robust.</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> +
+ struct background_worker_private p;
+};
+
+void background_worker_Init( struct background_worker* worker, void* owner,
int timeout ); +void background_worker_Cancel( struct background_worker*
worker, void* id ); +void background_worker_Signal( struct
background_worker* worker ); +void background_worker_Clean( struct
background_worker* worker ); +int background_worker_Push( struct
background_worker* worker, void* entity, void* id, int timeout ); +
+#endif</code></pre>
</blockquote>
</blockquote>
<p>Thank you very much for the review! I will return with the necessary changes as soon as possible (probably in the upcoming 2 hours, or sooner).</p>
<p>Best Regards,<br />
Filip</p>
</body>
</html>