[MPlayer-dev-eng] [PATCH] Support CineForm DirectShow codec

[Steinar H. Gunderson]

On Sun, Jan 10, 2010 at 04:39:27PM +0100, Reimar Döffinger wrote:
> The pthread stuff needs some serious explaining, and I really think
> it should be done without the pthread_mutexattr_settype stuff if at all
> possible (well, actually it is certainly possible).

I guess the first change (always unlock before destroy) is pretty
self-explanatory -- you're not allowed to delete a locked mutex. NetBSD used
to carry a patch for this since it triggered an assertion failure in their
pthreads implementation, AFAIK.

The other critical section changes require a bit more explanation. Let me try
to explain what's going on.

Default pthread mutexes are “non-recursive”, in that if one thread holds a
lock and tries to re-acquire it, it will deadlock. The primary reason for
this is that it helps correctness (the rationale is somewhat long and
boring), but it is also somewhat faster to implement than a recursive mutex.

However, Win32 critical sections (which are otherwise pretty much the same as
pthread mutexes) are recursive -- you can enter a critical section twice, and
it won't be unlocked until you do two unlocks.

win32.c tried to implement critical sections using non-recursive mutexes, by
having a ->locked variable in the critical section that was read outside the
lock and set inside the lock. On lock, first the pthread mutex would be
locked, then ->locked would be set, and then ->id would be set to the thread
ID (so EnterCriticalSection could distinguish between the thread re-entering
its own CS, which should immediately succeed, and some other thread that
would have to wait). Having a variable with this kind of access pattern is
almost always a red flag.

For instance, assume that thread B previously held the CS (so cs->id points
to thread B's ID). Then you could have something like this, where thread A
and thread B both try to enter the critical section:

   Thread A                                 Thread B
   if (cs->locked)
       if (cs->id==pthread_self())
           return;
   pthread_mutex_lock(&cs->mutex);
   cs->locked = 1;
                                            if (cs->locked)
					        if (cs->id==pthread_self())
						    return;
   cs->id=pthread_self();
  
IOW, if thread B previously held the CS it could just return right away in
the belief that it just took the CS, while in reality A held it. This is
obviously bad.

There are ways of fixing this, but it is very hard to get right while still
maintaining performance; in particular, you have to consider things like what
optimizations the compiler is allowed to make (they can be pretty
surprising), low-level cache coherency, barriers, etc. There are lots of
other ways this can race; my example is just one of many. (Lock
implementations are surprisingly hard to get right; there's _tons_ of bad
ad-hoc schemes in the world, since it _looks_ easy but can be amazingly
subtle.)

The fix is just to make the mutex recursive from the start; that way, the OS
will take care of all the nitty gritty details for you, and all the ->locked
and ->id stuff can just go away.

/* Steinar */
-- 
Homepage: http://www.sesse.net/