| Summary: | [Patch] Crash fixes and other improvements for surround sound in SDL_AudioCVT | ||
|---|---|---|---|
| Product: | SDL | Reporter: | Solra Bizna <solrabizna> |
| Component: | audio | Assignee: | Ryan C. Gordon <icculus> |
| Status: | RESOLVED FIXED | QA Contact: | Sam Lantinga <slouken> |
| Severity: | blocker | ||
| Priority: | P2 | Keywords: | target-2.0.6 |
| Version: | HG 2.0 | ||
| Hardware: | All | ||
| OS: | All | ||
| Attachments: |
The patch promised in the summary and explained in the description
A version of the first patch that contains one fewer misunderstanding |
||
Created attachment 2821 [details]
A version of the first patch that contains one fewer misunderstanding
Argh... when I wrote the previous patch, I forgot to account for the fact that correlated signals combine differently than uncorrelated ones. When distributing a /correlated/ signal across two speakers, multiplying by 0.5 is the right thing to do, rather than sqrt(0.5). It will still comb slightly, but without inter-frame state we couldn't do anything about that if we wanted to.
This patch looks good to me; some good improvements and it catches a few problems I caused, too. :) One note: we don't officially have 7.1 support yet. Those converters may still be included, though, as we will soon. I'll push this patch shortly, thanks! --ryan. (Sorry if you get a lot of copies of this email, we're touching dozens of bug reports right now.) Tagging a bunch of bugs as target-2.0.6. This means we're in the final stretch for an official SDL 2.0.6 release! These are the bugs we really want to fix before shipping if humanly possible. That being said, we don't promise to fix them because of this tag, we just want to make sure we don't forget to deal with them before we bless a final 2.0.6 release, and generally be organized about what we're aiming to ship. After some debate, we might just remove this tag again and deal with it for a later release. Hopefully you'll hear more about this bug soon. If you have more information (including "this got fixed at some point, nevermind"), we would love to have you come add more information to the bug report when you have a moment. Thanks! --ryan. This patch is now https://hg.libsdl.org/SDL/rev/4e12f78c2b0e, thanks! --ryan. |
Created attachment 2820 [details] The patch promised in the summary and explained in the description SDL 2.0.5's support for surround sound in SDL_AudioCVT is extremely dicey. A current Mercurial build has better support for the remaining common cases, but according to my tests is actually MORE dicey overall. For both, SDL_AudioCVT refuses to work on 7.1 audio at all. (This patch, or something like it, would be necessary before #248 could be closed.) This patch reworks upmixing and downmixing. Every combination of channels that SDL supports is now error-free, or at least free of the kind of errors that can be caught by exhaustive (but simple) automated testing and by valgrind. Major changes, roughly in order of appearance: - Use float math everywhere, instead of promoting to double and casting back all the time. - Conserve sound energy when downmixing any channel into two other channels. - Add a QuadToStereo filter. (The previous technique of reusing StereoToMono never worked, since it assumed an incorrect channel layout for 4.0.) - Add a 71to51 filter. This removes just under half of the cases the previous code would silently break in. - Add a QuadTo51 filter. More silent breakage fixed. - Add a 51to71 filter, removing another almost-half of the silently broken cases. - Add 8 to the list of values SDL_SupportedChannelCount will accept. - Change SDL_BuildAudioCVT's channel-related logic to handle every case, and to actually fail if it fails instead of silently corrupting sound data and/or crashing down the road. Remaining issues: - SDL provides no way to detect what the user's actual speaker setup is. As far as I know, this is not fixable; this feature is missing or untrustworthy on every OS I know of. - I don't know if the code to upmix a channel from two other channels is correct. It seems like it may reduce correlation as much as we can expect from a design as stateless as this one... but it will also cause clipping. I took the code that was already present for making a FC channel, and applied it to SL and SR when upmixing to 7.1. (This issue affects upmixing to 5.1 or 7.1.) - No state between calls means no real filtering, which means we can't decorrelate properly or synthesize an LFE channel. Fortunately, while working on this patch I learned that you /shouldn't/ synthesize an LFE channel when upmixing. - It can allocate a little too much memory when upmixing from a 4.0 or 5.1 original. It's a rare enough case (even compared to other conversions involving surround) that working around this wouldn't be worth the extra code complexity. It does allocate the correct amount of memory for any other combination of source and destination channel counts. - Conversion will still segfault if resampling to a >4x higher sample rate (e.g. when resampling 11025Hz audio to 48kHz) /and/ the destination has 6 or 8 channels. My patch did not introduce this crash, and (since the crash doesn't occur in the up-/downmixing code) makes no attempt to fix it. If I have time, I'll file a bug report for this... unfortunately, I've already spent way more time on this whole thing than I probably should've. - Most importantly, I no longer have a surround setup myself, so I haven't performed a listen test. I'm 95% confident that my code will produce acceptable results in nearly all circumstances (other than the clipping issue I mentioned above), but it might be prudent to have a few listen tests. Even without a listen test, I can say with certainty that there are numerous cases where, without my patch, SDL will crash, produce wrong results, or simply fail. Ideally, when up- or downmixing, one will use a matrix designed specifically for that combination (e.g. Stereo directly to 7.1), without intermediate steps. I chose to continue the existing approach, and use two or three chained filters instead. This saves a lot of code. The results should be "good enough". Achieving "perfect" up-/downmixing generically at the library level would be impossible anyway, since "perfect" depends on genre, medium, goals, taste, and sometimes even precise details about the end-user's setup. In my exhaustive automatic tests, I didn't run into the maximum filter count with any combination of rate, channel count, or format. The only automatically-detectable error still left is the previously-mentioned resampling segfault.