From 0b6d91e531a14c70f25bb1c8cd4a376a89bad7a4 Mon Sep 17 00:00:00 2001 From: Nikolay Bryskin Date: Mon, 25 May 2026 21:58:09 +0300 Subject: [PATCH] apprt/gtk: reuse one audio-bell MediaFile per surface to fix thread leak Each audio bell called gtk.MediaFile.newForFilename, which spins up a full GStreamer pipeline. The GTK4 GStreamer backend's GL sink starts gstglcontext/gldisplay-event threads that are never joined on teardown, so allocating a MediaFile per ring leaked a pipeline and ~4 threads on every bell. A long-running instance accumulated 705 threads over ~4h of normal use. Cache one MediaFile per surface (priv.bell_media), rebuilt only when bell-audio-path changes and unref'd on dispose. Each bell now replays the same pipeline via seek(0)+play() instead of creating a new one. The notify::ended -> unref handler is removed: it was what discarded (and leaked) a pipeline per ring. seek(0) is required so an ended stream plays again (#8957). Verified on a real instance: GStreamer's global element counter reached only oggdemux4 over an hour of use (one pipeline per bell-ringing surface, reused) and thread count stayed flat, versus per-bell growth before. Co-Authored-By: Claude Opus 4.7 --- src/apprt/gtk/class/surface.zig | 22 ++++++++++- src/apprt/gtk/media.zig | 67 +++++++++++++++++++++++---------- 2 files changed, 67 insertions(+), 22 deletions(-) diff --git a/src/apprt/gtk/class/surface.zig b/src/apprt/gtk/class/surface.zig index 3c9293a82..629866dc2 100644 --- a/src/apprt/gtk/class/surface.zig +++ b/src/apprt/gtk/class/surface.zig @@ -674,6 +674,12 @@ pub const Surface = extern struct { // false by a parent widget. bell_ringing: bool = false, + // The audio bell's MediaFile, reused across bells so we don't leak a + // GStreamer pipeline (and its GL threads) on every ring. Built lazily + // on the first audio bell and rebuilt when `bell-audio-path` changes; + // unref'd on dispose. See ringBell and media.zig. + bell_media: ?*gtk.MediaFile = null, + /// True if this surface is in an error state. This is currently /// a simple boolean with no additional information on WHAT the /// error state is, because we don't yet need it or use it. For now, @@ -1854,6 +1860,11 @@ pub const Surface = extern struct { priv.config = null; } + if (priv.bell_media) |v| { + v.unref(); + priv.bell_media = null; + } + if (priv.vadj_signal_group) |group| { group.setTarget(null); group.as(gobject.Object).unref(); @@ -2486,8 +2497,15 @@ pub const Surface = extern struct { 1.0, ); - const media_file = media.fromFilename(path) orelse break :audio; - media.playMediaFile(media_file, volume, required); + // Reuse one MediaFile per surface (rebuilt only when the path + // changes) so each bell replays the same pipeline instead of + // leaking a fresh one. Assign unconditionally: bellMediaFile frees + // any stale MediaFile and returns the current slot value (possibly + // null if the path is now inaccessible), so priv.bell_media never + // dangles. + priv.bell_media = media.bellMediaFile(priv.bell_media, path, required); + const media_file = priv.bell_media orelse break :audio; + media.playBell(media_file, volume); } } diff --git a/src/apprt/gtk/media.zig b/src/apprt/gtk/media.zig index 1015c933f..62bf0db8f 100644 --- a/src/apprt/gtk/media.zig +++ b/src/apprt/gtk/media.zig @@ -44,9 +44,38 @@ pub fn fromResource(path: [:0]const u8) ?*gtk.MediaFile { return gtk.MediaFile.newForResource(path); } -pub fn playMediaFile(media_file: *gtk.MediaFile, volume: f64, required: bool) void { - // If the audio file is marked as required, we'll emit an error if - // there was a problem playing it. Otherwise there will be silence. +/// Get-or-create a reusable bell MediaFile targeting `path`. +/// +/// `current` is the surface's currently-cached MediaFile (or null). If it +/// already targets `path` it is returned unchanged; otherwise it is unref'd and +/// a fresh MediaFile is built for `path`. Returns null (after freeing `current`) +/// if `path` is inaccessible, leaving the caller's slot empty. +/// +/// Reusing one MediaFile per surface is what prevents the GStreamer pipeline +/// leak: `gtk.MediaFile.newForFilename` spins up a full pipeline (and, via the +/// GTK4 GStreamer backend's GL sink, gstglcontext/gldisplay-event threads) that +/// is never torn down on the happy path, so allocating one per bell leaked a +/// pipeline + its threads on every ring. See the caller in surface.zig. +pub fn bellMediaFile( + current: ?*gtk.MediaFile, + path: [:0]const u8, + required: bool, +) ?*gtk.MediaFile { + if (current) |media_file| { + if (isForPath(media_file, path)) return media_file; + media_file.unref(); + } + + const media_file = fromFilename(path) orelse return null; + + // If the audio file is marked as required, we'll emit an error if there + // was a problem playing it. Otherwise there will be silence. We connect + // this once, here, because the MediaFile is reused across bells. + // + // NOTE: we intentionally do NOT connect notify::ended to unref. The + // MediaFile is owned by the surface and replayed via `seek(0)` for every + // bell; unref'ing on `ended` is precisely what previously discarded (and + // leaked) a pipeline per ring. if (required) { _ = gobject.Object.signals.notify.connect( media_file, @@ -57,21 +86,27 @@ pub fn playMediaFile(media_file: *gtk.MediaFile, volume: f64, required: bool) vo ); } - // Watch for the "ended" signal so that we can clean up after - // ourselves. - _ = gobject.Object.signals.notify.connect( - media_file, - ?*anyopaque, - mediaFileEnded, - null, - .{ .detail = "ended" }, - ); + return media_file; +} +/// (Re)play `media_file` at `volume`. `seek(0)` rewinds first so that a +/// previously-ended stream plays again; without it playback only ever happens +/// once (see #8957). Safe on a freshly-created stream as well. +pub fn playBell(media_file: *gtk.MediaFile, volume: f64) void { const media_stream = media_file.as(gtk.MediaStream); media_stream.setVolume(volume); + media_stream.seek(0); media_stream.play(); } +/// Whether `media_file` was created for `path`. +fn isForPath(media_file: *gtk.MediaFile, path: [:0]const u8) bool { + const file = media_file.getFile() orelse return false; + const cur = file.getPath() orelse return false; + defer glib.free(cur); + return std.mem.eql(u8, std.mem.span(cur), path); +} + fn mediaFileError( media_file: *gtk.MediaFile, _: *gobject.ParamSpec, @@ -92,11 +127,3 @@ fn mediaFileError( err.f_message orelse "", }); } - -fn mediaFileEnded( - media_file: *gtk.MediaFile, - _: *gobject.ParamSpec, - _: ?*anyopaque, -) callconv(.c) void { - media_file.unref(); -}