refactor(cava): fix thread-safety, resource leaks, and style violations
Comprehensive refactor of the cava module backend and frontends. Style & naming - Rename Cava -> CavaRaw; snake_case methods -> lowerCamelCase - Replace NULL with nullptr; replace C-style casts with static_cast - Add explicit standard-library includes (<memory>, <string>, <chrono>) - Fix missing trailing underscores on member variables Architecture - Remove Gtk::GLArea multiple inheritance in CavaGLSL (composition) - Return std::unique_ptr from factory; add doAction() to GLSL variant - Encapsulate thread timing arithmetic in AdaptiveDelay struct Thread safety & correctness - Replace raw sigc::signal with SafeSignal for cross-thread marshalling - Fix data race between loadConfig() and out_thread_ (recursive_mutex) - Fix audio_raw shallow-copy use-after-free via deep-copy AudioRaw payload - Make loadConfig() exception-safe with CavaConfigGuard RAII - Fix blocking read_thread_ race on shutdown (condition_variable + timeout join) - Fix isSilent() data race (acquire pthread mutex) - Eliminate doUpdate() recursion (iteration instead) - Guard audio_raw_clean() against uninitialized state - Fix format-icons underflow and cava_config buffer overflow - Store config by value to prevent dangling references on reload - Cache frontend config and refresh on runtime changes - Fix signed-char icon lookup bug on x86 - Prevent Json::Value mutation bloat via const-ref lookups - Broaden exception catches in worker threads (std::exception) Resource management & GL robustness - Fix OpenGL resource leaks (persist VBO/IBO/VAO; explicit destructor cleanup) - Fix shader error handling crashes (valid infoLog allocation) - Cache uniform locations instead of querying per frame - Fix gradient color uninitialized stack memory (zero-init + clamped count) - Fix shader time uniform integer division bug (float arithmetic) - Add explicit VAO bind in onRender() - Handle runtime surface config changes independently of shader changes - Clamp negative gradient_count before GL upload Follow-up - Singleton API split (inst() + configure()) intentionally deferred to a dedicated PR because it changes the public constructor contract.
This commit is contained in:
@@ -1,36 +1,64 @@
|
||||
#pragma once
|
||||
|
||||
#include <epoxy/gl.h>
|
||||
#include <gtkmm/glarea.h>
|
||||
#include <map>
|
||||
#include <string>
|
||||
#include <array>
|
||||
|
||||
#include <sigc++/connection.h>
|
||||
|
||||
#include "AModule.hpp"
|
||||
#include "cava_backend.hpp"
|
||||
|
||||
namespace waybar::modules::cava {
|
||||
|
||||
class CavaGLSL final : public AModule, public Gtk::GLArea {
|
||||
class CavaGLSL final : public AModule {
|
||||
public:
|
||||
CavaGLSL(const std::string&, const Json::Value&);
|
||||
~CavaGLSL() = default;
|
||||
~CavaGLSL();
|
||||
auto doAction(const std::string& name) -> void override;
|
||||
|
||||
private:
|
||||
using Action = void (CavaGLSL::*)();
|
||||
|
||||
Gtk::GLArea gl_area_;
|
||||
std::shared_ptr<CavaBackend> backend_;
|
||||
struct ::cava::config_params prm_;
|
||||
int frame_counter{0};
|
||||
// Cached config params (deep-copied strings to avoid dangling char* on backend reload)
|
||||
int sdl_width_{0};
|
||||
int sdl_height_{0};
|
||||
int bar_width_{0};
|
||||
int bar_spacing_{0};
|
||||
int gradient_count_{0};
|
||||
std::string vertex_shader_;
|
||||
std::string fragment_shader_;
|
||||
std::string bcolor_;
|
||||
std::string color_;
|
||||
std::array<std::string, 8> gradient_colors_;
|
||||
int frame_counter_{0};
|
||||
bool silence_{false};
|
||||
bool hide_on_silence_{false};
|
||||
bool mapped_{false};
|
||||
// Cava method
|
||||
auto onUpdate(const ::cava::audio_raw& input) -> void;
|
||||
void pauseResume();
|
||||
auto onUpdate(const CavaBackend::AudioRaw& input) -> void;
|
||||
auto onSilence() -> void;
|
||||
// Member variable to store the shared pointer
|
||||
std::shared_ptr<::cava::audio_raw> m_data_;
|
||||
GLuint shaderProgram_;
|
||||
auto onBackendConfigChanged() -> void;
|
||||
void cacheConfigParams(const ::cava::config_params& src);
|
||||
// Member variable to store audio data
|
||||
CavaBackend::AudioRaw m_data_;
|
||||
GLuint shaderProgram_{0};
|
||||
// OpenGL variables
|
||||
GLuint fbo_;
|
||||
GLuint texture_;
|
||||
GLuint fbo_{0};
|
||||
GLuint texture_{0};
|
||||
GLuint vbo_{0};
|
||||
GLuint ibo_{0};
|
||||
GLuint vao_{0};
|
||||
GLint uniform_bars_;
|
||||
GLint uniform_previous_bars_;
|
||||
GLint uniform_bars_count_;
|
||||
GLint uniform_time_;
|
||||
GLint uniform_input_texture_;
|
||||
// Methods
|
||||
void onRealize();
|
||||
bool onRender(const Glib::RefPtr<Gdk::GLContext>& context);
|
||||
@@ -39,5 +67,13 @@ class CavaGLSL final : public AModule, public Gtk::GLArea {
|
||||
void initSurface();
|
||||
void initGLSL();
|
||||
GLuint loadShader(const std::string& fileName, GLenum type);
|
||||
void cleanupGL();
|
||||
|
||||
// ModuleActionMap
|
||||
static const std::map<std::string, Action> actionMap_;
|
||||
|
||||
sigc::connection audio_raw_update_conn_;
|
||||
sigc::connection silence_conn_;
|
||||
sigc::connection config_changed_conn_;
|
||||
};
|
||||
} // namespace waybar::modules::cava
|
||||
|
||||
@@ -1,30 +1,38 @@
|
||||
#pragma once
|
||||
|
||||
#include <map>
|
||||
#include <string>
|
||||
|
||||
#include <sigc++/connection.h>
|
||||
|
||||
#include "ALabel.hpp"
|
||||
#include "cava_backend.hpp"
|
||||
|
||||
namespace waybar::modules::cava {
|
||||
|
||||
class Cava final : public ALabel, public sigc::trackable {
|
||||
class CavaRaw final : public ALabel {
|
||||
public:
|
||||
Cava(const std::string&, const Json::Value&);
|
||||
~Cava() = default;
|
||||
CavaRaw(const std::string&, const Json::Value&);
|
||||
~CavaRaw();
|
||||
auto doAction(const std::string& name) -> void override;
|
||||
|
||||
private:
|
||||
using Action = void (CavaRaw::*)();
|
||||
|
||||
std::shared_ptr<CavaBackend> backend_;
|
||||
// Text to display
|
||||
Glib::ustring label_text_{""};
|
||||
Glib::ustring label_text_;
|
||||
bool silence_{false};
|
||||
bool hide_on_silence_{false};
|
||||
std::string format_silent_{""};
|
||||
int ascii_range_{0};
|
||||
std::string format_silent_;
|
||||
// Cava method
|
||||
void pause_resume();
|
||||
void pauseResume();
|
||||
auto onUpdate(const std::string& input) -> void;
|
||||
auto onSilence() -> void;
|
||||
// ModuleActionMap
|
||||
static inline std::map<const std::string, void (waybar::modules::cava::Cava::* const)()>
|
||||
actionMap_{{"mode", &waybar::modules::cava::Cava::pause_resume}};
|
||||
static const std::map<std::string, Action> actionMap_;
|
||||
|
||||
sigc::connection update_conn_;
|
||||
sigc::connection silence_conn_;
|
||||
};
|
||||
} // namespace waybar::modules::cava
|
||||
|
||||
@@ -1,8 +1,17 @@
|
||||
#pragma once
|
||||
|
||||
#include <atomic>
|
||||
#include <chrono>
|
||||
#include <condition_variable>
|
||||
#include <memory>
|
||||
#include <mutex>
|
||||
#include <string>
|
||||
#include <vector>
|
||||
|
||||
#include <json/json.h>
|
||||
#include <sigc++/sigc++.h>
|
||||
|
||||
#include "util/SafeSignal.hpp"
|
||||
#include "util/sleeper_thread.hpp"
|
||||
|
||||
namespace cava {
|
||||
@@ -21,7 +30,6 @@ extern "C" {
|
||||
} // namespace cava
|
||||
|
||||
namespace waybar::modules::cava {
|
||||
using namespace std::literals::chrono_literals;
|
||||
|
||||
class CavaBackend final {
|
||||
public:
|
||||
@@ -29,19 +37,38 @@ class CavaBackend final {
|
||||
|
||||
virtual ~CavaBackend();
|
||||
// Methods
|
||||
int getAsciiRange();
|
||||
int getAsciiRange() const;
|
||||
void doPauseResume();
|
||||
void Update();
|
||||
const struct ::cava::config_params* getPrm();
|
||||
std::chrono::milliseconds getFrameTimeMilsec();
|
||||
void update();
|
||||
const ::cava::config_params& getPrm() const;
|
||||
std::chrono::milliseconds getFrameTimeMilsec() const;
|
||||
|
||||
struct AudioRaw {
|
||||
std::vector<float> bars_raw;
|
||||
std::vector<float> previous_bars_raw;
|
||||
int number_of_bars = 0;
|
||||
|
||||
AudioRaw() = default;
|
||||
explicit AudioRaw(const ::cava::audio_raw& raw) {
|
||||
number_of_bars = raw.number_of_bars;
|
||||
if (raw.bars_raw != nullptr && number_of_bars > 0) {
|
||||
bars_raw.assign(raw.bars_raw, raw.bars_raw + number_of_bars);
|
||||
}
|
||||
if (raw.previous_bars_raw != nullptr && number_of_bars > 0) {
|
||||
previous_bars_raw.assign(raw.previous_bars_raw, raw.previous_bars_raw + number_of_bars);
|
||||
}
|
||||
}
|
||||
};
|
||||
|
||||
// Signal accessor
|
||||
using type_signal_update = sigc::signal<void(const std::string&)>;
|
||||
type_signal_update signal_update();
|
||||
using type_signal_audio_raw_update = sigc::signal<void(const ::cava::audio_raw&)>;
|
||||
type_signal_audio_raw_update signal_audio_raw_update();
|
||||
using type_signal_silence = sigc::signal<void()>;
|
||||
type_signal_silence signal_silence();
|
||||
using SignalUpdate = SafeSignal<const std::string&>;
|
||||
SignalUpdate& signalUpdate();
|
||||
using SignalAudioRawUpdate = SafeSignal<AudioRaw>;
|
||||
SignalAudioRawUpdate& signalAudioRawUpdate();
|
||||
using SignalSilence = SafeSignal<>;
|
||||
SignalSilence& signalSilence();
|
||||
using SignalConfigChanged = SafeSignal<>;
|
||||
SignalConfigChanged& signalConfigChanged();
|
||||
|
||||
private:
|
||||
CavaBackend(const Json::Value& config);
|
||||
@@ -49,36 +76,78 @@ class CavaBackend final {
|
||||
util::SleeperThread out_thread_;
|
||||
|
||||
// Cava API to read audio source
|
||||
::cava::ptr input_source_{NULL};
|
||||
::cava::ptr input_source_{nullptr};
|
||||
|
||||
struct ::cava::error_s error_{}; // cava errors
|
||||
struct ::cava::config_params prm_{}; // cava parameters
|
||||
struct ::cava::audio_raw audio_raw_{}; // cava handled raw audio data(is based on audio_data)
|
||||
struct ::cava::audio_data audio_data_{}; // cava audio data
|
||||
struct ::cava::cava_plan* plan_{NULL}; //{new cava_plan{}};
|
||||
struct ::cava::cava_plan* plan_{nullptr}; //{new cava_plan{}};
|
||||
|
||||
std::chrono::seconds fetch_input_delay_{4};
|
||||
// Delay to handle audio source
|
||||
std::chrono::milliseconds frame_time_milsec_{1s};
|
||||
|
||||
const Json::Value& config_;
|
||||
struct AdaptiveDelay {
|
||||
std::chrono::milliseconds delay;
|
||||
std::chrono::seconds delta{0};
|
||||
|
||||
explicit AdaptiveDelay(std::chrono::milliseconds initial = std::chrono::seconds(1))
|
||||
: delay(initial) {}
|
||||
|
||||
bool increase() {
|
||||
if (delta == std::chrono::seconds{0}) {
|
||||
delta += std::chrono::seconds{1};
|
||||
delay += delta;
|
||||
return true;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
bool decrease() {
|
||||
if (delta > std::chrono::seconds{0}) {
|
||||
delay -= delta;
|
||||
delta -= std::chrono::seconds{1};
|
||||
return true;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
std::chrono::milliseconds current() const { return delay; }
|
||||
|
||||
void reset(std::chrono::milliseconds new_delay) {
|
||||
delay = new_delay;
|
||||
delta = std::chrono::seconds{0};
|
||||
}
|
||||
};
|
||||
|
||||
AdaptiveDelay adaptive_delay_;
|
||||
|
||||
Json::Value config_;
|
||||
int re_paint_{0};
|
||||
bool silence_{false};
|
||||
bool silence_prev_{false};
|
||||
std::chrono::seconds suspend_silence_delay_{0};
|
||||
int sleep_counter_{0};
|
||||
std::string output_{};
|
||||
// Methods
|
||||
void invoke();
|
||||
void execute();
|
||||
bool isSilence();
|
||||
bool isSilent();
|
||||
void doUpdate(bool force = false);
|
||||
void loadConfig();
|
||||
void freeBackend();
|
||||
|
||||
// Signal
|
||||
type_signal_update m_signal_update_;
|
||||
type_signal_audio_raw_update m_signal_audio_raw_;
|
||||
type_signal_silence m_signal_silence_;
|
||||
SignalUpdate m_signal_update_;
|
||||
SignalAudioRawUpdate m_signal_audio_raw_;
|
||||
SignalSilence m_signal_silence_;
|
||||
SignalConfigChanged m_signal_config_changed_;
|
||||
|
||||
std::atomic<bool> shutdown_{false};
|
||||
bool audio_raw_initialized_{false};
|
||||
mutable std::recursive_mutex state_mutex_;
|
||||
|
||||
// Synchronization for joining read_thread_ during destruction
|
||||
bool read_thread_exited_{false};
|
||||
mutable std::mutex read_thread_exit_mutex_;
|
||||
std::condition_variable read_thread_exit_cv_;
|
||||
};
|
||||
} // namespace waybar::modules::cava
|
||||
|
||||
@@ -1,5 +1,7 @@
|
||||
#pragma once
|
||||
|
||||
#include <memory>
|
||||
|
||||
#ifdef HAVE_LIBCAVA
|
||||
#include "cavaRaw.hpp"
|
||||
#include "cava_backend.hpp"
|
||||
@@ -9,16 +11,16 @@
|
||||
#endif
|
||||
|
||||
namespace waybar::modules::cava {
|
||||
AModule* getModule(const std::string& id, const Json::Value& config) {
|
||||
inline std::unique_ptr<AModule> getModule(const std::string& id, const Json::Value& config) {
|
||||
#ifdef HAVE_LIBCAVA
|
||||
const std::shared_ptr<CavaBackend> backend_{waybar::modules::cava::CavaBackend::inst(config)};
|
||||
switch (backend_->getPrm()->output) {
|
||||
switch (backend_->getPrm().output) {
|
||||
#ifdef HAVE_LIBCAVAGLSL
|
||||
case ::cava::output_method::OUTPUT_SDL_GLSL:
|
||||
return new waybar::modules::cava::CavaGLSL(id, config);
|
||||
return std::make_unique<waybar::modules::cava::CavaGLSL>(id, config);
|
||||
#endif
|
||||
default:
|
||||
return new waybar::modules::cava::Cava(id, config);
|
||||
return std::make_unique<waybar::modules::cava::CavaRaw>(id, config);
|
||||
}
|
||||
#else
|
||||
throw std::runtime_error("Unknown module");
|
||||
|
||||
Reference in New Issue
Block a user