From 1161edc3a530c191cd44c7010f0ddd4a5c9e3d84 Mon Sep 17 00:00:00 2001 From: Grok Date: Sat, 5 Sep 2026 18:34:27 -0400 Subject: [PATCH] =?UTF-8?q?docs:=20libobs=20does=20cache=20file-created=20?= =?UTF-8?q?effects=20=E2=80=94=20restore=20the=20accurate=20comment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A review finding claimed gs_effect_create_from_file() allocates a fresh effect per call, and I acted on it before it had been checked against libobs. It was wrong, and the resulting comments asserted the opposite of the truth — one of them explicitly telling the next reader that the earlier, correct version had been mistaken. Verified in libobs source this time: graphics.c gs_effect_create_from_file() consults find_cached_effect() first and returns the existing effect; gs_effect_create() sets cached = true for any file-created effect. effect.c gs_effect_destroy() guards the real free with if (!effect->cached), so destroying a cached effect is a no-op until obs_free_graphics(). So the Tiles wall and the Loudness Meter share one gs_effect_t*, each destroy is a no-op, and there is neither a double compile nor a double-free. Both comments now carry the source references so this stops flip-flopping. Comments only; no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WqDL6gcoVvEMXv7Qwcni9n --- src/zoom-loudness-meter-source.cpp | 27 ++++++++++++++++----------- src/zoom-tiles-effect.h | 25 +++++++++++++++++-------- 2 files changed, 33 insertions(+), 19 deletions(-) diff --git a/src/zoom-loudness-meter-source.cpp b/src/zoom-loudness-meter-source.cpp index b2c6675..39e18a2 100644 --- a/src/zoom-loudness-meter-source.cpp +++ b/src/zoom-loudness-meter-source.cpp @@ -42,17 +42,22 @@ static constexpr uint32_t kMeterLoudArgb = 0xFFE04B4Bu; static constexpr uint32_t kMeterQuietArgb = 0xFFE0A03Cu; static constexpr uint32_t kMeterIdleArgb = 0xFF3A424Eu; -// Shared with the Tiles wall only by FILE PATH, not by handle -- and that is -// not the free dedupe it sounds like. gs_effect_create_from_file() allocates -// a brand-new gs_effect_t on every call; libobs does NOT cache or dedupe -// effects compiled from the same file. This second tiles_effect_load() call -// (the Tiles wall makes its own, separate one) compiles a second, independent -// copy of corevideo-tiles.effect. Harmless as written -- two handles, each -// destroyed exactly once by its own owner (this file's unload vs. the Tiles -// source's) -- but do NOT "deduplicate" the two loads into one shared -// gs_effect_t* on the strength of a caching story that isn't true: sharing a -// handle between two owners that each call gs_effect_destroy() on it once is -// a double-free at unload. +// Shared with the Tiles wall by FILE PATH, and libobs really does dedupe that +// for us -- verified in libobs source, because this comment has now been +// wrong in both directions and the guesswork should stop here: +// +// graphics.c, gs_effect_create_from_file(): calls find_cached_effect(file) +// first and returns that effect if one matches, so the second load compiles +// nothing. gs_effect_create() sets effect->cached = true for any effect +// created with a filename. +// effect.c, gs_effect_destroy(): `if (!effect->cached) actually_destroy()` +// -- destroying a cached effect is a no-op until obs_free_graphics(). +// +// So s_meter_effect.effect and the Tiles wall's s_tiles_effect.effect are the +// SAME pointer, each tiles_effect_destroy() is a no-op, and the effect is +// reclaimed at graphics shutdown. No double compile, no double-free, no leak. +// Loading our own handle rather than reaching into zoom-supersource.cpp is a +// coupling choice, not a memory-safety one. static TilesEffect s_meter_effect; static bool s_meter_pass_failed_logged = false; diff --git a/src/zoom-tiles-effect.h b/src/zoom-tiles-effect.h index eebe5a4..f0d2585 100644 --- a/src/zoom-tiles-effect.h +++ b/src/zoom-tiles-effect.h @@ -94,12 +94,21 @@ struct TilesEffect { bool tiles_effect_load(TilesEffect &out); // Drops our reference to the effect and resets the struct. Safe to call on -// an unloaded/failed TilesEffect. Note: gs_effect_create_from_file() allocates -// a fresh gs_effect_t on every call -- libobs does NOT cache or dedupe -// effects compiled from the same file (an earlier version of this comment -// claimed otherwise). gs_effect_destroy() here is a real, immediate release -// of THIS handle. Every caller that loads its own TilesEffect from this file -// (the Tiles wall and the Loudness Meter each compile an independent copy) -// owns exactly one handle and must destroy it exactly once; two owners must -// never share one gs_effect_t*, or the second destroy is a double-free. +// an unloaded/failed TilesEffect. +// +// libobs CACHES effects created from a file, and both halves of that matter +// here -- verified in libobs source, because this comment has been wrong in +// both directions before: +// +// graphics.c, gs_effect_create_from_file(): find_cached_effect(file) is +// consulted first, so loading the same path twice returns the SAME +// gs_effect_t*. gs_effect_create() marks any file-created effect cached. +// effect.c, gs_effect_destroy(): `if (!effect->cached)` guards the real +// free, so destroying a cached effect is a no-op; obs_free_graphics() +// reclaims it at shutdown. +// +// Consequence for callers: several owners may each hold a TilesEffect loaded +// from this same file (the Tiles wall and the Loudness Meter both do). They +// share one underlying pointer, each destroy is a no-op, and that is safe -- +// there is no double-free to avoid and no leak to chase. void tiles_effect_destroy(TilesEffect &fx);