<!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 21:16, 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 29. maaliskuuta 2017, 19.21.07 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> Hi Rémi,
On 2017-03-29 20:08, Rémi Denis-Courmont wrote:</code></pre>
<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.28 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 added utility will make it easier to handle a queue of tasks that
is to be finished in the order received. It allows for users of the
utility to focus on the end-goal instead of having to deal with
synchronization issues in terms of task-queue handling.
--
Changes since previous submission:
- added documentation
- moved from src/playlist/ to src/misc
- background-worker are now constructed through a single call
- background-worker internals are now fully private
- rewrote thread-synchronization logic to be more robust (and easier
to prove correct)
---
src/Makefile.am | 2 +
src/misc/background_worker.c | 235
+++++++++++++++++++++++++++++++++++++++++++ src/misc/background_worker.h
|
182 +++++++++++++++++++++++++++++++++ 3 files changed, 419 insertions(+)
create mode 100644 src/misc/background_worker.c
create mode 100644 src/misc/background_worker.h
diff --git a/src/Makefile.am b/src/Makefile.am
index 1e0e70fa86..208128d076 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -310,6 +310,8 @@ libvlccore_la_SOURCES = \
text/filesystem.c \
text/iso_lang.c \
text/iso-639_def.h \
+ misc/background_worker.c \
+ misc/background_worker.h \
misc/md5.c \
misc/probe.c \
misc/rand.c \
diff --git a/src/misc/background_worker.c b/src/misc/background_worker.c
new file mode 100644
index 0000000000..527509fb98
--- /dev/null
+++ b/src/misc/background_worker.c
@@ -0,0 +1,235 @@
+/**********************************************************************
**** *** + * 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"
+
+struct bg_queued_item {
+ void* id; /**< id associated with entity */
+ void* entity; /**< the entity to process */
+ int timeout; /**< timeout duration in microseconds */
+};
+
+struct background_worker {
+ void* owner;
+ struct background_worker_config conf;
+
+ struct {
+ bool probe_request; /**< true if a probe is requested */</code></pre>
</blockquote>
<pre><code> Not clear what this has to do with a supposedly generic framework. In
fact, not clear what this does at all.</code></pre>
</blockquote>
<pre><code> It is used to signal that there is an incoming request to probe the
current running task so that the signal sent from within
`background_worker_RequestProbe` is not lost due to
`&worker->head.lock` being unlocked when the status and/or stopping of
the running task is done.</code></pre>
</blockquote>
<pre><code> That´s extremely confusingly named.</code></pre>
</blockquote>
<p>What would you propose would be a better name?</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> I was hoping that simply looking how the data-member is used would be
enough to see why it is there, and given that it is a private entity I
assumed that extensive documentation for the specific *data-member* was
not necessary.</code></pre>
<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_t lock; /**< acquire to inspect members that follow
*/
+ vlc_cond_t wait; /**< wait for update in terms of head */
+ mtime_t deadline; /**< deadline of the current task */</code></pre>
</blockquote>
<pre><code> Err, likewise. If this is per-item, it wouldn´t be here. And if this is
internal state of the thread, it should be on the stack.
And if the thread is doing nothing, the whole point of the existing system
is that the thread should not exist at all.</code></pre>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + void* id; /**< id of the current task */</code></pre>
</blockquote>
<pre><code> Suspicious.</code></pre>
</blockquote>
<pre><code> Could you elaborate on why that is suspicious? I was originally using
the head of the queue to denote the currently running task, but
splitting it up lead to easier to understand code (that was at least
the intent).</code></pre>
</blockquote>
<pre><code> Same as deadline.</code></pre>
</blockquote>
<p>I still do not see what you are saying.</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">
<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> + bool active; /**< true if there is an active thread */
+ } head;
+
+ struct {
+ vlc_mutex_t lock; /**< acquire to inspect members that follow
*/
+ vlc_array_t data; /**< queue of pending entities to process */
+ } tail;
+};
+
+static void* Thread( void* data )
+{
+ struct background_worker* worker = data;
+
+ for( ;; )
+ {
+ struct bg_queued_item* item = NULL;
+ void* handle;
+
+ vlc_mutex_lock( &worker->tail.lock );
+ {
+ if( vlc_array_count( &worker->tail.data ) )
+ {
+ item = vlc_array_item_at_index( &worker->tail.data, 0
);
+ handle = NULL;
+
+ vlc_array_remove( &worker->tail.data, 0 );
+ }
+
+ vlc_mutex_lock( &worker->head.lock );
+ {
+ worker->head.deadline = INT64_MAX;
+ worker->head.active = item != NULL;
+ worker->head.id = item ? item->id : NULL;
+
+ if( item && item->timeout > 0 )
+ worker->head.deadline = mdate() + item->timeout;
+ }
+ vlc_cond_broadcast( &worker->head.wait );</code></pre>
</blockquote>
<pre><code> The usage of the CV still does not make sense.</code></pre>
</blockquote>
<pre><code> The above broadcast is to signal that the current head has changed,
letting potential waiting threads (that waits for a single task to be
cancelled) know that a change has been made.
In what way does it *"not make sense"*? Constructive feedback and
criticism is always welcome, but you are currently not given me much
to work with.</code></pre>
</blockquote>
<pre><code> No, the thread just wakes its own self up, which makes no sense.</code></pre>
</blockquote>
<p>The thread is per definition not waiting for the conditiona variable at that time, so I cannot see how it would wake itself up.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> Also it´s highly questionable that more than one thread should ever wait on
the worker. Normally, we probe the item, we set the flag and release the item.
And move onto the next item, or exit.</code></pre>
</blockquote>
<p>I am taking the safe route as this path is also invoked from users of <em>libvlc</em>. We do not control who waits for what, and given that it is not documented that a single waiter is max (by intention given the implementaiton) the code is written as it is.</p>
<p>The implementation also takes into consideration that we <em>maybe</em> would like to have concurrently running tasks in the future (which this implementation would allow with very few changes), but that’s a whole other discussion.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> So the only case that one waits on the worker is at deletion.</code></pre>
</blockquote>
<p>See previous paragraph.</p>
</body>
</html>