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

get_microphone(), get_speaker(): Improved comparison between the call parameter "id" and existing device names #147

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

Conversation

adeuring
Copy link

This PR fixes the problems mentioned in #146 , "Problems with the "fuzzy matcher" in get_microphone().

The changes are a bit longer than I expected at first, but anyway:

I moved the functions _match_device() from the modules coreaudio and mediafoundation into a new module utils so that the same implementation can be used for all platforms. pulseaudio has of course a similar change to use utils.match_device().

Changes to match_device():

  1. I added another name lookup in match_device(): Before the function checks if the call parameter id is a substring of one of the device names, it checks if the call parameter identical with any of the device names. While I also added your suggestion to separate real and loopback devices in name lookups (see below), there still is at least in theory still a corner case without this "name identity lookup": A loopback device could have a name that is the substring of a real microphone. Admitted, not very likely, but you never know ;) If only a substring lookup for the loopback device is done where real devices have precedence, the loopback device would not be returned. (See also the diff of test_soundcard, line 229. That test will fail without the additional "full name check".)
  2. Symbols with a special meaning in regexes are escaped if they appear in the call parameter `id* . (I'd appreciate a double-check if I got all relevant symbols when I read https://docs.python.org/3/library/re.html#regular-expression-syntax ...)

Final note: I am not too happy with the tests I wrote: I think it would be nice to have some device names and IDs from Windows and MacOS too in the tests. Problem, as already mentioned in #146 : I have neither a Mac nor a machine with a Windows installation. But I'm happy to add device names from MacOS/Windows if I get some examples.

Functions _match_device()/_match_soundcard() in the modules
soundcard.coreaudio, soundcard.mediafoundation, soundcard.pulseaudio
replaced with soundcard.utils.match_device().
- Distinguish between real devices and loopback devices. Try a lookup
  for real devices first; when one of them matches, return it before
  trying to find a matching loopback device.
- When a device name for a loopback device is also a substring of the name
  of a real device, the loopback device with the completely matching name
  is returned.
- Symbols with a special meaning in regular expressions are escaped when
  pattern for regex matching is built.
- Call of re.match() replaced with re.search() so that the "search parameter"
  passed to match_device does not have to match the start of a device name.

2. Obsolete "import re" removed from modules soundcard.coreaudio,
soundcard.mediafoundation, soundcard.pulseaudio.
soundcard/utils.py Outdated Show resolved Hide resolved
soundcard/utils.py Outdated Show resolved Hide resolved
@bastibe
Copy link
Owner

bastibe commented May 20, 2022

That's a great addition, thank you! And you factored out the common code as well, and added some platform-independent tests! Thank you so much!

Here are pairs of device IDs, device names, and isloopback on my Windows box:

Mics:

{0.0.0.00000000}.{075a16c1-576e-419e-84c1-70d03e0d6276} BenQ PD2700U (Intel(R) Display-Audio) True
{0.0.0.00000000}.{27473622-d168-4a9c-87c5-230c148c09c9} Lautsprecher (4- Samson GoMic) True
{0.0.0.00000000}.{f08702cd-ee32-4c95-ac85-ff21a9d4d8ec} Lautsprecher (Realtek(R) Audio) True
{0.0.1.00000000}.{041738dd-f5c5-485e-a5da-c5f34c35171f} Digitale Audioschnittstelle (Cam Link 4K) False
{0.0.1.00000000}.{be293184-38fd-41cb-9c50-27971414ac0b} Stereomix (Realtek(R) Audio) False
{0.0.1.00000000}.{c0c95239-3a6c-427b-a788-9caeb13a7f43} Mikrofonarray (Realtek(R) Audio) False
{0.0.1.00000000}.{e31e9915-d1b1-49dd-88bf-3cc232ec2a11} Mikrofon (4- Samson GoMic) False

Speakers:

{0.0.0.00000000}.{075a16c1-576e-419e-84c1-70d03e0d6276} BenQ PD2700U (Intel(R) Display-Audio)
{0.0.0.00000000}.{27473622-d168-4a9c-87c5-230c148c09c9} Lautsprecher (4- Samson GoMic)
{0.0.0.00000000}.{f08702cd-ee32-4c95-ac85-ff21a9d4d8ec} Lautsprecher (Realtek(R) Audio)

@bastibe
Copy link
Owner

bastibe commented May 20, 2022

And a few id/names from macOS:
Mics (macOS does not support loopbacks at the moment):

42 Built-in Microphone False
59 Samson GoMic False

Speakers:

49 Built-in Output
59 Samson GoMic

@bastibe
Copy link
Owner

bastibe commented May 20, 2022

I think I agree with your assessment that the two-stage search of no-loopbacks before loopbacks is actually overkill. Your first solution with the substring match is perfectly acceptable to me, and much more elegant.

I would probably add a note in the docstring of get_* to the effect of "For certain device names, the fuzzy search may be ambiguous. In that case, search by device ID (mic.id/speaker.id), or manually search all_microphones/all_speakers to find the device in question".

- check if an integer ID was provided before attempting to match strings.
- re.escape() now used to escape possible special symbols in ID strings
  instead of checks against a handcrafted list of symbols.
- tests: Fake devices with IDs and names as used in MacOS and Windows added.
@adeuring
Copy link
Author

Thanks for the review! I think I addressed all issues you mentioned. I kept though the "two-stage search" for now as it can in some cases make sense, I believe.

Also thanks for the examples of Windows and MacOS IDs/device names. Added to the test data.

@bastibe
Copy link
Owner

bastibe commented May 21, 2022

This looks good to me! I'll test this code on Windows and macOS next week, and then we can merge!

Thank you very much!

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.

None yet

2 participants