[Bug 14717] resampled sound is horrible

http://bugs.winehq.org/show_bug.cgi?id=14717

--- Comment #257 from Alexander E. Patrakov  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  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