<!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:56, 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, 18.43.44 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">
<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>
<pre><code> Where would you recommend putting them? I was struggling to find a
good place, but I guess `src/misc` is good enough?</code></pre>
</blockquote>
<pre><code> I don´t particularly like src/misc either, but that still seems saner than
src/playlist/</code></pre>
</blockquote>
<p>No, me neither - but as it is hard to come up with another suitable location.. <code>src/misc</code> it is (at least for now). Thanks.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> (...)</code></pre>
</blockquote>
<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> 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>
<pre><code> The reason there is a *data-member* of type `struct
background_worker_private` was because, inspired by some recent
changes by you (which I really like), I wanted
`background_worker_Init` to be always successful (in order to prevent
the number of error-paths).</code></pre>
</blockquote>
<pre><code> I would just inline the private structure into the public one.</code></pre>
</blockquote>
<p>So you mean having a <code>struct</code> without an anonymous type directly within <code>struct background_worker</code>, that refers to the private data-members?</p>
<p>Yes, that is certainly another way of doing it but I somewhat like having a named structure (in terms of type) as one can declare a dereferencable pointer to it (shorting paths where a lot of access to the private members are).</p>
<p>One could of course <em>“de-wrap”</em> the struct and put the nested data-members directly inside the “top” layered struct, but I somewhat wanted a way to denote that the data-members inside the private section are not to be touched from the outside (even though the only real protection is in its name).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> Or rather, I would design the thing so that there are no public
members at all. You can still use init/deinit instead of new/delete
if you want. Only expose the structure layout is exposed for the
purpose of allocation/storage.</code></pre>
</blockquote>
<p>Noted. I will take all written into consideration as I prepare patch-batch version <code>#2</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> It can of course be moved to be *translation-unit* local to
`src/playlist/misc/background_worker.c`.</code></pre>
</blockquote>
<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> +
+ 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>
<pre><code> `(1)`
Oh that was an unfortunate side-effect of some heavy rebasing right
before pushing to *vlc-devel*, the signal can of course be lost if the
background-task finishes during the time the lock is released.
I will fix this in an upcoming patch, thank you!</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> + }
+
+ 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>
<pre><code> I was tempted to introduce an alternative to `vlc_array_index_of_item`
that would take a callback so that one could query something other
than the item-address itself (but have the same semantics as
`vlc_array_index_of_item`).</code></pre>
</blockquote>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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 `background_worker.c`, I was
hesitant to add a *translation-unit* local helper.</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> + 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>
<pre><code> A `pf_release` 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.</code></pre>
</blockquote>
<pre><code> Sorry, I am not sure what you are trying to express here.
My point is that pf_release is also used in function calls, and without the
lock. So it has to be reentrant as things stand. And I´m not sure that´s a
good idea. At the very least, this would need to be clearly and explicitly
documented.</code></pre>
</blockquote>
<p>I will try to state things in a clear manner in the upcoming documentation, and as I plan to change some synchronization details (given your earlier replies) - it might make more sense discussing this after those.</p>
<p>Thank you for your swift replies,<br />
Filip Roséen</p>
</body>
</html>