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

[David Bolen]

David Bolen <db3l.net at gmail.com> writes:

(...)
> I'll have some time this evening to run some tests at the site with
> the machines giving the warnings to see just how much more room is
> needed.

Just to close the loop on this ... I've attached a patch for the final
code I've gone back to and that has run smoothly this past weekend,
across approximately 4000-5000 media loads.

Some possibly uninteresting details:

I ran various tests Friday night and no matter how I slice it, some
extra time above get_delay() appears needed to ensure all callbacks
are complete.  I don't have the test environment to say whether
exiting before that point is actually truncating audio, or if it's
just latency in processing callbacks, but my goal was to not show any
buffered bytes beyond the loop completion.  My production machines
range from 2.8GHz P4s to 2.4GHz E4600 Core 2 Duo's, so relatively slow
machines.

During the day Friday (with just usec_sleep), one of my sites (4 PCs,
8 mplayer processes) hadn't received the final callback about 12% of
the time, while the other site was almost 50%.  This was over around
1200 media loads.  The majority of the warning messages were for
single (4K) buffer, but 20-33% of the time it was actually 2 (8K)
buffers, which makes me think it could have been truncating.

I was just going to add a buffer to the usec_sleep parameter but that
would artificially extend the delay even when it wasn't needed.  So
that put me back with the older looping code.  But I don't want too
large a loop for those cases where it really won't complete (the
problem that prompted all that), or else that takes too long to
recognize.

So I've settled back close to where I started, just using get_delay()
instead of my own calculation.  An argument could be made to have a
slightly larger buffer factor (say +5 instead of +2) but that extends
the failure detection beyond what I'd prefer at least for myself.

BTW, I also ran some quick tests with GCC (MingW 4.2.4), and verified
that it is not hoisting the buffered_bytes value comparison out of the
loop, neither with the default -O2 used by mplayer's build, nor -O3.

I also experimented with taking this opportunity to add volatile
declarations to the inter-thread buffered_bytes and full_buffer
variables, and found the result less desirable without locking in
place.  The volatile definition turned all manipulations into a
load-calculate-store sequence, and sometimes permitting other
intermediate instructions, which was much less often the case without
volatile.  This appears more risky to me in terms of a thread context
switch causing corruption.

For example, the buffered_bytes manipulation in the callback changes
from:

	subl	$4096, _buffered_bytes

to:

	movl	_buffered_bytes, %eax
	subl	$4096, %eax
	movl	%eax, _buffered_bytes

while the competing sequence to update full_buffers and buffered_bytes
in the write routine turns from:

	addl	$1, _full_buffers          <<
L30:
	leal	(%ebx,%eax), %eax
	addl	%ebx, %ebp
	movl	%eax, 4(%esi)
	subl	%ebx, %edi
	movl	%eax, _buf_write_pos
	movl	_hWaveOut, %eax
	addl	%ebx, _buffered_bytes      <<

to:

	movl	_full_buffers, %eax        <<
	addl	$1, %eax                   <<
	movl	%eax, _full_buffers        <<
L30:
	movl	_buffered_bytes, %eax      <<
	addl	%ebx, %ebp
	subl	%ebx, %edi
	leal	(%ebx,%eax), %eax          <<
	movl	%eax, _buffered_bytes      <<

So I don't think adding any volatile declarations at this point seems
advisable.

-- David

PS: A correction to my initial note about this patch - I had neglected
to exclude media files without a sound track, which I have a bunch of,
from my calculations, so before this patch I was seeing failures of
approximately 1 in 5000 uninits, not the 1 in 20000 I originally
indicated.

PPS: While reviewing this I became less clear on why get_delay() was
adding in ao_data.buffersize to the calculation, since as far as I can
tell that has no impact on how much buffered data has actually been given
to Windows to play, so if anyone can provide any insight as to why that
is part of the calculation, I'd appreciate it.


Index: ao_win32.c
===================================================================
--- ao_win32.c	(revision 28810)
+++ ao_win32.c	(working copy)
@@ -251,8 +251,17 @@
 // close audio device
 static void uninit(int immed)
 {
-    if(!immed)while(buffered_bytes > 0)usec_sleep(50000);
-    else buffered_bytes=0;
+    if (!immed) {
+        /* Limit wait by pending data.  +2 = round up + room for callbacks */
+        int wait_cnt = (get_delay() * 20) + 2;
+        while (wait_cnt-- && buffered_bytes > 0) usec_sleep(50000);
+    } else buffered_bytes = 0;
+
+    if (buffered_bytes > 0) {
+        mp_msg(MSGT_AO, MSGL_WARN,
+               "Warning: Failed to play %d buffered bytes\n", buffered_bytes);
+        buffered_bytes = 0;
+    }
 	waveOutReset(hWaveOut);
 	waveOutClose(hWaveOut);
 	mp_msg(MSGT_AO, MSGL_V,"waveOut device closed\n");