Resolve setup issues for macOS (Venture 13.4): pyaudiowpatch -> pyaud…

[mlubbad]

Resolve setup issues for macOS (Venture 13.4): pyaudiowpatch -> pyaudio, context manager error, host API not found, KeyError, ..etc

[s0d3s]

Figere unum conteram aliud

Firstly, your PR will make it impossible to use for Windows users (does not support the CoreAudio API).

Secondly, even for MacOS users, your code will not work correctly.

As far as I know, PyAudio, in its default form, cannot record audio from speakers on MacOS via CoreAudio (probably outdated). And I would recommend using the Jack API interface for this purpose.

How to fix

If you just want to avoid installation errors (rather than using this functionality), then it will be enough to wrap all pyaudiowpatch imports:

from

import pyaudiowpatch as pyaudio

to

try:
    import pyaudiowpatch as pyaudio
except ImportError:
    import pyaudio

But if you want to use the functionality of recording sound from speakers, then you need to look for a separate solution, or rewrite\modify PyAudio.

In this case, you may also have to separate the implementation for Windows and Linux\MacOS

[s0d3s]

I checked again, and came to the same conclusion - your code does not contribute to recording sound from the speakers. Especially on macOS using Core Audio.

Are you aware of what the code you modify is responsible for? This piece is responsible for recording sound from the speakers (what you hear). But at the moment, PyAudio does not support this functionality on macOS, especially through Core Audio. So your code shouldn't even work.

Then what did you mean when you said that everything works for you? You said that the application works well through the microphone, is it? This is how it worked before your changes.

Please answer the following questions:

• What is the purpose of your pull request?
  • Add the ability to record from speakers to macOS?
  • Or just bypass the errors to be able to run the application?

I ask because your code does not contribute to either the first or the second.

[s0d3s]

I created a PR for your fork that adequately solves the problem in the second way(just bypass the errors).

It has been tested on both Windows and MacOS. All the nuances are described in the PR itself.

As far as I understand, you were not going to solve the problem with recording from the speakers. So I suggest that you accept my PR and we can move on to consider the merge.

[mlubbad]

You are right, done from my end.

I’ll try to support the right function (recording from the speaker through the microphone of the device) soon.

I’ll inform you once, I get success.

For now, I just accept and merged your PR, could we proceed?

Thanks 🌹

[s0d3s]

@SevaSk Hi🖐, I've made a change that solves the OP's problem: bypass startup errors on macOS..

I checked the work on Windows and macOS - *everything works. (Need someone to check too)

IMPORTANT: This is not a solution to the problem of recording sound from speakers on macOS, but only a workaround so that the application can work under this OS through a microphone.
❗ *It is also worth knowing that the speaker recording stream (on macOS) completely duplicates the microphone stream. This was the easiest way to get around the bugs without having to rewrite a big part of your code.

Not sure about the relative usefulness of this PR, but it will buy time before the problem of multi-platform speaker recording is fully resolved.

[s0d3s]

@mlubbad Yes, we will find out the opinion of the maintainer and probably proceed to merge.

[SevaSk]

Hey @mlubbad and @s0d3s! thank for contributing also thanks @s0d3s for creating PyAudioWPatch! I left some minor comment about design also some comments about the intended behavior. Like you mentioned, the PR not too useful. But if this PR was written as error logging and recovery when we hit a known limitation, for example defaulting to ignoring speaker if we cant record from it. That would be valuable.

[SevaSk]

1. Would like a design where we only query the OS once, and inject the OS dependency into the generic code.

[s0d3s]

I agree with you that a structure like this:

if os.name != "nt":
    import pyaudiowpatch as pyaudio
else:
    import pyaudio

would look better. But since now the main platform is Windows, I thought that doing a check every time on an OS that is not fully supported would be redundant. And I thought the approach I chose was the best.

However, I don't mind changing that.

[SevaSk]

Here is what I mean, dependency injection seems appropriate here.

[s0d3s]

Dependency injection at the class level is not the best idea, since their scope will only be at the class declaration level (but not even at the class level). Minimal reproducible example:

class A:
    import os
    print("os_0", os)
    
    def __init__(self):
        print("os_1", os)
        
A()
print("os_2", os)

So it's better to leave the imports at the module level.

[SevaSk]

Sry what I meant was I would like the code to be extendable where we would just have a function at the beginning
SetUpForOS() which handles all the set up, meaning that we wouldn't need to query for the OS 4 times all over the place. Dependency injection is just one technique we could use to get this code generic, which I thought would help here since we only need to override specific functionally for the classes depending on OS.

[s0d3s]

You are talking about monkey patching, but putting it in a separate function is inappropriate. This will confuse the code and complicate its further maintenance. I suggest a more elegant option: group methods in one place, and replace them for a specific OS using a decorator.

MRE for this:

import sys


def os_dependent(target_class):
    current_os = sys.platform
    name_prefix = "_os_"
    name_prefix_len = len(name_prefix)
    
    for os_dep_name in (method_name for method_name in dir(target_class)
                        if method_name.startswith(name_prefix)):
        method_platform_end_index = os_dep_name.find("_", name_prefix_len)
        
        if os_dep_name[name_prefix_len:method_platform_end_index] == current_os:
            # If this method is for the current system, replace.
            setattr(target_class, os_dep_name[method_platform_end_index+1:],
                    getattr(target_class, os_dep_name))
        
    return target_class


@os_dependent
class OSDependentClass:
    def _os_linux_method(self):
        print("_os_linux_method")
        
    def _os_darwin_method(self):
        print("_os_darwin_method")
        
    def method(self):
        # win32 method
        print("method")
        
    def _os_win32_method1(self):
        ...
        
    def method1(self):
        ...
        
print(OSDependentClass.method.__name__)
print(OSDependentClass.method1.__name__)

Or it could be rewritten to apply the decorator not on the class but on os-dependent methods (should be faster, but less convenient)

pros:

• good readability
• method signatures are not hidden
• structure saving
• comfortably
• all replacements are made once at the first import

cons:

• takes a little longer (in processor cycles) than manual "if-else"

It's not just a one-time solution. This won't be the last time you'll need to write OS-specific code. It would be appropriate to put the decorator in a separate file, and use it in other places when necessary.

And to perform OS-dependent actions at the module level, need to use the if-else block(or leave it as it is now - try-catch block). It will be also more appropriate than a standalone function.

[SevaSk]

Don't think its appropriate to duplicate default mic here. Might be better to just have source as None and extend code to allow none sources where we do nothing with that recorder...not sure

[s0d3s]

This is what I originally wanted to do, BUT for this it will be necessary to make a number of changes to the main.py, AudioTranscriber.py and possibly in other places, since the current logic does not provide for such interventions.

Again, if you think this is a necessary change, we can discuss it. I thought that rewriting a significant part of the code to create a temporary (until the issue with the speakers is resolved) patch is unnecessary.

[SevaSk]

Don't think a rewrite is necessary, more of a extension of the code where we also allow no op recorder would be fine right? Only thing is AudioTranscriber init should be able to handle none source. Hopefully code is written where supporting MacOS with no op recording can be extended easily again to supporting MacOS with specific mac recording.

[mlubbad]

@SevaSk Any update here?

[s0d3s]

It's all demagogy and endless dialogue. If you agree with my last comment, then just write what you think is necessary for this PR and I will implement it in any form you want. After all, this is your repository😉