http://bugs.winehq.org/show_bug.cgi?id=29531
Bug #: 29531 Summary: Folder navigation causes audio playback glitches in CDBurnerXP Product: Wine Version: 1.3.36 Platform: x86-64 URL: http://cdburnerxp.se/download?more-options OS/Version: Linux Status: UNCONFIRMED Severity: normal Priority: P2 Component: winealsa.drv AssignedTo: wine-bugs@winehq.org ReportedBy: RandomAccountName@mail.com CC: aeikum@codeweavers.com Classification: Unclassified Regression SHA1: b0652dd8bdb144645be4a6bf77cbb68a8ade49d9
Created attachment 38258 --> http://bugs.winehq.org/attachment.cgi?id=38258 +tid,+mmdevapi,+winmm,+midi,+dsound,+dmusic,+mci,+oss,+alsa,+coreaudio log (4.3MB)
CDBurnerXP has an integrated audio player to allow playing the tracks in an audio CD compilation before burning it. It also has a file browser from which audio files can be added to the compilation.
Trying to use both of these features at once causes audio playback glitches since 1.3.31: when changing folders in the file browser, the currently-playing audio may stutter for a moment (not every time). Regression testing indicated:
b0652dd8bdb144645be4a6bf77cbb68a8ade49d9 is the first bad commit commit b0652dd8bdb144645be4a6bf77cbb68a8ade49d9 Author: Andrew Eikum aeikum@codeweavers.com Date: Mon Oct 10 15:56:55 2011 -0500
winealsa.drv: Don't try to control ALSA's behavior.
Now, winealsa maintains its own buffer, which is written to ALSA on the period cycle requested by the application. We also let ALSA start when it has enough data and stop when it runs out, recovering from the expected underruns. This seems to be more like how ALSA expects to be used.
:040000 040000 874ae9456f596d46ee3911eabcbc6f291afe48cb 62ca893abef4dd0459b201d4c45d0c7b0c87a989 M dlls
git checkout b0652dd8bdb144645be4a6bf77cbb68a8ade49d9 -> changing folders can interrupt audio playback git checkout b0652dd8bdb144645be4a6bf77cbb68a8ade49d9^ -> audio is never interrupted
The attached log is from a system using just ALSA, no PulseAudio. I also checked another system with PulseAudio running, and the same problem exists there.
Steps to reproduce:
1. Install .NET prerequisite with winetricks dotnet20 2. Install and run CDBurnerXP 3. Choose "audio disc" mode from the main menu 4. Use the file browser at the top and the "add" button to add at least one wav/flac/mp3 file to the compilation (drag and drop won't work, see bug 22691) 5. Double-click the audio file in the lower section of the screen to start playback (the "play" button won't work, even on Windows) 6. Now use either the treeview or listview portion of the file browser to enter various folders
http://bugs.winehq.org/show_bug.cgi?id=29531
A Wine user RandomAccountName@mail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Keywords| |dotnet, download, | |regression
http://bugs.winehq.org/show_bug.cgi?id=29531
--- Comment #1 from Andrew Eikum aeikum@codeweavers.com 2012-01-04 11:03:21 CST --- There's a huge gap in the winealsa timer ticks:
(navigate directory) (several "tick"s) 22.374:0041:err:alsa:alsa_write_data tick 22.385:0041:err:alsa:alsa_write_data tick ... 22.569:0041:err:alsa:alsa_write_data tick
which causes an underrun and the audio glitches. The behavior was masked before because we put a whole bunch of data into the ALSA buffer, which was enough to bridge the large gap without an underrun.
There might be a problem with the timer code, but more likely we should be using a more reliable method to trigger our write() function periodically.
http://bugs.winehq.org/show_bug.cgi?id=29531
Jörg Höhle hoehle@users.sourceforge.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |hoehle@users.sourceforge.ne | |t
--- Comment #2 from Jörg Höhle hoehle@users.sourceforge.net 2012-01-15 12:19:30 CST --- The ancient winealsa module said: /* unless someone makes a wineserver kernel module, Unix pipes are faster than win32 events */ and used pipes and poll to wakeup. We don't need pipes any more since there's nothing else but the timer to wait for. However, the DeleteTimerQueueTimer features an event-based protocol to guarantee that callbacks have finished executing. We'd have to find something else should we move away from them. DSound uses timeSetEvent.
22.385...22.569:0041:err:alsa:alsa_write_data tick was masked before because we put a whole bunch of data into the ALSA buffer
Can you explain how the old code could bridge a gap of 184ms? I can't believe DSound would exhibit such a latency.
Log line 30550 shows where time jumps from 25.155 to 25.228ms. It is also a place where 3 threads 0009, 41 and 42 access DSound. mmdevapi doesn't seem involved. As a property of timeSetEvent(?), DSound's event is then signaled 8 times in a row without delay, until the situation stabilizes again and 10ms intervals are restored.
There's an even worse situation at time 18.515 - 18.939 -- 400ms! 0041:trace:dsound:DSOUND_timer entering at 18515 0041:trace:dsound:DSOUND_timer completed processing at 18939, duration = 424 No other threads are involved, but DSound is not playing at that time. I'd say the Linux scheduler simply put the task to sleep.
My conviction is not that timer queues are unreliable, rather than that our current mmdevapi design is fragile, because of the following tension: - glitch free sound wants buffers as large as possible; - we made the buffer really small in bug #28723 to support the "fill at most 10ms each turn" use case; - I mentioned in bug #28723, comment #61 that even a quadcore Linux does not manage to guarantee a steady 50ms wakeup. There will be hiccups since the introduction of the completely fair scheduler in the kernel.
I've sketched in bug #28723 an alternative design that would allow to use large(r) buffers -- when the app supplies more data. That would help winmm, but I see that CDBurnerXP uses DSound. Currently I don't know whether DSound could be designed in such a way as to behave differently from XAudio2. XAudio2 seems to use the above 10ms use case. For what DSound action games wants to do, namely offer minimal latency, that is IMHO exactly the right behaviour: write as little as possible in advance so as to produce sounds of explosions close to when they happen.
Perhaps the old DSound implementation could remix data, however mmdevapi has no concept of seeking backward. Once you submit data, it's definitive.
From there you can derive that mmdevapi is a bad backend for DSound -- unless
you equate DSound's mixer with mmdevapi's. Then you could supply large numbers of samples for glitch-free BGM. Even short-latency explosions might render glitch-free, if their sound is not computed dynamically rather than a secondary buffer that's just told to Play (=mix in now).
We might even reinvent DSound HW buffers. These use their own connection to ALSA, whereas the SW buffers all get mixed into one ALSA snd_pcm_open connection.
BTW, what happens if you revert my winealsa patch "GetPosition() using snd_pcm_delay"? Bug #29472 shows that it can interfere with DSound.
http://bugs.winehq.org/show_bug.cgi?id=29531
--- Comment #3 from A Wine user RandomAccountName@mail.com 2012-01-16 01:56:33 CST --- (In reply to comment #2)
BTW, what happens if you revert my winealsa patch "GetPosition() using snd_pcm_delay"? Bug #29472 shows that it can interfere with DSound.
It doesn't seem to make any difference here.
http://bugs.winehq.org/show_bug.cgi?id=29531
--- Comment #4 from Jörg Höhle hoehle@users.sourceforge.net 2012-01-20 12:41:42 CST --- (In reply to comment #1)
There might be a problem with the timer code, but more likely we should be using a more reliable method to trigger our write() function periodically.
DSound uses timeSetEvent. I observed that timeSetEvent has very different, interesting semantics: it'll really maintain a constant rate even when system load increases. It's very far from "sleep 10ms after every event". It'll even call us 10 times in a row if the system was busy for 100ms! CreateTimerQueue shows callbacks at .015s, then .026, then .037 etc. timeSetEvent may go to .026, but then sleep less and hit .035s. Alas, timeSetEvent is from winmm, whereas mmdevapi is below that.
http://wiki.cockos.com/wiki/index.php/How_to_run_Reaper_in_Wine_on_Linux "Due to the way wine works there is a lot of overhead in using IPC (Inter Process Communication). Reaper uses ... Native Events." And they recommend to disable them -- and poll instead!
Yet I think we still have opportunities for optimization -- and first getting the code correct -- before switching the trigger. OTOH, why not now?
http://bugs.winehq.org/show_bug.cgi?id=29531
--- Comment #5 from Jörg Höhle hoehle@users.sourceforge.net 2012-01-23 08:18:37 CST --- Bug #29585, comment #3 is more appropriate here. Another improvement to winealsa and wineoss design is to split the use of the critical section This->lock: a) One prevents several app threads from calling the API concurrently. b) The second one shall prevent the internal callback from operating on the slots that Get/ReleaseBuffer modify concurrently.
It is not good that bombarding the API with ReleaseBuffer(0) or GetCurrentPadding blocks the audio callback. Instead of an internal lock, we should investigate the opportunity for a lock-less design. Then, the kernel would never put the callback to sleep in EnterCriticalSection. What kernel expert knows how much later a thread recovers from such a sleep?
The ancient ALSA player never used EnterCS except perhaps when logging, or with SetEvent, or when receiving a message for which there existed a specific CS wwo->msgRing.msg_crst http://source.winehq.org/source//dlls/winealsa.drv/waveout.c?v=wine-1.3.24
http://bugs.winehq.org/show_bug.cgi?id=29531
--- Comment #6 from Jörg Höhle hoehle@users.sourceforge.net 2012-01-25 09:22:04 CST --- To avoid padding issues, I experimentally set DefaultPeriod to 213334 (21.333ms) to match ALSA's dmix. Then I listed to mcicda playing an audio CD. Visit bug #20555 for a howto.
Boy, was that disappointing! It was full of something as if listening through a fan or other chopper. Back to 1.3.24 yields perfect sound. Using a 10ms period produced weird effects too, much less intense. I believe this indicates that there are still severe defects in DSound which should not be so sensitive to mmdevapi's period (mcicda uses dsound). It could be something else too.
http://bugs.winehq.org/show_bug.cgi?id=29531
--- Comment #7 from Jörg Höhle hoehle@users.sourceforge.net 2012-01-27 10:46:17 CST --- The sketch of the lock-less design is as follows. Strive for a particularly simple variant of RCU. List and partition every resource that the callback (CB) accesses, i.e. what's shared among CB and API functions like ReleaseBuffer or Stop. Make it such that there is one duplex resource with everything else either Read-Only or Write-Only.
Here's an excerpt of the objects off This->
held_frames RW => InterlockedAdd
pcm_handle RO constant means valid for the extent of the cb lifetime fmt->* RO constant
session->mute is the session pointer constant?
local_buffer RO constant bufsize_frames RO constant event RO made constant by upcoming "SetEventHandle once only" patch
started R variable
lcl_offs_frames WO = CB write pointer
Mmdevapi's "initialize once then never touch" design works towards our goal.
Cannot mix types. In GetBuffer (API side) or alsa_read_data(CB), (This->lcl_offs_frames + This->held_frames) is inacceptable. Instead, each side must use its own entity. Split the read from the write pointer, adding This->xy_offs_frames. CB updates one, API the other.
In CB, + if(!in_alsa && This->held_frames < This->hidden_frames){ + UINT32 s_frames = This->hidden_frames - This->held_frames; can see s_frames < 0. C in RCU means "Copy" which is trivial here as we use a single RW object. Well, RCU is just a guide, what I describe is not RCU. Our usage of non-constant objects (held_frames and started) is basic, otherwise we'd need a "U"(pdate)-like restart transaction loop.
Add extra W variables reflecting error state as needed. I'm thinking about using This->initted to hold a steady error state, such as AUDCLNT_DEVICE_INVALIDATED.
"This->" itself must exist for the life time of CB. That's why the waitable event in DeleteTimerQueueTimer is crucial.
Quite straightforward, isn't it?
http://bugs.winehq.org/show_bug.cgi?id=29531
jbh@alchemy.lu changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |jbh@alchemy.lu
--- Comment #8 from jbh@alchemy.lu 2012-02-15 12:32:34 CST --- (In reply to comment #4)
http://wiki.cockos.com/wiki/index.php/How_to_run_Reaper_in_Wine_on_Linux "Due to the way wine works there is a lot of overhead in using IPC (Inter Process Communication). Reaper uses ... Native Events." And they recommend to disable them -- and poll instead!
Just a comment since I've written the above :) I did run the server in process patch with reaper and native events enabled. There was no excessive cpu usage, so it's quite likely that this represents the overhead of making many calls into the wineserver.
http://bugs.winehq.org/show_bug.cgi?id=29531
--- Comment #9 from Jörg Höhle hoehle@users.sourceforge.net 2012-03-02 07:09:20 CST --- (In reply to comment #8) I now see your comment about SetEvent and Reaper in bug #28412
http://bugs.winehq.org/show_bug.cgi?id=29531
--- Comment #10 from Jörg Höhle hoehle@users.sourceforge.net 2012-03-09 04:56:58 CST --- Created attachment 39263 --> http://bugs.winehq.org/attachment.cgi?id=39263 lock-free winealsa renderer
The sketch of the lock-less design is as follows.
Here's an implementation. It requires the fix #2 from bug #30071 such that IAC_Reset cannot be executed while the callback is running.
I'd appreciate if somebody with a multi-core machine would test it. A mmdevapi render test loop ran fine while recompiling Wine and starting Firefox. I don't consider lock-less to be a silver bullet. If Linux decides to let other threads run for 100ms, an underrun will occur. Lock-less simply means less contention, giving Linux less reasons to schedule another thread. Also, ALSA may use locks or a mutex internally, so our renderer calling snd_pcm_write while the mmdevapi app calls IAC_GetPosition => snd_pcm_avail&delay still causes a contention. We can be happy that IAC_GetCurrentPadding avoids going through ALSA.
http://bugs.winehq.org/show_bug.cgi?id=29531
--- Comment #11 from Jörg Höhle hoehle@users.sourceforge.net 2012-12-15 05:50:50 CST --- Created attachment 42813 --> http://bugs.winehq.org/attachment.cgi?id=42813 lock-free ALSA callback, updated for wine-1.5.19
It would be nice if users would report how lock-less works for them.
http://bugs.winehq.org/show_bug.cgi?id=29531
Jörg Höhle hoehle@users.sourceforge.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #39263|0 |1 is obsolete| |
--- Comment #12 from Jörg Höhle hoehle@users.sourceforge.net 2012-12-18 14:41:01 CST --- Created attachment 42845 --> http://bugs.winehq.org/attachment.cgi?id=42845 lock-free patch incl. full memory barriers
I had an oversight: the audio buffer itself is R/W too ;-P
So now I'm using the InterlockedExchangeAdd(&,0) pattern to obtain a full memory barrier at entry of the callbacks (and IEA(&,delta) is used at exit).
I'm still looking for an on-the-fly read-only pattern. For instance, InterlockedExchangeAdd is not nice within GetPosition. That function should simply take an atomic snapshot of This->held_frames. Similarly, MCIMIDI wants the read-only pattern, the MIDI data is fixed while playing.
The attached patch requires my patch "Separate read and write pointers" from http://www.winehq.org/pipermail/wine-patches/2012-December/120834.html (or from my previous comment ?)
http://bugs.winehq.org/show_bug.cgi?id=29531
Bruno Jesus 00cpxxx@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |00cpxxx@gmail.com
--- Comment #13 from Bruno Jesus 00cpxxx@gmail.com 2012-12-18 16:51:35 CST --- After a few minutes trying I could not reproduce the issue in git, is there any special mp3 that can cause the issue?
https://bugs.winehq.org/show_bug.cgi?id=29531
Ken Sharp imwellcushtymelike@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Keywords| |Abandoned?
https://bugs.winehq.org/show_bug.cgi?id=29531
A Wine user RandomAccountName@mail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |RESOLVED Resolution|--- |ABANDONED
--- Comment #14 from A Wine user RandomAccountName@mail.com --- I can no longer reproduce this bug, even with wine-1.3.36 as in my original report. Not much choice but to abandon it...
https://bugs.winehq.org/show_bug.cgi?id=29531
Bruno Jesus 00cpxxx@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED
--- Comment #15 from Bruno Jesus 00cpxxx@gmail.com --- Closing abandoned bugs.
-
wine-bugs＠winehq.org