[vlc-devel] vlc_object_signal removal

[Rémi Denis-Courmont]

On Mon, 08 Sep 2008 11:00:44 +0200, Olivier Aubert
<olivier.aubert at liris.cnrs.fr> wrote:
> Hello
> 
>> Snapshot support (vout_intf.c) is buggy, it fails to include any
> condition to
>> loop on the waiting side.
> Could you please elaborate on that ? I agree that the whole embedded
> snapshot (for which I am to blame) is based on hacks (the use of an
> intermediary object to exchange data, signals et al.), because of the
> intent to factorize snapshot code instead of duplicating it. But it is
> working.
> 
> I must be missing something here, but why should the waiting side be
> looping on anything ? It just waits for the object to be unlocked, then
> gets its data (which may be NULL in case of problem) from its private
> structure. I need some more hints to fix the problem, if there is one.

Plagiarizing POSIX:

"When using condition variables there is always a boolean predicate
involving shared variables associated with each condition wait that is true
if the thread should proceed. Spurious wakeups from the vlc_cond_wait() or
vlc_cond_timedwait() functions may occur. Since the return from
vlc_cond_wait() or vlc_cond_timedwait() does not imply anything about the
value of this predicate, the predicate should be re-evaluated upon such
return."

and:

"The vlc_cond_signal() or vlc_cond_broadcast() functions may be called by a
thread whether or not it currently owns the mutex that threads calling
vlc_cond_wait() or vlc_cond_timedwait() have associated with the condition
variable during their waits; however, if predictable scheduling behaviour
is required, then that mutex is locked by the thread calling
vlc_cond_signal() or vlc_cond_broadcast()."

In other words, the current snapshot support may currently:
- wake up when it should not (detecting a bogus failure),
- miss the signal, and effectively deadlock.

As a side note, there are no less than two consecutive malloc/memcpy/free
in that code path... and we are dealing with full RGB images here...

-- 
Rémi Denis-Courmont