Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try to recover from AUDCLNT_E_DEVICE_INVALIDATED errors in wasapi #237

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

xxxbxxx
Copy link

@xxxbxxx xxxbxxx commented Oct 16, 2018

Instead of disconnecting the device and passing the error to the application, try to recover from the error in the mixing loop.

The error is triggered on some configurations when switching to fullscreen mode, or with some usb headsets. And it can take a few seconds to recover.
It also occurs when changing the sound card default sample rate in the windows configuration dialog.

@kcat
Copy link
Owner

kcat commented Oct 16, 2018

This looks like it can change the device format, which wouldn't be good since the rest of the device isn't updated to account for it. And the device shouldn't spontaneously change format on the app.

A better and more general option may be to have alcResetDeviceSOFT try to recover a disconnected device. That would change the format cleanly and expectantly as needed, without losing the allocated contexts or their state (though playing sources would be stopped and need to be restarted). Unfortunately I remember noticing a catch when I considered that previously a while ago, some possible race condition with the connected state, but I can't remember the specifics.

@xxxbxxx
Copy link
Author

xxxbxxx commented Oct 17, 2018

hi,

about the race condition: yeah I'm not too confident..
But my understanding is that the mixing thread is stopped before any changes like resetDevice, so I think it's ok.

If it helps you remembering the problem with alcResetDeviceSOFT() recovering a disconnected device, what you said at the time was:

The general idea sounds good, although that patch unfortunately has a potential race condition (an async alcGetIntegerv query can see the device as connected for a short time during the reset attempt, even though its actual status could always have been disconnected). You'd need to protect reading the Connected status with the list lock too, or more preferably, reset the Connected status only when the device is known to be working again, though that'll be trickier to avoid its own race condition.

So yes indeed we are lying during the recovery period, pretending all is fine. but that's kind of the point of the change. And it is at least coherent from the application perspective, since it stays connected the whole time until failure.

about the change of the device format:
But it's still applying the constraints given by the application (in ALCwasapiPlayback_resetProxy), so maybe it's not so bad if the application didn't care enough to enforce a specific format?

I know that changing the bitrate and bit depth (in the windows sound card configuration dialog) seem to work fine. I don't remeber what happens when changing the number of channels, I'll have to test.

@xxxbxxx
Copy link
Author

xxxbxxx commented Oct 19, 2018

I don't remember what happens when changing the number of channels, I'll have to test.

As expected, changing the speaker layout / number of channels doesn't work at all.

I'll try to add something like DEVICE_CHANNELS_REQUEST to keep the device in the same state or fail.

@kcat
Copy link
Owner

kcat commented Oct 19, 2018

I'll try to add something like DEVICE_CHANNELS_REQUEST to keep the device in the same state or fail.

That's not necessary for alcResetDeviceSOFT. It can reconfigure the device all it needs to for it to work (it only has to be on the same named device). Spontaneously changing the format at all (sample rate, update size, etc) isn't something that should happen since the app may have previously queried the ALC_FREQUENCY, ALC_REFRESH, etc, attributes expecting them to be accurate. The app should have to knowingly invoke something that can reconfigure the device before anything changes.

@xxxbxxx
Copy link
Author

xxxbxxx commented Oct 19, 2018

How about this approach:

  1. try to recover the device in the same format without notifying the application
  2. if that fails (no recover or not able to get the same format) report the devicelost
  3. the application can then try alcResetDeviceSOFT() (expecting the format may change) and restart the sources.

and maybe some hint at device creation "allow device frequency to change" to recover more cases?

@kcat
Copy link
Owner

kcat commented Oct 19, 2018

try to recover the device in the same format without notifying the application

I'd be curious how often this happens and would work. What would cause a device to disconnect that doesn't change the sample rate, period size, or channel configuration, and such that it can be recovered with little delay?

I think having alcResetDeviceSOFT recover a lost device is more pertinent, both in the general sense and with the given use-case. Having the WASAPI backend try to automatically recover can be looked at afterward if it's still beneficial.

maybe some hint at device creation "allow device frequency to change" to recover more cases?

Maybe. I'm still a bit leery on doing too much without the app realizing anything happened, but we'll see. There's a number of potential paths to take with regard to sample rate mismatches.

xxxbxxx and others added 2 commits October 30, 2018 09:47
…kend.

Instead of disconnecting the device and passing the error onto the application, try to recover from the error in the mixing loop.
@xxxbxxx xxxbxxx force-pushed the for-upstream branch 2 times, most recently from 0764d10 to 7a84d94 Compare October 30, 2018 21:17
@xxxbxxx
Copy link
Author

xxxbxxx commented Oct 30, 2018

So I've created an other pull request with an attempt to make alcResetDeviceSOFT() recover from device lost.

I've also changed this one, should you want to pull it:

  • actually enforce the DEVICE_FREQUENCY_REQUEST request (not sure if that is the intent of the feature?)
  • still try to recover without any change from the application if the frequency and channels can be recovered identically.

Now, please note that I've tried to reproduce the issues that prompted me to make the changes without success. It looks like since earlier this year, windows 10/the drivers have been updated..

  • no longer any errors when using hdmi audio and changing to fullscreen mode
  • no longer any erros with usb headsets.

So now the only wait I have to produce AUDCLNT_E_DEVICE_INVALIDATED errors is by manually changing the devices properties in the windows control panel.
so, of course when changing the bit rate or speaker layout this pull request is of no use
(the other one about alcResetDeviceSOFT() instead helps in that case)
But if I change special effects processing (like bass boost, reverb or equalizer filters - what's available in the control panel is dependent on the device driver) then I have AUDCLNT_E_DEVICE_INVALIDATED and recovery with the exact same frequencies and channel layouts.
And from what I recall that was also the case when I had the issues with fullscreen, but I'm not 100% sure.

@j4reporting
Copy link

j4reporting commented Dec 5, 2018

And from what I recall that was also the case when I had the issues with fullscreen, but I'm not 100% sure.

I can reproduce something similar, when I switch from desktop resolution to another fullscreen resolution. e.g. desktop is 2560x1440 and game using openal is configured to 1920x1080 fullscreen.
Audio output is not possible. Some applications can restart sound subsystem, but not all.
If you could build a patched win32 version of open-al then I could test these changes.

is this somehow related to this discussion (see answer from Ronald_Intel )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants