[RFC/RFH] winmm:wave hang fix question (commit 717df5b2972b3cb998ca5a43279ae2283b117eaa) [bug 15559]

[Reece Dunn]

Hi,

Looking at the history of that test
(http://test.winehq.org/data/tests/winmm:wave.html) I see a lot of
timeout errors on the Wine builds, even after this fix went in.

The fix itself has caused a regression of playback in many
applications and games  that play audio
(http://bugs.winehq.org/show_bug.cgi?id=15559).

I want to submit the following patch to revert the part of the patch
that caused the regression and fix some of the tracing. I have been
using this patch on Cepstral SwiftTalker without issue and have tested
it also with the game Azguard Defense.

I am submitting this here and not to wine-patches initially, as I
would like to understand what the fix was actually fixing and (based
on the test history still showing timeouts) whether it actually fixes
the issue it was trying to.

diff --git a/dlls/winealsa.drv/waveout.c b/dlls/winealsa.drv/waveout.c
index 6cb2da8..b3f52aa 100644
--- a/dlls/winealsa.drv/waveout.c
+++ b/dlls/winealsa.drv/waveout.c
@@ -103,14 +103,19 @@ static BOOL wodUpdatePlayedTotal(WINE_WAVEDEV*
wwo, snd_pcm_status_t* ps)
     state = snd_pcm_state(wwo->pcm);
     snd_pcm_delay(wwo->pcm, &delay);

+    if (state != SND_PCM_STATE_RUNNING && state != SND_PCM_STATE_PREPARED)
+    {
+        WARN("Unexpected state (%d) while updating Total Played,
resetting\n", state);
+        delay=0;
+    }
+
     /* A delay < 0 indicates an underrun; for our purposes that's 0.  */
     if (delay < 0)
     {
-        WARN("Unexpected state (%d) or delay (%ld) while updating
Total Played, resetting\n", state, delay);
+        WARN("Unexpected delay (%ld) while updating Total Played,
resetting\n", delay);
         delay=0;
     }
-    if (state == SND_PCM_STATE_XRUN)
-        snd_pcm_start(wwo->pcm);
+
     InterlockedExchange((LONG*)&wwo->dwPlayedTotal,
wwo->dwWrittenTotal - snd_pcm_frames_to_bytes(wwo->pcm, delay));
     return TRUE;
 }

- Reece

[Chris Robinson]

On Friday 21 November 2008 10:26:19 am Reece Dunn wrote:
> I want to submit the following patch to revert the part of the patch
> that caused the regression and fix some of the tracing. I have been
> using this patch on Cepstral SwiftTalker without issue and have tested
> it also with the game Azguard Defense.
>
> I am submitting this here and not to wine-patches initially, as I
> would like to understand what the fix was actually fixing and (based
> on the test history still showing timeouts) whether it actually fixes
> the issue it was trying to.

Hi Reese,

As far as I understand, the issue is that winealsa uses the delay value to 
determine how much data is unplayed in the pcm buffer, and it expects the 
delay to be 0 when it's empty. The problem is, snd_pcm_delay measures the 
time it would take for the next sample written to get through the DAC, which 
is only marginally related, and may not get down to 0 (depending on the 
backend, there may always be some delay even if the next samples written get 
played right away).

The proper way to fix this, AFAIK from the conclusion on IRC, would be to 
subtract the snd_pcm_avail_update() value from the total pcm buffer size, as 
a way to determine how much is unplayed in the buffer instead of abusing 
snd_pcm_delay.

[Reece Dunn]

2008/11/21 Chris Robinson <chris.kcat(a)gmail.com>:
> On Friday 21 November 2008 10:26:19 am Reece Dunn wrote:
> > I want to submit the following patch to revert the part of the patch
> > that caused the regression and fix some of the tracing. I have been
> > using this patch on Cepstral SwiftTalker without issue and have tested
> > it also with the game Azguard Defense.
> >
> > I am submitting this here and not to wine-patches initially, as I
> > would like to understand what the fix was actually fixing and (based
> > on the test history still showing timeouts) whether it actually fixes
> > the issue it was trying to.
>
> Hi Reese,
>
> As far as I understand, the issue is that winealsa uses the delay value to
> determine how much data is unplayed in the pcm buffer, and it expects the
> delay to be 0 when it's empty. The problem is, snd_pcm_delay measures the
> time it would take for the next sample written to get through the DAC, which
> is only marginally related, and may not get down to 0 (depending on the
> backend, there may always be some delay even if the next samples written get
> played right away).
>
> The proper way to fix this, AFAIK from the conclusion on IRC, would be to
> subtract the snd_pcm_avail_update() value from the total pcm buffer size, as
> a way to determine how much is unplayed in the buffer instead of abusing
> snd_pcm_delay.

Thanks for the info.

I will have a stab at implementing this and then submit a patch.

Thanks,
- Reece