rbn42 / panon

An Audio Visualizer Widget in KDE Plasma
GNU General Public License v3.0
192 stars 30 forks source link

[BUG] soundcard bug or incorrect code? #48

Closed yuannan closed 3 years ago

yuannan commented 3 years ago

https://github.com/yuannan/panon/commit/b56cda2451baf45e8311d94800a2218822ab187d https://github.com/yuannan/panon/commit/fd297096ef5e160869ce0e46dfebb324c111b3dc

See my 2 pushes here. It should be the much faster and cleaner input for getting the current output but it doesn't work.

I'm not sure if it's just some super obvious bug but I'm pretty sure it's not my code but instead sound card being buggy.

yuannan commented 3 years ago

do you also have discord or some other service I can contact you on or do you prefer I just open git issues?

rbn42 commented 3 years ago

do you also have discord or some other service I can contact you on or do you prefer I just open git issues?

Sorry, I don't have one, please just open issues.

it doesn't work.

Well, I guess you need to replace line 121 in b56cda2451baf45e8311d94800a2218822ab187d

new_id = l[1]

with

new_id = int(l[1])
rbn42 commented 3 years ago

On a second thought, are you sure a speaker and its monitor share the same ID? Or we need to find another pactl command to list the monitors instead of the speakers.

yuannan commented 3 years ago

new_id = int(l[1])

using the python shell (just run python by itself)

mic = sc.get_microphone(1,False, False)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.9/site-packages/soundcard/pulseaudio.py", line 317, in get_microphone
    return _Microphone(id=_match_soundcard(id, microphones, include_loopback)['id'])
  File "/usr/lib/python3.9/site-packages/soundcard/pulseaudio.py", line 338, in _match_soundcard
    if id in name:
TypeError: 'in <string>' requires string as left operand, not int

This error happens. when trying to run it. This is why I think it's a bug. The docs say it takes int or string when it clearly doesn't.

On a second thought, are you sure a speaker and its monitor share the same ID? Or we need to find another pactl command to list the monitors instead of the speakers.

no, it works fine in the python console which leads to think it's my fault. 🤔

>>> s = subprocess.run(['pactl', 'list', 'sink-inputs', 'short'], capture_output=True).stdout
>>> l = s.decode(errors='ignore').split('\t')
>>> l
['137', '0', '8857', 'protocol-native.c', 'float32le 2ch 48000Hz\n']
>>> mic = sc.get_microphone(l[1],False, False)
>>> mic
<Loopback Monitor of Family 17h (Models 00h-0fh) HD Audio Controller Analog Stereo (2 channels)>
>>> mic = sc.get_microphone("alsa_output.pci-0000_0d_00.3.analog-stereo.monitor",False, False)
>>> mic
<Loopback Monitor of Family 17h (Models 00h-0fh) HD Audio Controller Analog Stereo (2 channels)>

I literally don't understand why it's not working and if it's me or the library. It works just fine in the python console. I spent all of last night hitting my head on this tiny improvement 😂 which will save a few function calls at most.

The reason I wanted to do this was because when calling the update function it's far too slow. Right now the update is only called when the buffer is empty. This only happens after around 5 second as the buffer first needs to emptied and get processed thru the decay and other functions first.

If you have it just update on every maincycle() loop it's instant and work just great. A much improved and instant version of "mixing all speakers".

But if you just have it update on every cycle it's far too slow to hit a stable frame rate. I can hit over 300FPS but with the update on every mainloop() cycle it hits ~40-60FPS. The thing is that the panon process in htop is only using 40-60% CPU at most.

In the function body of update_smart_device() the only codes that I can imagine is causing this hit is the subprocess call. Unless it's the 2 boolean evaluations for the if statements (I really don't think it's this).

rbn42 commented 3 years ago

If you have it just update on every maincycle() loop it's instant and work just great.

If this is your purpose, I think you are working on the wrong direction. Because one subprocess call per cycle is never cheap.

IMO, the right direction is to add a listener to PulseAudio. The listener will inform panon when a device is activated. I believe it is possible, but it is out of my knowledge.

rbn42 commented 3 years ago

This only happens after around 5 second as the buffer first needs to emptied and get processed thru the decay and other functions first.

Ah, but still, we can reduce the latency to 0.5 second, if you want, by verifying if latest_wave_data is empty.

if np.max(latest_wave_data)>0: ...

I didn't do it so aggressively, because latest_wave_data could occasionally be empty when a song is not yet over, but muted for one second.

yuannan commented 3 years ago

If you have it just update on every maincycle() loop it's instant and work just great.

If this is your purpose, I think you are working on the wrong direction. Because one subprocess call per cycle is never cheap.

I would agree as well. This method of interfacing with pulse is very expensive.

IMO, the right direction is to add a listener to PulseAudio. The listener will inform panon when a device is activated. I believe it is possible, but it is out of my knowledge.

It is possible and I didn't want to do this as it would mean basically ripping up the ground works on your code and creating a new panon from the ground up. I have found libraries that claim to do this but in the case that I would have to rewrite the entire pipeline as your current code lives on python-soundcard pretty much. I might as well just use C which has actual pulse libs and would run much much faster. In which case I would be much better off just making a fork instead of mutilating your project.

I think I'll close this ticket and then make panon multi-process when I have a some time to read more into python multi-processing. My knowledge in parallel programming is in Go-Lang so I'll have to brush up on my skills. But I'll try and implement what I said in my original ticket about this.