[vlc-devel] commit: atmo filter: disable until it is fixed ( Rémi Denis-Courmont )

[André Weber \(atmo\)]

Hi Rémi,

>  #if defined(_ATMO_VLC_PLUGIN_)
> +#error This makes no sense!
>     vlc_mutex_lock( &m_WakeupLock );
>     vlc_cond_signal( &m_WakeupCond );
>     vlc_mutex_unlock( &m_WakeupLock );
mmh - but there are other modules - doing this the same way - from there
I "copied" the usage of vlc_cond_signal with the mutex.
Usually for my use cases I wouldn't need the extra mutex - but the
vlc_cond_timedwait, (or vlc_cond_wait) - expect a mutex which
they can unlock and lock.

f.e. auhal.c, alsa.c - doing it also that way - the only difference to my code seems
that this code parts are locking / unlocking the mutex also around
the call to "vlc_cond_wait" (which does internaly unlock the mutex,
before really waiting and reaquire the lock after waiting.)

So my code seems to miss the Lock / Unlock - around the vlc_cond_timedwait(..)?
to avoid the deadlock problem? Or is there a way to wait for a condition - without a
extra mutex? inside VLC code.

>>this->m_FrameArrived
this code block no longer exists in my code - I'am currently working on it - so
my next commit will no longer contain this flag - its no longer needed.


>      for(DWORD i=0;(i<timeout) && !m_FrameArrived;i++)
>  #if defined (_ATMO_VLC_PLUGIN_)
> +#error A condition variable or a semaphore is needed.
>          msleep(1000);
>This is inefficient.
also no longer there - I hope I get my changes finished this week.
(this works now also on a vlc_cond_timedwait())


André Weber





----- Original Message ----- 
From: "Rémi Denis-Courmont" <remi at remlab.net>
To: "Mailing list for VLC media player developers" <vlc-devel at videolan.org>; <WeberAndre at gmx.de>
Sent: Tuesday, August 25, 2009 5:10 PM
Subject: Re: [vlc-devel] commit: atmo filter: disable until it is fixed ( Rémi Denis-Courmont )


Le mardi 25 août 2009 17:53:47 git version control, vous avez écrit :
> atmo filter: disable until it is fixed
>
> > http://git.videolan.org/gitweb.cgi/vlc.git/?a=commit;h=b59d0081f056925f90
> >e9f5b850f13ad416904617
(...)
>  #if defined(_ATMO_VLC_PLUGIN_)
> +#error This makes no sense!
>     vlc_mutex_lock( &m_WakeupLock );
>     vlc_cond_signal( &m_WakeupCond );
>     vlc_mutex_unlock( &m_WakeupLock );

A thread condition variable is always associated with a piece of state. There
are no memory access within the lock, so this simply cannot be right. No
wonder there were deadlocks and race conditions in the waiting thread.

> @@ -173,8 +174,10 @@ DWORD CAtmoExternalCaptureInput::Execute(void) {
>  void CAtmoExternalCaptureInput::WaitForNextFrame(DWORD timeout)
>  {
>      this->m_FrameArrived = ATMO_FALSE;
> +#error m_FrameArrived is not protected (no, volatile does not work)

volatile protects against compiler optimization. However it does not provide
atomicity nor memory cache flushes.

>      for(DWORD i=0;(i<timeout) && !m_FrameArrived;i++)
>  #if defined (_ATMO_VLC_PLUGIN_)
> +#error A condition variable or a semaphore is needed.
>          msleep(1000);

This is inefficient.

-- 
Rémi Denis-Courmont
http://www.remlab.net/