Waybar icon indicating copy to clipboard operation
Waybar copied to clipboard

cava nonsafe threads

Open LukashonakV opened this issue 2 months ago • 4 comments

Dear @Alexays , This PR is based on investigation of the #4567 . Thanks to @DaDiamondArmor and @mutoroglin I realized backend and frontend was not implemented as well. During investigation I found non safe threading that's why it is segfaults from time to time, freezes and race conditions. Now backend was deeply reworked. Dev tests shown that module is very stabled now. In additional it was fixed an issue when audio server for some reason is restarted. No need more to restart in that case waybar. cava will get this. But the most surprising for me was - the significant performance benefit. Even with the single monitor CPU consumption decreased 10x times. I wasn't expected such big improvement. In the same time it was again updated libcava because of -> karlstav/cava#698 . I also provided big rework to the upstream in order to avoid of memory leaking. So for now waybar + cava - the only one small memory leak still exists because of issue somewhere deep in the pipewire driver. Finally this PR closes #4567 Thank you

LukashonakV avatar Oct 23 '25 22:10 LukashonakV

Hey, I'm trying to build this from source to test your fix. I'm suffering from https://github.com/Alexays/waybar/issues/4567

I get this failure after ninja -C build, build exits without success.

../src/modules/cava/cava_backend.cpp: In member function ‘void waybar::modules::cava::CavaBackend::freeBackend()’:
../src/modules/cava/cava_backend.cpp:150:3: error: ‘free_config’ was not declared in this scope
  150 |   free_config(&prm_);
      |   ^~~~~~~~~~~
../src/modules/cava/cava_backend.cpp: In member function ‘void waybar::modules::cava::CavaBackend::loadConfig()’:
../src/modules/cava/cava_backend.cpp:165:36: warning: the address of ‘waybar::modules::cava::CavaBackend::error_’ will never be NULL [-Waddress]
  165 |   if (!load_config(cfgPath, &prm_, &error_)) {
      |                                    ^~~~~~~
In file included from ../src/modules/cava/cava_backend.cpp:1:
../include/modules/cava/cava_backend.hpp:48:26: note: ‘waybar::modules::cava::CavaBackend::error_’ declared here
   48 |   struct ::cava::error_s error_{};          // cava errors
      |                          ^~~~~~
../src/modules/cava/cava_backend.cpp:165:19: error: too few arguments to function ‘bool cava::load_config(char*, config_params*, bool, error_s*, int)’
  165 |   if (!load_config(cfgPath, &prm_, &error_)) {
      |        ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /usr/include/cava/common.h:4,
                 from ../include/modules/cava/cava_backend.hpp:15:
/usr/include/cava/config.h:140:6: note: declared here
  140 | bool load_config(char configPath[PATH_MAX], struct config_params *p, bool colorsOnly,
      |      ^~~~~~~~~~~

Wolfizen avatar Nov 22 '25 19:11 Wolfizen

Dear @Alexays , cam you please consider to get this PR.

  1. It solves the issues with thread race conditions
  2. It gets new release of the libcava
  3. It's fixed some small memory leaks cases. For now in my machine I'm no more observing such cases when waybar with cava is working

LukashonakV avatar Nov 26 '25 10:11 LukashonakV

Commit 13519ca builds successfully and fixes my crashes when combining waybar with cava. Thank you @LukashonakV

@Alexays +1 to recommending consideration to approving this PR.

I hope the Nix-Tests failures are able to be overcome so this can be mergeable :D

Wolfizen avatar Dec 01 '25 04:12 Wolfizen

I can confirm that this build solved the issue with waybar and cava I had experienced.

JohnnyJumper avatar Dec 09 '25 07:12 JohnnyJumper