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

Fix default device not respecting ducking rules bug and handle leak #33

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

Conversation

SpriteOvO
Copy link

Issue: telegramdesktop/tdesktop#8192

For my network reasons I was unable to compile tdesktop's dependency library FFmpeg, so I referenced libtgvoip and wrote a minimal test example that successfully reproduced the problem.

In this line:
res=enumerator->GetDefaultAudioEndpoint(eRender, eCommunications, &device);

The problem only occurs in devices returned via GetDefaultAudioEndpoint with eCommunications. If replace eCommunications with eConsole or eMultimedia, the problem will not occur.

I think it's a Windows bug, but we can't fix it that way because the msdn docs say that the role argument may affect the returned default device in the future.

Link
When the user changes the default rendering or capture device, the system assigns all three device roles (eConsole, eMultimedia, and eCommunications) to that device.
Thus, GetDefaultAudioEndpoint always selects the default rendering or capture device, regardless of which role is indicated by the role parameter.
In a future version of Windows, the user interface might enable the user to assign individual roles to different devices.
In that case, the selection of a rendering or capture device by GetDefaultAudioEndpoint might depend on the role parameter.
Thus, the behavior of an audio application developed to run in Windows Vista might change when run in a future version of Windows.

But there is a workaround to fix it, that we just get the name of the default device from GetDefaultAudioEndpoint, and then call GetDevice with the name to get the device.

Some references:
https://social.microsoft.com/Forums/SECURITY/zh-CN/1b500f1e-744b-41d7-9c20-66ed83bda055/wasapi-stream-with-default-communictaions-device-does-not-trigger-ducking-for-other-applications?forum=windowspro-audiodevelopment
https://chromium.googlesource.com/chromium/src/media/+/d3d352cbce26b68596104356a5420d65544fe903%5E!/
https://github.com/mumble-voip/mumble/blob/54e313481d2fe2fe36fbd30f55c5f055cbbee615/src/mumble/WASAPI.cpp#L335-L359

Minimal test example:
https://github.com/SpriteOvO/TgCallBugTest

BTW, there is a handle leak in the device enumeration, that is, the device Release should be called after IMMDeviceCollection::Item.

Link
Through this method, the caller obtains a counted reference to the interface.
The caller is responsible for releasing the interface, when it is no longer needed, by calling the interface's Release method.

Finally, since I cannot compile tdesktop, the changes in this PR are untested. I only tested successfully with my minimal test example. But I'm happy to help test the compiled files.

@@ -216,8 +216,20 @@ void AudioOutputWASAPI::ActuallySetCurrentDevice(std::string deviceID){

if(deviceID=="default"){
isDefaultDevice=true;
res=enumerator->GetDefaultAudioEndpoint(eRender, eCommunications, &device);
IMMDevice* temp_device;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks strange since all other variables are in camelCase

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Y

@ilya-fedin
Copy link

Finally, since I cannot compile tdesktop, the changes in this PR are untested.

If the problem is in GetDefaultAudioEndpoint, then just selecting the device itself instead of "default" in settings should solve the issue as well, does that solve it (i guess you need a version before video calls were added for the test)?

@ilya-fedin
Copy link

ilya-fedin commented Sep 10, 2020

If replace eCommunications with eConsole or eMultimedia, the problem will not occur.

Of course it won't occur since the stream is not a communication stream anymore and, therefore, call reducing feature is not called by the os.

@ilya-fedin
Copy link

Looks like you just disabled the ducking feature entirely, didn't you?

@SpriteOvO
Copy link
Author

SpriteOvO commented Sep 11, 2020

If the problem is in GetDefaultAudioEndpoint, then just selecting the device itself instead of "default" in settings should solve the issue as well, does that solve it?

I think it could work.

Looks like you just disabled the ducking feature entirely, didn't you?

I tested it and ducking does not work. You're right.
But if the deviceID argument of ActuallySetCurrentDevice is not default, ducking won't work either.
Maybe a switch should be added to control ducking?

@SpriteOvO
Copy link
Author

SpriteOvO commented Sep 11, 2020

i guess you need a version before video calls were added for the test

I think the latest version of tdesktop is still using libtgvoip WASAPI.
For older OS it is using WaveAPI.

Because of this, and I found some informations about libtgvoip in the memory of tdesktop v2.3.1.

@ilya-fedin
Copy link

ilya-fedin commented Sep 11, 2020

I think the latest version of tdesktop is still using libtgvoip WASAPI.

No, it uses WASAPI via webrtc

But if the deviceID argument of ActuallySetCurrentDevice is not default, ducking won't work either.

Yeah, and I guess it is better to leave it as is, since a lot of users except ducking

Maybe a switch should be added to control ducking?

Oh, core developers are against adding new switches usually...

Copy link

@bokssssss bokssssss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SpriteOvO:tdesktop

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.

4 participants