http://bugs.winehq.org/show_bug.cgi?id=28622
--- Comment #54 from Andrew Eikum aeikum@codeweavers.com 2012-10-11 11:23:57 CDT --- (In reply to comment #52)
I wonder if it isn't worth just ignoring the clock skew issue I'd like to submit this patch very soon
I oppose this that way. Don't understand me wrong. I'd be pleased to have my old Ubuntu Intrepid work without "pactl exit".
We already have the hack that writes no more than 3-4 periods and preferably full periods. On top of that, you add a rate-limiting hack based on a separate timer. All this just so that PA works, thus the hacks ought to be only active when PA is detected.
This isn't a PA-specific hack, although that is the case that caused me to write the fix. Here's what's happening when the PA bug occurs. The PA plugin limits its reported available size to the remainder of a fake ring buffer. So if the plugin's hw pointer is in the range (ring_buffer_size - period_size, ring_buffer_size) then avail_update() will return (ring_buffer_size - hw_pointer). That means it never reports at least one period available, and winealsa never writes.
If the widely-used and supported PA plugin exhibits this behavior, do you really want to depend on every other device/plugin not performing this way? I don't. We clearly can't depend on avail_update() growing over time, so I'm choosing another way to limit writes.
That argument doesn't even address older OS versions which we still, in theory, support.
Quite to the contrary, we might simplify the code even further and solely keep the "write full periods" in the regular driver and put "but no more than 3-4" into the if (This->quirks & PULSE_QUIRK) branch. (Well, I acknowledge that not writing too much in advance minimizes noise when audio shall be paused).
Actually, the 3-4 period limit has nothing to do with PA. That's there because we can't depend on the ALSA backend implementing pause, so rather than pause the device, we just keep a very small amount of data in it and let it run out for IAudioClient::Stop(). The proper way to fix that is to use snd_pcm_drop()... or snd_pcm_forward()... or write silence... or something else entirely. I don't know. What we have seems to work, so I'm fine sticking with it.
Additionally, if you use the file plugin, then there is no rate limiter at all. You can just write forever and it appears to "play" instantly. That will break any number of applications that use audio play position to control their own behavior.
so don't rely on avail_update() to determine buffer fullness
We should not work against the underlying APIs. If it's broken, it need be fixed on their side, but not kludged dramatically on the Wine side. I'm not opposed to small compatibility hacks but broken by design is not acceptable.
What I like about the current audio driver code in Wine is that it's quite straight forward and not too hard to review. We can go to any audio guy and claim: "hey, look at the Wine audio code, it's so simple, any bug must be on your side!".
The fact is, ALSA is broken and it's not getting fixed until someone defines its behavior, writes unit tests, and fixes the various drivers and plugins. That's not happening any time this decade. gettimeofday(3) is not broken, regardless of the ALSA backend. Yes, there's some risk of clock skew, but that problem is far more preferable to waiting on ALSA to fix their broken crap.
Going back to one of your earlier objections... (In reply to comment #50)
o There are now 3 timers
- CreatetimerQueue
- gettimeofday
- the ALSA device's timer
To eliminate one of the clocks, I tried using ALSA's snd_pcm_status_get_tstamp() and snd_pcm_status_get_htstamp(). Turns out they're broken at least with the PA plugin. They just return 0. Maybe the PA plugin will get fixed some day. Meanwhile gettimeofday(3) will work on every OS in the past couple of decades.