MuseScore icon indicating copy to clipboard operation
MuseScore copied to clipboard

WASAPI: use closest supported mix format and convert if the WASAPI device has more than two channels

Open cbjeukendrup opened this issue 1 year ago • 1 comments

Builds on: #15633

and thus resolves: one of the crashes on launch and additionally resolves: playback problems

Should resolve: https://github.com/musescore/MuseScore/issues/15685 Should resolve: https://github.com/musescore/MuseScore/issues/15235 Should resolve: https://github.com/musescore/MuseScore/issues/15029 (not 100% sure because it's difficult to tell from those issue descriptions whether it is indeed the same crash that this PR solves or a different one, for example caused by a VST)

And these musescore.org issues: https://musescore.org/en/node/338043 https://musescore.org/en/node/338149 https://musescore.org/en/node/338271 https://musescore.org/en/node/338378 https://musescore.org/nl/node/339691 https://musescore.org/en/node/340458 https://musescore.org/en/node/340562 https://musescore.org/en/node/341307 https://musescore.org/en/node/341487 and probably there are more reports of the same issue


So, after the crash was solved by #15633 by adding proper error handling, it was time to fix the error itself. Turns out that we specify a wave format to the WASAPI device without checking if that format is actually supported by that device. In particular, WASAPI seems not very happy if we specify that we want two channels while the device has more.

What I try to do in this PR is that we check if the format is supported, and if not, use the closest supported format. However, that gives one more problem: because the device needs samples for >2 channels and we only provide samples for 2 channels, the playback is much too fast and therefore also high-pitched; similar to https://github.com/musescore/MuseScore/issues/14921, which seems like it wasn't really fixed yet, so maybe that will be fixed by this PR too.

My solution to that problem is a bit of a "proof of concept" right now; indeed it fixes the problem, but it is a bit ugly. This problem will need some more thought. Deeper in the audio module, we seem to rely quite a lot on the assumption that everything has only two channels. For example: mu::audio::dsp::balanceGain, the hardcoded 2 in AudioModule::setupAudioDriver, and my comment at https://github.com/musescore/MuseScore/pull/14684#discussion_r1030872639. But because everything has a method called audioChannelsCount(), this assumption is not very explicit. Not sure whether this is ideal. And apparently there is also no guarantee that the driver always supports exactly two channels, so we need to do a conversion somewhere. Right now, I have added this in the Wasapi driver itself, but it feels not great. Probably we should better do that on a different level.

cbjeukendrup avatar Jan 03 '23 18:01 cbjeukendrup

@vpereverzev It would be great if we could include a version of this fix in the next patch release :)

cbjeukendrup avatar Jan 15 '23 17:01 cbjeukendrup

Wouldn't it need to get out its Draft state?

Jojo-Schmitz avatar Jan 27 '23 09:01 Jojo-Schmitz

@DmitryArefiev it seems that #15029 and #15235 have been re-tested and marked as "Done - Tested Again". I can also verify that #15685 has been fixed. I think this only leaves #15637 to re-test (which I can't do because I'm not running Windows 10). Is this all that's now blocking us from being able to mark the present issue (#15698) as "Done - Tested Again"?

bkunda avatar Mar 10 '23 11:03 bkunda

I can test on Win10. I can't promise it will be today, but it will be before Sunday.

L0uisc avatar Mar 10 '23 11:03 L0uisc

I can test on Win10. I can't promise it will be today, but it will be before Sunday.

@L0uisc Oh, thanks!

DmitryArefiev avatar Mar 10 '23 12:03 DmitryArefiev

@DmitryArefiev it seems that #15029 and #15235 have been re-tested and marked as "Done - Tested Again". I can also verify that #15685 has been fixed. I think this only leaves #15637 to re-test (which I can't do because I'm not running Windows 10). Is this all that's now blocking us from being able to mark the present issue (#15698) as "Done - Tested Again"?

@bkunda I think we can move easily it to Done - Tested Again and reopen a separate bugs attached to the PR if they occur again.

DmitryArefiev avatar Mar 10 '23 12:03 DmitryArefiev