[MPlayer-dev-eng] [PATCH] Prevent ao_win32 from hanging during uninit(0)

[David Bolen]

Reimar Döffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de> writes:

> On Thu, Mar 05, 2009 at 02:47:59PM -0500, David Bolen wrote:
>> But yeah, we need locking for those cross-thread values.
>
> I'd like to point out that as long as we assume atomic writes to ints
> (not an issue considering this is Windows-specific code, and in general
> a very minor one at most) there is no need for locks.
> You'd just need to change waveOutProc to increase a read_buf (%
> BUFFER_COUNT), i.e. the basic thread-safe, lock-free produce-consumer
> FIFO.

If you imply removing any other read-calculate-write parts of the
code, I'd agree.  Otherwise, just atomic writes aren't sufficient.
After long experience I'm also generally suspicious of even assuming
the atomic nature of any C code, since I have trouble assuming that
any particular assignment or increment/decrement operation in code is
guaranteed to compile down to single instructions, and tracking down
such errors is so problematic.  But I agree the risk is likely minor.

It does seems to me, however, that as long as you can have partial
buffers in the pending pool, you need some mechanism (e.g.,
buffered_bytes) to identify actual pending bytes, not just buffers, or
else stuff like get_delay() won't be accurate.

Probably the way around that would be to let get_delay() (and related
functions) walk the buffer pool counting up pending buffers.  A new
buffer's size is only written to when first set up, and its status
only cleared in the callback, and the buffer walk is read-only, so
lock-free should be as safe as possible, at most just over-counting.

It does seem that my initial reading that the whole buffer thing was
problematic agrees with your impression.  When I get a moment, I may
try a complete replacement if that's something that would be welcome.

I suspect this is also a lot of attention to code that may not even
see a ton of use any more.  Heck, I only ended up on ao_win32 because
the default ao_dsound repeats audio at the end when the audio track is
smaller than the video, but I just ended up trading one bug for
another...

-- David