<!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:08, 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.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>
<p>It is used to signal that there is an incoming request to probe the current running task so that the signal sent from within <code>background_worker_RequestProbe</code> is not lost due to <code>&worker->head.lock</code> being unlocked when the status and/or stopping of the running task is done.</p>
<p>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 <em>data-member</em> was not necessary.</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_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>
<p>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).</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> +        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>
<p>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.</p>
<p>In what way does it <em>“not make sense”</em>? Constructive feedback and criticism is always welcome, but you are currently not given me much to work with.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> So nack.</code></pre>
</blockquote>
<p>/F</p>
</body>
</html>