http://bugs.winehq.org/show_bug.cgi?id=14717
Summary: resampled sound is horrible Product: Wine Version: 1.0.0 Platform: All OS/Version: All Status: UNCONFIRMED Severity: enhancement Priority: P2 Component: directx-dsound AssignedTo: wine-bugs@winehq.org ReportedBy: patrakov@gmail.com
The problem is reported because the default sampling frequency in wine (as configured by winecfg) is 48000 Hz, while apps generally want 44100 Hz.
However, the resampling code in dlls/dsound/mixer.c, function DSOUND_MixToTemporary(), if I read it correctly, is simply a zero-order hold, which is completely unacceptable. Even if I don't read it correctly, the quality is horrible with 48000 Hz and OK with 44100 Hz (tested with MP3 files in foobar 2000). Please change the resampling method to something more scientific, e.g., by using libsamplerate.
http://bugs.winehq.org/show_bug.cgi?id=14717
Jasmine Iwanek jasmine@iwanek.co.uk changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |jasmine@iwanek.co.uk
--- Comment #1 from Jasmine Iwanek jasmine@iwanek.co.uk 2008-08-01 09:41:43 --- ello fancy seeing you here still using LFS?
any ideas on a workable patch mate?
http://bugs.winehq.org/show_bug.cgi?id=14717
Vitaliy Margolen vitaliy@kievinfo.com changed:
What |Removed |Added ---------------------------------------------------------------------------- OS/Version|All |other Platform|All |Other
--- Comment #2 from Vitaliy Margolen vitaliy@kievinfo.com 2008-08-01 23:35:20 --- Patches are welcome.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #3 from Austin English austinenglish@gmail.com 2009-02-02 16:33:46 --- Is this still an issue in current (1.1.14 or newer) wine?
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #4 from Alexander E. Patrakov patrakov@gmail.com 2009-02-03 00:33:34 --- The bug still exists and is easily triggered by playing music by Caravelli (album: grand prix) in foobar2000. Anyway, you can check for this bug yourself using the procedure below and any mp3 file containing music.
1. Install Jack (and, for convenience, qjackctl) and recompile wine so that it has a Jack output driver. Install PortAudio with the Jack backend, install Audacity.
2. Start Jack (preferrably, using qjackctl) with 192 kHz sampling rate and two channels (assuming that your hardware supports it). The intention is to make long runs of the same sample easily visible in the recorded waves. Verify that you can record from Jack in Audacity.
3. Using winecfg, select Jack as the output driver, set "Emulation" as the hw acceleration level, and tick the "Emulation" checkbox (not sure if this step is relevant)
4. Using regedit, go to the HKEY_CURRENT_USER\Software\Wine\DirectSound key, and change the DefaultSampleRate value to 192000.
5. Install foobar2000 in wine and run it. Start playing a long sound file.
6. Start audacity, select 192000 Hz as the project sampling rate, select wine_jack_out_0 as the recording source, start recording. jackrec may work, too, but I didn't test it.
7. Record several seconds of wine output through jack in audacity. Export as a 32-bit headerless raw file.
8. Look at the raw file with a hex editor, or use the following command:
od -A d -v -t d4 --width=8 wine.raw | less
Here is part of the output from my file:
0000000 -572671040 -563692352 0000008 -572671040 -563692352 0000016 -572671040 -563692352 0000024 -357640864 -732190528 0000032 -357640864 -732190528 0000040 -357640864 -732190528 0000048 -357640864 -732190528 0000056 -326575840 -840262720 0000064 -326575840 -840262720 0000072 -326575840 -840262720 0000080 -326575840 -840262720 0000088 -336275488 -845636800 0000096 -336275488 -845636800 0000104 -336275488 -845636800 0000112 -336275488 -845636800 0000120 -336275488 -845636800 0000128 -209590528 -826761856 0000136 -209590528 -826761856 0000144 -209590528 -826761856 0000152 -209590528 -826761856 0000160 -81791424 -820666816
You can clearly see the long runs of duplicated sample values, which means that wine is using zero-order-hold resampling.
Also, in audacity, you can look at the spectrum of what you recorded. Ideally, below half of the sampling rate of the sound file, it should exactly match what's in the sound file, while above that, it should be zero. Actually, there are distortions of -30 dB both above and below that frequency on real music files.
http://bugs.winehq.org/show_bug.cgi?id=14717
Andrew O. Shadoura (I DO NOT USE COMPIZ) bugzilla@tut.by changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |bugzilla@tut.by
--- Comment #5 from Andrew O. Shadoura (I DO NOT USE COMPIZ) bugzilla@tut.by 2009-03-01 06:12:59 --- WINE can use libSAD (Scale & Dither library) by Eugene Zagidullin, it's shipped with Audacious as for now.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #6 from Andrew O. Shadoura (I DO NOT USE COMPIZ) bugzilla@tut.by 2009-03-01 06:17:49 --- (In reply to comment #5)
WINE can use libSAD (Scale & Dither library) by Eugene Zagidullin, it's shipped with Audacious as for now.
Hmm, looks like not, looks like it doesn't support resampling.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #7 from Austin English austinenglish@gmail.com 2009-09-03 12:18:39 --- Is this still an issue in current (1.1.29 or newer) wine?
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #8 from Alexander E. Patrakov patrakov@gmail.com 2009-09-04 10:54:31 --- Austin, could you please stop re-asking the same question over and over again without looking at the code and without trying a simple test case that is already present above your comment?
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #9 from Alexander E. Patrakov patrakov@gmail.com 2009-09-04 11:10:25 --- Sorry, my previous comment wasn't clear enough.
The problematic code in dlls/dsound/mixer.c (and in fact the entire file) didn't change from the previous time you asked, so I see no point in retesting. Please reask only if you clearly see in the code that output samples are not copies of the input, but are combined from several nearby input samples using a windowed-sinc or similar filter.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #10 from Henri Verbeet hverbeet@gmail.com 2009-09-04 11:32:17 --- We ask if bugs are still present if they haven't seen activity for a relatively long time, since they might have been either fixed or abandoned in that time. Due to the amount of bugs it's not practical to verify this ourselves for every bug. In that regard you should consider it more or less as an automated reply.
As for the bug itself, yes, you're reading that code correctly, it sucks. Unfortunately dsound is only seeing minimal maintenance at the moment (as you can tell from the git log), so it seems unlikely this will be fixed soon.
http://bugs.winehq.org/show_bug.cgi?id=14717
p.drezet@inx-systems.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |p.drezet@inx-systems.com
--- Comment #11 from p.drezet@inx-systems.com 2010-04-08 19:45:42 --- I can confirm this problem also. Reproduced using an icecast system to stream windows generated audio. This problem renders the method nearly useless.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #12 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-11-23 12:25:44 CST --- Created an attachment (id=32094) --> (http://bugs.winehq.org/attachment.cgi?id=32094) First version of my patch
New code is simplier, cleaner, and best of all it seems considerably faster, at least on my hardware. It's rather major change to buffering code so new bugs may appear. For best result set default samplerate to 48000.
http://bugs.winehq.org/show_bug.cgi?id=14717
Krzysztof Nikiel zzdz2@yahoo.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #32094|0 |1 is obsolete| |
--- Comment #13 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-11-23 12:35:32 CST --- Created an attachment (id=32095) --> (http://bugs.winehq.org/attachment.cgi?id=32095) First version of my patch
New code is simplier, cleaner, and best of all it seems considerably faster, at least on my hardware. It's rather major change to buffering code so new bugs may appear. For best result set default samplerate to 48000.
http://bugs.winehq.org/show_bug.cgi?id=14717
Krzysztof Nikiel zzdz2@yahoo.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #32095|0 |1 is obsolete| |
--- Comment #14 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-11-23 12:37:24 CST --- Created an attachment (id=32097) --> (http://bugs.winehq.org/attachment.cgi?id=32097) lloks like binary files cannot be attached so plain text this time
http://bugs.winehq.org/show_bug.cgi?id=14717
Krzysztof Nikiel zzdz2@yahoo.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #32094|wine-dsound.diff.gz |wine-dsound.bin.gz filename| |
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #15 from Alexander E. Patrakov patrakov@gmail.com 2010-11-24 00:26:58 CST --- I am currently at work, so cannot run-test the patch. Will do so at home. However, I can review the code.
The math is very understandable:
* Precalculate a windowed sinc filter that is sufficient for a 50x change in the sample rate.
* Calculate the step of the FIR, differently in the cases of upsampling and downsampling. Round it to an integer, this only affects the position of the transition in the frequency response.
* Interpolate the FIR linearly between the precalculated points, calculate the convolution in the simplest possible way (i.e., without any polyphase tricks that can speed up the process).
Questions:
1) Why is the sinc Kaiser-windowed? There are simpler windows that don't require calculation of modified Bessel functions.
2) Why are the start of the stopband and the transition width chosen this way? As far as I understand, this will lead to attenuation near the Nyquist frequency of worse than -6 dB (not checked, but guessed by the fact that the transition width is more than the distance between the start of the stopband and the Nyquist frequency).
3) Did you test your patch for security bugs like buffer overflows for certain stupid requests like "resample this from a 100 Hz buffer to a 192 kHz output buffer"?
http://bugs.winehq.org/show_bug.cgi?id=14717
Krzysztof Nikiel zzdz2@yahoo.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |zzdz2@yahoo.pl
--- Comment #16 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-11-24 05:17:25 CST ---
Questions:
- Why is the sinc Kaiser-windowed? There are simpler windows that don't
require calculation of modified Bessel functions.
Kaiser seems a good compromise, good filter transition and simple to calculate, much simplier than equiripple.
- Why are the start of the stopband and the transition width chosen this way?
As far as I understand, this will lead to attenuation near the Nyquist frequency of worse than -6 dB (not checked, but guessed by the fact that the transition width is more than the distance between the start of the stopband and the Nyquist frequency).
Well, lowering stopband would reduce the passband. Shorting transition would increase the necessary impulse width. It's a trade-off. Longer impulse has worse time domain properties and requires more processing power.
- Did you test your patch for security bugs like buffer overflows for certain
stupid requests like "resample this from a 100 Hz buffer to a 192 kHz output buffer"?
I did some basic tests and it seems ok. I did the coding, now you are welcome to find bugs ;)
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #17 from Alexander E. Patrakov patrakov@gmail.com 2010-11-24 11:27:50 CST --- With this patch, the stereo channels are swapped.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #18 from Alexander E. Patrakov patrakov@gmail.com 2010-11-24 12:10:19 CST --- As for the transition band width vs impulse width vs stopband attenuation trade-off: I think it is necessary to study how these parameters are chosen by other resamplers out there (i.e., the speex resampler, libsamplerate and libavcodec) so that our resampler doesn't stand out either as a particularly bad or slow.
As for the ALSA-plugin for the libavcodec based resampler, here is their formula:
cutoff = 1.0 - 1.0/filter_size; if (cutoff < 0.80) cutoff = 0.80;
(supposedly in terms of Nyquist frequency)
Libsamplerate uses the following parameters:
Fastest: half-length of the filter is 2464 points, increment 128 (i.e. every 128th point is used for a given convolution), half-period of the sine 154
Medium: half-length of the filter is 22438 points, increment is 491, half-period of the sine is 534
Highest quality: half-length of the filter is 340239, increment is 2381, half-period of the sine is 2465
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #19 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-11-24 13:00:50 CST --- Created an attachment (id=32114) --> (http://bugs.winehq.org/attachment.cgi?id=32114) Transfer function of the resample filter
Hopefully the attached image will get correct mime type from bugzilla. It's created using the same table as in the patch. The approach is rather conservative, transition is well below Nyquist frequency. Usually(in commercial/hardware apps) the transition band seems rather centered around Nyquist and that's probably the best setting.
As to the comparison to the other resampler engines, my goal is to create ultimate resampler. It may be the other way around, our resampler will be better than them.
cutoff = 1.0 - 1.0/filter_size; if (cutoff < 0.80) cutoff = 0.80;
What does it mean, is it passband width, start of the stopband?
The whole band may be divided into parts: passband: signal passes unattenuated; transition band: signal becomes more and more attenuated; and finally the stopband: signal is attenuated as much as possible.
In my code the passband if from 0 to (0.96-0.15)*Nyquist and transition from (0.96-0.15) to 0.96. Stopband ripples are more than 80dB below the main signal. That gives just top quality output.
I don't remember now where the "0.96" came from. It actually was quite old code of mine, initially based on the original "resample" source, but then rewritten a few times. Now rewritten again.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #20 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-11-24 13:09:41 CST --- (In reply to comment #17)
With this patch, the stereo channels are swapped.
You found a bug, very good. That kind of bug can be squashed quickly.
http://bugs.winehq.org/show_bug.cgi?id=14717
Krzysztof Nikiel zzdz2@yahoo.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #32097|0 |1 is obsolete| |
--- Comment #21 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-11-24 13:46:15 CST --- Created an attachment (id=32115) --> (http://bugs.winehq.org/attachment.cgi?id=32115) fixed channel swap bug
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #22 from Alexander E. Patrakov patrakov@gmail.com 2010-11-26 10:36:41 CST --- I have built Wine with the second version of your patch. The sound is not horrible (in fact, I cannot distinguish it from the one from the best resampler), but, according to both your plot and the following unscientific test, your resampler is definitely not the best.
Now the test:
1) Install foobar2000 in wine and JAAA in linux
2) Configure Wine to output sound through JACK at either 96 or 192 kHz sampling rate, as permitted by your hardware
3) Run foobar2000, play a CD rip. Route its audio output to "jaaa -j" using something like qjackctl or patchage
4) Inspect the spectrum of wine output in realtime using JAAA. Use the "peak hold" option. You will see that, on typical contemporary music, the ultrasound components after your resampler peak at something like -80 dB. This also agrees with your plot.
Yes, I know that I should really look at distortions below 20 kHz, not at ultrasound components, but there are no such easy-to-use tools for it.
5) Now enable the resampler plugin in foobar2000's DSP manager (so that your resampler is out of the way now) and look at the spectrum again. The ultrasound components are at -120 dB now.
Moreover, without the "peak hold" option, it is obvious that foobar's ultrasound is noise + occasional glitches, while your resampler's ultrasound is visibly correlated with the sound that is piped through it.
The default ALSA resamplers can also be compared with your resampler this way. Use an .asoundrc that routes the audio output from ffplay through a plug plugin and then to jack. All of the working ones (pph aka speex, and libsamplerate) keep the ultrasound noise at -120 dB.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #23 from Alexander E. Patrakov patrakov@gmail.com 2010-11-26 11:38:01 CST --- As for the code itself:
You are using a reference-counted buffer for the FIR. This can work only if you guarantee that all the callers for FIR creation and destruction are in the same thread. Otherwise, you can attempt to generate two FIRs concurrently. If thread safety is indeed an issue, I'd suggest just dumping the coefficients into a static array in a header file.
As for my earlier remark about the cutoff frequency in your code, I missed the "cutoff = stopstart * resamp_ratio - (widthfac0 / FIR.width)" that explains it all. Still, your FIR is shorter even than the "fastest" option in libsamplerate. Libsamplerate seems to use a simple Gaussian window, maybe that's the reason that you don't need such a long FIR.
About my earlier remark about buffer overflows: I found no such things in your code, but it looks like the guard value that you allocated "just in case" at the end of the FIR is sometimes accessed. I may be wrong, though. I know I shouldn't review code at night :)
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #24 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-11-27 05:58:26 CST --- (In reply to comment #22) Do we really need more than 80dB ultrasound filtering, it will be masked by the souncard noise anyway, and masked even more by the amplifier noise. On the other side, i think it's possible a long impulse response may introduce some audible artifacts and it requires more CPU. Mixing speed may be important when mixing many buffers e.g. 3D buffers.
It would be possible to add some quality control. We could store the quality index in some reg key e.g. Software\Wine\DirectSound\ResampleQuality
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #25 from Alexander E. Patrakov patrakov@gmail.com 2010-11-27 06:19:25 CST --- I think it is a good idea to make the resampling quality adjustable, and here is why.
As I said, I can't tell the proposed resampler from the "default" one (speexrate) in alsa-plugins by ear, but still want their characteristics to be as similar as possible (up to copying the entire impulse response table) so that this bug has no chance to be reopened by "better" audiophiles. This indeed brings up the question of CPU usage with 3D effects, that's why the quality knob.
OTOH, Windows 2000 and XP already have a resampling quality knob. Does anyone know the characteristics of their resampler or the location of the registry value?
And even alsa-plugins and PulseAudio offer some presets with different quality of resampling.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #26 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-11-27 06:32:20 CST --- (In reply to comment #23)
As for the code itself:
You are using a reference-counted buffer for the FIR. This can work only if you guarantee that all the callers for FIR creation and destruction are in the same thread. Otherwise, you can attempt to generate two FIRs concurrently. If thread safety is indeed an issue, I'd suggest just dumping the coefficients into a static array in a header file.
Right, another bug, thanks. IIRC there are some special functions like EnterCriticalSection to get the process mutex so this can be solver easily.
As for my earlier remark about the cutoff frequency in your code, I missed the "cutoff = stopstart * resamp_ratio - (widthfac0 / FIR.width)" that explains it all. Still, your FIR is shorter even than the "fastest" option in libsamplerate. Libsamplerate seems to use a simple Gaussian window, maybe that's the reason that you don't need such a long FIR.
IIRC, i compared different windows in the past and Kaiser looked pretty good. Wikipedia has some nice article about these windows but i would say Gaussian doesn't look better than Kaiser. Note that mp3 format uses even worse hanning windows.
About my earlier remark about buffer overflows: I found no such things in your code, but it looks like the guard value that you allocated "just in case" at the end of the FIR is sometimes accessed. I may be wrong, though. I know I shouldn't review code at night :)
Oh, i'm pretty sure those bytes are unused but i may be wrong. May be better remove them to avoid confusion.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #27 from Alexander E. Patrakov patrakov@gmail.com 2010-11-27 08:41:26 CST --- While such functions as EnterCriticalSection exist in Windows API, modern books call them a design error, because they protect code, not data. Also, why use them if the static precalculated FIR achieves the same result (thread safety) without any synchronization primitives?
http://bugs.winehq.org/show_bug.cgi?id=14717
Krzysztof Nikiel zzdz2@yahoo.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #32115|0 |1 is obsolete| |
--- Comment #28 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-11-27 10:58:17 CST --- Created an attachment (id=32175) --> (http://bugs.winehq.org/attachment.cgi?id=32175) a couple bugs fixed, a few tiny improvements, better comments
Hopefully this version is thread safe, using InterlockedInc/Dec()
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #29 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-11-27 11:07:51 CST --- (In reply to comment #27)
While such functions as EnterCriticalSection exist in Windows API, modern books call them a design error, because they protect code, not data.
Looks like Wine uses these functions extensively, perhaps those books are in error. The patch is using something different anyway.
Also, why use them if the static precalculated FIR achieves the same result (thread safety) without any synchronization primitives?
Precalculated tables might be good, for now it's calculated on the fly.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #30 from Alexander E. Patrakov patrakov@gmail.com 2010-11-27 12:15:43 CST --- Unfortunately, the InterlockedIncrement-based synchronozation still looks wrong. Suppose that two threads call the DSOUND_CreateFIR() function simultaneously. Then, obviously, only one copy will fill in the values. The other copy will return immediately, as if the FIR is already filled. Then the thread that called that copy will immediately continue, with the assumption that the FIR is ready, while in fact it isn't yet.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #31 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-11-27 12:53:14 CST --- (In reply to comment #30)
Unfortunately, the InterlockedIncrement-based synchronozation still looks wrong. Suppose that two threads call the DSOUND_CreateFIR() function simultaneously. Then, obviously, only one copy will fill in the values. The other copy will return immediately, as if the FIR is already filled. Then the thread that called that copy will immediately continue, with the assumption that the FIR is ready, while in fact it isn't yet.
Indeed, it may happen that a thread produces some samples using uninitialized table, some noise. Looks like it needs a mutex or predefined tables to be thread safe.
http://bugs.winehq.org/show_bug.cgi?id=14717
Krzysztof Nikiel zzdz2@yahoo.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #32114|0 |1 is obsolete| |
--- Comment #32 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-11-29 03:37:26 CST --- Created an attachment (id=32218) --> (http://bugs.winehq.org/attachment.cgi?id=32218) table create/delete moved to DllMain
http://bugs.winehq.org/show_bug.cgi?id=14717
Krzysztof Nikiel zzdz2@yahoo.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #32175|0 |1 is obsolete| |
--- Comment #33 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-11-29 03:41:28 CST --- Created an attachment (id=32219) --> (http://bugs.winehq.org/attachment.cgi?id=32219) resample filter transfer function
Red is upsampling relative to input nyquist. Green is downsampling relative to output nyquist.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #34 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-11-29 03:43:32 CST --- (From update of attachment 32219) Red is upsampling relative to input nyquist. Green is downsampling relative to output nyquist.
Next, I will try to find some tables with better stopband.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #35 from Dmitry Timoshkov dmitry@codeweavers.com 2010-11-29 03:45:23 CST --- (In reply to comment #27)
While such functions as EnterCriticalSection exist in Windows API, modern books call them a design error, because they protect code, not data. Also, why use them if the static precalculated FIR achieves the same result (thread safety) without any synchronization primitives?
APIs exist for programmers, and the programmers do decide what they want to protect. Calling a design error an API is at least a confusion, if not a disinformation.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #36 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-11-29 05:38:58 CST --- (In reply to comment #35)
From my experience, windows as a whole sucks but the win API is not that bad,
it looks thoroughly rethought in most cases. The main problem is they keep it backward compatible with ancient versions.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #37 from Alexander E. Patrakov patrakov@gmail.com 2010-12-05 08:36:26 CST ---
Next, I will try to find some tables with better stopband.
I already suggested you to copy and paste them from libsamplerate.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #38 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-12-05 11:40:08 CST --- (In reply to comment #37)
Next, I will try to find some tables with better stopband.
I already suggested you to copy and paste them from libsamplerate.
My idea is to generate filter tables at build time. I'm working on it, possibly will post new version in a few days.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #39 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-12-08 05:29:08 CST --- Created an attachment (id=32410) --> (http://bugs.winehq.org/attachment.cgi?id=32410) libsamplerate reverse engineered
I have investigated those famous SRC tables and was able to create almost identical result with Kaiser sinc. The pictures look remarkably similar. Apparently i was wrong. Kaiser is not a compromise, Kaiser is the ultimate digital filter window. That's very good news, it's fast enough to generate tables at runtime.
http://bugs.winehq.org/show_bug.cgi?id=14717
Raymond superquad.vortex2@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |superquad.vortex2@gmail.com
--- Comment #40 from Raymond superquad.vortex2@gmail.com 2010-12-15 20:48:26 CST --- (In reply to comment #22)
I have built Wine with the second version of your patch. The sound is not horrible (in fact, I cannot distinguish it from the one from the best resampler), but, according to both your plot and the following unscientific test, your resampler is definitely not the best.
Now the test:
Install foobar2000 in wine and JAAA in linux
Configure Wine to output sound through JACK at either 96 or 192 kHz sampling
rate, as permitted by your hardware
- Run foobar2000, play a CD rip. Route its audio output to "jaaa -j" using
something like qjackctl or patchage
But foo_out_dsound plugin seem not available anymore
Are you using foo_out_ks or foo_out_wasapi plugin ?
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #41 from Alexander E. Patrakov patrakov@gmail.com 2010-12-16 01:56:03 CST --- I am using whatever is the default plugin for new installations of foobar2000.
http://bugs.winehq.org/show_bug.cgi?id=14717
Krzysztof Nikiel zzdz2@yahoo.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #32218|0 |1 is obsolete| |
--- Comment #42 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-12-20 04:42:10 CST --- Created an attachment (id=32551) --> (http://bugs.winehq.org/attachment.cgi?id=32551) resampler with quality setting
Finally, I have new version of the patch.
It now generates filter tables at build time and has quality setting. Registry is used to change quality level (string value): [Software\Wine\DirectSound] "ResampleQuality"="0" ... "3"
To build audiophile version of the resampler you need to run something like: cd dlls/dsound make clean make RDEFS='-DRESAMPLE_INSANE=1'
then you can set "ResampleQuality"="4", it uses the same(almost) filter as libsamplerate high_qual.
http://bugs.winehq.org/show_bug.cgi?id=14717
Krzysztof Nikiel zzdz2@yahoo.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #32219|0 |1 is obsolete| |
--- Comment #43 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-12-20 04:43:59 CST --- Created an attachment (id=32552) --> (http://bugs.winehq.org/attachment.cgi?id=32552) filter gain and resampled spectrum
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #44 from Raymond superquad.vortex2@gmail.com 2010-12-20 07:50:32 CST --- (In reply to comment #41)
I am using whatever is the default plugin for new installations of foobar2000.
Them your foobar200 v1.1 are not using dsound since it is not installed by default anymore
you can use WINEDEBUG=+dsound to verify that foobar2000 is using dsound or not
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #45 from Alexander E. Patrakov patrakov@gmail.com 2010-12-20 08:11:03 CST ---
your foobar200 v1.1 are not using dsound since it is not installed by default anymore
you can use WINEDEBUG=+dsound to verify that foobar2000 is using dsound or not
The result (from an older wine) is:
trace:dsound:DSOUND_MixInBuffer (0x143210,58152,2624) trace:dsound:DSOUND_bufpos_to_secpos Converted 148976/211680 to 148976/211680 trace:dsound:DSOUND_MixerVol (0x143210,2624) trace:dsound:DSOUND_MixerVol left = ffff, right = ffff trace:dsound:mix16 0x1e29d8 - 0x1b0360 2624
I think that the WaveOut API implementation in Wine uses dsound internally.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #46 from Raymond superquad.vortex2@gmail.com 2010-12-20 17:32:37 CST --- (In reply to comment #45)
your foobar200 v1.1 are not using dsound since it is not installed by default anymore
you can use WINEDEBUG=+dsound to verify that foobar2000 is using dsound or not
The result (from an older wine) is:
trace:dsound:DSOUND_MixInBuffer (0x143210,58152,2624) trace:dsound:DSOUND_bufpos_to_secpos Converted 148976/211680 to 148976/211680 trace:dsound:DSOUND_MixerVol (0x143210,2624) trace:dsound:DSOUND_MixerVol left = ffff, right = ffff trace:dsound:mix16 0x1e29d8 - 0x1b0360 2624
I think that the WaveOut API implementation in Wine uses dsound internally.
if winejack.drv can nly supply only jack server'sample rate
wwo->sample_rate = fp_jack_get_sample_rate(client);
but in waveoutcaps , it incorrectly state that it can support 16bits 11025Hz, 22050Hz and 44100Hz if the jack server is running at 48000Hz
948 /* NOTE: we don't support any 8 bit modes so note that */ 949 /* WOutDev[i].caps.dwFormats |= WAVE_FORMAT_4M08; 950 WOutDev[i].caps.dwFormats |= WAVE_FORMAT_4S08; */ 951 WOutDev[i].caps.dwFormats |= WAVE_FORMAT_4S16; 952 WOutDev[i].caps.dwFormats |= WAVE_FORMAT_4M16; 953 /* WOutDev[i].caps.dwFormats |= WAVE_FORMAT_2M08; 954 WOutDev[i].caps.dwFormats |= WAVE_FORMAT_2S08; */ 955 WOutDev[i].caps.dwFormats |= WAVE_FORMAT_2M16; 956 WOutDev[i].caps.dwFormats |= WAVE_FORMAT_2S16; 957 /* WOutDev[i].caps.dwFormats |= WAVE_FORMAT_1M08; 958 WOutDev[i].caps.dwFormats |= WAVE_FORMAT_1S08;*/ 959 WOutDev[i].caps.dwFormats |= WAVE_FORMAT_1M16; 960 WOutDev[i].caps.dwFormats |= WAVE_FORMAT_1S16; 961 }
It only 16bit and WAVE_FORMAT_48M16 | WAVE_FORMAT_48S16 by providing 16bit -> float conversion
if you start jack server at 96000Hz, winejack.drv should WAVE_FORMAT_96S16 | WAVE_FORMAT_96M16 in waveoutcaps
to get best result , you should start the jack server at 44100Hz
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #47 from Alexander E. Patrakov patrakov@gmail.com 2010-12-20 23:12:58 CST --- (In reply to comment #46)
if winejack.drv can nly supply only jack server'sample rate
wwo->sample_rate = fp_jack_get_sample_rate(client);
but in waveoutcaps , it incorrectly state that it can support 16bits 11025Hz, 22050Hz and 44100Hz if the jack server is running at 48000Hz
The above statement is a different bug (if a bug at all, because wine resamples, and Windows via HEL also resamples sound for cards that can support only 48 kHz), please discuss this elsewhere. Please leave discussion in this bug limited to the fact that the existing resampler in wine is a zero-order hold and that there is a patch being developed.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #48 from Raymond superquad.vortex2@gmail.com 2010-12-21 01:32:23 CST --- (In reply to comment #47)
(In reply to comment #46)
if winejack.drv can nly supply only jack server'sample rate
wwo->sample_rate = fp_jack_get_sample_rate(client);
but in waveoutcaps , it incorrectly state that it can support 16bits 11025Hz, 22050Hz and 44100Hz if the jack server is running at 48000Hz
The above statement is a different bug (if a bug at all, because wine resamples, and Windows via HEL also resamples sound for cards that can support only 48 kHz), please discuss this elsewhere. Please leave discussion in this bug limited to the fact that the existing resampler in wine is a zero-order hold and that there is a patch being developed.
The mixing is done in Kmixer (Kernel Audio Mixer) , dsound just use the hardware supported rate only
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #49 from Alexander E. Patrakov patrakov@gmail.com 2010-12-21 03:54:15 CST --- Then you are just saying that this bug has been assigned to a wrong component. Feel free to reassign as appropriate, but note that there is no "Kmixer" component in the drop-down box. The fact is that wine contains a zero-order-hold resampler in dlls/dsound/mixer.c, and that I complain about this bad-quality resampler.
JACK is used only for an objective testcase, bad quality also manifests itself with the ALSA backend for 44.1 -> 48 kHz conversion. Quality is bad when the application outputs sound with a sample rate other than the Default Sample Rate configured via winecfg.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #50 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-12-21 04:18:12 CST ---
dsound just use the hardware supported rate only
I think it only applies to primary buffer. Primary is best at hardware rate. Secondary buffers are different. You can have multiple sec bufs each running at different rate. Simpler hardware can't support such things for sure.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #51 from Raymond superquad.vortex2@gmail.com 2010-12-21 06:23:42 CST --- (In reply to comment #49)
Then you are just saying that this bug has been assigned to a wrong component. Feel free to reassign as appropriate, but note that there is no "Kmixer" component in the drop-down box. The fact is that wine contains a zero-order-hold resampler in dlls/dsound/mixer.c, and that I complain about this bad-quality resampler.
JACK is used only for an objective testcase, bad quality also manifests itself with the ALSA backend for 44.1 -> 48 kHz conversion. Quality is bad when the application outputs sound with a sample rate other than the Default Sample Rate configured via winecfg.
The jack server is playing the role of kernel audio mixer in linux but it just mixing all the jack clients at the server 's sample rate only
Foobar2000 also has resampler, the point is that wine incorrectly inform the application that it support 44100Hz when jack server 's running at a different rate when those dsound application call DirectSoundBuffer_SetFormat()
jack cannot be used as a test case since it does not really support dsound , just use the wine specific WAVE_DIRECTSOUND feature only
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #52 from Alexander E. Patrakov patrakov@gmail.com 2010-12-21 06:43:02 CST ---
jack cannot be used as a test case since it does not really support dsound , just use the wine specific WAVE_DIRECTSOUND feature only
OK, since you argue that my test case with JACK depends on a bug in the winejack driver (btw, please file it separately), please either help me to provide another objective test case that will excercise the code in dlls/dsound/mixer.c with freqAdjust != 1 << DSOUND_FREQSHIFT or state that this inequality should not happen at all. By "objective test case", I mean that the output samples sent to the backend should be recorded digitally.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #53 from Alexander E. Patrakov patrakov@gmail.com 2010-12-21 06:48:19 CST --- (In reply to comment #51)
The jack server is playing the role of kernel audio mixer in linux but it just mixing all the jack clients at the server 's sample rate only
Correct. However, Wine mixes all secondary buffers of the application into primary before sending the result to jack. And while mixing secondary buffers into primary, Wine resamples (badly, using zero-order hold) if there is a mismatch in the sample rates of primary and secondary buffers.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #54 from Raymond superquad.vortex2@gmail.com 2010-12-21 20:27:22 CST --- (In reply to comment #53)
(In reply to comment #51)
The jack server is playing the role of kernel audio mixer in linux but it just mixing all the jack clients at the server 's sample rate only
Correct. However, Wine mixes all secondary buffers of the application into primary before sending the result to jack. And while mixing secondary buffers into primary, Wine resamples (badly, using zero-order hold) if there is a mismatch in the sample rates of primary and secondary buffers.
it is wine specifc feature which need user to select the default sample rate, default bits, so winecfg should limit the default sample rate to jack server sample rate and default bits since winejack does not support 8bits after user selecting winejack
since wine can only use one driver at a time , winecfg should use radio button instead of checkbox in driver selection
Does foobar2000 use the primary buffer or DSCAPS_LOC_SOFTWARE/DSCAPS_LOC_HARDWARE for a software/hardware secondary buffer ?
The primary buffer must use the hardware supported rate (i.e. the jack server 's sample rate when using winejack )
There are two important issue in software mixing of the secondary buffer , mixing at different rate and mixing with software atten
if you look at winejack.drv 's source code, you will notice that winejack does not really support dsound
emulating using waveoutdevice does not work unless wine implement software atten during mixing of the secondary buffers because winjack does not have any volume control for each waveout stream
I guess the wine developer expect the linux audio system provide the volume control "software atten" to implement SetvolumePan() of dsound buffers
1) winealsa.drv
For those hardware mixing sound cards use IFACE_PCM control of the sound card
http://git.alsa-project.org/?p=alsa-tools.git;a=blob;f=hwmixvolume/README
a softvol control for each dmix stream for those non hardware mixing sound cards
2) winepulse.drv - provide software volume control for each PA stream but winepulse drop dsound support in version 0.27
Otherwise they have to implement software atten/mixing in dsound or use openal to emulate ds3d
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #55 from Raymond superquad.vortex2@gmail.com 2010-12-21 21:09:41 CST --- your change is mainly because jack server only support FLOAT when mixing
The major problem of your patch is removing those 8bits/16bits mixing for winealsa.drv and wineoss.drv and change the default sample rate to 48000 since most dsound or ds3d application use 11025Hz, 22050Hz and 44100Hz
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #56 from Alexander E. Patrakov patrakov@gmail.com 2010-12-22 04:12:04 CST --- (In reply to comment #54)
The primary buffer must use the hardware supported rate (i.e. the jack server 's sample rate when using winejack )
Yes. In my test from comment 4, this is indeed so: both jackd and the primary buffer run at 192 kHz. I agree that it is a bug that winejack advertises other rates as available for the primary buffer, but it is a separate bug 25587.
There are two important issue in software mixing of the secondary buffer , mixing at different rate and mixing with software atten
Yes. This bug is about the implementation of mixing at a different rate. Any DSP book will state that it should not be done by copying samples (as it is done now), but by band-limited interpolation.
[please indicate whether you agree with my statements below]
In the rest of your comment you list how various drivers could implement mixing at a different volume. The overall conclusion seems to be that most drivers can use the existing system services (like softvol and pulseaudio) to implement attenuation. It is true. In this case, a primary buffer is no longer present in Wine.
The same logic about moving the mixing procedure out of Wine can be applied even to streams with different sample rate: both ALSA and PulseAudio support this. So, if we remove winejack that can neither attenuate nor resample, move the primary buffer out of Wine, and let the existing sound system resample and attenuate sound, then there will be no need to have a resampler or mixer in Wine at all. The existing external resamplers and mixers are good. I.e., this is indeed an alternative solution to this bug. It may or may not be better in the long run, but it is rather intrusive.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #57 from Alexander E. Patrakov patrakov@gmail.com 2010-12-22 04:25:12 CST --- (In reply to comment #54)
Does foobar2000 use the primary buffer or DSCAPS_LOC_SOFTWARE/DSCAPS_LOC_HARDWARE for a software/hardware secondary buffer ?
How do I check?
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #58 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-12-22 05:26:24 CST --- (In reply to comment #55)
your change is mainly because jack server only support FLOAT when mixing
The major problem of your patch is removing those 8bits/16bits mixing for winealsa.drv and wineoss.drv and change the default sample rate to 48000 since most dsound or ds3d application use 11025Hz, 22050Hz and 44100Hz
Well, i'm starting to think you are a troll. You seem to produce more and more meaningless BS. The patch is not about jack at all, i don't think it makes much sense to use directsound with jack. Alexander only found a clever way to capture/analyse the dsound output with jack. The comment about 8/16/alsa/oss i don't understand.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #59 from Alexander E. Patrakov patrakov@gmail.com 2010-12-22 06:01:44 CST --- As for bit depth conversion: the original code provides functions for converting any sample format (8, 16, 24 or 32 bits) into any. The new code achieves the same goal by providing the getsample() and putsample() functions that convert anything to and from double and thus achieve the same goal.
There is one bug, though, in their implementation for 24-bit samples: it doesn't take the sign bit into account.
Also, it would be nice to avoid filtering the input through the FIR altogether if no resampling is needed (e.g., 44.1k -> 44.1k).
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #60 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-12-22 06:18:48 CST --- (In reply to comment #59)
There is one bug, though, in their implementation for 24-bit samples: it doesn't take the sign bit into account.
Indeed.
Also, it would be nice to avoid filtering the input through the FIR altogether if no resampling is needed (e.g., 44.1k -> 44.1k).
I think there is no filtering when in/out rates are equal but i haven't tested the output. Look at the 'if (dsb->freq != dsb->outfreq)' (line 197). I should just copy the samples if they are equal.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #61 from Alexander E. Patrakov patrakov@gmail.com 2010-12-22 06:22:28 CST --- (In reply to comment #60)
I think there is no filtering when in/out rates are equal but i haven't tested the output. Look at the 'if (dsb->freq != dsb->outfreq)' (line 197). I should just copy the samples if they are equal.
Indeed, you already do this.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #62 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-12-22 06:42:16 CST --- (In reply to comment #61)
(In reply to comment #60)
I think there is no filtering when in/out rates are equal but i haven't tested the output. Look at the 'if (dsb->freq != dsb->outfreq)' (line 197). I should just copy the samples if they are equal.
Indeed, you already do this.
It should verified. I just noticed there can be some amp problem. amp[] is calculated to match the filer but it may be wrong when copying samples.
http://bugs.winehq.org/show_bug.cgi?id=14717
Krzysztof Nikiel zzdz2@yahoo.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #32551|0 |1 is obsolete| |
--- Comment #63 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-12-22 07:14:10 CST --- Created an attachment (id=32588) --> (http://bugs.winehq.org/attachment.cgi?id=32588) fixed equal samplerate processing
After closer look it turned out to be a mess. Now it should handle equal rates correctly.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #64 from Raymond superquad.vortex2@gmail.com 2010-12-22 21:53:08 CST --- (In reply to comment #58)
(In reply to comment #55)
your change is mainly because jack server only support FLOAT when mixing
The major problem of your patch is removing those 8bits/16bits mixing for winealsa.drv and wineoss.drv and change the default sample rate to 48000 since most dsound or ds3d application use 11025Hz, 22050Hz and 44100Hz
Well, i'm starting to think you are a troll. You seem to produce more and more meaningless BS. The patch is not about jack at all, i don't think it makes much sense to use directsound with jack. Alexander only found a clever way to capture/analyse the dsound output with jack. The comment about 8/16/alsa/oss i don't understand.
foobar2000 or any media player which play a single audio stream is not suitable for testing dsound 's mixing of software secondary buffers
if you want good sound quality when listen music , it should bypass kernal audio mixer (i.e. jack server)
The test case should be those interactive games where you can hear the voice of the remote player and the sound effect of the game immediately without any noticable latency
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #65 from Alexander E. Patrakov patrakov@gmail.com 2010-12-25 06:30:57 CST --- Unfortunately, your patch breaks the build in Gentoo:
../../tools/makedep -C/home/build/portage/app-emulation/wine-1.3.10/work/wine-1.3.10/dlls/dsound -S/home/build/portage/app-emulation/wine-1.3.10/work/wine-1.3.10 -T../.. buffer.c capture.c dsound.c dsound_main.c duplex.c mixer.c primary.c propset.c resample.c sound3d.c version.rc dsound_classes_r.res firtab.h: No such file or directory firtab.h was first included from resample.c:39
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #66 from Alexander E. Patrakov patrakov@gmail.com 2010-12-25 06:42:22 CST --- I think that it would be more correct to write this line in Makefile.in:
resample.c depend: firtab.h
because "make depend" actually depends on firtab.h being present. At least, this fixed the build for me. Will test a bit later.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #67 from Alexander E. Patrakov patrakov@gmail.com 2010-12-25 08:22:17 CST ---
From the sound quality viewpoint, the patch works as expected (except the
24-bit issue that is still not fixed). Thanks!
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #68 from Alexander E. Patrakov patrakov@gmail.com 2010-12-25 08:33:38 CST --- Raymond, here are the results of tracing foobar2000. It uses the secondary buffer, and, while creating it, by default (i.e., without the special resampling plugin that is disabled by default) passes WAVEFORMATEX that matches the characteristics of the media file. So even in Windows, OS-based resampling is sometimes necessary.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #69 from Alexander E. Patrakov patrakov@gmail.com 2010-12-25 08:35:37 CST --- Sorry, I was not right that the patch works correctly. When playing mono files in foobar2000, it plays with too-high volume.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #70 from Raymond superquad.vortex2@gmail.com 2010-12-25 18:46:54 CST --- (In reply to comment #68)
Raymond, here are the results of tracing foobar2000. It uses the secondary buffer, and, while creating it, by default (i.e., without the special resampling plugin that is disabled by default) passes WAVEFORMATEX that matches the characteristics of the media file. So even in Windows, OS-based resampling is sometimes necessary.
dsound just pass 8bits/16bits to winealsa.drv since dscaps has DSCAPS_PRIMARY16BIT and DSCAPS_PRIMARY8BIT , DSCAPS_SECONDARY8BIT and DSCAPS_SECONDARY16BIT
are you sure that 24bits audio should be play through dsound instead wasapi ?
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #71 from Raymond superquad.vortex2@gmail.com 2010-12-25 21:10:50 CST --- it is the Linux system mixer need to support all formats and rates when mixing (e.g. plug:dmix , pulse or pulseaudio )
only jack server support mixing at single sampling rate and only support float and does not support the common 16bit audio
dsound only need to support mixing of 8bit or 16bits audio and does not need to support mixing of float since the hardware driver inform dsound whether it support 8bit or 16bit in DSCAPS
e.g. you need to use plug:jack (alsa-jack plugin) as default device when using winealsa.drv
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #72 from Alexander E. Patrakov patrakov@gmail.com 2010-12-26 01:05:05 CST --- Raymond, I am not sure that I understand what you demand. Could you please confirm below each of these points whether we agree on it?
1. Windows has kmixer, the component that mixes and resamples sound. In Wine, dsound-based resampling and mixing is done in dlls/dsound/mixer.c, by adding the contents of secondary dsound buffers (stretched or shunk by duplicating or dropping samples - that's important!) into primary.
2. Sound produced by the WaveOut API does not go through dlls/dsound/mixer.c (tested with WinAmp), is not affected by the "default sample rate" setting, is not resampled in Wine at all, and thus is not affected by this bug. It is affected by a valid bug 25587 in winejack, which existed before the patch and is a separate problem that this patch is not trying to solve.
3. The best quality is obtained when the application output is fed into the hardware buffer of the sound card without any resampling, but this is not always possible.
4. Even when resampling, it is possible to obtain sound quality that is arbitrarily close to the ideal one at frequencies below the half of the lower sample rate, by using a long-enough filter, at the cost of some latency and CPU usage. Duplicating and dropping samples is a fast, very-low-latency, but bad-quality technique of resampling.
5. When the application uses only secondary dsound buffers, wine opens its backend (winealsa.drv or whatever) at the sample rate configured as the "default sample rate" in winecfg.
6. Even when there is only one secondary buffer, wine currently mixes it into primary, possibly with volume and/or sample rate adjustments.
7. The patch by Krzysztof Nikiel implements a windowed-sinc based resampler in wine' dsound mixer.
8. Your words "it is the Linux system mixer need to support all formats and rates when mixing" can be equivalently reformulated "it is a bug that wine itself handles mixing and resampling (including that of secondary dsound buffers into primary) at all".
As for the 24-bit case, well, I am not sure. But the old code also tried to handle it in convert_??_to_24() and convert_24_to_??() functions, so 24-bit secondary buffers at least were meant to work. The new code tries to preserve the existing behaviour. Let's try not to do two things at once (implement both proper resampling and proper subset of supported formats). We can always rip out 24-bit support before or later, as it is a different issue than the zero-order-hold resampler. In other words, I want to see "drop 24-bit support from dsound" and "replace the zero-order-hold internal resampler with something else" as separate patches.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #73 from Raymond superquad.vortex2@gmail.com 2010-12-26 04:59:04 CST --- if you have visited foobar2k forum, those users prefer foo_out_asio or foo_out_ks which bypass kmixer
dsound is designed for game , the cpu usage and latency is more important when mixing sound effects
if your aim is to get best quantity of music in wine , use winealsa and change the default device to bypass any mixer ( dmix , pulseaudio, jack ... )
e.g. pulseaudio just override the default device to replace dmix as system mixer for alsa application in some linux distribution
pcm.!default { type pulse }
if your sound card support 44100Hz or 48000Hz
pcm.!default { type hw card 0 }
if your sound card only support 48000Hz and does not support 44100Hz
just use a better resampler instead of using alsa's default resampler
http://git.alsa-project.org/?p=alsa-plugins.git;a=blob;f=doc/samplerate.txt
http://git.alsa-project.org/?p=alsa-plugins.git;a=blob;f=doc/speexrate.txt
pcm.!default { type rate slave { pcm "hw:0,0" rate 48000 } converter "xxxx" }
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #74 from Alexander E. Patrakov patrakov@gmail.com 2010-12-26 05:31:46 CST --- (In reply to comment #73)
if you have visited foobar2k forum, those users prefer foo_out_asio or foo_out_ks which bypass kmixer
dsound is designed for game , the cpu usage and latency is more important when mixing sound effects
Have you looked at the patch? At the lowest quality setting ("fast"), the additional latency is 4 samples, and for the "slow" setting, it is 28 samples. This is still less than 1 ms. Compare this to the documented 10ms write lead that both Windows and wine apply to secondary buffers - I think that in Windows this delay is introduced specifically to allow for resampling.
if your aim is to get best quantity of music in wine , use winealsa and change the default device to bypass any mixer ( dmix , pulseaudio, jack ... )
This works well for WaveOut apps only. For DirectSound-based apps, this works only when the "default sample rate" setting in winecfg is the same as the actual sample rate used by the application for secondary buffers. Obviously, this is not good when there are files with different sample rates (i.e., some ripped from CD and some from DVD) in the playlist.
Proof: use winealsa, change the default device to be "hw:0" (in .asoundrc: pcm.!default "hw:0"), play some files with different parameters in foobar2000, see (cat /proc/asound/card0/pcm0p/sub0/hw_params) that Wine always opens the device with the "default sample rate" - i.e. resamples.
just use a better resampler instead of using alsa's default resampler
http://git.alsa-project.org/?p=alsa-plugins.git;a=blob;f=doc/samplerate.txt
http://git.alsa-project.org/?p=alsa-plugins.git;a=blob;f=doc/speexrate.txt
Yes, I know about these documents. The default ALSA resampler is settable with the "defaults.pcm.rate_converter" variable in .asoundrc. And these resamplers are good! The whole problem is that wine forces the use of its own bad resampler instead of a resampler from ALSA when an application requests a secondary DirectSound buffer with a rate different from the default sample rate configured in winecfg, and the patch attempts to replace this bad resampler with a better one.
pcm.!default { type rate slave { pcm "hw:0,0" rate 48000 } converter "xxxx" }
Yes, that's equivalent to what I wrote. And it suffers from the same problem: wine forces the use of its own sample-duplicating-and-dropping resampler (equivalent to "samplerate-order") for DirectSound apps before the ALSA resampler can see the data.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #75 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-12-26 10:35:45 CST --- The following patch should fix 24-bit output (not tested).
diff --git a/dlls/dsound/Makefile.in b/dlls/dsound/Makefile.in index 2c114e0..3c777a6 100644 --- a/dlls/dsound/Makefile.in +++ b/dlls/dsound/Makefile.in @@ -25,7 +25,7 @@ firtab.h: mkfir ./$< > $@ $(RM) $<
-resample.c: firtab.h +depend: firtab.h
@MAKE_DLL_RULES@
diff --git a/dlls/dsound/resample.c b/dlls/dsound/resample.c index 0a3128b..c785c1c 100644 --- a/dlls/dsound/resample.c +++ b/dlls/dsound/resample.c @@ -58,6 +58,8 @@ static fir_t *g_fir[] = { };
+#define SAMPLE24(p) (*((BYTE*)p)|*((BYTE*)p+1)<<8|*((CHAR*)p+2)<<16) + static inline sample_t getsample(LPBYTE buf, INT bps) { sample_t tmp; @@ -72,8 +74,7 @@ static inline sample_t getsample(LPBYTE buf, INT bps) tmp = *((SHORT *) buf); break; case 3: - tmp = *((BYTE *) buf) | *((BYTE *) (buf + 1)) << 8 | - *((BYTE *) (buf + 2)) << 16; + tmp = SAMPLE24(buf); tmp *= (1.0 / 256.0); break; case 4: @@ -104,10 +105,9 @@ static inline void putsample(LPBYTE buf, INT bps, sample_t smp) *((SHORT *) buf) = ismp; break; case 3: - CLIPSAMPLE(smp, (double) 0x7fff); ismp = lrint(smp * 256.0); - ismp += *((BYTE *) buf) | *((BYTE *) (buf + 1)) << 8 | - *((BYTE *) (buf + 2)) << 16; + ismp += SAMPLE24(buf); + CLIPSAMPLE(ismp, 0x7fffff); *((BYTE *) buf) = ismp & 0xff; ismp >>= 8; *((BYTE *) (buf + 1)) = ismp & 0xff;
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #76 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-12-26 11:16:02 CST --- This one should be better:
diff --git a/dlls/dsound/Makefile.in b/dlls/dsound/Makefile.in index 2c114e0..3c777a6 100644 --- a/dlls/dsound/Makefile.in +++ b/dlls/dsound/Makefile.in @@ -25,7 +25,7 @@ firtab.h: mkfir ./$< > $@ $(RM) $<
-resample.c: firtab.h +depend: firtab.h
@MAKE_DLL_RULES@
diff --git a/dlls/dsound/resample.c b/dlls/dsound/resample.c index 0a3128b..03fcf6f 100644 --- a/dlls/dsound/resample.c +++ b/dlls/dsound/resample.c @@ -58,6 +58,8 @@ static fir_t *g_fir[] = { };
+#define SAMPLE24(p) (*((BYTE*)p)|*((BYTE*)p+1)<<8|*((CHAR*)p+2)<<16) + static inline sample_t getsample(LPBYTE buf, INT bps) { sample_t tmp; @@ -72,8 +74,7 @@ static inline sample_t getsample(LPBYTE buf, INT bps) tmp = *((SHORT *) buf); break; case 3: - tmp = *((BYTE *) buf) | *((BYTE *) (buf + 1)) << 8 | - *((BYTE *) (buf + 2)) << 16; + tmp = SAMPLE24(buf); tmp *= (1.0 / 256.0); break; case 4: @@ -104,15 +105,14 @@ *((SHORT *) buf) = ismp; break; case 3: - CLIPSAMPLE(smp, (double) 0x7fff); ismp = lrint(smp * 256.0); - ismp += *((BYTE *) buf) | *((BYTE *) (buf + 1)) << 8 | - *((BYTE *) (buf + 2)) << 16; + ismp += SAMPLE24(buf); + CLIPSAMPLE(ismp, 0x7fffff); *((BYTE *) buf) = ismp & 0xff; ismp >>= 8; - *((BYTE *) (buf + 1)) = ismp & 0xff; + *((BYTE *) buf + 1) = ismp & 0xff; ismp >>= 8; - *((BYTE *) (buf + 2)) = ismp & 0xff; + *((BYTE *) buf + 2) = ismp & 0xff; break; case 4: CLIPSAMPLE(smp, (double) 0x7fff);
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #77 from Raymond superquad.vortex2@gmail.com 2010-12-26 18:24:08 CST --- (In reply to comment #74) The whole problem is that wine forces the use of its own bad
resampler instead of a resampler from ALSA when an application requests a secondary DirectSound buffer with a rate different from the default sample rate configured in winecfg, and the patch attempts to replace this bad resampler with a better one.
Do you know that some sound card can not use dmix since dmix require sound driver provide mmap access ? (i.e. there can be no system mixer for some sound cards )
It is because winealsa.drv provide the mixing of secondary software buffers in the primary hardware buffer (emulation mode) when there is no kmixer
This is the difference of dsound in wine and win since you don't even need to set the default sample rate , 8/16 bits data because the kmixer can automatically adjust the reampling rate to the maximum of the playing streams but those system mixer or sound servers in linux cannot
This also make the implementation of secondary hardware buffers for those hardware mixing sound cards very difficult since it use up one of the hardware buffer of mixing software secondary buffers
Most of those hardware mixing sound cards are also support Wavetable synth, they can provide at least 16 voices for playing midi
That is why they have 16 secondary hardware buffers
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #78 from Raymond superquad.vortex2@gmail.com 2010-12-26 20:14:59 CST --- (In reply to comment #72)
Raymond, I am not sure that I understand what you demand. Could you please confirm below each of these points whether we agree on it?
- Windows has kmixer, the component that mixes and resamples sound. In Wine,
dsound-based resampling and mixing is done in dlls/dsound/mixer.c, by adding the contents of secondary dsound buffers (stretched or shunk by duplicating or dropping samples - that's important!) into primary.
win98 did not has any kmixer
The current implementation of wineoss and winealsa is a trimmed down version of vxd driver which provide software mixing in dsound and winealsa.drv still does not provide access to secondary hardware buffers of those hardware mixing sound cards
THis mean that it does not support multiple waveoutopen bug#22880
Kmixer just set dwHwMixingBuffers,.... in DSCAPS so that dsound applications believe that all supported sound cards have hardware secondary buffers
- Sound produced by the WaveOut API does not go through dlls/dsound/mixer.c
(tested with WinAmp), is not affected by the "default sample rate" setting, is not resampled in Wine at all, and thus is not affected by this bug.
yes , waveoutopen in winealsa.drv use the "default" device which you can override to use "hw" directly, dmix or pulse ( in the past, wine use default:x to use the default playback device of the sound card x )
It is affected by a valid bug 25587 in winejack, which existed before the patch and is a separate problem that this patch is not trying to solve.
it is winejack incorrectly informed foobar that the it can support 44100Hz but you start jack server to run at 48000Hz, the only way is to use foobar's resampler or you add a resampler inside winejack.
or winejack can stop the jack server and restart the jack server at the application's requested sample rate
adding a cpu intensive resampler in dsound is a nightmare unless you implement an option to allow user to select the quality of SRC in winecfg.
the default SRC quality should be the one with minimum CPU usage
some windows drivers allow you to select the quality of the SRC instead of the sampling rate
- The best quality is obtained when the application output is fed into the
hardware buffer of the sound card without any resampling, but this is not always possible.
Some professional users are looking for bit exact, no mixing , up-sampling or down sampling when listing music (i.e. no format conversion ) and it need the sound card support the sampling rate of the audio stream
- When the application uses only secondary dsound buffers, wine opens its
backend (winealsa.drv or whatever) at the sample rate configured as the "default sample rate" in winecfg.
No, winejack does not support dsound,
you can only use one of the wine audio driver at a time.
The checkbox in winecfg mislead the user to believe that they can use any of them at the same time, but in fact wine only use the first checked driver in the list
winecfg should use radio button instead of checkbox to select audio driver
- Even when there is only one secondary buffer, wine currently mixes it into
primary, possibly with volume and/or sample rate adjustments.
Yes, but there is no volume gain/atten when mixing done you should notice about those "FIXME: SetVolumePan" messages in the log
- Your words "it is the Linux system mixer need to support all formats and
rates when mixing" can be equivalently reformulated "it is a bug that wine itself handles mixing and resampling (including that of secondary dsound buffers into primary) at all".
No, because none of those linux system mixer can support all sound cards, wine have to provide a fallback in "emulation" mode to provide mixing for those sound cards
/dev/dsp does not support mixing for most of the sound cards so wineoss also need dsound to perform software mixing
snd-emu10k1 must use "front" device for playback since there is a hook to change the internal routing to different jacks and this is why wineoss suddenly fail to work with SB live! sound cards when using ALSA 's OSS emulation
As for the 24-bit case, well, I am not sure. But the old code also tried to handle it in convert_??_to_24() and convert_24_to_??() functions, so 24-bit secondary buffers at least were meant to work. The new code tries to preserve the existing behaviour. Let's try not to do two things at once (implement both proper resampling and proper subset of supported formats). We can always rip out 24-bit support before or later, as it is a different issue than the zero-order-hold resampler. In other words, I want to see "drop 24-bit support from dsound" and "replace the zero-order-hold internal resampler with something else" as separate patches.
I think the problem is jack server only support float format , winejack must provide 16bits to float conversion inside winejack for waveoutopen
As winejack does not really provide dsound support ( no DSCAPS ) in wine,
I have doubt about adding 24bits support to dsound when your test case is using winejack and especially foobar is not an dsound game application
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #79 from Alexander E. Patrakov patrakov@gmail.com 2010-12-27 00:31:59 CST --- Raymond, thanks for your answers. However,
1) Your answer to statement 5 seems to be in contradiction with my test in comment 74, which used the hardware audio device via alsa, with no jack running. Could you please suggest what's different in the formulations of the test and the statement?
2) You seem to be confused by my use of jack in the earlier testcase in comment 4. It is completely irrelevant to this bug. I think that an equivalent testcase (i.e., something that records the digital output of wine, so that the runs of identical samples can be clearly seen) can be done using the "tee" ALSA PCM, without jack. Should I do this and post instructions?
By your own words, wine must provide mixing and resampling on its own, as a fallback. All I want is for this fallback (unfortunately triggered, because there is no equivalent of windows logic "kmixer can automatically adjust the sampling rate to the maximum of the playing streams" in wine) not to suck as badly as it does now. So wine does need a better internal resampler.
As for your comment about the CPU-intensive resampling: I think it would be more reasonable to require that the default sample rate conversion algorithm in wine matches the quality of the default algorithm in Windows, and that the quality is adjustable via winecfg (with the patch attached to this bug, it is adjustable only by a registry setting).
I think I can reverse-engineer the Windows logic by running Windows 2000 or XP in a patched kvm (so that it logs the commands to set the emulated hardware sampling rate, as well as the samples) and playing some test wav files. Would that be an acceptable test from a legal viewpoint?
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #80 from Raymond superquad.vortex2@gmail.com 2010-12-27 02:55:56 CST --- The answer is quite simple , the only wine audio driver which assign non-zero value to DSACPS is wineoss.drv ,
but ALSA 's kernel OSS emulation does not enable DSP_CAP_MULTI and emu10k1 does not work with wineoss any more after snd-emu10k1 need to use "front" device for playback to route the stereo to "front" jack
845 #ifdef DSP_CAP_MULTI /* not every oss has this */ 846 /* check for hardware secondary buffer support (multi open) */ 847 if ((arg & DSP_CAP_MULTI) && 848 (ossdev->out_caps.dwSupport & WAVECAPS_DIRECTSOUND)) { 849 TRACE("hardware secondary buffer support available\n"); 850 851 ossdev->ds_caps.dwMaxHwMixingAllBuffers = 16; 852 ossdev->ds_caps.dwMaxHwMixingStaticBuffers = 0; 853 ossdev->ds_caps.dwMaxHwMixingStreamingBuffers = 16; 854 855 ossdev->ds_caps.dwFreeHwMixingAllBuffers = 16; 856 ossdev->ds_caps.dwFreeHwMixingStaticBuffers = 0; 857 ossdev->ds_caps.dwFreeHwMixingStreamingBuffers = 16; 858 } 859 #endif
The value provided by winealsa in DSCAPS is all zero, so dxdiag only play with software buffers and does not play with hardware buffers.
even when you set hardware accelearion to full ( I am not sure why wine developer believe that hardware acclearation is related to mmap , may be alsa 's dmix need mmap support )
every sound cards using winealsa/wineoss are still practically remained in emulated mode since it just inform all dsound application that there is no Free HwMixing Streaming Buffers or Hw 3D buffers
so all dsound application have to use software secondary buffers
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #81 from Alexander E. Patrakov patrakov@gmail.com 2010-12-27 04:08:37 CST --- Thanks for explaining the apparent contradiction.
So our disagreements about the patch are:
1) too-aggressive defaults - I have to test Windows behaviour to decide. And yes, I have read flame wars caused by PulseAudio's default resampler, so I fully understand your desire to keep resource utilization to a minimum by default.
2) 24-bit and 32-bit support - can't test, but can read the code. Anyway, as I said, this is not a regression if reading and writing 24-bit values is done correctly, as the old code attempted to handle this situation, too. If this is indeed dead code, it can be dropped at any time. Will read some more code to form a more informed decision.
3) Bugs (such as already-found bug about wrong volume of mono streams). They just have to be found and fixed :)
Did I miss anything?
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #82 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-12-27 09:57:16 CST ---
As for your comment about the CPU-intensive resampling: I think it would be more reasonable to require that the default sample rate conversion algorithm in wine matches the quality of the default algorithm in Windows, and that the quality is adjustable via winecfg (with the patch attached to this bug, it is adjustable only by a registry setting).
It doesn't look like there is any true default quality. You can set it to good or best or whatever but it all seem to depend on the soudcard drivers. I tried it with two different cards and the quality is different at the same windows setting.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #83 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-12-27 09:59:40 CST --- broken volume fix:
diff --git a/dlls/dsound/resample.c b/dlls/dsound/resample.c index 03fcf6f..c133b83 100644 --- a/dlls/dsound/resample.c +++ b/dlls/dsound/resample.c @@ -286,11 +286,9 @@ DWORD DSOUND_PullBuffer(IDirectSoundBufferImpl * dsb, }
if (chan < 2) - rsum[chan] *= amp[chan]; + putsample(bufptr, oBPS, rsum[chan]*amp[chan]); else - rsum[chan] *= amp[2]; - - putsample(bufptr, oBPS, rsum[chan]); + putsample(bufptr, oBPS, rsum[chan]*amp[2]);
bufptr += oBPS; }
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #84 from Alexander E. Patrakov patrakov@gmail.com 2010-12-27 12:17:20 CST --- (In reply to comment #82)
It doesn't look like there is any true default quality. You can set it to good or best or whatever but it all seem to depend on the soudcard drivers. I tried it with two different cards and the quality is different at the same windows setting.
After experimenting with sb16 and ac97 in kvm, I must conclude that their Windows drivers use exactly the same resampler (I don't know if it is something default or just a coincidence), and it is not a zero-order-hold.
Technique of the experiment:
1. Create a wav file with 44.1 kHz sampling rate, containing 15 or so seconds of silence.
2. Create a wav file with 32 kHz sampling rate, containing several separated pulses of a known amplitude. I did this with a python script.
3. Put the follofing in your .asoundrc (assuming that your card supports 44.1 kHz):
pcm.filetee { type file file "/tmp/out.wav" slave.pcm "hw:0" format "wav" }
4. Start Windows XP in kvm as follows:
QEMU_AUDIO_DRV=alsa QEMU_ALSA_DAC_DEV=filetee QEMU_ALSA_ADC_DEV=hw:0 QEMU_AUDIO_DAC_FIXED_SETTINGS=0 QEMU_AUDIO_ADC_FIXED_SETTINGS=0 QEMU_AUDIO_DAC_TRY_POLL=0 QEMU_AUDIO_ADC_TRY_POLL=0 kvm -m 512 -soundhw ac97 -hda hda.dsk
(or with -soundhw sb16, but then you have to install the driver inside Windows XP)
5. Transfer foobar2000 installer and test wav files into Windows XP.
6. Open the silent wav file in Windows Media Player and the crackling one in foobar2000.
7. While the silent wav file is still playing, copy /tmp/out.wav somewhere else.
8. Examine the resulting wav file with a hex editor.
9. Note a particular sequence of non-zero non-header bytes in sb16 wav file, search for it in the ac97 wav file.
The worst quality setting has two or three output samples influenced by each input sample, the medium quality has the response width of 29 samples, and the best quality (default in the fresh XP install) has the response width of 79 output samples (1.8 ms). As I said, all of this is for the 32000 -> 44100 Hz resampling.
Further experimentation with 8000 -> 48000 Hz resampling reveals that the worst quality setting is in fact linear interpolation (so it is still better than the current zero-order hold in wine).
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #85 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-12-27 14:25:24 CST --- (In reply to comment #84)
After experimenting with sb16 and ac97 in kvm, I must conclude that their Windows drivers use exactly the same resampler (I don't know if it is something default or just a coincidence), and it is not a zero-order-hold.
Maybe some drivers use their own resampler and other drivers use windows resampler.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #86 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-12-27 14:45:36 CST --- (In reply to comment #85)
The worst quality setting has two or three output samples influenced by each input sample, the medium quality has the response width of 29 samples, and the best quality (default in the fresh XP install) has the response width of 79 output samples (1.8 ms). As I said, all of this is for the 32000 -> 44100 Hz resampling.
If there is 29 samples in 44100 it would be 21 samples in the source 32000 i.e. similar to this patch 'good' (19 samples). 79@44100 -> 57@32000 similar to patch 'slow' (55 samples).
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #87 from Raymond superquad.vortex2@gmail.com 2010-12-27 19:04:30 CST --- (In reply to comment #84)
After experimenting with sb16 and ac97 in kvm, I must conclude that their Windows drivers use exactly the same resampler (I don't know if it is something default or just a coincidence), and it is not a zero-order-hold.
it is meaningless to test those emulated sound card sb16 and ac97 in kvm or virtualbox.
you need to test your real sound card in xp
(e.g. SB Live!5.1 if you want to test foo_out_ks )
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #88 from Raymond superquad.vortex2@gmail.com 2010-12-27 21:05:57 CST --- (In reply to comment #74)
Yes, that's equivalent to what I wrote. And it suffers from the same problem: wine forces the use of its own sample-duplicating-and-dropping resampler (equivalent to "samplerate-order") for DirectSound apps before the ALSA resampler can see the data.
have you noticed that wine developer disable the resampling of the "default" device in dsoutput.c
#if SND_LIB_VERSION >= 0x010009 snd_pcm_hw_params_set_rate_resample(pcm, hw_params, 0); #endif
The capability of sampling provided by "rate" or "plug" plugin in your "default" device is disabled but no effect with "pulse" plugin because it is PA server perform resampling )
i.e. plug:dmix support only single sample rate only
this actually break the resampling of secondary software buffers when the sampling rate of the primary buffer is not supported by your hardware
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #89 from Raymond superquad.vortex2@gmail.com 2010-12-27 21:17:13 CST --- after disable the resampling using alsa-lib in dsoutput.c
winealsa.drv should not set DSCAPS_CONTINUOUSRATE when the default device is "plug:dmix" or using "rate" plugin
In theory , DSCAPS_CONTINUOUSRATE can be set only for those alsa drivers which support SNDRV_PCM_RATE_CONTINUOUS
DSCAPS_CONTINUOUSRATE mean The device supports all sample rates between the dwMinSecondarySampleRate and dwMaxSecondarySampleRate member values
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #90 from Alexander E. Patrakov patrakov@gmail.com 2010-12-27 23:49:59 CST --- (In reply to comment #87)
it is meaningless to test those emulated sound card sb16 and ac97 in kvm or virtualbox.
I would agree if the emulated sound card used the driver specifically written for it (i.e., something analogous to the VMware video driver). However, the emulated card pretends to be a real thing, identical to something manufactured in real-world silicon, and uses the drivers meant to be used with the real-silicon cards (i.e., the drivers that come with Windows itself). So these drivers would send identical output to the emulated and real-world cards and thus are suitable for reverse-engineering the resampler, provided that kvm itself does no resampling (that's why QEMU_AUDIO_DAC_FIXED_SETTINGS=0).
The reason for using an emulator is that it allows to capture the output digitally. A real sound card with S/PDIF output would also work if I had something that could record from S/PDIF.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #91 from Krzysztof Nikiel zzdz2@yahoo.pl 2010-12-28 10:34:59 CST --- (In reply to comment #84)
As I said, all of this is for the 32000 -> 44100 Hz resampling.
Was it really 44100 Hz, ac97 is more likely to use 48000. Maybe both waves were resampled to 48000 ?
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #92 from Alexander E. Patrakov patrakov@gmail.com 2010-12-28 11:29:37 CST --- It was ac97. According to the wav file header written to out.wav, windows sometimes configures the hardware to use 44100 Hz and sometimes 48000 Hz. This depends only on what is playing inside Windows: as you said, it picks the highest sample rate of the currently playing streams. I was surprised, too, when seeing a header with 44100 Hz in it.
The PCI ID of the emulated device is 8086:2415 (rev 01).
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #93 from Raymond superquad.vortex2@gmail.com 2010-12-31 00:52:27 CST ---
http://forums.logitech.com/t5/Internet-Headsets-Phones-READ/Windows-XP-Advan...
pcm.filetee { type file file "/tmp/out.wav" slave.pcm "hw:0" format "wav" }
What is your sound card "hw:0,0" ?
does it support 44100Hz or only 48000Hz ?
As I said, all of this is for the 32000 -> 44100 Hz
resampling.
Most of SB16 models does not support 48000Hz , this emulated sb16 seem not support 48000Hz when run linux inside qemu
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #94 from Alexander E. Patrakov patrakov@gmail.com 2010-12-31 03:39:09 CST --- hw:0 is an onboard Intel HDA codec. It supports both 44100 and 48000 Hz, but not 32000 Hz.
As for sb16 - you are right, it doesn't support 48 kHz. That's why I tested resampling from 32000 to 44100 Hz.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #95 from Alexander E. Patrakov patrakov@gmail.com 2010-12-31 06:07:03 CST --- (In reply to comment #76)
diff --git a/dlls/dsound/resample.c b/dlls/dsound/resample.c index 0a3128b..03fcf6f 100644 --- a/dlls/dsound/resample.c +++ b/dlls/dsound/resample.c @@ -58,6 +58,8 @@ static fir_t *g_fir[] = { };
+#define SAMPLE24(p) (*((BYTE*)p)|*((BYTE*)p+1)<<8|*((CHAR*)p+2)<<16)
static inline sample_t getsample(LPBYTE buf, INT bps) { sample_t tmp; @@ -72,8 +74,7 @@ static inline sample_t getsample(LPBYTE buf, INT bps) tmp = *((SHORT *) buf); break; case 3:
- tmp = *((BYTE *) buf) | *((BYTE *) (buf + 1)) << 8 |
*((BYTE *) (buf + 2)) << 16;
- tmp = SAMPLE24(buf); tmp *= (1.0 / 256.0); break; case 4:
@@ -104,15 +105,14 @@ *((SHORT *) buf) = ismp; break; case 3:
- CLIPSAMPLE(smp, (double) 0x7fff); ismp = lrint(smp * 256.0);
- ismp += *((BYTE *) buf) | *((BYTE *) (buf + 1)) << 8 |
*((BYTE *) (buf + 2)) << 16;
- ismp += SAMPLE24(buf);
- CLIPSAMPLE(ismp, 0x7fffff); *((BYTE *) buf) = ismp & 0xff; ismp >>= 8;
- *((BYTE *) (buf + 1)) = ismp & 0xff;
- *((BYTE *) buf + 1) = ismp & 0xff; ismp >>= 8;
- *((BYTE *) (buf + 2)) = ismp & 0xff;
- *((BYTE *) buf + 2) = ismp & 0xff; break; case 4: CLIPSAMPLE(smp, (double) 0x7fff);
As I said, I could not test it, only read. However, in my opinion, the new code for acessing 24-bit samples is correct with the possible exception that it can be dead.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #96 from Alexander E. Patrakov patrakov@gmail.com 2010-12-31 08:19:56 CST --- Further tests confirm that the volume bug is indeed fixed for upsampling. As indicated below, I can't reliably test downsampling due to another bug in your patch.
In the downsampling case your patch causes foobar2000 to eat 100% CPU. stutter (e.g., it says "wr-wr-ro-ro-ong-ong-n" instead of "wrong") and move the position slider back and forth on quality settings other than 0 or 1. And on these quality settings, the transition frequency is severely miscalculated (according to my test with playing white noise wav file in foobar2000 and recording wine output digitally, it is around 2 kHz at quality 1 and around 3 kHz at quality 0).
To reproduce, try to upsample any mono wav file to 96 kHz in audacity and then play it through foobar2000 when both wine and dmix are configured to use 44100 Hz as the default sample rate.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #97 from Raymond superquad.vortex2@gmail.com 2010-12-31 17:44:26 CST --- The problem is wine did not really implement setcooperativelevel() DSSCL_xxxx and provide a correct supported rates , format and channels of the sound card in DSBCAPS by winealsa.drv
it only pretend to support all rates, formats and channels in ALSA_Supported_format().
it should only modified DSBCAPS when it actually provide the extra supported formats of the sound card
DSSCL_NORMAL - default primary buffer of 22 KHz, stereo, 8-bit DSSCL_PRIORITY - change the data format of the primary buffer DSSCL_WRITEPRIMARY - the application must write directly to the primary buffer. Secondary buffers cannot be played while this is happening.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #98 from Raymond superquad.vortex2@gmail.com 2010-12-31 18:28:37 CST --- (In reply to comment #94)
hw:0 is an onboard Intel HDA codec. It supports both 44100 and 48000 Hz, but not 32000 Hz.
This mean that winecfg should only allow you to seclect 44100Hz or 48000Hz when you set "UseDirectHW"="Y" or your default device is "hw:0,0"
if your "default" is "plug:dmix" , the default rate is dmix 's default rate which you can change from 48000Hz to 44100Hz in /usr/share/alsa/alsa.conf
if your "default" is "pulse", you can select any rate supported by the "alsa-pulse" plugin
since winealsa.drv disable the resampling of "plug" or "rate" plugin only.
As for sb16 - you are right, it doesn't support 48 kHz. That's why I tested resampling from 32000 to 44100 Hz.
you should also notice that when sb16 playback at 16bits, it can only record at 8bits.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #99 from Raymond superquad.vortex2@gmail.com 2010-12-31 22:50:24 CST --- for 24-bits support, I think you have to specifiy which 24-bits format are supported when using winealsa.drv
http://www.alsa-project.org/alsa-doc/alsa-lib/group___p_c_m.html
SND_PCM_FORMAT_S24_LE Signed 24 bit Little Endian using low three bytes in 32-bit word SND_PCM_FORMAT_S24_BE Signed 24 bit Big Endian using low three bytes in 32-bit word SND_PCM_FORMAT_U24_LE Unsigned 24 bit Little Endian using low three bytes in 32-bit word SND_PCM_FORMAT_U24_BE Unsigned 24 bit Big Endian using low three bytes in 32-bit word
SND_PCM_FORMAT_S24_3LE Signed 24bit Little Endian in 3bytes format SND_PCM_FORMAT_S24_3BE Signed 24bit Big Endian in 3bytes format SND_PCM_FORMAT_U24_3LE Unsigned 24bit Little Endian in 3bytes format SND_PCM_FORMAT_U24_3BE Unsigned 24bit Big Endian in 3bytes format
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #100 from Alexander E. Patrakov patrakov@gmail.com 2011-01-01 00:07:03 CST --- (In reply to comment #98)
(In reply to comment #94)
hw:0 is an onboard Intel HDA codec. It supports both 44100 and 48000 Hz, but not 32000 Hz.
This mean that winecfg should only allow you to seclect 44100Hz or 48000Hz when you set "UseDirectHW"="Y" or your default device is "hw:0,0"
Yes, that would be correct. However, now winecfg allows to select any rate from the list. If I select the invalid rate, winealsa.drv opens the device at the nearest rate and lets wine's implementation of dsound deal with it - i.e., resample.
if your "default" is "plug:dmix" , the default rate is dmix 's default rate which you can change from 48000Hz to 44100Hz in /usr/share/alsa/alsa.conf
Yes, or by adding this line to .asoundrc:
defaults.pcm.dmix.rate 44100
As for sb16 - you are right, it doesn't support 48 kHz. That's why I tested resampling from 32000 to 44100 Hz.
you should also notice that when sb16 playback at 16bits, it can only record at 8bits.
This may be true, I didn't test recording inside Windows in kvm. Can you suggest any freeware program that is not part of Windows (so that I can compare results in Windows and wine), can record sounds and uses DirectSound for that?
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #101 from Raymond superquad.vortex2@gmail.com 2011-01-01 02:16:55 CST --- the compilance test is running dsound_test for winealsa.drv wineoss.drv
WINETEST_INTERACTIVE=1 wine dlls/dsound/tests/dsound_test dsound
WINETEST_INTERACTIVE=1 wine dlls/dsound/tests/dsound_test dsound8
WINETEST_INTERACTIVE=1 wine dlls/dsound/tests/dsound_test ds3d
WINETEST_INTERACTIVE=1 wine dlls/dsound/tests/dsound_test capture
In theory
dxdiag 's "Test sound" button should also able to play sound with software and hardware buffers
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #102 from Raymond superquad.vortex2@gmail.com 2011-01-01 06:13:46 CST --- (In reply to comment #100)
(In reply to comment #98)
(In reply to comment #94)
hw:0 is an onboard Intel HDA codec. It supports both 44100 and 48000 Hz, but not 32000 Hz.
This mean that winecfg should only allow you to seclect 44100Hz or 48000Hz when you set "UseDirectHW"="Y" or your default device is "hw:0,0"
Yes, that would be correct. However, now winecfg allows to select any rate from the list. If I select the invalid rate, winealsa.drv opens the device at the nearest rate and lets wine's implementation of dsound deal with it - i.e., resample.
It depends on which HDA codec ,
I guess wine developers 's HDA codec are similar to mine which support
rates [0x7ff]: 8000 11025 16000 22050 32000 44100 48000 88200 96000 176400 192000 bits [0xe]: 16 20 24 formats [0x1]: PCM
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #103 from Alexander E. Patrakov patrakov@gmail.com 2011-01-01 07:07:57 CST --- My HDA codec has this instead:
rates [0x7e0]: 44100 48000 88200 96000 176400 192000 bits [0xe]: 16 20 24 formats [0x1]: PCM
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #104 from Krzysztof Nikiel zzdz2@yahoo.pl 2011-01-01 10:57:22 CST --- (In reply to comment #96)
In the downsampling case your patch causes foobar2000 to eat 100% CPU. stutter (e.g., it says "wr-wr-ro-ro-ong-ong-n" instead of "wrong") and move the position slider back and forth on quality settings other than 0 or 1. And on these quality settings, the transition frequency is severely miscalculated (according to my test with playing white noise wav file in foobar2000 and recording wine output digitally, it is around 2 kHz at quality 1 and around 3 kHz at quality 0).
This should help:
diff --git a/dlls/dsound/resample.c b/dlls/dsound/resample.c index e8bc2ad..7486d78 100644 --- a/dlls/dsound/resample.c +++ b/dlls/dsound/resample.c @@ -346,9 +346,8 @@ void DSOUND_RecalcFormat(IDirectSoundBufferImpl * dsb)
dsb->inpos = dsb->infrac = 0; /* reset resampler pointer */
- if (dsb->freq < dsb->outfreq) - dsb->firstep = g_fir[dsb->quality]->step; - else if (dsb->freq > dsb->outfreq) + dsb->firstep = g_fir[dsb->quality]->step; + if (dsb->outfreq < dsb->freq) { /* move transition band below output nuquist */ dsb->firstep = (9 * dsb->firstep * dsb->outfreq) / (10 * dsb->freq); @@ -360,7 +359,7 @@ void DSOUND_RecalcFormat(IDirectSoundBufferImpl * dsb) if (dsb->firstep < 1) dsb->firstep = 1; } - else + else if (dsb->freq == dsb->outfreq) dsb->firstep = 1;
TRACE("resample quality: %d\n", dsb->quality);
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #105 from Alexander E. Patrakov patrakov@gmail.com 2011-01-01 12:26:10 CST --- Now playback of various audio files in foobar2000 seems to expose no audible driver-independent quality or correctness issues (will perform objective tests later). Also I tested with some games, so far successfully - but it only means that I have to do more tests, including those suggested by Raymond :)
Well, there is one very minor nitpick:
At quality level 3, the resampler is more CPU-hungry than wine seems to advertise in dwPlayCpuOverheadSwBuffers (hard-coded as 1%): 5% per stream on my system for 44100 -> 48000 Hz stereo. Not sure if we need to do anything with it.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #106 from Raymond superquad.vortex2@gmail.com 2011-01-01 21:58:47 CST --- does your alsa snd-hda-intel driver support multi streaming playback ?
This expose another playback device through dsound
http://www.intel.com/support/motherboards/desktop/sb/cs-020642.htm
Enabling Multistreaming Playback Multistreaming allows you to listen to two different audio sources on two different speaker sets. For example, you can listen to one audio source through the back panel speakers and a second audio source through front panel headphones or speakers.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #107 from Alexander E. Patrakov patrakov@gmail.com 2011-01-01 23:18:50 CST --- Sorry, I have Windows only in a virtual machine, so can't test using your suggested method. The device and driver capabilities in Linux may be different because I have to override the model in order to have working front panel audio:
options snd-hda-intel model=ref-no-jd
As for jack retasking between mic in/line in/line out, I have the corresponding controls in alsamixer and they work.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #108 from Alexander E. Patrakov patrakov@gmail.com 2011-01-01 23:44:25 CST --- On the other thought, playing two clients on different outputs of the sound card is achievable (without mixing) with .asoundrc similar to http://alsa.opensrc.org/index.php/Dshare, so in some sense this is supported on any multichannel card that supports mmap.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #109 from Alexander E. Patrakov patrakov@gmail.com 2011-01-02 00:20:03 CST --- (In reply to comment #88)
have you noticed that wine developer disable the resampling of the "default" device in dsoutput.c
#if SND_LIB_VERSION >= 0x010009 snd_pcm_hw_params_set_rate_resample(pcm, hw_params, 0); #endif
Thanks for pointing this out. This line indeed has an interesting effect. If I configure the default sample rate in wine to be 44100 Hz in the default "plug:dmix" setup without changing the default dmix rate via defaults.pcm.dmix.rate, here is what happens when nothing else is playing:
1) this line disables resampling in the plug, effectively leaving us with dmix only
2) wine tries to open dmix at "something near 44100 Hz"
3) alsa is smart enough to ignore the explicit 48 kHz rate configured for dmix in favour of wine request (confirmed by /proc/asound/Intel/pcm0p/sub0/hw_params)
End result: dmix running at 44100 Hz (even though the alsa configuration tells otherwise) and playing what wine feeds to it.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #110 from Alexander E. Patrakov patrakov@gmail.com 2011-01-02 03:38:30 CST --- After the patch, dsound8 tests from comment 101 stutter. OTOH, they (unlike the original unpatched wine) sound correctly (except for this stutter) for 24-bit secondary buffers.
http://bugs.winehq.org/show_bug.cgi?id=14717
Krzysztof Nikiel zzdz2@yahoo.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #32588|0 |1 is obsolete| |
--- Comment #111 from Krzysztof Nikiel zzdz2@yahoo.pl 2011-01-02 06:27:19 CST --- Created an attachment (id=32697) --> (http://bugs.winehq.org/attachment.cgi?id=32697) fixed bugs and dsound/8 test clicking
After the patch, dsound8 tests from comment 101 stutter. OTOH, they (unlike the original unpatched wine) sound correctly (except for this stutter) for 24-bit secondary buffers.
I heard this clicking at all bit depths and also with dsound tests.
Now the write position/lead logic is changed to use 'firdelay' instead of 'writelead' and is seems to work well.
Hopefully this version is close to final.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #112 from Alexander E. Patrakov patrakov@gmail.com 2011-01-02 07:59:13 CST --- Clicks for resampled sound are indeed gone. However, they appeared for the case when there is no change in the sample rate. I guess that the resampling delay should be added to the write lead, not used instead of it. Could you please change the patch?
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #113 from Alexander E. Patrakov patrakov@gmail.com 2011-01-02 08:33:24 CST --- Also with your latest patch, foobar2000 gets a lot of underruns
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #114 from Alexander E. Patrakov patrakov@gmail.com 2011-01-02 09:39:40 CST --- I have tried to do this addition myself:
if (dsb->freq == dsb->outfreq) { dsb->firstep = 1; dsb->firdelay = (dsb->freq / 100) * dsb->pwfx->nBlockAlign; } else { dsb->firdelay = (2 * g_fir[dsb->quality]->size / dsb->firstep) + (dsb->freq / 100); dsb->firdelay *= dsb->pwfx->nBlockAlign; }
It fixed the dsound8 test completely, but made matters with foobar2000 even worse.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #115 from Krzysztof Nikiel zzdz2@yahoo.pl 2011-01-02 10:56:28 CST --- (In reply to comment #112)
Clicks for resampled sound are indeed gone. However, they appeared for the case when there is no change in the sample rate. I guess that the resampling delay should be added to the write lead, not used instead of it. Could you please change the patch?
OK, the initial modification was too quick. This should make it better:
diff --git a/dlls/dsound/buffer.c b/dlls/dsound/buffer.c index 3148502..10e68c8 100644 --- a/dlls/dsound/buffer.c +++ b/dlls/dsound/buffer.c @@ -500,6 +500,7 @@ static HRESULT WINAPI IDirectSoundBufferImpl_Lock( ) { HRESULT hres = DS_OK; IDirectSoundBufferImpl *This = (IDirectSoundBufferImpl *)iface; + INT firspace;
TRACE("(%p,%d,%d,%p,%p,%p,%p,0x%08x) at %d\n", This, @@ -542,9 +543,11 @@ static HRESULT WINAPI IDirectSoundBufferImpl_Lock( return DSERR_INVALIDPARAM; }
- writebytes -= This->firdelay; - if ((INT)writebytes < 0) - writebytes = 0; + firspace = This->inpos - This->firdelay - writecursor; + while (firspace < 0) + firspace += This->buflen; + if (writebytes > firspace) + writebytes = firspace;
/* **** */ RtlAcquireResourceShared(&This->lock, TRUE); diff --git a/dlls/dsound/resample.c b/dlls/dsound/resample.c index d2bbda2..3204a2b 100644 --- a/dlls/dsound/resample.c +++ b/dlls/dsound/resample.c @@ -47,7 +47,7 @@ WINE_DEFAULT_DEBUG_CHANNEL(dsound); #define SAMPLE24(p) (*((BYTE*)p)|*((BYTE*)p+1)<<8|*((CHAR*)p+2)<<16) #ifdef WORDS_BIGENDIAN #define SAMPLE16(p) (*((BYTE*)p)|*((CHAR*)p+1)<<8) -#define PUT16(p) (*((BYTE*)p)|*((CHAR*)p+1)<<8) +#define PUT16(p,s) *((BYTE*)p)=s;*((BYTE*)p+1)=(s>>8) #else #define SAMPLE16(p) (*((SHORT*)p)) #define PUT16(p,s) *((SHORT*)p)=s @@ -364,13 +364,12 @@ void DSOUND_RecalcFormat(IDirectSoundBufferImpl * dsb) if (dsb->freq == dsb->outfreq) { dsb->firstep = 1; - dsb->firdelay = 0; + dsb->firdelay = 1; } else - { dsb->firdelay = (2 * g_fir[dsb->quality]->size / dsb->firstep) + 1; - dsb->firdelay *= dsb->pwfx->nBlockAlign; - } + + dsb->firdelay *= dsb->pwfx->nBlockAlign;
TRACE("resample quality: %d\n", dsb->quality); TRACE("resample firstep %d\n", dsb->firstep);
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #116 from Alexander E. Patrakov patrakov@gmail.com 2011-01-02 12:18:35 CST --- The new patch works fine for both foobar2000 and the dsound8 testcase. However, it removes the "documented 10ms lead", and I am not so comfortable with that. I.e. this needs attention from an expert, but can be done later.
Another issue (not sure if this is new to this patch) is that the "test sound" button in winecfg now only produces a click (as usually comes from opening and closing the device) and this message:
fixme:winmm:proc_PlaySound Couldn't play header
Also I would like to see a separate patch that adds the quality slider to winecfg.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #117 from Krzysztof Nikiel zzdz2@yahoo.pl 2011-01-02 12:32:11 CST --- (In reply to comment #116)
The new patch works fine for both foobar2000 and the dsound8 testcase. However, it removes the "documented 10ms lead", and I am not so comfortable with that. I.e. this needs attention from an expert, but can be done later.
Do you have any idea where the "documented 10ms lead" came from? I think the patch does it the right way.
Another issue (not sure if this is new to this patch) is that the "test sound" button in winecfg now only produces a click (as usually comes from opening and closing the device) and this message:
fixme:winmm:proc_PlaySound Couldn't play header
It's winmm/playsound.c , doesn't seem to use dsound.
Also I would like to see a separate patch that adds the quality slider to winecfg.
Yeah, me too.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #118 from Krzysztof Nikiel zzdz2@yahoo.pl 2011-01-02 12:58:22 CST --- (In reply to comment #117)
(In reply to comment #116)
The new patch works fine for both foobar2000 and the dsound8 testcase. However, it removes the "documented 10ms lead", and I am not so comfortable with that. I.e. this needs attention from an expert, but can be done later.
Do you have any idea where the "documented 10ms lead" came from? I think the patch does it the right way.
I think I found it. I'm not sure if it's a good idea to cite the windows docs but let me try:
"The write cursor indicates the position at which it is safe to write new data to the buffer. The write cursor always leads the play cursor, typically by about 15 milliseconds' worth of audio data. For more information, see DirectSound in the Graphics and Multimedia guide."
So, it doesn't look like something defined, rather some tip about the expected behavior.
Anyway, I have realized that it can be done better and simpler. Try this fix:
diff --git a/dlls/dsound/buffer.c b/dlls/dsound/buffer.c index 3148502..e1e5428 100644 --- a/dlls/dsound/buffer.c +++ b/dlls/dsound/buffer.c @@ -422,7 +422,12 @@ static HRESULT WINAPI IDirectSoundBufferImpl_GetCurrentPosition( }
if (playpos) - *playpos = pos; + { + *playpos = pos - This->firdelay; + if ((INT)*playpos < 0) + *playpos += This->buflen; + } + if (writepos) *writepos = pos; } @@ -542,10 +547,6 @@ static HRESULT WINAPI IDirectSoundBufferImpl_Lock( return DSERR_INVALIDPARAM; }
- writebytes -= This->firdelay; - if ((INT)writebytes < 0) - writebytes = 0; - /* **** */ RtlAcquireResourceShared(&This->lock, TRUE);
diff --git a/dlls/dsound/resample.c b/dlls/dsound/resample.c index d2bbda2..7738dd4 100644 --- a/dlls/dsound/resample.c +++ b/dlls/dsound/resample.c @@ -1,7 +1,7 @@ /* DirectSound * * Resample core - * Copyright 2010 Krzysztof Nikiel + * Copyright 2010, 2011 Krzysztof Nikiel * * Initially based on resample: * http://www-ccrma.stanford.edu/~jos/resample/ @@ -47,7 +47,7 @@ WINE_DEFAULT_DEBUG_CHANNEL(dsound); #define SAMPLE24(p) (*((BYTE*)p)|*((BYTE*)p+1)<<8|*((CHAR*)p+2)<<16) #ifdef WORDS_BIGENDIAN #define SAMPLE16(p) (*((BYTE*)p)|*((CHAR*)p+1)<<8) -#define PUT16(p) (*((BYTE*)p)|*((CHAR*)p+1)<<8) +#define PUT16(p,s) *((BYTE*)p)=s;*((BYTE*)p+1)=(s>>8) #else #define SAMPLE16(p) (*((SHORT*)p)) #define PUT16(p,s) *((SHORT*)p)=s @@ -346,8 +346,6 @@ void DSOUND_RecalcFormat(IDirectSoundBufferImpl * dsb) dsb->outfreq = dsb->device->pwfx->nSamplesPerSec; dsb->outfreq_1 = 1.0 / dsb->outfreq;
- dsb->inpos = dsb->infrac = 0; /* reset resampler pointer */ - dsb->firstep = g_fir[dsb->quality]->step; if (dsb->outfreq < dsb->freq) { @@ -364,13 +362,15 @@ void DSOUND_RecalcFormat(IDirectSoundBufferImpl * dsb) if (dsb->freq == dsb->outfreq) { dsb->firstep = 1; - dsb->firdelay = 0; + dsb->firdelay = 1; } else - { dsb->firdelay = (2 * g_fir[dsb->quality]->size / dsb->firstep) + 1; - dsb->firdelay *= dsb->pwfx->nBlockAlign; - } + + dsb->firdelay *= dsb->pwfx->nBlockAlign; + + dsb->infrac = 0; + dsb->inpos = dsb->firdelay;
TRACE("resample quality: %d\n", dsb->quality); TRACE("resample firstep %d\n", dsb->firstep);
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #119 from Raymond superquad.vortex2@gmail.com 2011-01-02 19:40:35 CST --- (In reply to comment #109)
(In reply to comment #88)
have you noticed that wine developer disable the resampling of the "default" device in dsoutput.c
#if SND_LIB_VERSION >= 0x010009 snd_pcm_hw_params_set_rate_resample(pcm, hw_params, 0); #endif
Thanks for pointing this out. This line indeed has an interesting effect. If I configure the default sample rate in wine to be 44100 Hz in the default "plug:dmix" setup without changing the default dmix rate via defaults.pcm.dmix.rate, here is what happens when nothing else is playing:
- this line disables resampling in the plug, effectively leaving us with dmix
only
it still provide 8bits to 16bits , mono to stereo conversion as your hda does not support 8bits and mono.
wine tries to open dmix at "something near 44100 Hz"
alsa is smart enough to ignore the explicit 48 kHz rate configured for dmix
in favour of wine request (confirmed by /proc/asound/Intel/pcm0p/sub0/hw_params)
End result: dmix running at 44100 Hz (even though the alsa configuration tells otherwise) and playing what wine feeds to it.
you can specify the winecfg's default dsound rate as dmix's sample rate by open pcm device plug:'dmix:0,RATE=22050' if your hardware support 22050Hz without changing anything
This mean wine have to specify the default rate (specified in winefg) in snd_pcm_open() in dsoutput.c
I think it is not correct for wine to allow dsound to use 'dmix' or 'pulse' as the primary buffer to mix those ds3d games's 16 voices
it is not a good solution for ds3d to use "plughw" device since the mono 3D spatialisation need software left/right panning of 16 voices performed inside dsound when your sound card does not provide hardware volume panning for 16 voices
Try using "hw:0,0" instead of "dmix" when running RMAA or rightmark 3dsound
http://appdb.winehq.org/objectManager.php?sClass=version&iId=8993
RightMark 3DSound: Positioning Accuracy test RightMark 3DSound: CPU Utilization test
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #120 from Raymond superquad.vortex2@gmail.com 2011-01-03 01:58:16 CST --- 44100Hz stereo sound was very bad when using OSS emulation with your patch but sound good without your patch
trace:dsound:setup_dsound_options ds_emuldriver = 0 trace:dsound:setup_dsound_options ds_hel_buflen = 65536 trace:dsound:setup_dsound_options ds_snd_queue_max = 10 trace:dsound:setup_dsound_options ds_snd_queue_min = 6 trace:dsound:setup_dsound_options ds_hw_accel = Full trace:dsound:setup_dsound_options ds_default_playback = 0 trace:dsound:setup_dsound_options ds_default_capture = 0 trace:dsound:setup_dsound_options ds_default_sample_rate = 44100 trace:dsound:setup_dsound_options ds_default_bits_per_sample = 16 trace:dsound:setup_dsound_options ds_snd_shadow_maxsize = 2 trace:dsound:setup_dsound_options ds_resample_quality = 2
trace:wave:wodOpen OSS_OpenDevice requested this format: 44100x16x2 WAVE_FORMAT_PCM trace:wave:wodOpen requesting 32 2048 byte fragments (11 ms/fragment) trace:wave:OSS_OpenDevice (0x690ef740,2,0x290e64c,0,44100,2,10) trace:wave:OSS_RawOpenDevice (0x690ef740,0) trace:wave:OSS_RawOpenDevice open_access=O_RDWR trace:wave:wodOpen OSS_OpenDevice returned this format: 44100x16x2 trace:wave:wodOpen got 32 2048 byte fragments (11 ms/fragment) trace:wave:wodOpen fd=32 fragstotal=32 fragsize=2048 BufferSize=65536 trace:wave:wodPlayer waiting 4294967295ms (4294967295,4294967295) trace:wave:wodOpen wBitsPerSample=16, nAvgBytesPerSec=176400, nSamplesPerSec=44100, nChannels=2 nBlockAlign=4!
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #121 from Alexander E. Patrakov patrakov@gmail.com 2011-01-03 02:06:06 CST --- Raymond, just to clarify:
1) are you saying that in the case when no resampling is actually needed the patch introduced something bad?
2) which application are you using to test?
3) what does the problem sound like? clicks, underruns, wrong pitch?
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #122 from Alexander E. Patrakov patrakov@gmail.com 2011-01-03 02:09:13 CST --- Also, are you testing the patch as attached to this bug (it is indeed bad, even for ALSA), or with corrections from comment 115 or comment 118?
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #123 from Krzysztof Nikiel zzdz2@yahoo.pl 2011-01-03 02:57:19 CST --- (In reply to comment #117)
Also I would like to see a separate patch that adds the quality slider to winecfg.
Yeah, me too.
I have just written a simple script to set quality:
#!/bin/sh
QUAL="$1"
if test -z "$QUAL"; then QUAL="2"; fi
REGEDIT="wine regedit" REGKEY='REGEDIT4
[HKEY_CURRENT_USER\software\wine\directsound] "ResampleQuality"="%d" '
echo setting resample quality $QUAL printf "$REGKEY" $QUAL | $REGEDIT -
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #124 from Raymond superquad.vortex2@gmail.com 2011-01-03 05:35:19 CST --- (In reply to comment #122)
Also, are you testing the patch as attached to this bug (it is indeed bad, even for ALSA), or with corrections from comment 115 or comment 118?
using wine-git-11.diff , the audio by foobar2000 is playing at a speed much faster than normal without any error messages
foobar200 play 44100Hz stereo 16bits wav - foo_out_ds.dll through wineoss to HDA-Intel /dev/dsp1 , througn winealsa front:1 is normal
without the patch through wineoss is also normal
cat /proc/asound/Intel/pcm0p/sub0/hw_params access: MMAP_INTERLEAVED format: S16_LE subformat: STD channels: 2 rate: 44100 (44100/1) period_size: 512 buffer_size: 16384 OSS format: S16_LE OSS channels: 2 OSS rate: 44100 OSS period bytes: 2048 OSS periods: 32 OSS period frames: 512
http://bugs.winehq.org/show_bug.cgi?id=14717
Marton Balint cus@fazekas.hu changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |cus@fazekas.hu
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #125 from Raymond superquad.vortex2@gmail.com 2011-01-17 02:44:26 CST --- (In reply to comment #115)
diff --git a/dlls/dsound/buffer.c b/dlls/dsound/buffer.c index 3148502..10e68c8 100644 --- a/dlls/dsound/buffer.c +++ b/dlls/dsound/buffer.c @@ -500,6 +500,7 @@ static HRESULT WINAPI IDirectSoundBufferImpl_Lock( ) { HRESULT hres = DS_OK; IDirectSoundBufferImpl *This = (IDirectSoundBufferImpl *)iface;
- INT firspace;
Why winealsa did not specify DSDDESC_DONTNEEDPRIMARYLOCK when using "hw" device since it already exclusively open "hw:0,0" just as same as wineoss ?
if ( snd_pcm_type(pcm) == SND_PCM_TYPE_HW ) WOutDev[This->drv->wDevID].ds_desc.dwflags |= DSDDESC_DONTNEEDPRIMARYLOCK ;
http://bugs.winehq.org/show_bug.cgi?id=14717
Jerome Leclanche adys.wh@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |adys.wh@gmail.com
--- Comment #126 from Jerome Leclanche adys.wh@gmail.com 2011-02-05 00:05:22 CST --- +patch A patch was sent, but was not committed.
http://www.winehq.org/pipermail/wine-patches/2010-November/096302.html
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #127 from Alexander E. Patrakov patrakov@gmail.com 2011-02-07 04:18:08 CST --- One problem with this patch is that it is one big patch, essentially a full rewrite. One cannot do a bisection inside this patch and find out why it broke OSS for Raymond. As soon as my new laptop arrives (will take 2-3 days), I will work on a replacement _series_ of patches that is a bit more conservative.
http://bugs.winehq.org/show_bug.cgi?id=14717
Jeff Zaroyko jeffz@jeffz.name changed:
What |Removed |Added ---------------------------------------------------------------------------- Keywords| |patch
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #128 from Krzysztof Nikiel zzdz2@yahoo.pl 2011-02-09 04:42:21 CST --- (In reply to comment #126)
+patch A patch was sent, but was not committed.
http://www.winehq.org/pipermail/wine-patches/2010-November/096302.html
It was rejected as "Needs splitting". I will take it eventually.
http://bugs.winehq.org/show_bug.cgi?id=14717
Krzysztof Nikiel zzdz2@yahoo.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #32697|0 |1 is obsolete| |
--- Comment #129 from Krzysztof Nikiel zzdz2@yahoo.pl 2011-02-09 06:36:01 CST --- Created an attachment (id=33202) --> (http://bugs.winehq.org/attachment.cgi?id=33202) high quality resampler
In the mean time some changes were commited to the git so the patch needed a bit of merging but it seems OK. It looks like it should be split ASAP so hopefully it gets accepted, otherwise I will need to merge it again.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #130 from Krzysztof Nikiel zzdz2@yahoo.pl 2011-02-09 08:48:10 CST --- Created an attachment (id=33205) --> (http://bugs.winehq.org/attachment.cgi?id=33205) high quality resampler (split patch)
Alexander, would you like to review the split version. It should be a little more readable than single file.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #131 from Jerome Leclanche adys.wh@gmail.com 2011-02-09 09:11:01 CST --- (In reply to comment #130)
Created an attachment (id=33205)
--> (http://bugs.winehq.org/attachment.cgi?id=33205) [details]
high quality resampler (split patch)
Alexander, would you like to review the split version. It should be a little more readable than single file.
Just an advice on formatting: new files should have 4-space indentation, like the majority of the wine codebase. Changes to already-existing files should follow the codestyle of the file if there is one.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #132 from Alexander E. Patrakov patrakov@gmail.com 2011-02-09 12:19:01 CST --- Unfortunately, due to a payment problem, I still have no personal laptop. And it is against the rules to build wine on a corporate laptop. So I can't meaningfully review the split patch now.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #133 from Raymond superquad.vortex2@gmail.com 2011-02-09 22:47:19 CST --- I still don't understand the reason of adding a resampler to dsound
if you want good quality, just use kernel streaming which bypass kernel mixer
do you mean your HDA does not support KS ?
http://wiki.hydrogenaudio.org/index.php?title=Foobar2000:Components/Kernel_S...)
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #134 from Alexander E. Patrakov patrakov@gmail.com 2011-02-10 02:13:26 CST --- We are not talking about adding a resampler to dsound, because a resampler already exists there in Wine code base. We are talking about replacing it with a resampler that provides the same quality as Windows resampler (at least for SB 128 PCI) for the case of mismatching primary and secondary buffer sample rates.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #135 from Raymond superquad.vortex2@gmail.com 2011-02-10 19:41:45 CST --- if you were talking about ens1371 (SB PCI 128) , the problem is related to the rate constraint
you cannot get exactly 11025 Hz, 22050 Hz and 44100Hz,
stream : PLAYBACK access : RW_INTERLEAVED format : S16_LE subformat : STD channels : 2 rate : 22050 exact rate : 22050.9 (722565000/32768) msbits : 16 buffer_size : 11025 period_size : 2756 period_time : 124988
static struct snd_ratden es1371_dac_clock = { .num_min = 3000 * (1 << 15), .num_max = 48000 * (1 << 15), .num_step = 3000, .den = 1 << 15, }; static struct snd_pcm_hw_constraint_ratdens snd_es1371_hw_constraints_dac_clock = { .nrats = 1, .rats = &es1371_dac_clock, }; static struct snd_ratnum es1371_adc_clock = { .num = 48000 << 15, .den_min = 32768, .den_max = 393216, .den_step = 1, }; static struct snd_pcm_hw_constraint_ratnums snd_es1371_hw_constraints_adc_clock = { .nrats = 1, .rats = &es1371_adc_clock, };
static struct snd_pcm_hardware snd_ensoniq_playback1 = { .info = (SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER | SNDRV_PCM_INFO_MMAP_VALID | SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_SYNC_START), .formats = SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S16_LE, .rates = #ifndef CHIP1370 SNDRV_PCM_RATE_CONTINUOUS | SNDRV_PCM_RATE_8000_48000, #else (SNDRV_PCM_RATE_KNOT | /* 5512Hz rate */ SNDRV_PCM_RATE_11025 | SNDRV_PCM_RATE_22050 | SNDRV_PCM_RATE_44100), #endif .rate_min = 4000, .rate_max = 48000, .channels_min = 1, .channels_max = 2, .buffer_bytes_max = (128*1024), .period_bytes_min = 64, .period_bytes_max = (128*1024), .periods_min = 1, .periods_max = 1024, .fifo_size = 0, };
static struct snd_pcm_hardware snd_ensoniq_playback2 = { .info = (SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER | SNDRV_PCM_INFO_MMAP_VALID | SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_SYNC_START), .formats = SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S16_LE, .rates = SNDRV_PCM_RATE_CONTINUOUS | SNDRV_PCM_RATE_8000_48000, .rate_min = 4000, .rate_max = 48000, .channels_min = 1, .channels_max = 2, .buffer_bytes_max = (128*1024), .period_bytes_min = 64, .period_bytes_max = (128*1024), .periods_min = 1, .periods_max = 1024, .fifo_size = 0, };
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #136 from Krzysztof Nikiel zzdz2@yahoo.pl 2011-02-11 03:09:17 CST --- (In reply to comment #133)
I still don't understand the reason of adding a resampler to dsound
Yes, you already demonstrated that you can't understand much. Anyway, I'm going to sent the patch to wine-patches.
(In reply to comment #132)
Unfortunately, due to a payment problem, I still have no personal laptop. And it is against the rules to build wine on a corporate laptop. So I can't meaningfully review the split patch now.
It's just the same patch only split into parts. Let's hope it will be accepted.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #137 from Raymond superquad.vortex2@gmail.com 2011-02-11 03:19:41 CST --- (In reply to comment #4)
The bug still exists and is easily triggered by playing music by Caravelli (album: grand prix) in foobar2000. Anyway, you can check for this bug yourself using the procedure below and any mp3 file containing music.
if you are using mp3 , the quality is already bad
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #138 from Alexander E. Patrakov patrakov@gmail.com 2011-02-12 11:08:59 CST --- Yes I know that mp3 is lossy. But the effects of a zero-order-hold resampler can be heard even on mp3s. In other words, the current bad resampler is much much worse than a properly resampled (e.g. by mplayer) mp3.
http://bugs.winehq.org/show_bug.cgi?id=14717
Nicky nheart@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |nheart@gmail.com
--- Comment #139 from Nicky nheart@gmail.com 2011-02-19 12:45:19 CST --- Hey, Krzysztof, great work on your patch! If you had done it couple of years earlier I would have never ditched foobar2000. mp3s (and FLACs) now sound just as played by mpd. Did you get any feedback from Alexandre as to why your patch isn't accepted? Bugzilla showed it as "Not even looked at"...
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #140 from Alexander E. Patrakov patrakov@gmail.com 2011-02-19 14:06:24 CST --- Indeed, I have not looked at the patch yet. But as I am writing this from my new personal laptop, there are very good chances that I will review the split patch tomorrow.
OTOH, since the description looks like "the same old patch but split and updated to apply cleanly to new Wine", I think that the "introduces regression for Raymond with OSS" objection is still valid.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #141 from Raymond superquad.vortex2@gmail.com 2011-02-20 02:49:21 CST --- (In reply to comment #134)
We are not talking about adding a resampler to dsound, because a resampler already exists there in Wine code base. We are talking about replacing it with a resampler that provides the same quality as Windows resampler (at least for SB 128 PCI) for the case of mismatching primary and secondary buffer sample rates.
Take a look at the ENS1371 Specification
if oss emulation work better than alsa
It look like ens1371 driver need to specify
snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES, 32); snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_BUFFER_BYTES, 32);
so that dsound are forced to select a period bytes which is multiple of 32 bytes since Eight Long words will always be transferred during Audio Write Transfers.
5.2. Audio Write Transfers The CCB will first write up to 8 long words into the intermediate PCI buffer. The CCB will then request a write transfer from the PCIB to main memory and specify the starting address of the transfer. The PCIB arbitrates for the PCI bus and transfers 8 Long Words into system memory. Eight Long words will always be transferred during this operation.
4.11. Internal Memory
The internal memory for the sound cache in AudioPCI 97 is organized as 4 blocks of 64 bytes each. Each block is divided into 4 pages of 16 bytes each (4 longwords). Memory can be accessed as longwords only.
The internal memory organization for the Sample Rate Converter in AudioPCI 97 is shown below. The memory is accessed through the Sample Rate Converter interface register located at address 10H.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #142 from Alexander E. Patrakov patrakov@gmail.com 2011-02-20 04:23:57 CST --- Raymond, your comment totally misses the point. SB 128 PCI was only an example. My point is: zero-order-hold resamplers must die, in all sound-related software, unconditionally. Wine has one (if you don't see it - ask via private email), and words "please configure wine and/or other software so that it isn't used" are not good enough.
If you still have something that you don't understand, please add me to your jabber contact list, and we'll discuss. My JID is the same as e-mail. Private e-mail is welcome, too.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #143 from Alexander E. Patrakov patrakov@gmail.com 2011-02-20 05:15:59 CST --- The split patch deserves exactly the same review as the non-split one. It is split mechanically, meaning that we can't figure out by bisection what broke OSS for Raymond. OTOH, it demonstrates that the new code is not complex at all and can be reviewed, well, as chunks of new code.
New review items that were found since the last attempt but also apply to the old patch:
1) I'd remove the $(RM) $< line from the Makefile. It causes mkfir to be recompiled during "make install".
2) The registry setting MaxShadowSize and the corresponding variables in dsound_main.c are still used. Thus, it seems that there are still two possible ways how the resampler code can be invoked: resampling in the mixer thread and resampling when an application supplies sound data. Are both tested by playing with the MaxShadowSize? Are you sure that having two ways still makes sense from the mathematical point of view given the possibility of rewinds of the secondary buffer?
IMHO, the default "resample not in mixer" approach worked before exactly because of the zero-order-hold resampler that has well-defined non-overlapping regions of the primary buffer affected by each secondary buffer sample.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #144 from Raymond superquad.vortex2@gmail.com 2011-02-20 21:36:44 CST --- without your patch, both au8830 and hda-intel work normal with foobar2000 using oss emulation and alsa
AFter apply your patch , using foobar2000 run about 12 times faster using oss emulation for both sound cards
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #145 from Alexander E. Patrakov patrakov@gmail.com 2011-02-21 06:40:07 CST --- OK, so let me reject the patch until further debugging.
My plans for this evening: write a simple test program for directsound to test the case of rewinding and overwriting the secondary buffer.
Task for Raymond: test all his sound cards with both alsa and oss without the patch, but with the HKCU/Software/Wine/DirectSound/MaxShadowSize set to "0". This is a prerequisite for my development of a replacement patch series, as it will remove this registry setting.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #146 from Krzysztof Nikiel zzdz2@yahoo.pl 2011-02-21 12:26:23 CST --- (In reply to comment #139) Hi,
Did you get any feedback from Alexandre as to why your patch isn't accepted? Bugzilla showed it as "Not even looked at"...
No, no reply from Alexandre but I got some replies from other wine-devel members. I looks like no one really knows how to split such patch the wine way but I'm slowly getting some idea how to split it so not all hope is lost yet.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #147 from Krzysztof Nikiel zzdz2@yahoo.pl 2011-02-21 13:03:16 CST --- (In reply to comment #144)
without your patch, both au8830 and hda-intel work normal with foobar2000 using oss emulation and alsa
AFter apply your patch , using foobar2000 run about 12 times faster using oss emulation for both sound cards
I know I shouldn't feed the troll but I just can't resist to reply this one.
What's the point of using wine with oss emulation? If you tested it with native oss it would make some sense. Have you used the latest version of the patch or picked some broken one?
And BTW, are you having some hearing disabilities?
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #148 from Raymond superquad.vortex2@gmail.com 2011-02-21 19:36:36 CST --- (In reply to comment #143)
IMHO, the default "resample not in mixer" approach worked before exactly because of the zero-order-hold resampler that has well-defined non-overlapping regions of the primary buffer affected by each secondary buffer sample.
I still don't understand two patches 0011 and 0012 , it look like you removed the mixer of those software secondary buffers
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #149 from Raymond superquad.vortex2@gmail.com 2011-02-21 20:22:23 CST --- (In reply to comment #142)
Raymond, your comment totally misses the point. SB 128 PCI was only an example. My point is: zero-order-hold resamplers must die, in all sound-related software, unconditionally. Wine has one (if you don't see it - ask via private email), and words "please configure wine and/or other software so that it isn't used" are not good enough.
I don't understand your point , since dsound is used for playing game and those sound effects can happen at any time, so dsound need to use small period size )I guess is 10ms ) and try to mix the new sound effect and play them out as soon as possible.
so not all sound cards can perform the same , some specific sound cards are perform much better than the other sound cards.
dsound is not designed for playing high quality music
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #150 from Raymond superquad.vortex2@gmail.com 2011-02-21 21:44:31 CST --- Just give another example of sound card which only support 48000Hz (e.g. sb0220 )
static struct snd_pcm_hardware snd_emu10k1x_playback_hw = { .info = (SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER | SNDRV_PCM_INFO_MMAP_VALID), .formats = SNDRV_PCM_FMTBIT_S16_LE, .rates = SNDRV_PCM_RATE_48000, .rate_min = 48000, .rate_max = 48000, .channels_min = 2, .channels_max = 2, .buffer_bytes_max = (32*1024), .period_bytes_min = 64, .period_bytes_max = (16*1024), .periods_min = 2, .periods_max = 8, .fifo_size = 0, };
if you look at winealsa.drv/dsoutput.c , you should notice that emu10k1x only can have 8 periods, but dsoutput.c assume all sound cards support 16 periods
snd_pcm_hw_params_set_periods_integer(pcm, hw_params); snd_pcm_hw_params_set_buffer_time_near(pcm, hw_params, &buffer_time, NULL); buffer_time = 10000; snd_pcm_hw_params_set_period_time_near(pcm, hw_params, &buffer_time, NULL);
err = snd_pcm_hw_params_get_period_size(hw_params, &psize, NULL); buffer_time = 16; snd_pcm_hw_params_set_periods_near(pcm, hw_params, &buffer_time, NULL);
if (!This->mmap) { HeapFree(GetProcessHeap(), 0, This->mmap_buffer); This->mmap_buffer = NULL; }
err = snd_pcm_hw_params_set_access (pcm, hw_params, SND_PCM_ACCESS_MMAP_INTERLEAVED); if (err >= 0) This->mmap = 1; else { This->mmap = 0; err = snd_pcm_hw_params_set_access (pcm, hw_params, SND_PCM_ACCESS_RW_INTERLEAVED); }
err = snd_pcm_hw_params(pcm, hw_params);
/* ALSA needs at least 3 buffers to work successfully */ This->mmap_commitahead = 3 * psize; while (This->mmap_commitahead <= 512) This->mmap_commitahead += psize;
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #151 from Alexander E. Patrakov patrakov@gmail.com 2011-02-22 03:39:56 CST --- DSound may be not designed for playing high-quality music, but even the highest-quality resampler in the patch easily fits the latency budget that you imply by saying "dsound has to use small period size (I guess is 10ms) and try to mix the new sound effect and play them out as soon as possible".
As for the acceptable tradeoffs in terms of CPU/latency/quality, I think they need to be discussed, otherwise someone will be able to say "mixing must be as fast as possible, so let's convert anything to 8 bits for performance reasons" (and that's in the same realm of strawman arguments as your suggestion to use a zero-order-hold resampler). My viewpoint is that wine either must provide the same resampling quality & latency as Windows does for DirectSound apps, or use the system's mixer/resampler and thus have the same characteristics as linux sound apps.
As for the submitted patch, I am working on new testcases that would prove or disprove the rewind-and-overwrite bug that may exist.
Also you still didn't answer my request for testing wine without the patch but with MaxShadowSize=0, and I don't see you in my jabber contact list.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #152 from Alexander E. Patrakov patrakov@gmail.com 2011-02-22 18:47:27 CST --- I wrote a simple directsound program that creates primary & secondary buffers at different rates and locks/unlocks each portion of the secondary buffer twice: once writing a portion of a sine wave there, and once writing silence. The end result is expected to be silence, and is actually silence. The patch doesn't break this test with both relevant values of MaxShadowSize, so my concerns about this were invalid.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #153 from Raymond superquad.vortex2@gmail.com 2011-02-22 19:54:19 CST --- (In reply to comment #151)
DSound may be not designed for playing high-quality music, but even the highest-quality resampler in the patch easily fits the latency budget that you imply by saying "dsound has to use small period size (I guess is 10ms) and try to mix the new sound effect and play them out as soon as possible".
As for the acceptable tradeoffs in terms of CPU/latency/quality, I think they need to be discussed, otherwise someone will be able to say "mixing must be as fast as possible, so let's convert anything to 8 bits for performance reasons" (and that's in the same realm of strawman arguments as your suggestion to use a zero-order-hold resampler). My viewpoint is that wine either must provide the same resampling quality & latency as Windows does for DirectSound apps, or use the system's mixer/resampler and thus have the same characteristics as linux sound apps.
The main problem is dsound did not correctly caclulated the number of period , period size and buffer size
You can see the above examples, some sound cards have 128K byte , 64K bytes or 32 Kbytes Buffer,
when winealsa.drv try to set a buffer time of 0.5 seconds for 44100 S16 stereo, it will get 371ms buffer for those sound cards with 64K Bytes buffer , but when you will get a different buffer times when the application select a different rate or U8 or mono
since oss use a power of two frag size, wineoss.drv is usually better than winealsa.drv when using emulation
bug#8668
if you search wine-developer mailing list archive, you will find many wine developer had said they will fix this issue
http://www.intel.com/support/motherboards/desktop/sb/cs-020642.htm#multistre...
you can also use dsound in foobar2000 to play audio to the front panel headphone if your hda-intel codec have the extra DAC for multistream playback
Take a look at dsound/primary.c
DWORD DSOUND_fraglen(DWORD nSamplesPerSec, DWORD nBlockAlign) { /* Given a timer delay of 10ms, the fragment size is approximately: * fraglen = (nSamplesPerSec * 10 / 1000) * nBlockAlign * ==> fraglen = (nSamplesPerSec / 100) * nBlockSize * * ALSA uses buffers that are powers of 2. Because of this, fraglen * is rounded up to the nearest power of 2: */
if (nSamplesPerSec <= 12800) return 128 * nBlockAlign;
if (nSamplesPerSec <= 25600) return 256 * nBlockAlign;
if (nSamplesPerSec <= 51200) return 512 * nBlockAlign;
return 1024 * nBlockAlign; }
static void DSOUND_RecalcPrimary(DirectSoundDevice *device) { TRACE("(%p)\n", device);
device->helfrags = device->buflen / device->fraglen; TRACE("fraglen=%d helfrags=%d\n", device->fraglen, device->helfrags);
if (device->hwbuf && device->drvdesc.dwFlags & DSDDESC_DONTNEEDWRITELEAD) device->writelead = 0; else /* calculate the 10ms write lead */ device->writelead = (device->pwfx->nSamplesPerSec / 100) * device->pwfx->nBlockAlign; }
http://bugs.winehq.org/show_bug.cgi?id=14717
Jörg Höhle hoehle@users.sourceforge.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |16513
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #154 from Raymond superquad.vortex2@gmail.com 2011-02-28 07:22:20 CST --- (In reply to comment #151) .
As for the acceptable tradeoffs in terms of CPU/latency/quality, I think they need to be discussed,
According top , CPU usage with wine-1.3.14 when using foobar2000 is about 4% , and after apply your patch foobar2000 use more than 9% even when playing 44100Hz S16 stereo
Look at the patch 0001, even if the sound card support the sample rate, your patch add 44100 x ( signed integer in to double , double to signed integer ) + a lot of overhead per second
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #155 from Alexander E. Patrakov patrakov@gmail.com 2011-02-28 07:28:24 CST --- Valid remark. I'd also prefer the math to be converted to fixed-point.
And although I could not write a test that fails, I am still waiting for confirmation that the patch has been tested with apps and that the logic is correct both for resampleinmixer == true and resampleinmixer == false.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #156 from Raymond superquad.vortex2@gmail.com 2011-02-28 08:22:03 CST --- (In reply to comment #145)
OK, so let me reject the patch until further debugging.
Task for Raymond: test all his sound cards with both alsa and oss without the patch, but with the HKCU/Software/Wine/DirectSound/MaxShadowSize set to "0". This is a prerequisite for my development of a replacement patch series, as it will remove this registry setting.
Your patch actually make wineoss (hardware acceleration) unusable ,
set Maxshadowsize=0 does not help too
however set resample quality to 0 or 1 seem improve that situation a little bit but it still not stable than the version since the quality become worse when using WINEDEBUG=+dsound
Both sound card support 11025, 22050 and 44100 Hz, and don't need any resampling when playing 44100Hz stereo s16
seem like resampler use too much CPU since wineoss driver use a period size lower than winealsa for the same sound card
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #157 from Raymond superquad.vortex2@gmail.com 2011-03-05 21:22:34 CST --- (In reply to comment #47)
(In reply to comment #46)
if winejack.drv can nly supply only jack server'sample rate
wwo->sample_rate = fp_jack_get_sample_rate(client);
but in waveoutcaps , it incorrectly state that it can support 16bits 11025Hz, 22050Hz and 44100Hz if the jack server is running at 48000Hz
The above statement is a different bug (if a bug at all, because wine resamples, and Windows via HEL also resamples sound for cards that can support only 48 kHz), please discuss this elsewhere. Please leave discussion in this bug limited to the fact that the existing resampler in wine is a zero-order hold and that there is a patch being developed.
This won't improve winejack.drv , since winejack does not support dsound nor WINE_DIRECTSOUND in winejack.drv/audio.c
dwFlags &= ~WAVE_DIRECTSOUND; /* direct sound not supported, ignore the flag
winejack.drv only support waveoutopen only the jack server rate, it seem has trick to shutdown jack server when request sample rate is not equal to jack server sample rate. (i.e. user must know the application's request sample rate and set the jack server to run at that rate )
adding a resampler and convertor inside winejack.drv is the way to improve winejack since the waveout is still bad even when you add a resampler inside dsound
Yes , I agree that the current resampler work best only when convert 11025Hz or 22050Hz to 44100Hz, (i.e. the sound card 's rate is a multiple of the requested sample rate ) and it work bad when the sound card's supported rate is not a multipler of the requested sample rate
does those sound cards which only support 48000Hz have same quality as those certified sound cards in windows ?
http://bugs.winehq.org/show_bug.cgi?id=14717
Jörg Höhle hoehle@users.sourceforge.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks|16513 |
http://bugs.winehq.org/show_bug.cgi?id=14717
Heikki Repo heikki.repo@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |NEW Ever Confirmed|0 |1
--- Comment #158 from Heikki Repo heikki.repo@gmail.com 2011-03-22 16:41:10 CDT --- *** This bug has been confirmed by popular vote. ***
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #159 from Raymond superquad.vortex2@gmail.com 2011-03-22 18:56:17 CDT --- (In reply to comment #158)
*** This bug has been confirmed by popular vote. ***
why ?
only a few sound card support SNDRV_PCM_RATE_CONTINUOUS but wine assign DSCAPS_CONTINUOUSRATE without testing whether the sound card really support CONTINUOUS RATE and disable resampling in winealsa.drv
*flags = DSCAPS_CERTIFIED | DSCAPS_CONTINUOUSRATE;
it did not affect winepulse since pulseaudio perform resampling
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #160 from Raymond superquad.vortex2@gmail.com 2011-03-22 19:00:35 CDT --- (In reply to comment #152)
I wrote a simple directsound program that creates primary & secondary buffers at different rates and locks/unlocks each portion of the secondary buffer twice: once writing a portion of a sine wave there, and once writing silence. The end result is expected to be silence, and is actually silence. The patch doesn't break this test with both relevant values of MaxShadowSize, so my concerns about this were invalid.
I have patched my alsa drivers so that to pretend it only support 48000Hz
using PPHS resampler inside foobar2000 and play 44100Hz with wine 1.3.15 only use 7% cpu which is still faster than using this resampler
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #161 from Raymond superquad.vortex2@gmail.com 2011-03-22 19:27:27 CDT --- (In reply to comment #90)
The reason for using an emulator is that it allows to capture the output digitally. A real sound card with S/PDIF output would also work if I had something that could record from S/PDIF.
This is because wine developer using openal as backend of mmdevapi , but openal does not support record from S/PDIF , it is just software version of ds3d which is used for 3D game
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #162 from Raymond superquad.vortex2@gmail.com 2011-03-22 21:08:20 CDT --- (In reply to comment #90)
(In reply to comment #87)
it is meaningless to test those emulated sound card sb16 and ac97 in kvm or virtualbox.
I would agree if the emulated sound card used the driver specifically written for it (i.e., something analogous to the VMware video driver). However, the emulated card pretends to be a real thing, identical to something manufactured in real-world silicon, and uses the drivers meant to be used with the real-silicon cards (i.e., the drivers that come with Windows itself). So these drivers would send identical output to the emulated and real-world cards and thus are suitable for reverse-engineering the resampler, provided that kvm itself does no resampling (that's why QEMU_AUDIO_DAC_FIXED_SETTINGS=0).
if you look at winealsa.drv/waveinit.c
wwo.outcaps.wMid = MM_CREATIVE; wwo.outcaps.wPid = MM_CREATIVE_SBP16_WAVEOUT; wwo.outcaps.vDriverVersion = 0x0100;
wwi.incaps.wMid = MM_CREATIVE; wwi.incaps.wPid = MM_CREATIVE_SBP16_WAVEOUT; wwi.incaps.vDriverVersion = 0x0100;
it is emulating SB 16 waveout and wavein for all alsa sound cards
http://bugs.winehq.org/show_bug.cgi?id=14717
Heikki Repo heikki.repo@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |heikki.repo@gmail.com
--- Comment #163 from Heikki Repo heikki.repo@gmail.com 2011-03-23 05:25:59 CDT --- Something which hasn't been taken into account in this discussion is that some games might have sound files which are in different sample rates. I wanted to test the patch included before making my contribution to this issue, but now that I have tested it, I really stand behind voting for this bug.
The issue becomes easy to see when playing games. Even when I'm running wine with default sample rate of 44100 there is some distortion in audio in games such as Dragon Age: Origins. How can this be? I hear no such distortion if I use for example Spotify --- which leads me to believe that there are different sample rates used within one game. Changing to 48000 made the distortion worse in DA:O.
I downloaded the patch in comment 129, applied it to wine-1.3.16. Now the distortion in DA:Origins is gone. The distortion wasn't especially bad, but noticeable for me -- and something that did bother me.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #164 from Raymond superquad.vortex2@gmail.com 2011-03-24 20:40:59 CDT --- (In reply to comment #163)
Something which hasn't been taken into account in this discussion is that some games might have sound files which are in different sample rates. I wanted to test the patch included before making my contribution to this issue, but now that I have tested it, I really stand behind voting for this bug.
The issue becomes easy to see when playing games. Even when I'm running wine with default sample rate of 44100 there is some distortion in audio in games such as Dragon Age: Origins. How can this be? I hear no such distortion if I use for example Spotify --- which leads me to believe that there are different sample rates used within one game. Changing to 48000 made the distortion worse in DA:O.
I downloaded the patch in comment 129, applied it to wine-1.3.16. Now the distortion in DA:Origins is gone. The distortion wasn't especially bad, but noticeable for me -- and something that did bother me.
Does your sound card support 44100Hz ?
or your "default" device use dmix at 48000Hz
since it is a bug in winecfg which allow user to select 44100Hz even 44100Hz is not supported by the "default" device
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #165 from Heikki Repo heikki.repo@gmail.com 2011-03-25 09:47:59 CDT --- (In reply to comment #164)
(In reply to comment #163)
Something which hasn't been taken into account in this discussion is that some games might have sound files which are in different sample rates. I wanted to test the patch included before making my contribution to this issue, but now that I have tested it, I really stand behind voting for this bug.
The issue becomes easy to see when playing games. Even when I'm running wine with default sample rate of 44100 there is some distortion in audio in games such as Dragon Age: Origins. How can this be? I hear no such distortion if I use for example Spotify --- which leads me to believe that there are different sample rates used within one game. Changing to 48000 made the distortion worse in DA:O.
I downloaded the patch in comment 129, applied it to wine-1.3.16. Now the distortion in DA:Origins is gone. The distortion wasn't especially bad, but noticeable for me -- and something that did bother me.
Does your sound card support 44100Hz ?
or your "default" device use dmix at 48000Hz
since it is a bug in winecfg which allow user to select 44100Hz even 44100Hz is not supported by the "default" device
Yes, my sound card does support 44100 Hz. Also, if it didn't, Spotify would sound distorted in the same way when using 44100 Hz as the output.
http://bugs.winehq.org/show_bug.cgi?id=14717
GyB gyebro69@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |gyebro69@gmail.com
http://bugs.winehq.org/show_bug.cgi?id=14717
Antonio lorefice2@libero.it changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |lorefice2@libero.it
--- Comment #166 from Antonio lorefice2@libero.it 2011-05-02 03:43:48 CDT --- *** Bug 26984 has been marked as a duplicate of this bug. ***
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #167 from Antonio lorefice2@libero.it 2011-05-02 03:45:24 CDT --- (In reply to comment #166)
*** Bug 26984 has been marked as a duplicate of this bug. ***
Attached to the 'duplicated' bug, there is an audio sample which clearly exposed the problem the the human ear. It also show how a "bad" mp3 can become even worse, please listen from the beginning to the end.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #168 from Antonio lorefice2@libero.it 2011-05-02 05:37:10 CDT --- Audio resampling is a cpu intensive task, so i think the user has to choice the audio quality he wants. Windows has that option in controlpanel -> sounds -> audio -> advanced.
Under GNU we have libsamplerate in different flavours (samplerate_order,samplerate_linear,samplerate,samplerate_medium,samplerate_best) which leads to different cpu loads and different quality. I found that anything over samplerate_order is better than default wine resampler, but to get acceptable quality, at least "samplerate" has to be used. It has an overhead of 2+2% on an E7500@2.93GHz when upsampling from 44khz to 48lhz.
http://bugs.winehq.org/show_bug.cgi?id=14717
Alex Bradbury asb@asbradbury.org changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |asb@asbradbury.org
http://bugs.winehq.org/show_bug.cgi?id=14717
sacrediou sacrediou@yahoo.fr changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |sacrediou@yahoo.fr
http://bugs.winehq.org/show_bug.cgi?id=14717
Alexey Loukianov mooroon2@mail.ru changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |mooroon2@mail.ru
--- Comment #169 from Alexey Loukianov mooroon2@mail.ru 2011-10-09 05:00:52 CDT --- Just wondering: Alexander, any news concerning the future of this patch with regards to the recent Wine's sound subsystem rewrite (move to the mmdevapi) by Andrew Eikum?
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #170 from Alexander E. Patrakov patrakov@gmail.com 2011-10-09 05:36:43 CDT --- No news, I was totally swamped by work.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #171 from Raymond superquad.vortex2@gmail.com 2011-10-09 05:54:09 CDT --- If AudioClient_GetMixFormat() return a format (float) which does not support by dmix which use S32_LE or S16_LE , it need to use more CPU power when playing S16_LE
aplay -Ddmix:Intel any.wav Playing WAVE 'any.wav' : Signed 16 bit Little Endian, Rate 48000 Hz, Stereo aplay: set_params:1073: Sample format non available Available formats: - S32_LE
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #172 from Alexander E. Patrakov patrakov@gmail.com 2011-10-09 06:16:40 CDT --- Raymond, your remark (comment 171) is a valid but different bug.
http://bugs.winehq.org/show_bug.cgi?id=14717
Andrew Eikum aeikum@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |28748
http://bugs.winehq.org/show_bug.cgi?id=14717
Andrew Eikum aeikum@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |aeikum@codeweavers.com Blocks|28748 |
--- Comment #173 from Andrew Eikum aeikum@codeweavers.com 2011-10-18 14:09:57 CDT --- I see Alexey and Alexander are still active on this bug. Is Krzysztof still around? I would really like to see this work in Wine.
I have taken the patchset from Comment 130 and am currently working on rebasing it after the dsound changes from 1.3.30. Then I'll work on reviewing it and seeing what we can do to get it into Wine as soon as possible. There are problems where the resampler isn't being used when it should, causing audio silence or repeating (Bug 28748 is one example). Having a useful resampler is a good starting point for fixing these bugs.
I'll update after I understand this patchset more.
http://bugs.winehq.org/show_bug.cgi?id=14717
Andrew Eikum aeikum@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |28748
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #174 from Raymond superquad.vortex2@gmail.com 2011-10-18 19:34:11 CDT --- (In reply to comment #173)
I see Alexey and Alexander are still active on this bug. Is Krzysztof still around? I would really like to see this work in Wine.
since support of winejack has been removed , is there any need to add resampler
After the patch which use "plughw" instead of "hw" and winealsa.drv/mmdevdrv.c does not disable alsa's resampler too
Anyone who want to use jack server need to use alsa-jack plugin and overwrite
pcm.!default by plug:jack
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #175 from Alexey Loukianov mooroon2@mail.ru 2011-10-18 19:38:51 CDT --- Side and unrelated note: I wouldn't recommend taking into account comments from people that can't understand what they are talking about.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #176 from Alexander E. Patrakov patrakov@gmail.com 2011-10-18 23:52:02 CDT --- To Andrew Eikum:
Here Raymond is right, even if he doesn't know what he is talking about. I recommend not basing your work on the patch in this bug or on my not-yet-published patchset, because it is based on the same idea of adding a resampler to wine.
Indeed, no resampler is needed in wine, and no software mixing layer, either. This is the job of the underlying platform such as ALSA. Any desktop audio platform that doesn't provide these capabilities is nowadays considered broken, or can be augmented with pulseaudio or ESD, and thus wine should not cater for it. Yes, this will kill direct output to "hw:X" ALSA devices.
Just always pretend that you have a sound card that has unlimited sample rate range, 8 channels and unlimited hardware voices, as this is an accurate (from the Windows app standpoint) description of "plug:dmix", "plug:jack" or pulseaudio.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #177 from Andrew Eikum aeikum@codeweavers.com 2011-10-19 08:40:01 CDT --- (In reply to comment #176)
Here Raymond is right, even if he doesn't know what he is talking about. I recommend not basing your work on the patch in this bug or on my not-yet-published patchset, because it is based on the same idea of adding a resampler to wine.
Indeed, no resampler is needed in wine, and no software mixing layer, either. This is the job of the underlying platform such as ALSA. Any desktop audio platform that doesn't provide these capabilities is nowadays considered broken, or can be augmented with pulseaudio or ESD, and thus wine should not cater for it. Yes, this will kill direct output to "hw:X" ALSA devices.
I'm unconvinced. What you're describing here is activating an IAudioClient for every secondary buffer (or primary buffer in primarywrite mode), and letting the driver backends take care of mixing the AudioClients' buffers together. I don't think that would work.
Some applications use a lot of secondary buffers to store their sound effects, and call Play() on each of those to play the effects. For these applications, you'd be creating a whole bunch of IAudioClients. In ALSA+PA and OSS, this effectively creates a ton of virtual devices, each with its own set of controls in the mixer panel. FreeBSD's standard OSS settings limit each device to 16 virtual devices, which is actually quite few. Creating devices only as they are Play()ed is impractical due to latency problems.
There's also the issue of 3D sound support, which isn't available through MMDevAPI, and would require its own mixing anyway, in some form. This is a smaller concern since we don't support 3D audio now anyway.
On top of this, dsound is really fragile and I hesitate to make any large changes like that to it, if isn't not clearly the correct thing to do.
As much as I dislike it as well, I'm not sure we can avoid having a mixer in dsound.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #178 from Alexey Loukianov mooroon2@mail.ru 2011-10-19 09:04:35 CDT --- (In reply to comment #176)
Indeed, no resampler is needed in wine, and no software mixing layer, either. This is the job of the underlying platform such as ALSA. Any desktop audio platform that doesn't provide these capabilities is nowadays considered broken, or can be augmented with pulseaudio or ESD, and thus wine should not cater for it. Yes, this will kill direct output to "hw:X" ALSA devices.
Em, correct me if I'm wrong, but the last time I had been looking into Wine sources there were function called something like DSOUND_MixToTemporary which contained zero-order hold resampler implemented as a part of it. If I got your idea right you propose to get rid of it altogether and simply rely of the mixing done upstream at the ALSA plugins level (or ever further upstream at pulse). I'm not aware about capabilities of the older version of OSS (v3) but what if it is not capable of resampling stream to a desired frequency (and to do the mixing of multiple streams)? What about OSS v4 and Core Audio, do they provide the required functionality?
Legacy support also is a concern here. PulseAudio in unavailable or is in a very buggy state on older long-term-support distros like RHEL5. Implementing Wine in a way that those systems would be left behind might be an issue.
Another thing that might be con for having resampler in Wine is that it would allow to more closely mimic Windows behavior, including latency concerns, buffer roll-backs, advertised buffer draw rate which is many cases is happening in chunks holding about 10ms of audio data, e.t.c.
Actually the question about should there be resampler in wine or not is a matter of drawbacks. We need someone who is well-aware of current Wine sound-related codebase, who knows all the details about ALSA/OSSv3/OSSv4/CoreAudio capabilities and intimate details about how does this things work on different Windows versions to judge the maintenance burden and possible drawback if Wine would go either ways. Andrew seems to be just the right person to do it - he knows a lot, he had done a lot of work on Wine like successfully reimplementing winmm over mmdevapi, e.t.c., e.t.c.
In case someone would happen to ask me, personally I "feel" that having good resampler in wine might be a better idea than to relying on upstream subsystems. But I hadn't had any experience with low-level Windows sound programming (except for ASIO but that's totally another beast) for last five years so my "feelings" might be safely ignored.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #179 from Alexander E. Patrakov patrakov@gmail.com 2011-10-19 10:15:14 CDT --- Andrew: your objections to my approach are valid. I will update my version of the patch and post it here when time permits.
Alexey: you understood the proposed idea correctly. As for OSSv4 functionality, see Andrew's reply.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #180 from Andrew Eikum aeikum@codeweavers.com 2011-10-20 08:37:43 CDT --- Just a note: Plants vs Zombies (demo at http://www.fileplanet.com/199524/190000/fileinfo/Plants-vs-Zombies-Demo) uses a number of different sample rates for its sound effects, so makes a pretty good test case for resampling issues.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #181 from Andrew Eikum aeikum@codeweavers.com 2011-10-24 14:20:54 CDT --- (In reply to comment #179)
Andrew: your objections to my approach are valid. I will update my version of the patch and post it here when time permits.
Hi again, Alexander.
This bug is currently at the top of my TODO list, so I'm going to try to push some progress on it.
Right now, I'm starting to look at Krzysztof's patches from Comment 130. Is this the best place to start? Does that patchset put dsound into an acceptable state? If the code is good, I can certainly work on cleaning it up and getting it into a submitable state.
Or do you have some patch not attached to this bug that I should be concerned with instead? Do you expect to have this work done soon? Should I even be spending time on this?
I am going to move forward with the Comment 130 patchset until I hear back from you. I just want to make sure we don't duplicate effort here :) Thanks
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #182 from Alexander E. Patrakov patrakov@gmail.com 2011-10-24 23:48:57 CDT --- No, I don't have a patch that is ready. I have an old (but bisectable) patchset that just cleanly removes the resample-not-in-mixer case on my home laptop (writing this from work, will get there in about 8 hours). Will work on it as soon as I arrive home.
As for not duplicating work - I think it is better for both of us to duplicate work rather than waste time waiting for each other.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #183 from Andrew Eikum aeikum@codeweavers.com 2011-10-26 10:25:45 CDT --- Created attachment 37125 --> http://bugs.winehq.org/attachment.cgi?id=37125 [NEEDS SPLITTING] dsound: Use a better resampling and mixing engine
Here is Krzysztof's patch based on current wine-git. It still needs some cleanup work (tabs vs spaces, better variable names...) as well as splitting up. However, it fixes Bug 28748, as predicted. I even dropped it down to 8kHz and it worked wonderfully.
http://bugs.winehq.org/show_bug.cgi?id=14717
Andrew Eikum aeikum@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #37125|0 |1 is obsolete| |
--- Comment #184 from Andrew Eikum aeikum@codeweavers.com 2011-10-26 14:12:46 CDT --- Created attachment 37133 --> http://bugs.winehq.org/attachment.cgi?id=37133 dsound resample patchset
Here's a patchset which cleans up Krzysztof's work into submission quality. I can't think of any reasonable way to split the first patch up further, it seems pretty atomic as it is. I'll probably sleep on this patchset for a day or two, running it through a few more applications before submitting it.
I'm undecided about adding a dropdown to winecfg for selecting the dsound resampler quality. I don't really seen any reason to use anything other than Best except for on very low performance systems, and they can just set the registry entry. But maybe I'm wrong about how useful this option will be...
Alexander, do you have any thoughts on the patchset?
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #185 from Raymond superquad.vortex2@gmail.com 2011-10-26 19:06:25 CDT --- (In reply to comment #180)
Just a note: Plants vs Zombies (demo at http://www.fileplanet.com/199524/190000/fileinfo/Plants-vs-Zombies-Demo) uses a number of different sample rates for its sound effects, so makes a pretty good test case for resampling issues.
it seem all .wav are 44100Hz mono and the game request for DSBCAPS_CTRLFREQUENCY , the application can change the pitch of the sound during playback back at any time
it is unlike foobar2000 resampling
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #186 from Alexander E. Patrakov patrakov@gmail.com 2011-10-26 23:38:01 CDT --- I will not be able to comment until Sunday
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #187 from Andrew Eikum aeikum@codeweavers.com 2011-11-01 10:26:38 CDT --- Any thoughts? I'm planning to send this on Friday afternoon so it gets a full two weeks of development testing before being contained in a release.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #188 from GyB gyebro69@gmail.com 2011-11-01 11:16:32 CDT --- (In reply to comment #187)
Any thoughts? I'm planning to send this on Friday afternoon so it gets a full two weeks of development testing before being contained in a release.
Andrew, did you mean the patch in attachment 37133 ? I ask because the patch is missing some firtab.h dsound_main.c:66: error: firtab.h: No such file or directory make[1]: *** [depend] Error 1
I applied the 2 patches on wine-1.3.31-213-g996b451 and received the above errors during compilation.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #189 from Alexey Loukianov mooroon2@mail.ru 2011-11-01 11:46:07 CDT --- I can second that patch from attachment 37133 lacks firtab.h that was auto-generated in original patchset by Krzysztof. I don't have enough spare time ATM to test if using auto-generated file from older patchset would fix the build.
http://bugs.winehq.org/show_bug.cgi?id=14717
Andrew Eikum aeikum@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #37133|0 |1 is obsolete| |
--- Comment #190 from Andrew Eikum aeikum@codeweavers.com 2011-11-01 12:52:20 CDT --- Created attachment 37245 --> http://bugs.winehq.org/attachment.cgi?id=37245 Corrected dsound resample patchset
Oops, here it is. Sorry about that.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #191 from GyB gyebro69@gmail.com 2011-11-02 13:24:29 CDT --- I keep testing the patchset, here's what I found so far:
1. fixes at least bug #16513, bug #11628, bug #28748, bug #27749, bug #27603 and a yet to be reported audio issue in Bloodrayne 2.
2. introduces audio issues in a number of games, these issues are ranging from - looping audio (Dark Fall, Atlantis 3) - crackling sounds (Operation Flashpoint) - some sound effects are too loud, others are quiet (Max Payne 2) - voice-over cuts out (Dracula: Origin)
to - garbled, unrecognizable audio playback (Armored Fist 3, HoverAce) - performance degradation, severe audio stuttering (GTA: San Andreas, Star Wolves 3)
Fedora 15 x86 Alsa 1.0.24 Audio device: nVidia Corporation MCP61 High Definition Audio (rev a2) PA is not running
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #192 from Raymond superquad.vortex2@gmail.com 2011-11-02 20:31:11 CDT --- (In reply to comment #190)
Created attachment 37245 [details] Corrected dsound resample patchset
Oops, here it is. Sorry about that.
(In reply to comment #182)
No, I don't have a patch that is ready. I have an old (but bisectable) patchset that just cleanly removes the resample-not-in-mixer case on my home laptop (writing this from work, will get there in about 8 hours). Will work on it as soon as I arrive home.
As for not duplicating work - I think it is better for both of us to duplicate work rather than waste time waiting for each other.
Famitracker of bug#9358 stop working after applied your patch
"It appears the current sound settings aren't working, change settings and try again!" appear in status bar which is IDS_SOUND_FAIL
// Wait for signals from the player thread
if (WaitForSingleObject(m_hAliveCheck, 0) == WAIT_OBJECT_0) { // return immediately
if ((GetTickCount() - LastTime) > 1000) {
((CMainFrame*) GetMainWnd())->SetMessageText(AFX_IDS_IDLEMESSAGE);
}
LastTime = GetTickCount();
}
else {
// Timeout after 1 s
if ((GetTickCount() - LastTime) > 1000) {
// Display message
((CMainFrame*) GetMainWnd())->SetMessageText(IDS_SOUND_FAIL);
}
}
http://bugs.winehq.org/show_bug.cgi?id=14717
Andrew Eikum aeikum@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #37245|0 |1 is obsolete| |
--- Comment #193 from Andrew Eikum aeikum@codeweavers.com 2011-11-03 15:20:14 CDT --- Created attachment 37278 --> http://bugs.winehq.org/attachment.cgi?id=37278 dsound: Use a better resampling and mixing engine
(In reply to comment #191)
I keep testing the patchset, here's what I found so far:
Thanks a lot! This was really helpful. I've attached a patch which fixes a couple issues mentioned below...
- fixes at least bug #16513, bug #11628, bug #28748, bug #27749, bug #27603
and a yet to be reported audio issue in Bloodrayne 2.
Great!
- introduces audio issues in a number of games, these issues are ranging from
- looping audio (Dark Fall, Atlantis 3)
Wasn't able to get Dark Fall running, and couldn't find a demo for Atlantis 3.
- crackling sounds (Operation Flashpoint)
Tested Resistence, Cold War Crisis, and Dragon Rising and all seemed to work fine. I ran into a "crackling sound" bug with the Critical Mass demo on Steam, but that's actually an application bug that's made worse by this patch. I'm still investigating that.
- some sound effects are too loud, others are quiet (Max Payne 2)
This was caused by the volume patch. It seems to create more problems than it solves, so I'm ditching it for now.
- voice-over cuts out (Dracula: Origin)
Fixed in attached patch.
- garbled, unrecognizable audio playback (Armored Fist 3, HoverAce)
Armored Fist 3 is a buggy app. The bad audio is caused by the same problem as described in Bug 28945, Comment 4.
I couldn't get the HoverAce demo to run. It just loaded up some text viewer with half of an HTML document in it.
- performance degradation, severe audio stuttering (GTA: San Andreas, Star
Wolves 3)
Couldn't find a GTA:SA demo, and SW3 demo died with StarForce DRM problems.
(In reply to comment #192)
Famitracker of bug#9358 stop working after applied your patch
Thanks for reporting. I think this is also fixed by the attached patch.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #194 from Raymond superquad.vortex2@gmail.com 2011-11-03 18:47:18 CDT --- (In reply to comment #193)
(In reply to comment #192)
Famitracker of bug#9358 stop working after applied your patch
Thanks for reporting. I think this is also fixed by the attached patch.
No, it still fail, famitracker is a software synth which using synth (e.g. SB Live 's wavetable synth) or dsound from 5 type of signal (Square1, Square2, Triangle, Noise and DPCM)
There is another bug is playing 400Hz Sine in dsound_test when the primary buffer is 32bits
WINETEST_INTERACTIVE=1 ./wine dlls/dsound/tests/dsound_test.exe.so dsound8
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #195 from GyB gyebro69@gmail.com 2011-11-03 22:36:57 CDT ---
- introduces audio issues in a number of games, these issues are ranging from
- looping audio (Dark Fall, Atlantis 3)
Wasn't able to get Dark Fall running, and couldn't find a demo for Atlantis 3.
Here's a demo for Dark Fall, you have to run it with either 1.3.31 or with the latest git because there was a regression in between (bug #28912). http://www.fileplanet.com/126673/120000/fileinfo/Dark-Fall-Demo
You can also try the demo for Haegemonia (music is looping): http://www.fileplanet.com/113670/110000/fileinfo/Hegemonia:-Legions-of-Iron-...
I'm going to give a try to the new patch as soon as I get home.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #196 from Alexander E. Patrakov patrakov@gmail.com 2011-11-04 06:42:29 CDT --- The patch seems to drop support for floating-point secondary buffers.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #197 from Alexander E. Patrakov patrakov@gmail.com 2011-11-04 07:34:04 CDT --- The FIRs in the patch use significantly less points than those from libsamplerate. Since values in between the stored FIR points are obtained via linear interpolation (both in the proposed patch and in libsamplerate), this means that the patch adds more noise to the resampled signal and does not achieve the stated noise figures.
For example, let's consider the best FIR. It has the following four samples near the top (I am just quoting the beginning of the table in the patch):
0.0151608889127230161, 0.015155099431551319, 0.0151377390515672269, 0.0151088319498221702,
Let's suppose that the resampler needs to access the non-existing point in the middle between the 2nd and the 3rd sample. Since it uses linear interpolation, it would get (0.015155099431551319 + 0.0151377390515672269) / 2 = 0.015146419241559274. I am too lazy to rebuild the FIR with more points, so I will just use cubic interpolation to estimate a more precise value. So here we go, according to http://paulbourke.net/miscellaneous/interpolation/ I need to set mu = 0.5 in their CubicInterpolate() function. Result: 0.015149308944130944.
So, two different interpolation methods give values that differ by 0.02%. So this is the estimation of the error given by the less precise one (linear interpolation). Such errors propagate to the audio signal, as noise. 0.02 % = -74 dB of noise. This is obviously not balanced with the -120 dB of stopband attenuation provided by the same FIR, and just not good enough for working even with 16-bit audio signals. The "fastest" FIR from libsamplerate has, according to the same type of calculations, -89 dB of noise.
This is even worse for the "fast" fir in the patch: with linear interpolation, it has -54 dB of noise (and -65 dB of stopband attenuation).
So either use more points (this doesn't increase compulational complexity, BTW), or use cubic interpolation near lines 271-275 of mixer.c.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #198 from Alexander E. Patrakov patrakov@gmail.com 2011-11-04 09:11:09 CDT --- I took the stopband attenuation figures from the attached plots, and this was my mistake. You can use the following C code for checking the FIRs after generation:
/* Just random enough not no hit zeroes in the transfer function by accident */ static const double testfreqs[5] = {0.55, 1.08, 1.31, 1.38, 1.41};
static void checkfir (fir_t* fir) { int i, j; for (j = 0; j < 5; j++) { double dct_coeff = -0.5 * fir->wing[0]; double testfreq = testfreqs[j] / fir->step; for (i = 0; i < fir->size; i++) dct_coeff += fir->wing[i] * cos(M_PI * i * testfreq);
dct_coeff *= 2; /* two wings */ fprintf(stderr, "attenuation at %1.2f Fn: %1.1f dB\n", testfreqs[j], 20 * log10(fabs(dct_coeff))); } double interp1 = 0.5 * (fir->wing[1] + fir->wing[2]); double interp3 = 0.625 * (fir->wing[1] + fir->wing[2]) - 0.125 * (fir->wing[0] + fir->wing[3]);
double noise = (interp1 - interp3) / interp3; fprintf(stderr, "noise: %1.1f dB\n", 20 * log10(fabs(noise))); }
Still, my point stands: the noise and stopband rejection figures are not well-balanced. You can improve the noise figure by increasing the "step" member of the firparams structure.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #199 from Alexander E. Patrakov patrakov@gmail.com 2011-11-04 10:38:23 CDT --- At least the fast and good FIRs do not contain a whole number of sine periods. This negates the effectiveness of the window.
http://bugs.winehq.org/show_bug.cgi?id=14717
Andrew Eikum aeikum@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #37278|0 |1 is obsolete| |
--- Comment #200 from Andrew Eikum aeikum@codeweavers.com 2011-11-04 11:09:19 CDT --- Created attachment 37298 --> http://bugs.winehq.org/attachment.cgi?id=37298 dsound: Use a better resampling and mixing engine
(In reply to comment #195)
Here's a demo for Dark Fall, you have to run it with either 1.3.31 or with the latest git because there was a regression in between (bug #28912). http://www.fileplanet.com/126673/120000/fileinfo/Dark-Fall-Demo
You can also try the demo for Haegemonia (music is looping): http://www.fileplanet.com/113670/110000/fileinfo/Hegemonia:-Legions-of-Iron-...
These are both working for me with the latest version of the patch, attached here. Changes detailed below.
(In reply to comment #194)
No, it still fail
I don't know what to say, it's working great here with the file named "7_opus_number_1.ftm". Do you have a clean WINEPREFIX? Want to attach a log with this latest patch (http://wiki.winehq.org/Sound)?
(In reply to comment #196)
The patch seems to drop support for floating-point secondary buffers.
Added in this version.
(In reply to comment #197)
So either use more points (this doesn't increase compulational complexity, BTW), or use cubic interpolation near lines 271-275 of mixer.c.
I changed the step values in mkfir to match those in libsamplerate and the header file turned out to be 1.1MB in size. That's not going to be acceptable, so I implemented cubic interpolation instead.
(In reply to comment #199)
At least the fast and good FIRs do not contain a whole number of sine periods. This negates the effectiveness of the window.
They're taken unchanged from the output of mkfir, so it's a bug in Krzysztof's program. I don't really understand the program or its output, but do you think line 128 should be changed from: tmp2 = M_PI * cnt * cutoff; to: tmp2 = 2 * M_PI * cnt * cutoff; to complete the sine period?
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #201 from Andrew Eikum aeikum@codeweavers.com 2011-11-04 11:18:02 CDT --- Created attachment 37299 --> http://bugs.winehq.org/attachment.cgi?id=37299 firtab.h with 2*M_PI
Here's a firtab.h generated with the 2*M_PI change. It seems to work, but does it look better to your trained eye?
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #202 from Alexander E. Patrakov patrakov@gmail.com 2011-11-04 12:05:51 CDT --- Yes, this is a bug in the original program, and PI -> 2 * PI won't solve it. The invariant that should hold is that sin(M_PI * x * cutoff) == 0.
Here x = g_fir->size if one uses linear interpolation and correctly interpolates one point beyond the end (you don't), or g_fir->size - 1 if one uses the variant of linear interpolation as in your old patch that does not invent an extra point (in this case, the argument to Kaiser function should be modified as well). In other words, at the end of the FIR (including the interpolated area) the sine should be zero and the argument of the Kaiser function (Izero) should be zero. But this is impossible if one specifies the cutoff frequency precisely, as this would require a FIR with a fractional length.
With e.g. Gaussian window (but not Kaiser, because one cannot evaluate it one point beyond its intended domain due to sqrt()) and a slightly different implementation of the cubic interpolation (i.e. just refuse to interpolate in the last segment instead of inventing a point like you do in line 221) one would get x = g_fir->size - 2 and one extra point at the end of the FIR table that guarantees the correct derivatives. With Kaiser window, I'd use cubic interpolation everywhere except the last segment, where linear interpolation would go. But honestly, I don't see much point in this Kaiser window. A Gaussian window would work about just as well. BTW I also tried to reverse-engineer a FIR used by Windows XP, and found that Kaiser and Gaussian windows fit it equally well (not exactly, but much better than raised-cosine family of windows) because at this length these two types of windows are very similar.
And BTW your handling of boundary conditions is wrong for the case near firpos == 0. You invent extra points by linear continuation, instead of using the symmetry. I think it would be better to store both wings of the FIR explicitly (concept: fir[-x] == fir[x]) instead of inserting ifs.
I solved the boundary condition problem locally (and yes, I am working on a different implementation and intentionally duplicating your work so that we can compare the results and spot missing cleanups, etc) with the following code:
fprintf(stderr, "creating %s filter...\n", firparams[idx].name); fflush(stderr);
g_fir->step = firparams[idx].step; g_fir->size = firparams[idx].width0 * g_fir->step / firparams[idx].passband + 1; cutoff = firparams[idx].width0 / (g_fir->size - 1); ALLOC(g_fir->wing, g_fir->size);
but this is obviously valid only for linear interpolation.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #203 from Alexander E. Patrakov patrakov@gmail.com 2011-11-04 12:22:45 CDT --- (In reply to comment #200)
I changed the step values in mkfir to match those in libsamplerate and the header file turned out to be 1.1MB in size. That's not going to be acceptable, so I implemented cubic interpolation instead.
I think that it is wrong, as the other parameters are different between these two filters. Our best filter does not have the same stopband attenuation as the best filter from libsamplerate (in fact it approximaely corresponds to their fast filter, only 72 KB in size).
I have no preference WRT cubic vs linear interpolation if it is done in such a way that noise is about the same as attenuation at ~1.4 Nyquist frequency. You can get both figures from the checkfir() function from comment #198. This function should be a useful test if you modify the window. E.g. it also catches the error that you made when trying to substitute PI -> 2 * PI, because this just moves the boundary between passband and stopband (i.e. is equivalent to changing the cutoff parameter).
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #204 from Alexander E. Patrakov patrakov@gmail.com 2011-11-04 12:25:53 CDT --- Oops. The checkfir() function obviously gives only a valid noise figure for the case of linear interpolation. To get a valid figure for cubic interpolation, you'll need to compare with a more precise (5th order interpolation or exact) method, and I am not sure that comparison in the center of the interval is a representative test in this case.
http://bugs.winehq.org/show_bug.cgi?id=14717
Andrew Eikum aeikum@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #37298|0 |1 is obsolete| | Attachment #37299|0 |1 is obsolete| |
--- Comment #205 from Andrew Eikum aeikum@codeweavers.com 2011-11-04 15:34:43 CDT --- Created attachment 37301 --> http://bugs.winehq.org/attachment.cgi?id=37301 dsound: Use a better resampling and mixing engine
One more version. This adds the writelead back in the secondary buffer GetCurrentPosition. This fixes the underruns in Critical Mass. I'm not aware of any issues introduced by this patch with any of the games I've been testing with.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #206 from Alexander E. Patrakov patrakov@gmail.com 2011-11-05 01:19:32 CDT --- Sorry, yesterday I referenced a cubic interpolation formula that is not good for our task. Namely, it produces a cubic curve that passes through all four given points. It is (when compared to the exact value) actually as noisy as the linear interpolation, just changes the sign of the error.
A different formula gives a cubic curve that passes through the middle two given points and also gives correct derivatives here, but doesn't pass through the two outer points: http://www.paulinternet.nl/?page=bicubic (just below the words "So our cubic interpolation formula becomes"). From the noise viewpoint, it is much better.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #207 from Alexander E. Patrakov patrakov@gmail.com 2011-11-05 05:26:05 CDT --- Created attachment 37313 --> http://bugs.winehq.org/attachment.cgi?id=37313 Incomplete patchset
This is an incomplete, but bisectable patchset that I developed independently. It does NOT contain a good resampler - the resampler is unchanged from the zero-order-hold that is in wine. Please, if the ready patch from Andrew Eikum introduces a regression not related to CPU utilization, retest your regression against my patchset.
And I must say that the patch from Andrew is almost perfect. One only has to fix the FIR generation and review the interpolation (I think it contains an off-by-one error, but not sure). I will explain the required math in a private e-mail.
http://bugs.winehq.org/show_bug.cgi?id=14717
Alexander E. Patrakov patrakov@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #37313|0 |1 is obsolete| |
--- Comment #208 from Alexander E. Patrakov patrakov@gmail.com 2011-11-05 09:08:40 CDT --- Created attachment 37320 --> http://bugs.winehq.org/attachment.cgi?id=37320 A split patchset with an alternative resampler implementation
This time, I attach a patchset with a seemingly working (for me) resampler, with the FIR modelled after the waveform produced by Windows XP at the default (best) quality setting. It is definitely better than the zero-order-hold, but sounds a bit different than mplayer. Maybe my code contains a bug.
This resampler is different from the one produced by Krzysztof Nikiel and Andrew Eikum. The main point is that it is bisectable and non-intrusive (i.e. not a complete rewrite), but the old implementation obviously has received more work and might still be better suited for inclusion. Unlike the old implementation, the new one is fixed-point.
OTOH, the old resampler is faster even though it uses a longer FIR. This is mainly due to the use of cubic interpolation (instead of linear) in the new resampler and repeated calculation of the same interpolating points for different channels. TODO if someone goes to submit my version: fix this.
Ideally, we should review each other's code and possibly make a combined version.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #209 from GyB gyebro69@gmail.com 2011-11-05 17:49:59 CDT --- I tried again the resampler and mixer patch in attachment 37301 . Certain problems still exist with that one:
1. vehicle engine sounds are broken in several games. I hear the same loud, grinding sound (instead of the appropriate engine sound) in these games: Operation Flashpoint Mafia 1 Railroad Tycoon 3 18 Wheels of Steel: American Longhaul
Mafia demo: http://www.fileplanet.com/116422/110000/fileinfo/Mafia-Demo
2. audio is stuttering and game performance drops dramatically: GTA San Andreas Star Wolves 2 and 3 Droplitz Lumines (demo available on Steam)
3. Riven (The sequel to Myst) is missing audio. No demo available.
Some other, minor issues: - there is some scratching noise in Max Payne 1; always reproducible when you're reloading your weapon. - clicking on an icon or game option produces *two* audible clicks in quick succession (Galcon Fusion, Family Farm). Galcon Fusion demo: http://www.galcon.com/fusion/files/GalconFusion_Setup.exe - some scratching noise in Star Wars Battlefront II. Seems to happen randomly(?) but quite often.
I also tried the split patchset, provided by Alexander E. Patrakov (attachment 37320): none of the above mentioned problems exist in these games with that patchset. Alexander's patchset fixes almost all of the bugs I referred to in comment #191 , except for bug #27749 and bug #27603 : only loud, static noise is coming from the speakers with Alexander's patchset in Still Life 2.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #210 from Alexander E. Patrakov patrakov@gmail.com 2011-11-06 01:30:06 CST --- GyB: it looks like you have something bisectable with Still Life 2. Namely, without my patch but with winetricks dsoundbug9612 you have recognizeable but garbled sound, while with my patchset, you have just loud noise. Could you please figure out which of my patches makes the difference?
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #211 from GyB gyebro69@gmail.com 2011-11-06 01:56:10 CST --- (In reply to comment #210)
GyB: it looks like you have something bisectable with Still Life 2. Namely, without my patch but with winetricks dsoundbug9612 you have recognizeable but garbled sound, while with my patchset, you have just loud noise. Could you please figure out which of my patches makes the difference?
Hi Alexander
I think you misunderstood something in the description of bug #27749: I have no audio in Still Life 2 with dsoundbug9612 (just like without setting MaxShadowSize). It was native dsound.dll which produced the audible but distorted audio in SL2. Not sure how to bisect your patchset: should I try the patches one by one to see if the static noise appears in one of them?
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #212 from Alexander E. Patrakov patrakov@gmail.com 2011-11-06 02:09:48 CST --- Indeed, I misunderstood.
Yes, apply only part of the patchset, starting from 1 to where you want. E.g. you already know that without all patches there is no audio, and with all 12 patches there is noise. So apply the first 6 patches and listen. If there is no audio, apply patches from 1 to 9 to pristine sources, and if there is noise, apply patches from 1 to 3 to them. Then repeat. Or, if you don't understand, just apply patches one by one and test after each, but that's more work. In the end, you'll find that with patches from 1 to N-1 there is no audio, and with patches from 1 to N there is noise. Report the description of patch N.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #213 from GyB gyebro69@gmail.com 2011-11-06 03:03:59 CST --- (In reply to comment #212)
Yes, apply only part of the patchset, starting from 1 to where you want. E.g. you already know that without all patches there is no audio, and with all 12 patches there is noise. So apply the first 6 patches and listen. If there is no audio, apply patches from 1 to 9 to pristine sources, and if there is noise, apply patches from 1 to 3 to them. Then repeat. Or, if you don't understand, just apply patches one by one and test after each, but that's more work. In the end, you'll find that with patches from 1 to N-1 there is no audio, and with patches from 1 to N there is noise. Report the description of patch N.
The first patch in the series already causes the static noise in SL2: 0001-Always-resample-in-mixer.txt
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #214 from Alexander E. Patrakov patrakov@gmail.com 2011-11-06 06:07:25 CST --- Sorry, this doesn't make sense. Could you please confirm that setting MaxShadowSize without any of my patches does not introduce noise, while applying 0001-Always-resample-in-mixer.txt does? This patch is supposed to be exactly equivalent to always setting MaxShadowSize to 0, and I would be surprised if it isn't.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #215 from GyB gyebro69@gmail.com 2011-11-06 06:34:26 CST --- (In reply to comment #214)
Sorry, this doesn't make sense. Could you please confirm that setting MaxShadowSize without any of my patches does not introduce noise, while applying 0001-Always-resample-in-mixer.txt does? This patch is supposed to be exactly equivalent to always setting MaxShadowSize to 0, and I would be surprised if it isn't.
Do these make any sense to you?
vanilla Wine 1.3.32, without setting MaxShadowSize: no audio vanilla Wine 1.3.32, MaxShadowSize=0: no audio vanilla Wine 1.3.32, MaxShadowSize=-1: noise
1.3.32 patched with 0001-Always-resample-in-mixer, without setting MaxShadowSize: noise 1.3.32 patched with 0001-Always-resample-in-mixer, MaxShadowSize=0: noise 1.3.32 patched with 0001-Always-resample-in-mixer, MaxShadowSize=-1: noise
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #216 from Alexander E. Patrakov patrakov@gmail.com 2011-11-06 07:21:40 CST --- Yes, it now makes perfect sense and clearly demonstrates that the noise problem is not in my code. It looks like I was wrong that my patch was equivalent to setting MaxShadowSize to 0. In order to force the old wine to resample in the mixer thread, you would need to set MaxShadowSize to -1, not 0. Not sure why the guides tell to set it to 0, though.
And of course, after any of the patches here, MaxShadowSize is not even read from the registry.
OTOH, it would be nice to find out what exactly in the old patch fixed it.
http://bugs.winehq.org/show_bug.cgi?id=14717
Alexander E. Patrakov patrakov@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #37320|0 |1 is obsolete| |
--- Comment #217 from Alexander E. Patrakov patrakov@gmail.com 2011-11-06 07:32:03 CST --- Created attachment 37336 --> http://bugs.winehq.org/attachment.cgi?id=37336 A split patchset with an alternative resampler implementation
A somewhat improved patchset that should eat less CPU time. Still, not as good as the patch by Andrew Eikum.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #218 from Alexander E. Patrakov patrakov@gmail.com 2011-11-06 09:34:51 CST --- Quick note: my patchset is not ready for submission, because it sometimes adds clicks (due to integer overflow? will investigate later).
Testing method: open Audacity, set project rate to 96000 Hz, create a stereo track, fill it with a 30-second-long sine sweep from 0 to 48000 Hz at 0.8 amplitude. Export to a 32-bit wav file.
In winecfg, set the default sample rate to 44100 Hz.
Add this to ~/.asoundrc:
pcm.dump { type file file "/tmp/dump1.wav" format "wav" slave { pcm "hw:0" } }
pcm.!default { type rate slave.pcm "dump" slave.rate 44100 slave.format S16_LE }
Play sweep.wav through foobar2000, copy the resulting /tmp/dump1.wav file somewhere into a more safe place.
Restore the old .asoundrc. Configure audacity to display spectrograms with 2048 samples per point, Gaussian window (a = 4.5), 100 dB dynamic range, up to 22 kHz.
Open the copy of dump1.wav in audacity, configure it to display tracks as spectrograms by clicking on the triangle near the track. Anything except the bright diagonal line corresponds to unwanted frequency components added by a resampler.
Result: my resampler has a few vertical lines in the display, those correspond to the added clicks. There are also the unwanted diagonal lines corresponding to linear combination of the signal frequency and the old and new sampling rate (i.e. aliasing). The resampler by Andrew Eikum does not produce clicks, but, due to imperfect FIR (to be fixed later), has more aliasing.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #219 from Alexander E. Patrakov patrakov@gmail.com 2011-11-06 09:54:29 CST --- OK, I found that clicks appear due to the use of freqAccNext in DSOUND_bufpos_to_secpos(). Applying ths patch removes them completely, but I am not sure if the rationale in the comment above DSOUND_bufpos_to_secpos() is valid.
--- a/dlls/dsound/mixer.c +++ b/dlls/dsound/mixer.c @@ -149,12 +149,12 @@ static DWORD DSOUND_bufpos_to_secpos(const IDirectSoundBufferImpl *dsb, DWORD bu DWORD64 acc;
framelen = bufpos/oAdv; - acc = framelen * (DWORD64)dsb->freqAdjust + (DWORD64)dsb->freqAccNext; + acc = framelen * (DWORD64)dsb->freqAdjust + (DWORD64)dsb->freqAcc; acc = acc >> DSOUND_FREQSHIFT; pos = (DWORD)acc * iAdv; - if (pos >= dsb->buflen) - /* Because of differences between freqAcc and freqAccNext, this might happen */ - pos = dsb->buflen - iAdv; +// if (pos >= dsb->buflen) +// /* Because of differences between freqAcc and freqAccNext, this might happen */ +// pos = dsb->buflen - iAdv; TRACE("Converted %d/%d to %d/%d\n", bufpos, dsb->tmp_buffer_len, pos, dsb->buflen); return pos; } @@ -472,10 +472,6 @@ static void DSOUND_MixToTemporary(const IDirectSoundBufferImpl *dsb, DWORD tmp_l INT size = tmp_len / oAdvance; DWORD freqAcc;
- DWORD len = DSOUND_bufpos_to_secpos(dsb, dsb->buf_mixpos + tmp_len) - dsb->sec_mixpos; - - assert(dsb->sec_mixpos + len <= dsb->buflen); - if (dsb->device->tmp_buffer_len < tmp_len || !dsb->device->tmp_buffer) { dsb->device->tmp_buffer_len = tmp_len;
http://bugs.winehq.org/show_bug.cgi?id=14717
Andrew Eikum aeikum@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #37301|0 |1 is obsolete| | Attachment #37336|0 |1 is obsolete| |
--- Comment #220 from Andrew Eikum aeikum@codeweavers.com 2011-11-07 15:05:22 CST --- Created attachment 37368 --> http://bugs.winehq.org/attachment.cgi?id=37368 Alexander's split patchset, cleaned up for submission
I've taken Alexander's latest patchset and cleaned it up to about submission quality.
GyB: I tested the Still Life 2 demo that you linked with this patchset and it seemed to work completely fine.
I also tested Total Overdose and it worked fairly well, although there were audio syncing issues, some sound effects seemed trimmed, and it seemed like the background music looped. I wasn't able to get the demo running in VirtualBox due to DRM issues (in a demo???), so I can't verify if that behavior is different from Windows.
Alexander: The last patch in the series does quite a lot with bitshifting and masking. It's unclear what the purpose is. Is it possible to write the code more clearly, or to add comments explaining what DSOUND_FREQSHIFT and related struct members are used for?
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #221 from Alexander E. Patrakov patrakov@gmail.com 2011-11-07 23:42:24 CST --- Thanks for nicely grouping my patches.
Some random notes on the reworked patches. I understand that they also apply to the original patch.
- if (dsb->device->tmp_buffer_len < len || !dsb->device->tmp_buffer) - { - /* If we just resampled in DSOUND_MixToTemporary, we shouldn't need to resize here */ - assert(!dsb->resampleinmixer); - dsb->device->tmp_buffer_len = len; - if (dsb->device->tmp_buffer) - dsb->device->tmp_buffer = HeapReAlloc(GetProcessHeap(), 0, dsb->device->tmp_buffer, len); - else - dsb->device->tmp_buffer = HeapAlloc(GetProcessHeap(), 0, len); - } + assert(dsb->device->tmp_buffer_len >= len && dsb->device->tmp_buffer);
This is a stray assert from the old version of the patch. It is later (in the patch in comment 219) deleted as the only user of the otherwise-unused len variable. Maybe it should not be added in the first place?
And please include the patch from comment 219. Without it, there are clicks in the output. As for the note above the DSOUND_bufpos_to_secpos() function, it is invalid for resampling in the mixer thread. Anyway, at the end of the series, DSOUND_bufpos_to_secpos() is called from exactly one place, and that place clearly needs freqAcc, not freqAccNext.
Also, the FIR generator includes two extra points, one at each end of the FIR. They were needed by cubic interpolation to set the correct derivative at the end, but are unused by the linear interpolation. Can we drop them by removing these two lines
+ /* For cubic interpolation, one extra dummy point */ + wing_len += 1;
and removing the "1 +" from "DWORD idx = 1 + (hires_pos >> DSOUND_FREQSHIFT);"?
As for your question about bitmasking and shifting: this is rather boring fixed-point math, of the similar kind as we already do with freqAcc. Values freqAdjust and freqAcc in the original Wine are to be interpreted as follows: they represent fractional values multiplied by (1 << DSOUND_FREQSHIFT). The same interpretation applues to invFreqAdjust, dsb->firstepdsb->firstep, the mu parameter for linear_interp(), hires_pos and max_pos. So, with the current value of DSOUND_FREQSHIFT equal to 20, e.g. the value of hires_pos = 3 << 19 (i.e. 1.5 * (1 << DSOUND_FREQSHIFT)) means "something right in the middle between the first and the second (counting from zero) used points of the FIR". Then the expression
DWORD idx = 1 + (hires_pos >> DSOUND_FREQSHIFT);
really means "take the integer part of the value represented by hires_pos and add one, to compensate for the dummy point added for the cubic interpolator", and
DWORD rem = hires_pos & ((1 << DSOUND_FREQSHIFT) - 1);
means "take the fractional part".
That's all about time-related variables. Also sample-related variables use the same fixed-point scheme, but there is no explicit nice "normalization" value like 1 << DSOUND_FREQSHIFT. The values returned by get() and accepted by put() are, obviously, normalized in a way that 0x7fffffff represents the maximum possible sample value.
Note that gcc with -march=i686 or higher compiles the "((LONGLONG)something * something_else) >> 32" expression into a single "imul" CPU instruction, that's why it is extensively used. With the "0x7fffffff means just a bit less than 1.0" normalization, this expression means "something_float * something_else_float / 2.0". Conceptually, the math is simple: linear_interp() gets the interpolated FIR value, this is multiplied by the input sample and added to sample[channel], and then the sum is multiplied by gain, clipped and stored.
So the concern is how to preserve the most of the significant bits in sample[channel] without leading to the overflow. Because of the overflow when accumulating sample[channel], especially in the downsampling case, I cannot normalize the FIR to 0x7fffffff. That's why it is normalized to 0x1000000, giving 20 significant bits. This is well-balanced with the 20 bits of precision in time-related variables like freqAcc.
As for the gain normalization vs "((LONGLONG)sample[channel] * gain) >> 17", the things are indeed rather clumsy. When this was written, I didn't know what the correct value of the gain would be (i.e. supposed that I might need either to attenuate or to amplify the samples), so wrote something that would work for either case. Then I figured out the correct value in DSOUND_RecalcFreqAcc(). So it turns out that the overall effect for upsampling is to left-shift the samples by 8 bits, thus giving 24 bits of possible precision here (of course, the FIR is worse than that).
Given that the maximum value of the gain to be applied after the FIR is now known, it is indeed better to change this part so that firgain is normalized to 0x7fffffff. I will do that later today.
One more remark: you merged my patches related to channel copying and with resampling. These are conceptually different things, I disagree with the merge and thus will split the patch into two: the first patch will copy channels using the bad resampler, and the second patch will add a resampler.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #222 from Alexander E. Patrakov patrakov@gmail.com 2011-11-07 23:49:10 CST --- Oops,
That's why it is normalized to 0x1000000, giving 20 significant bits. This is well-balanced with the 20 bits of precision in time-related variables like freqAcc.
This part is wrong. There are 24 significant bits, of course, and this is better than our time resolution.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #223 from Alexander E. Patrakov patrakov@gmail.com 2011-11-08 10:20:10 CST --- Created attachment 37381 --> http://bugs.winehq.org/attachment.cgi?id=37381 My patchset, after some more cleanups
I have split the channel handling and added comments to all fixed-point math. If you want even less bitshifting, I can convert the added code to floating-point math, as in your old patch.
The patch about freqAccNext -> freqAcc change is also incorporated. With this change, freqAccNext becomes almost unused, and it would be a good cleanup to kill it.
The patchset works for me and matches the default Windows XP resampling quality, but eats ~18% CPU for 44100 -> 48000 Hz resampling. This is the price to pay for the quality that is better than provided by the resamplers on linux desktops at the default settings, and for the lack of any MMX/SSE optimizations found in other resampling libraries.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #224 from GyB gyebro69@gmail.com 2011-11-10 04:46:46 CST --- I was busy upgrading to Fedora 16 which didn't went smoothly and still have problems, so I could only test the reworked patchset in attachment #37368 .
Most of the problems I reported in comment #209 have been cleared with this patchset, except the strange audio stuttering and performance drops, affecting GTA: San Andreas and a couple of other games. It turned out that Fallout 3 GOTY and Fallout: New Vegas are also affected.
-Darwinia also suffers from audio stuttering but that might be a different issue because no performance degradation can be observed. Darwinia demo: http://www.introversion.co.uk/darwinia/downloads/index.html
-F.E.A.R 1 (First Encounter Assault Recon) crashes in various places, it's easy to reproduce in F.E.A.R Perseus Mandate, which begins with an action packed scene. As for the demo of F.E.A.R, it's so short that I was able to run through it twice without crashes, on a third occasion it did crashed. Backtrace when F.E.A.R crashes: http://pastebin.com/4gsjSyWs
- Evil Genius also crashes during the initial loading stage, this might be a different issue: http://pastebin.com/YXKYCFTt Evil Genius demo: http://www.fileplanet.com/145023/140000/fileinfo/Evil-Genius-Demo
p.s.: audio is working again in Riven and in Still Life 2.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #225 from Alexander E. Patrakov patrakov@gmail.com 2011-11-10 06:45:25 CST --- Dear all,
please note that my patch is not supposed to magically fix all sound related bugs ;) , its only purpose is to improve sound quality when some of the application sounds use a sample rate different from the one you set in winecfg. However, this bugzilla entry seems to be hijacked by the things unrelated to sound quality that the attached patches also happen to fix.
I think most of the improvements that are not related to quality come from always resampling sound in the mixer thread, which was not the default in the old wine. I had to remove the other (i.e. the default) possibility, because it cannot be implemented correctly when a resampler other than zero-order-hold is used and the application is allowed to rewind its buffer.
The old wine resampled in the mixer thread if you set MaxShadowSize to -1 in the registry. Personally, I am not interested in any breakage or improvements that result merely from moving the resampling from the application thread to the mixer thread. You are, of course, welcome to post such reports here, because someone else may be interested, but please clearly indicate in every report whether setting MaxShadowSize to -1 in the unpatched Wine leads to the same improvement or breakage as you observe with my patch.
If most of the improvements actually come from moving the old bad resampler to the mixer thread, then I suggest that Wine developers merge the first patch in my series, and then we can discuss the rest of the patches separately, so that the two types of issues are not mixed together.
Thanks!
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #226 from Alexey Loukianov mooroon2@mail.ru 2011-11-10 18:23:24 CST --- Alexander, thanks for your patience keeping working on this problem for about three years and coming up with your pretty nice patchset. I want to mention one side note:
... its only purpose is to improve sound quality when some of the application sounds use a sample rate different from the one you set in winecfg.
AFAIK, winecfg in recent Wine versions doesn't allow one to set sample rate. You have to set it using registry editor in case it is really required. I agree with you on all other points you had written about.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #227 from Alexey Loukianov mooroon2@mail.ru 2011-11-10 18:37:11 CST --- P.S. Sorry for hi-jacking the bug report a bit more but I have a semi-related question for Andrew to ask: looks like that the default samplerate for dsound is set to be 44100Hz, which differs from the typical 48000Hz value that's being used by most AC97 codecs. HDA codecs typically use 96000Hz or 192000Hz as their base internal samplerate (ones that don't use PDM internally, but latter are extremely rare and are typical only for Hi-End sound interfaces). It leads us to a fact that if we've got DSound resampling everything that's being fed into it to the 44100Hz, then it would get another resampling to 48KHz (96/192), either in software (dmix/pulse/name your beloved soundserver here) or in hardware (SB Live! cards resample internally to 48KHz using built-in DSP for such cases). Maybe it would be wise to change the default to be 48KHz? It would save users from extra resampling in good cases and wouldn't hurt quality much for unfortunate case having non-matching hardware internal sample rate.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #228 from Raymond superquad.vortex2@gmail.com 2011-11-11 00:49:25 CST --- CPU usage is quite high (over 20%) for vlc especially when user mis-configure default sample rate to 96000Hz when the sound card only support 48000Hz
when normal usage is only 1% for vlc for playing 16bit 44100Hz
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #229 from Raymond superquad.vortex2@gmail.com 2011-11-11 01:01:51 CST --- (In reply to comment #227)
HDA codecs typically use 96000Hz or 192000Hz as their base internal samplerate (ones that don't use PDM internally,
Take a look at HDA specification,
5.4.2 Link Sample Delivery Timing
48-kHz Delivery Timings This becomes straight forward for streams whose sample rate is a natural harmonic of 48 kHz. In this case the base rate multiple also defines the number of complete sample blocks that must be transmitted in each frame. For either input or output, there must be transmitted on the link “y” complete sample blocks (block includes one sample for each channel in the stream) on every nth frame as shown in Table 57. For example, with a multiple of 4 (192 kHz sample rate), there must be four sample blocks transmitted in every frame on the link;
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #230 from Alexander E. Patrakov patrakov@gmail.com 2011-11-11 02:52:02 CST --- (In reply to comment #228)
CPU usage is quite high (over 20%) for vlc especially when user mis-configure default sample rate to 96000Hz when the sound card only support 48000Hz
when normal usage is only 1% for vlc for playing 16bit 44100Hz
Yes, this is expected, and, I as already told, this is a necessary consequence of matching the default Windows resampling quality without MMX/SSE acceleration which I cannot implement anyway. That's why I still insist that, in the ideal world, resampling and mixing the sound should be left to the underlying platform libraries. If they are not good enough (as evidenced e.g. with the "ton of sliders" UI issue), we should talk to the authors of these libraries to make the API suitable for wine needs, not reinvent the wheel as I have done with my patch. However, that's a long-term plan.
You can tweak the constants in firgen.c to lower the CPU usage, but, of course, the resulting sound will be worse than in Windows. Try e.g. these values:
exp_width = 15.0
lobes_per_wing = 11
approx_bandwidth = 0.85
fir_step = 120 // does not affect CPU usage
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #231 from Alexander E. Patrakov patrakov@gmail.com 2011-11-11 03:32:28 CST --- (In reply to comment #227)
P.S. Sorry for hi-jacking the bug report a bit more but I have a semi-related question for Andrew to ask: looks like that the default samplerate for dsound is set to be 44100Hz, which differs from the typical 48000Hz value that's being used by most AC97 codecs. HDA codecs typically use 96000Hz or 192000Hz as their base internal samplerate. <snip about SB Live! internal bad-quality resampling, it's a known issue>
The native codec sample rate and its internal resampling is irrelevant, because Wine doesn't talk to the codec directly. Wine talks to either oss4, or dmix, or pulseaudio. I know nothing about oss4, so let's omit it.
With dmix, the default sample rate is indeed 48 kHz, but pulseaudio defaults to 44.1 kHz. The point is that there is no single static good default that would eliminate double software resampling (once by wine and once by platform libraries) for all users.
What can be done, though, is to disable the ALSA resampler (only if called from DirectSound! that's important!), ask ALSA for approximately the default sample rate, and use the result. Something like this pseudocode (copied from MPlayer that gets the logic right, but untested in my own code):
sample_rate = default_sample_rate; snd_pcm_hw_params_set_rate_resample(?, ?, 0); snd_pcm_hw_params_set_rate_near(?, ?, &sample_rate, 0); run_primary_buffer_at(sample_rate);
This way, dmix users will have the primary buffer running at 48 kHz (or whatever their defaults.pcm.dmix.rate is), no matter what sample rate is configured in the registry. And pulseaudio users will get default_sample_rate. So with the above logic and default_sample_rate = 44100, we'll match the ideal sample rate for both default dmix and default pulseaudio setups.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #232 from Alexey Loukianov mooroon2@mail.ru 2011-11-11 03:44:07 CST --- (In reply to comment #231)
(In reply to comment #227) The native codec sample rate and its internal resampling is irrelevant, because Wine doesn't talk to the codec directly. Wine talks to either oss4, or dmix, or pulseaudio. I know nothing about oss4, so let's omit it.
You're omitting the case I've got here at home with emu10k-based card - it has hardware mixing so default pcm in my case is configured to directly talk to hardware pcm. There's no dmix or PA out here.
With dmix, the default sample rate is indeed 48 kHz, but pulseaudio defaults to 44.1 kHz. The point is that there is no single static good default that would eliminate double software resampling (once by wine and once by platform libraries) for all users.
Sure, that's why I had been a bit pissed off then default sample rate setting had been removed from winecfg.
What can be done, though, is to ...
Not a bad offer, really. Don't know how would it work with my case having no PA or dmix downstream to soundcard hardware.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #233 from Alexey Loukianov mooroon2@mail.ru 2011-11-11 03:45:06 CST --- (In reply to comment #232)
.. a bit pissed off then ...
It's a typo, should be read "when".
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #234 from Alexey Loukianov mooroon2@mail.ru 2011-11-11 03:52:08 CST --- (In reply to comment #229)
Take a look at HDA specification ...
I've read all the Inter HDA specs when been hacking in some fixes to voodoohda driver for Max OS X. The paragraph you quoted has nothing to do with my question to Andrew. HDA link specs are on their own, internal codec design when it comes to the samplerate that is actually used on DAC (and the modulation else - there are high-end HDA codecs out there that are capable of using PDM as their digital data format and also use DACs requiring input data in PDM-modulated form). Don't quote unrelated specs, please, unless you know what are they about and sure that this info would be relevant for discussion.
P.S. PDM is not a typo here, don't confuse it with PCM.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #235 from Alexander E. Patrakov patrakov@gmail.com 2011-11-11 04:54:19 CST --- (In reply to comment #232)
Not a bad offer, really. Don't know how would it work with my case having no PA or dmix downstream to soundcard hardware.
It will select 44.1 kHz unless you override the default sample rate in the registry. I.e. exactly the same situation as the current status quo. OTOH, if I were an ALSA developer, I would have changed the emu10k1 driver to expose only 48 kHz, because almost every software resamples with equal or better quality than this particular piece of hardware.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #236 from Andrew Eikum aeikum@codeweavers.com 2011-11-11 08:37:08 CST --- (In reply to comment #225)
If most of the improvements actually come from moving the old bad resampler to the mixer thread, then I suggest that Wine developers merge the first patch in my series, and then we can discuss the rest of the patches separately, so that the two types of issues are not mixed together.
Well, your resampler improvements depend on moving resampling back to the mixer thread, and if that causes regressions, then your improvements are blocked. So we're trying to fix these regressions so your fixes can get in. I guess we could move the discussion to Bug 9612, but I don't really see any reason to.
(In reply to comment #227)
P.S. Sorry for hi-jacking the bug report a bit more but I have a semi-related question for Andrew to ask: looks like that the default samplerate for dsound is set to be 44100Hz, which differs from the typical 48000Hz value that's being used by most AC97 codecs.
Maybe it would be wise to change the default to be 48KHz?
I don't have a problem with that. If you can provide some evidence that most cards use that rate (or some multiple), it will help getting through the wine-patches submission process.
(In reply to comment #228)
CPU usage is quite high (over 20%) for vlc especially when user mis-configure default sample rate to 96000Hz when the sound card only support 48000Hz
when normal usage is only 1% for vlc for playing 16bit 44100Hz
I think I will add options for different resampler qualities (see comment 230) so users can tweak this down when they need to.
(In reply to comment #231)
What can be done, though, is to disable the ALSA resampler (only if called from DirectSound! that's important!), ask ALSA for approximately the default sample rate, and use the result. Something like this pseudocode (copied from MPlayer that gets the logic right, but untested in my own code):
I wonder if a better solution is to have dsound use the sample rate reported by GetMixFormat() as its default primary buffer rate. Then the drivers could be improved to report the device's preferred sample rate, if they don't already.
(In reply to comment #232)
Sure, that's why I had been a bit pissed off then default sample rate setting had been removed from winecfg.
Part of the problem with Wine's audio situation was the plethora of different configuration options that were readily available to users. When a bug report comes in, nobody remembers which combinations of settings resulted in which end result, so they file conflicting or incorrect reports. Requiring the user to go through the registry to tweak options means they're less likely to mess with the defaults and create bogus reports. And if someone _does_ go through the effort, they're more likely to either remember it, or actually understand what they're doing.
I'm sorry it's not as convenient as it used to be, but I felt that consistent, sane bug reports were worth the cost. As the audio situation starts settling down, we can re-evaluate which options are worth having handy access to and perhaps add a couple back to the Audio tab.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #237 from Alexander E. Patrakov patrakov@gmail.com 2011-11-11 10:25:00 CST --- (In reply to comment #236)
(In reply to comment #225)
If most of the improvements actually come from moving the old bad resampler to the mixer thread, then I suggest that Wine developers merge the first patch in my series, and then we can discuss the rest of the patches separately, so that the two types of issues are not mixed together.
Well, your resampler improvements depend on moving resampling back to the mixer thread, and if that causes regressions, then your improvements are blocked. So we're trying to fix these regressions so your fixes can get in.
I understand this, and agree that if regressions are found due to the move, then it is a valid reason to block the rest of my patches. What I am trying to do is to make sure that regressions due to the move and due to other refactorings are clearly separated in user reports, e.g. by submitting the first patch separately, and maybe even targetting the rest of the patchset for a different Wine version.
And one more point: I am certain that the first patch (unlike others) already has its final form, and I see nothing to discuss about it except motivation, regressions uncovered by it and possibly dead code that I might have missed. In other words, there is no point to delay its submission (unlike the rest of the patchset, where I am not sure).
(In reply to comment #227)
P.S. Sorry for hi-jacking the bug report a bit more but I have a semi-related question for Andrew to ask: looks like that the default samplerate for dsound is set to be 44100Hz, which differs from the typical 48000Hz value that's being used by most AC97 codecs.
Maybe it would be wise to change the default to be 48KHz?
I don't have a problem with that. If you can provide some evidence that most cards use that rate (or some multiple), it will help getting through the wine-patches submission process.
As I have already said, this is irrelevant because most of the distros are defaulting to PulseAudio now, and pulseaudio runs at 44.1 kHz even if the codecs use some other frequency internally.
(In reply to comment #228)
CPU usage is quite high (over 20%) for vlc especially when user mis-configure default sample rate to 96000Hz when the sound card only support 48000Hz
when normal usage is only 1% for vlc for playing 16bit 44100Hz
I think I will add options for different resampler qualities (see comment 230) so users can tweak this down when they need to.
Thanks! And I will evaluate whether going to floating point improves anything (for speex, it reportedly does).
(In reply to comment #231)
What can be done, though, is to disable the ALSA resampler (only if called from DirectSound! that's important!), ask ALSA for approximately the default sample rate, and use the result. Something like this pseudocode (copied from MPlayer that gets the logic right, but untested in my own code):
I wonder if a better solution is to have dsound use the sample rate reported by GetMixFormat() as its default primary buffer rate. Then the drivers could be improved to report the device's preferred sample rate, if they don't already.
If the waveOut API does not call GetMixFormat(), I'd say that our solutions are equivalent: e.g., one can insert my lines into GetMixFormat() and thus get what you described. There would certainly be no problem for the case when such a preferred sample rate exists (i.e., for dmix). But in the case when the single preferred sample rate does not exist (e.g. on any sound card that can do either 44100 or 48000 Hz in hardware and is accessed via hw:X) or when this rate is not reported via ALSA layer (e.g. with pulseaudio), we need some fallback mechanism, possibly inside GetMixFormat().
The current code in both OSS and ALSA mmdevdrvs contains some variant of the heuristic that the highest supported sample rate is the preferred one. On my hardware, this would cause the ALSA mmdevdrv to choose 48 kHz because of the explicit cap at that frequency (or 192 kHz if the cap were not there). However, this is not the correct result for PulseAudio, and there is no way to get the correct-for-pulseaudio result via ALSA API.
So we do need an explicit preference in the registry - i.e. we can only move it from dsound to drivers (which is IMHO pointless), but not remove it. But if you try to implement the use of that preference via ALSA API, you will probably come to something resembling my pseudo-code.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #238 from Raymond superquad.vortex2@gmail.com 2011-11-13 04:49:44 CST --- (In reply to comment #231)
(In reply to comment #227)
<snip about SB Live! internal bad-quality resampling, it's a known issue>
The native codec sample rate and its internal resampling is irrelevant, because Wine doesn't talk to the codec directly. Wine talks to either oss4, or dmix, or pulseaudio.
http://source.winehq.org/git/wine.git/?a=commit;h=232893e0832bf5082285995303...
winealsa.drv: Use the plughw instead of the hw interface for opening devices.
dsound still talk to codec through plughw , so it is still using internal resampling for those alsa driver which support SNDRV_PCM_RATE_CONTINUOUS (e.g. sblive )
it is just wine mme implementation of getmixerformat() hardcode the rate to 48000Hz
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #239 from Raymond superquad.vortex2@gmail.com 2011-11-13 05:00:16 CST --- (In reply to comment #180)
Just a note: Plants vs Zombies (demo at http://www.fileplanet.com/199524/190000/fileinfo/Plants-vs-Zombies-Demo) uses a number of different sample rates for its sound effects, so makes a pretty good test case for resampling issues.
The sound is lagged when you have 30~40 plants firing at zombies at high level
http://bugs.winehq.org/show_bug.cgi?id=9358
Low latency interactive buffers are not mixed properly in directsound
The music produced by famitracker at small buffer size is distorted
The difference between your patch is the pitch is changed too
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #240 from Alexander E. Patrakov patrakov@gmail.com 2011-11-13 08:17:39 CST --- Raymond,
your report is missing the following critically important piece of information: are the defects that you describe also reproducible without my patch, but with MaxShadowSize = -1?
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #241 from Andrew Eikum aeikum@codeweavers.com 2011-11-15 13:05:31 CST --- (In reply to comment #224)
Most of the problems I reported in comment #209 have been cleared with this patchset, except the strange audio stuttering and performance drops, affecting GTA: San Andreas and a couple of other games. It turned out that Fallout 3 GOTY and Fallout: New Vegas are also affected.
This also happens on my computer. It's because of the combination of the high CPU usage resampler. GTA:SA has quite a few secondary buffers, and when enough of them are in a playing state, the mixer can't keep up. This causes underruns in the audio, and GTA:SA is apparently written to block if the dsound write pointer doesn't move quickly enough. If I use the low quality resampler given in Comment 230, GTA:SA runs fine (unless, presumably, enough sound effects become active at once and it falls behind again).
-Darwinia also suffers from audio stuttering but that might be a different issue because no performance degradation can be observed. Darwinia demo: http://www.introversion.co.uk/darwinia/downloads/index.html
This is the same problem. Changing to the low-quality resampler fixes it.
-F.E.A.R 1 (First Encounter Assault Recon) crashes in various places, it's easy to reproduce in F.E.A.R Perseus Mandate, which begins with an action packed scene. As for the demo of F.E.A.R, it's so short that I was able to run through it twice without crashes, on a third occasion it did crashed. Backtrace when F.E.A.R crashes: http://pastebin.com/4gsjSyWs
Haven't looked into this yet.
- Evil Genius also crashes during the initial loading stage, this might be a
different issue: http://pastebin.com/YXKYCFTt Evil Genius demo: http://www.fileplanet.com/145023/140000/fileinfo/Evil-Genius-Demo
The trace is missing a newline. I added the newline and it stopped crashing. Audio seems to work, though I'm not familiar with the game.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #242 from Alexander E. Patrakov patrakov@gmail.com 2011-11-26 05:32:29 CST --- In the latest patch, the fix from comment 219 somehow got lost.
http://bugs.winehq.org/show_bug.cgi?id=14717
Alexander E. Patrakov patrakov@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #37381|0 |1 is obsolete| |
--- Comment #243 from Alexander E. Patrakov patrakov@gmail.com 2011-11-27 04:38:48 CST --- Created attachment 37659 --> http://bugs.winehq.org/attachment.cgi?id=37659 Third version of my patchset
The third version of the patchset removes some dead code. It does not address the performance concerns.
In order to address them, I need to add a buffer so that dsb->get is not called more than once for the same sample. However, in order to add such buffer, I must understand the existing buffering scheme. It just takes time.
What I want to do next (in order to simplify buffering) is to clean up the volume logic. Currently, it is an example of yo-yo code that attempts to handle all bit depths in a "switch" statement. I will try to put this into resampled_copy() so that the code only has to deal with 32-bit samples.
One more thing concerning performance: the code does a lot of fixed-point arithmetics, but fixed-point arithmetics is exactly what prevents me from using SSE2 optimizations. So, may I refactor the code in such a way that get() returns and put() accepts a floating-point number instead of a 32-bit integer? May I convert freqAcc from a 20-bit fixed-point number to a floating-point number? The only implication is that the code will no longer be bit-exact with 32-bit primary buffers (if such thing exists at all).
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #244 from Andrew Eikum aeikum@codeweavers.com 2011-11-28 07:58:00 CST --- Using double for the intermediary data type makes sense to me, especially if it will make optimizations easier or automatic. Krzysztof's changes use double, and I thiiiiiiink that resample and libresample from https://ccrma.stanford.edu/~jos/resample/Free_Resampling_Software.html both use float or double as well (their sources aren't trivial, so I'm not sure).
http://bugs.winehq.org/show_bug.cgi?id=14717
Jörg Höhle hoehle@users.sourceforge.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |hoehle@users.sourceforge.ne | |t
--- Comment #245 from Jörg Höhle hoehle@users.sourceforge.net 2011-12-12 08:04:40 CST --- I'm not that familiar with the DSound API, but I'd like to know whether Wine could come up with a scheme that allows its mixer to sit near mmdevapi, or use some COM-based objects to access mixer functionality. The point is, we don't know yet whether mmdevapi might not need a mixer and resampler eventually.
Why does mmdevapi's GetMixFormat return floating point? Obviously because that what the native mixer uses!
So I don't understand why in comment #243, Alexander asks for permission to use FP. That is obviously the right choice. The output of anything that is going to be mixed should be FP, then mixed (if HW has not enough channels) then converted to 16bit (if HW doesn't grok FP, Intel audio looks like it does).
How comes that native's winmm capabilities claim support of all frequencies since w7, even with cards that don't, whereas the same HW runding MS<Vista returns "real" capabilities? Because there must be a resampler somewhere between winmm for w7 and mmdevapi! (winmm for w7 supposedly uses mmdevapi)
In Wine, this was solved incompatibly by requiring the audio back ends to support all rates and formats, therefore using plughw (bug #27956) and having IsFormatSupported constantly return S_OK, unlike native.
Perhaps Wine must implement AUDCLNT_STREAMFLAGS_RATEADJUST one day, what is it going to do? (Read it and you'll see it's not a set_hw_params_rate).
I'd feel much better if there was a mixer that is shared both by DSound and mmdevapi and a resampler that is used and shared by DSound, winmm, msacm and mmdevapi. I don't see that as an impossible task, but then, I'm not familiar with DSound (and weird constraints on the buffer implementation such as bug #28748, comment #1 -- I wonder if apps do the same with mmdevapi's buffer?!?).
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #246 from Raymond superquad.vortex2@gmail.com 2011-12-13 00:29:59 CST --- (In reply to comment #245)
So I don't understand why in comment #243, Alexander asks for permission to use FP. That is obviously the right choice. The output of anything that is going to be mixed should be FP, then mixed (if HW has not enough channels) then converted to 16bit (if HW doesn't grok FP, Intel audio looks like it does).
http://git.kernel.org/?p=linux/kernel/git/tiwai/hda-emu.git;a=tree;f=codecs;...
Only those macbook with cs4206 codecs support float
formats [0x3]: PCM FLOAT formats [0x7]: PCM FLOAT AC3
Most x86_64( linux machines) 's hda codec does not support float
even 32bits is not common , only 16 bits is available on all HDA codecs
bits [0x2]: 16 bits [0xa]: 16 24 bits [0xf]: 8 16 20 24 bits [0xe]: 16 20 24 bits [0x1e]: 16 20 24 32
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #247 from Andrew Eikum aeikum@codeweavers.com 2011-12-16 15:20:02 CST --- Created attachment 37994 --> http://bugs.winehq.org/attachment.cgi?id=37994 Speex-based resampler patchset
This is based on Alexander's latest patchset, but imports the resampler from the Speex project instead of trying to implement our own. It also adds a registry entry to let users with faster or slower computers choose the resampler quality. It has about 4-6% CPU usage in VLC for me with the default quality.
I would appreciate testing of this patchset, as I hope it's near final. Note that you'll need to rebuild the dsound Makefile ("make dlls/dsound/Makefile" from the Wine source root). I'd like to get this work in during the next Wine release cycle.
Alexander: If you have the time, I would most appreciate a review of patch 6 in this patchset "dsound: convert from fixed to floating point", especially the changes in DSOUND_secpos_to_bufpos(). The asserts in that function were failing, so I think I misunderstood something during my conversion. Also notice that I made significant changes to your patch "Replace convert() functions with get() and put()" to convert to float as the intermediate representation instead of LONG.
Do you think that you will have time to submit the first five patches to wine-patches during the next two or so weeks? If not, would you like me to submit them in your name? Or take ownership of them and give you credit in the patch comments? It's your code, so you get to pick what happens to it :)
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #248 from Alexander E. Patrakov patrakov@gmail.com 2011-12-17 04:28:24 CST --- The conversion of DSOUND_secpos_to_bufpos() to floating point is missing a ceil() call in the assignment to acc, that's what is causing the failed assertions.
Other objections:
1) Even a short hold is unacceptable, as it manifests itself as a click.
2) We are now double-counting the correspondence between input and output samples, once in wine and once in speex. This is a sure way to get a disagreement.
Is it possible to completely remove the temporary buffer, the DSOUND_RecalcTmpBufLen() function and all primary-to-secondary sample conversion functions? This way, when we fed N samples to Speex and it gave us M samples, we should advance the sample pointers for secondary and primary buffers by N and M samples, correspondingly.
While I understand that Speex is technically superior, I will continue optimizing my own resampler because I don't know if it would be easier for me to optimize my patch or for you to remove the double-counting problem.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #249 from Alexander E. Patrakov patrakov@gmail.com 2011-12-17 04:40:04 CST --- On sending the first five or even six patches out - yes, I will send them during the next week, with slight modifications to the third patch (namely, during the conversion to floating point, you removed clipping, and thus I have to restore it) and with some added comments.
http://bugs.winehq.org/show_bug.cgi?id=14717
Andrew Eikum aeikum@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #37994|0 |1 is obsolete| |
--- Comment #250 from Andrew Eikum aeikum@codeweavers.com 2011-12-21 13:29:00 CST --- Created attachment 38064 --> http://bugs.winehq.org/attachment.cgi?id=38064 Speex-based resampler patchset #2
Here's another iteration of the Speex-based resampler. This fixes the missing ceil() call, clamping in put*() functions, and a crash caused by calling DSBuffer::Duplicate() on resampled buffers.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #251 from Andrew Eikum aeikum@codeweavers.com 2011-12-21 13:31:37 CST --- Alexander: Please feel free to submit your dsound cleanup patches whenever you have time and are comfortable with them. I'll follow with the Speex resampler, assuming there are no problems found with the patchset in the meantime. Then we can finally close this bug :)
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #252 from Alexander E. Patrakov patrakov@gmail.com 2011-12-24 04:01:48 CST --- I have looked at the patch, but not as thoroughly as I want. Some of the concerns about maintaining the buffer positions twice are addressed, but I don't yet understand why it is impossible that converted > count (i.e. that there is no buffer overflow) and why the calculation of the temporary buffer size is consistent with what speex thinks.
And I have to test the integer <-> float conversion manually for every possible case. In particular, I want to ensure that samples are passed through, unchanged, if there is only one stream with no volume applied and no resampling.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #253 from Alexander E. Patrakov patrakov@gmail.com 2011-12-24 07:22:33 CST --- Andrew, your resampler produces clicks. To test: run foobar2000 and play a wav file with the sample rate of 11025 Hz. And there is also a bug with 24-bit dsb support: sign extension has to happen in get(), not in put().
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #254 from Alexander E. Patrakov patrakov@gmail.com 2011-12-24 07:53:05 CST --- Created attachment 38100 --> http://bugs.winehq.org/attachment.cgi?id=38100 Andrew's patchset with the third patch reworked
I made some changes to Andrew's patch, in the part where convert() functions are replaced with get() and put().
First, to ensure that 8-, 16- and 24-bit audio can pass through unmodified if there is no resampling and no volume changes, I have changed all int <-> float conversion factors to be powers of two. Second, the 24-bit case was handled incorrectly. Third, the original code always rounded floating-point samples towards zero, I have changed this to a uniform staircase by using lrintf().
No other bugs are fixed. I.e., this patch still causes clicks.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #255 from Alexander E. Patrakov patrakov@gmail.com 2011-12-24 09:24:29 CST --- The first six patches of the series have been sent as "PATCH RFC" to wine-patches.
http://bugs.winehq.org/show_bug.cgi?id=14717
Andrew Eikum aeikum@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #38064|0 |1 is obsolete| | Attachment #38100|0 |1 is obsolete| |
--- Comment #256 from Andrew Eikum aeikum@codeweavers.com 2011-12-30 15:27:31 CST --- Created attachment 38186 --> http://bugs.winehq.org/attachment.cgi?id=38186 Speex-based resampler patchset, improved positioning
(In reply to comment #253)
Andrew, your resampler produces clicks. To test: run foobar2000 and play a wav file with the sample rate of 11025 Hz. And there is also a bug with 24-bit dsb support: sign extension has to happen in get(), not in put().
Indeed it did. I'm fairly certain it was due to the double-counting issue you mentioned in comment 248. I think input samples were being used twice, but didn't completely confirm that.
So here's another patchset, based on yours from comment 254. It contains a first draft of a patch which removes the position conversions from primary to secondary buffer, and instead has the secondary buffers maintain their position independently. Then we only advance the secondary buffer by the number of samples that Speex actually converted, fixing the double count issue. It fixes the click problem -- or at least, the one I noticed using your instructions.
I'm still suspicious of the primary_mixpos member. It seems redundant, but I'm not totally convinced yet. I'll give it a closer look on Monday. It would also be nice to confirm that converting fewer frames than requested doesn't cause problems (it shouldn't due to the prebuffer, I think, but this should be verified).
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #257 from Alexander E. Patrakov patrakov@gmail.com 2012-01-01 02:03:30 CST --- Your 0006 patch is indeed a very nice cleanup. This is useful even in the unlikely case if speex turns out to be a bad idea. Thanks!
I have not tried to compile wine with your patches yet, but here are my (very minor) comments.
1. You call HeapAlloc() in cp_fields() to allocate temporary buffers. I'd rather have them on stack with some fixed size, and cap the requests to this size, given that we can return less samples than requested. This will also be a good test for your "It would also be nice to confirm that converting fewer frames than requested doesn't cause problems" worry.
2. Have you tested the patchset with any game that uses DSBCAPS_CTRLFREQUENCY? I am asking because in this case speex rebuilds the entire sinc table (to avoid any interpolation), as opposed to my approach of using a table that is good for all cases.
Overall, if (2) is not a problem, this looks very close to the final state of the patch.
--- Comment #258 from Andrew Eikum aeikum@codeweavers.com 2012-01-03 09:28:36 CST --- (In reply to comment #257)
Your 0006 patch is indeed a very nice cleanup. This is useful even in the unlikely case if speex turns out to be a bad idea. Thanks!
I have not tried to compile wine with your patches yet, but here are my (very minor) comments.
- You call HeapAlloc() in cp_fields() to allocate temporary buffers. I'd
rather have them on stack with some fixed size, and cap the requests to this size, given that we can return less samples than requested. This will also be a good test for your "It would also be nice to confirm that converting fewer frames than requested doesn't cause problems" worry.
Can you explain why?
There's some difficulties with that approach. Mainly, each mixer iteration process some integer number of fragments, which are not a fixed number of frames (see primary.c:DSOUND_fraglen; it seems to try to be about 10ms of audio per fragment). So we'd either have huge stack sizes, or process far too few frames.
If we simply want to avoid allocation/deallocation overhead, perhaps a better idea is to have (another) temporary buffer at the device level, which expands as needed and is dealloced only on destruction.
- Have you tested the patchset with any game that uses DSBCAPS_CTRLFREQUENCY?
I am asking because in this case speex rebuilds the entire sinc table (to avoid any interpolation), as opposed to my approach of using a table that is good for all cases.
Both GTA:SA (Steam) and Darwinia (Demo available) call _SetFrequency() with "unusual" values quite often, and I didn't notice a problem.
Overall, if (2) is not a problem, this looks very close to the final state of the patch.
Do you have any thoughts on Jörg's multichannel support question[1]? I'm going to examine that today, in addition to primary_mixpos and partial conversions.
[1] http://www.winehq.org/pipermail/wine-devel/2012-January/093633.html
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #259 from Jörg Höhle hoehle@users.sourceforge.net 2012-01-03 10:27:33 CST --- In bug #9358, comment #21, Maarten Lankhorst explained that primary.c:DSOUND_fraglen is a hack to second-guess the period size as "the current driver model doesn't" tell. This is no more the case. The hack should be removed. The period size is IAudioClient_GetDevicePeriod. With mmdevapi, DSound receives one event every such default period and that's the quantity that it should mix (currently winealsa/oss: 10ms, winecoreaudio: 20ms). IOW, it should act like the XAudio2 in bug #28723 -- ouch! I don't know when DSound is allowed to send more data in advance.
BTW, in bug #1631, comment #129 Maarten Lankhorst talks about trouble with <100ms buffers. 4 years later, I've doubts we won't face trouble with 10ms periods and 40ms buffers. We can't expect machines that much faster and reliable, see bug #28723, comment #51 and 61.
I'm not familiar with DSound, but why another buffer? Why not GetCurrentPadding & GetBuffer, then mix into that one, eventually ReleaseBuffer (may release a smaller number of frames)? Sadly, DSound's primary buffer may match HW well, but not mmdevapi's buffer model...
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #260 from Andrew Eikum aeikum@codeweavers.com 2012-01-03 12:19:08 CST --- (In reply to comment #259)
In bug #9358, comment #21, Maarten Lankhorst explained that primary.c:DSOUND_fraglen is a hack to second-guess the period size as "the current driver model doesn't" tell. This is no more the case. The hack should be removed. The period size is IAudioClient_GetDevicePeriod. With mmdevapi, DSound receives one event every such default period and that's the quantity that it should mix (currently winealsa/oss: 10ms, winecoreaudio: 20ms). IOW, it should act like the XAudio2 in bug #28723 -- ouch! I don't know when DSound is allowed to send more data in advance.
While investigating multi-channel support, I found that primary buffers with sufficiently large frame sizes (6 channel, 24 bit) can actually cause bugs where the total number of fragments in the buffer is the same as the prebuffer size (buffer = 65536 bytes, fraglen = 6144 bytes -> helfrags = 10; snd_queue_max = 10; helfrags <= snd_queue_max) which creates no audio output due to "pwplay %= snd_queue_max"-style logic.
So I wonder if a solution is to make the primary buffer size be (snd_queue_max + 1) * fraglen, and perhaps calculate fraglen from the mmdevapi period size, like you suggest.
I'm not familiar with DSound, but why another buffer? Why not GetCurrentPadding & GetBuffer, then mix into that one, eventually ReleaseBuffer (may release a smaller number of frames)? Sadly, DSound's primary buffer may match HW well, but not mmdevapi's buffer model...
It's unrelated. We need to resample into a temporary buffer, and then mix that into the primary buffer, which later gets copied into the mmdevapi buffer. We might be able to optimize it later, but this works well enough for now.
http://bugs.winehq.org/show_bug.cgi?id=14717
Andrew Eikum aeikum@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #38186|0 |1 is obsolete| |
--- Comment #261 from Andrew Eikum aeikum@codeweavers.com 2012-01-03 15:36:31 CST --- Created attachment 38246 --> http://bugs.winehq.org/attachment.cgi?id=38246 Speex resampler again
Another patchset, possibly final. A couple of changes:
I removed the "convert to float" patch, and instead just removed freqAdjust entirely in the patch that introduces Speex. It wasn't really used anyway.
I made one change to each of the first two patches. The first patch has this new diff hunk:
diff --git a/dlls/dsound/dsound_convert.c b/dlls/dsound/dsound_convert.c index 63e3dbe..dd982c6 100644 --- a/dlls/dsound/dsound_convert.c +++ b/dlls/dsound/dsound_convert.c @@ -66,8 +66,8 @@ static inline void src_advance(const void **src, UINT stride, INT *count, UINT * ULONG adv = (*freqAcc >> DSOUND_FREQSHIFT); *freqAcc &= (1 << DSOUND_FREQSHIFT) - 1; *(const char **)src += adv * stride; - *count -= adv; } + *count -= 1; }
The original patch changed the "count" (aka "size") variable to be in terms of the number of output frames, but the conversion functions still decremeted it in step with the src parameter. That caused data corruption and crashes, which this diff fixes.
The second patch was changed as follows (diff of a diff):
+static inline void cp_fields(const IDirectSoundBufferImpl *dsb, const BYTE *ibuf, UINT istride, UINT ostride, UINT count, UINT freqAcc, UINT adj) { DirectSoundDevice *device = dsb->device; - INT istep = dsb->pwfx->wBitsPerSample / 8, ostep = device->pwfx->wBitsPerSample / 8; + float value; + ULONG adv; -+ DWORD ipos = 0, opos = 0; ++ DWORD ipos = dsb->sec_mixpos, opos = 0;
since the get() functions work on (dsb->buff+ipos), not on the passed ibuf value (which is actually unused, and removed in the following patch. perhaps the following patch should be squashed? it's quite small).
Concerning channel conversions, I believe the patchset here more flexible than the old channel conversion. The old code supported (secondary -> primary): n -> n 6 -> 2 2 -> 8 (hack, first 2 only) 2 -> 6 (hack, first 2 only) 2 -> 1 1 -> 2 silence (catchall)
While the new code supports: n -> n 1 -> 2 n -> 1 n -> 2 (catchall)
I tested each patch against the dsound tests and four applications (pikachugame.exe, Dracula Origin Demo, Plants Vs Zombies Demo, and HighwayPursuit). The entire patch sequence together has been tested on my computer against many more applications in addition to those.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #262 from Jörg Höhle hoehle@users.sourceforge.net 2012-01-03 16:28:47 CST --- Didn't you identify the need for 2->6 in bug #26233, comment #9?
BTW, I think you should change mmdevdrv: if(max_channels > 2){ // <- with 8 here FIXME("Don't know what to do with %u channels, pretending there's 2 Supposing that your 5:1 sound card returns max 6 channels.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #263 from Alexander E. Patrakov patrakov@gmail.com 2012-01-04 01:38:28 CST --- First, thanks for the fixes. It is indeed important to keep this patchset bisectable.
As for the patch, I think I have found a theoretical problem. Your X_speex_resampler_get_required_input_length() function can cause the resampler to return less than count samples because it rounds down. So, for 11025 -> 48000 Hz conversion, if count happens to be 1 for some odd reason, it can return 0 and nothing will be converted, leading to an infinite loop. So please make absolutely sure that speex cannot request 0 samples. And for safety and for simplification of the accounting, I'd even make it always return count samples by supplying enough input, even if the secondary stream ends here (i.e. pad it with zeros in this case instead of truncating total_length).
Since Speex converts until either the input or the output buffer space runs out, for the reason above, I think it would be better to add a ceil() to the X_speex_resampler_get_required_input_length() function. Or rewrite it using fixed-point math to avoid the rounding issues, as shown below (completely untested, and the standard coffee warning applies):
spx_uint32_t X_speex_resampler_get_required_input_length(SpeexResamplerState *st, spx_uint32_t out_len) { spx_uint32_t ret = out_len * st->num_rate; ret = (ret + st->den_rate - 1) / st->den_rate; return ret + st->filt_len; }
Note that I have changed in_rate and out_rate with num_rate and den_rate, because speex might choose to round the resampling factor to a more convenient fraction.
OTOH, the addition of st->filt_len looks rather bogus to me (but harmless anyway), because under normal conditions it only should apply to the first call, and even then (read: if at all), maybe only if you haven't called speex_resampler_skip_zeros() (and I am not quite sure if you should or should not call it).
As for your question regarding any thoughts on Jörg's multichannel support question: no, I don't have any thoughts about that that I am sure of and not sent to wine-devel. And I didn't look at the old patches.
Regarding your statement about GTA:SA and Darwinia calling _SetFrequency() with unusual values, I have an additional question: do they do that while the stream is playing?
Overall, the patchset indeed looks nearly finished in terms of this bug. I will give it some testing today and report the result.
BTW, there are definitely more (out-of-scope for this bug) cleanups that can be made in dsound - e.g. mixing can be done in floating point instead of the current mess with the mix and norm functions for every possible bitness of the primary buffer.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #264 from Andrew Eikum aeikum@codeweavers.com 2012-01-04 07:12:36 CST --- (In reply to comment #262)
Didn't you identify the need for 2->6 in bug #26233, comment #9?
Sorry, I should have been more clear. In the case of n -> m, it will take the first 2 input channels and map them to the first 2 output channels, leaving the remaining (n-2) and (m-2) channels unused. This is how the current code works, but only for the couple cases I listed.
(In reply to comment #263)
As for the patch, I think I have found a theoretical problem. Your X_speex_resampler_get_required_input_length() function can cause the resampler to return less than count samples because it rounds down. So, for 11025 -> 48000 Hz conversion, if count happens to be 1 for some odd reason, it can return 0 and nothing will be converted, leading to an infinite loop. So please make absolutely sure that speex cannot request 0 samples. And for safety and for simplification of the accounting, I'd even make it always return count samples by supplying enough input, even if the secondary stream ends here (i.e. pad it with zeros in this case instead of truncating total_length).
Padding with zeroes instead of truncating is a great idea. I'll do that in the next patchset.
Since Speex converts until either the input or the output buffer space runs out, for the reason above, I think it would be better to add a ceil() to the X_speex_resampler_get_required_input_length() function. Or rewrite it using fixed-point math to avoid the rounding issues, as shown below (completely untested, and the standard coffee warning applies):
spx_uint32_t X_speex_resampler_get_required_input_length(SpeexResamplerState *st, spx_uint32_t out_len) { spx_uint32_t ret = out_len * st->num_rate; ret = (ret + st->den_rate - 1) / st->den_rate; return ret + st->filt_len; }
Note that I have changed in_rate and out_rate with num_rate and den_rate, because speex might choose to round the resampling factor to a more convenient fraction.
Makes sense. I'll use your code.
Regarding your statement about GTA:SA and Darwinia calling _SetFrequency() with unusual values, I have an additional question: do they do that while the stream is playing?
Yes. Highway Patrol does as well, to simulate the car engine revving.
http://bugs.winehq.org/show_bug.cgi?id=14717
Andrew Eikum aeikum@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #38246|0 |1 is obsolete| |
--- Comment #265 from Andrew Eikum aeikum@codeweavers.com 2012-01-04 09:44:21 CST --- Created attachment 38261 --> http://bugs.winehq.org/attachment.cgi?id=38261 Speex resampler, ready to submit?
One more iteration. If this turns up no problems, we should submit this ASAP. We can start with Alexander's patches, and I'll follow after those get in.
I added two missing newlines to FIXME and ERR statements in patches 1 and 2. The missing newline in patch 2 was causing a crash in the Evil Genius demo.
I also used your code from the comment above, and added in zero-padding past the end of non-looping buffers.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #266 from Raymond superquad.vortex2@gmail.com 2012-01-05 01:23:10 CST --- (In reply to comment #264)
(In reply to comment #262)
Regarding your statement about GTA:SA and Darwinia calling _SetFrequency() with unusual values, I have an additional question: do they do that while the stream is playing?
Yes. Highway Patrol does as well, to simulate the car engine revving.
sdk example is adjustsound which has a slider to allow user to change the pitch of a playing sound
Do you mean un-usunal rate is 0 which is DSBFREQUENCY_ORIGINAL
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #267 from Alexander E. Patrakov patrakov@gmail.com 2012-01-05 03:06:02 CST --- Sorry, I still cannot accept this patch, for the reason of clicking in the left channel, using the same testcase (11025 Hz stereo wav file in foobar2000).
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #268 from Alexander E. Patrakov patrakov@gmail.com 2012-01-05 03:34:05 CST --- So it still converts incomplete buffers (needed to convert 512 samples, converted 511) and from time to time even wants to convert 4 samples at a time. This even happens if I force the total_frames variable to an insanely high number like 4096 - so the fix proposed by me was in fact ineffective.
Still, I wonder why the bug affects only the left channel.
http://bugs.winehq.org/show_bug.cgi?id=14717
Roman Tetelman kevlarman+winebugs@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |kevlarman+winebugs@gmail.co | |m
--- Comment #269 from Roman Tetelman kevlarman+winebugs@gmail.com 2012-01-06 01:11:36 CST --- (In reply to comment #268)
So it still converts incomplete buffers (needed to convert 512 samples, converted 511) and from time to time even wants to convert 4 samples at a time. This even happens if I force the total_frames variable to an insanely high number like 4096 - so the fix proposed by me was in fact ineffective.
Still, I wonder why the bug affects only the left channel.
fwiw, the clicking was already there in some cases without the patches (alice:madness returns will do nothing but click in the left channel once the game starts in plain 1.3.35, the patchset seems to actually make it happen less) though the patch seems to trigger it in programs that didn't do it before.
http://bugs.winehq.org/show_bug.cgi?id=14717
Alexander E. Patrakov patrakov@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #37659|0 |1 is obsolete| |
--- Comment #270 from Alexander E. Patrakov patrakov@gmail.com 2012-01-06 05:09:09 CST --- Created attachment 38279 --> http://bugs.winehq.org/attachment.cgi?id=38279 Optimized version of my patch
In the meantime, I have optimized my non-speex-based resampler patchset. So now, it has the quality of Windows XP resampler at its default (best) settings, and the CPU usage is comparable with the speex-based resampler. And no clicks in foobar2000. And even some potential for further optimization if it is allowed to obey the frequency ratio slightly inexactly (but this can lead to "plays well for 20 minutes then rebuffers" regressions for various internet radio programs).
As for the old clicks in the left channel, maybe they happen because the app thinks that the safe-to-overwrite position is in fact where we are collecting input samples. Just a guess, though, this may or may not be right.
http://bugs.winehq.org/show_bug.cgi?id=14717
Alexander E. Patrakov patrakov@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #38279|0 |1 is obsolete| |
--- Comment #271 from Alexander E. Patrakov patrakov@gmail.com 2012-01-16 11:22:30 CST --- Created attachment 38385 --> http://bugs.winehq.org/attachment.cgi?id=38385 Optimized version of my patch, with some cleanups
Andrew Eikum spotted some areas where a cleanup is needed. I did that.
1. The 0006 and 0007 patches needlessly changed the return type of cp_fields back and forth.
2. There were whitespace issues with the 0007 patch.
3. firgen.c compiled with warnings.
This patch doesn't have other changes and thus brings no speed improvement over patches already submitted.
http://bugs.winehq.org/show_bug.cgi?id=14717
Fael Mc fael_mc@msn.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |fael_mc@msn.com
--- Comment #272 from Fael Mc fael_mc@msn.com 2012-02-14 13:37:08 CST --- Thanks again Andrew Eikum
I'll compile the wine with the patch, and post the results here.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #273 from Alexander E. Patrakov patrakov@gmail.com 2012-02-14 22:39:16 CST --- Note that the attached patch doesn't apply cleanly against the latest version in git. I have an updated patch at home, will post it here when I get home (i.e. in three hours or so).
http://bugs.winehq.org/show_bug.cgi?id=14717
Alexander E. Patrakov patrakov@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #38385|0 |1 is obsolete| |
--- Comment #274 from Alexander E. Patrakov patrakov@gmail.com 2012-02-15 02:03:57 CST --- Created attachment 38883 --> http://bugs.winehq.org/attachment.cgi?id=38883 Updated patch
Updated to build against 1.4-rc2
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #275 from Fael Mc fael_mc@msn.com 2012-02-15 13:49:51 CST --- thanks for the patch Alexander the new version is working very well with wine 1.4-rc3 I can now play PES 2012 on wine, no problem with sound
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #276 from Andrew Eikum aeikum@codeweavers.com 2012-03-07 13:34:03 CST --- Created attachment 39233 --> http://bugs.winehq.org/attachment.cgi?id=39233 Alexander's resampler with simultaneous access fixed
(In reply to comment #274)
Created attachment 38883 [details] Updated patch
Updated to build against 1.4-rc2
Okay, great. Now that 1.4 is out, let's finally get this in. I took your latest patchset (plus modification below) and ran every patch through a series of tests. Specifically, every patch passes the dsound tests and plays the audio in the following games correctly on my Pulse/ALSA machine: Dracula (demo), Darwinia (demo), Alice (demo), Marble Arena 1, Highway Pursuit, Age of Kings (demo), and foobar2000 (both with and without resampling). I'm going to move on to testing with OSS and CoreAudio, but I don't expect problems.
I noticed you moved get_current_sample() from the last patch to the second-to-last patch. That way, we never return less than a full chunk of data. I agree with that change, it makes the code a bit simpler.
While testing the second-to-last patch, I got this FIXME a number of times: IDirectSoundBufferImpl_GetCurrentPosition Bad play position. playpos: 33440, buflen: 32768 It's caused by simultaneous access to the buffer structure in _GetCurrentPosition and in the mixer thread. We increment the buffer's sec_mixpos directly, and "fix" its overflow later. _GetCurrentPosition manages to sneak in before we fix it and dumps that FIXME.
Unfortunately, this isn't harmless. Consider a non-looping buffer that has overflowed. The _GetCurrentPosition implementation will report it as partway into the second loop, when really it should report 0.
To fix this, I kept the local copy of sec_mixpos in cp_fields, and do a single write to sec_mixpos at the end of the function. This only affects the last two patches.
Really, dsound needs a thorough thread safety review, but now isn't the time for that.
You own the first four patches so, if you're still happy with the patchset, you can get those sent off at your leisure (no hurry, the first week or two after release tends to be a zoo anyway).
I'll then get the next two in.
Then we can work on getting the resampler patch itself in. firgen.c (both the idea and the code itself) is a little funky and might require some tweaking to be acceptable in Wine.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #277 from Jörg Höhle hoehle@users.sourceforge.net 2012-03-08 04:18:39 CST ---
We increment the buffer's sec_mixpos directly, and "fix" its overflow later. _GetCurrentPosition manages to sneak in before we fix it
Choices:
1. Choose another design. E.g. the lock-less winealsa.drv design sketched in bug #29531, comment #7 solely shares held_frames among threads, where InterlockedExchangeAdd suffices, there's no need for a modulo operation.
2. What about: see = This->sec_mixpos; repeat { old = see; new = (see + added) % size; see = InterlockedCompareExchange(&This->sec_mixpos,new,old); } until (see == old);
You can turn that into an inline InterlockedModuloAdd (except C89 does not say what exactly happens when see+added < 0, unlike C99 IIRC). You want to guarantee that 0 <= new < size, not face new < 0.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #278 from Andrew Eikum aeikum@codeweavers.com 2012-04-03 14:34:18 CDT --- Ping
Had a chance to work on this? We can start by getting the first couple of patches in now, and work our way towards the resampler patch.
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #279 from Alexander E. Patrakov patrakov@gmail.com 2012-04-04 03:27:51 CDT --- I had no chance to work on this. Sorry.
http://bugs.winehq.org/show_bug.cgi?id=14717
Alexander E. Patrakov patrakov@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #38883|0 |1 is obsolete| |
--- Comment #280 from Alexander E. Patrakov patrakov@gmail.com 2012-04-22 09:58:23 CDT --- Created attachment 39886 --> http://bugs.winehq.org/attachment.cgi?id=39886 Updated patch
This new patch has the following changes:
1) The big generated fir.h file is no longer in the patch. Instead, it is created as a part of the build process. 2) The firgen program has been renamed to make_fir, rewritten in Perl and placed in the tools directory. This was necessary because I could not force wine's build system to add -lm while compiling this C program. 3) get_current_sample() has been moved to patch 0005, because otherwise rounding errors can lead to an out-of-bounds array access.
Note that I am not happy with the last patch. The cp_fields_resample() function can be optimized and the stupid "+1 just in case" logic removed.
http://bugs.winehq.org/show_bug.cgi?id=14717
Alexander E. Patrakov patrakov@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #39886|0 |1 is obsolete| |
--- Comment #281 from Alexander E. Patrakov patrakov@gmail.com 2012-04-22 12:16:27 CDT --- Created attachment 39889 --> http://bugs.winehq.org/attachment.cgi?id=39889 Updated patch
Improved floating-point calculations, removed the "+1 just in case" logic because it is no longer needed, and also found & fixed one more off-by-one error to the safe side.
Now I am reasonably happy with the patch.
Still, it has only received testing by my ears, not by the "tee" ALSA plugin - so not scientific enough. More scientific tests will follow, and if all goes well, the final version will differ only in the comments.
http://bugs.winehq.org/show_bug.cgi?id=14717
Jerome Leclanche adys.wh@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #33202|0 |1 is obsolete| |
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #282 from Andrew Eikum aeikum@codeweavers.com 2012-04-23 09:07:15 CDT --- Excellent, Alexander! I'm going to take some time to carefully test each patch today. I'll let you know my results.
I did find one problem. In "put24" in patch 2,
+ t = lrintf(value * 0x800000);
should be
+ t = lrintf(value * 0x80000000);
to mirror the coefficient in "get24".
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #283 from Andrew Eikum aeikum@codeweavers.com 2012-04-23 11:38:52 CDT --- I ran each patch through the dsound tests and against a handful of games that use dsound in different ways. I didn't run into any issues.
In patch 5, the first argument to get_current_sample should be const:
-static inline float get_current_sample(IDirectSoundBufferImpl *dsb, +static inline float get_current_sample(const IDirectSoundBufferImpl *dsb,
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #284 from Alexander E. Patrakov patrakov@gmail.com 2012-04-23 11:58:19 CDT --- (In reply to comment #282)
I did find one problem. In "put24" in patch 2,
t = lrintf(value * 0x800000);should be
t = lrintf(value * 0x80000000);to mirror the coefficient in "get24".
This indeed looks like a bug. However, the fix is wrong. First, 0x80000000U just to tell gcc that it is a large positive number, not the most negative one. Second, the overflow-case values above that line should be also modified.
Or take a simpler approach:
+ buf[0] = t & 0xFF; + buf[1] = (t >> 8) & 0xFF; + buf[2] = (t >> 16) & 0xFF;
This does make the numbers inconsistent, but note that the shift values are different, too.
Also I think this piece of code needs unit tests. Namely, that three bytes (0xff, 0xff, 0x7f) convert to something like 0.9999998807907104 and back, that 1.01 gets clipped and converted to (0xff, 0xff, 0x7f), and that (0x00, 0x00, 0x80) means -1.0
http://bugs.winehq.org/show_bug.cgi?id=14717
--- Comment #285 from Alexander E. Patrakov patrakov@gmail.com 2012-04-23 11:59:56 CDT --- (In reply to comment #283)
-static inline float get_current_sample(IDirectSoundBufferImpl *dsb, +static inline float get_current_sample(const IDirectSoundBufferImpl *dsb,
I will resend the patches tomorrow, taking your suggestions into account.
http://bugs.winehq.org/show_bug.cgi?id=14717
Alexander E. Patrakov patrakov@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #39889|0 |1 is obsolete| |
--- Comment #286 from Alexander E. Patrakov patrakov@gmail.com 2012-05-01 10:14:36 CDT --- Created attachment 39987 --> http://bugs.winehq.org/attachment.cgi?id=39987 Updated patch
I have fixed the bug using the approach that you suggested. Will now send the first 4 patches to wine-patches.
http://bugs.winehq.org/show_bug.cgi?id=14717
joaopa jeremielapuree@yahoo.fr changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |jeremielapuree@yahoo.fr
--- Comment #287 from joaopa jeremielapuree@yahoo.fr 2012-05-08 14:32:44 CDT --- Patch commited. Bug should be fixed now.
http://bugs.winehq.org/show_bug.cgi?id=14717
Austin English austinenglish@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Fixed by SHA1| |275dfb83f2371a476b615b26046 | |5afb3cdf67f32 Status|NEW |RESOLVED Resolution| |FIXED
--- Comment #288 from Austin English austinenglish@gmail.com 2012-05-08 16:11:38 CDT --- (In reply to comment #287)
Patch commited. Bug should be fixed now.
http://source.winehq.org/git/wine.git/commitdiff/275dfb83f2371a476b615b26046...
http://bugs.winehq.org/show_bug.cgi?id=14717
Julian Rüger jr98@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |jr98@gmx.net
http://bugs.winehq.org/show_bug.cgi?id=14717
Alexandre Julliard julliard@winehq.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED
--- Comment #289 from Alexandre Julliard julliard@winehq.org 2012-05-11 13:22:17 CDT --- Closing bugs fixed in 1.5.4.
-
wine-bugs＠winehq.org