From e30b5efd5d0d74334bbd393415b942fc550adc43 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 22 Mar 2025 14:12:40 -0700 Subject: [PATCH] apprt/gtk: make GL context current in unrealize Fixes #6872 This commit explicitly acquires the GL context in the `unrealize` signal handler of the GTK Surface prior to cleaning up GPU resources. A GLArea only guarantees that the associated GdkContext is current for the `render` signal (see the docs[1]). This is why our OpenGL renderer "defers" various operations such as resize, font grid changing, etc. (see the `deferred_`-prefix fields in `renderer/OpenGL.zig`). However, we missed a spot. The `gtk-widget::unrealize` signal is emitted when the widget is destroyed, and it is the last chance we have to clean up our GPU resources. But it is not guaranteed that the GL context is current at that point, and we weren't making it current. On the NGL GTK renderer, this was freeing GPU resources we didn't own. As best I can understand, the old GL renderer only ever used a handful of GL resources that were early in the ID space, so by coincidence we were probably freeing nothing and everything was fine. But with the new NGL renderer uses a LOT more GL resources (make a few splits and the ID space is already in the thousands, from GTK!), so we were freeing real resources that we didn't own, which caused rendering issues. :) I suspect the above also resulted in VRAM memory leaks (which would be RAM memory leaks for unified memory GPUs). This potentially relates to The fix is to explicitly make the GL context current in the `unrealize` handler. [1]: https://docs.gtk.org/gtk4/method.GLArea.make_current.html --- src/apprt/gtk/App.zig | 13 ------------- src/apprt/gtk/Surface.zig | 29 +++++++++++++++++++++++++---- src/config/Config.zig | 12 +++++------- 3 files changed, 30 insertions(+), 24 deletions(-) diff --git a/src/apprt/gtk/App.zig b/src/apprt/gtk/App.zig index f9a3ab160..eb9a11c18 100644 --- a/src/apprt/gtk/App.zig +++ b/src/apprt/gtk/App.zig @@ -223,19 +223,6 @@ pub fn init(core_app: *CoreApp, opts: Options) !App { _ = internal_os.setenv("GDK_DISABLE", value[0 .. value.len - 1 :0]); } - if (version.runtimeAtLeast(4, 14, 0)) { - switch (config.@"gtk-gsk-renderer") { - .default => {}, - else => |renderer| { - // Force the GSK renderer to a specific value. After GTK 4.14 the - // `ngl` renderer is used by default which causes artifacts when - // used with Ghostty so it should be avoided. - log.warn("setting GSK_RENDERER={s}", .{@tagName(renderer)}); - _ = internal_os.setenv("GSK_RENDERER", @tagName(renderer)); - }, - } - } - c.gtk_init(); const display: *c.GdkDisplay = c.gdk_display_get_default() orelse { // I'm unsure of any scenario where this happens. Because we don't diff --git a/src/apprt/gtk/Surface.zig b/src/apprt/gtk/Surface.zig index 6c39677d5..de0a8f812 100644 --- a/src/apprt/gtk/Surface.zig +++ b/src/apprt/gtk/Surface.zig @@ -1325,14 +1325,35 @@ fn gtkRealize(area: *c.GtkGLArea, ud: ?*anyopaque) callconv(.C) void { /// This is called when the underlying OpenGL resources must be released. /// This is usually due to the OpenGL area changing GDK surfaces. fn gtkUnrealize(area: *c.GtkGLArea, ud: ?*anyopaque) callconv(.C) void { - _ = area; - - log.debug("gl surface unrealized", .{}); const self = userdataSelf(ud.?); - self.core_surface.renderer.displayUnrealized(); + log.debug("gl surface unrealized", .{}); // See gtkRealize for why we do this here. c.gtk_im_context_set_client_widget(self.im_context, null); + + // There is no guarantee that our GLArea context is current + // when unrealize is emitted, so we need to make it current. + c.gtk_gl_area_make_current(area); + if (c.gtk_gl_area_get_error(area)) |err| { + // I don't know a scenario this can happen, but it means + // we probably leaked memory because displayUnrealized + // below frees resources that aren't specifically OpenGL + // related. I didn't make the OpenGL renderer handle this + // scenario because I don't know if its even possible + // under valid circumstances, so let's log. + const message: []const u8 = if (err.*.message) |v| + std.mem.span(v) + else + "(no message)"; + log.warn( + "gl_area_make_current failed in unrealize msg={s}", + .{message}, + ); + log.warn("OpenGL resources and memory likely leaked", .{}); + return; + } else { + self.core_surface.renderer.displayUnrealized(); + } } /// render signal diff --git a/src/config/Config.zig b/src/config/Config.zig index 802c77e2e..681984c7d 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -2089,13 +2089,11 @@ keybind: Keybinds = .{}, /// debug builds, `false` for all others. @"gtk-opengl-debug": bool = builtin.mode == .Debug, -/// After GTK 4.14.0, we need to force the GSK renderer to OpenGL as the default -/// GSK renderer is broken on some systems. If you would like to override -/// that bekavior, set `gtk-gsk-renderer=default` and either use your system's -/// default GSK renderer, or set the GSK_RENDERER environment variable to your -/// renderer of choice before launching Ghostty. This setting has no effect when -/// using versions of GTK earlier than 4.14.0. -@"gtk-gsk-renderer": GtkGskRenderer = .opengl, +/// Obsolete configuration that should not be set. This was deprecated in +/// Ghostty 1.1.3 and no longer has any effect. The configuration key will +/// be fully removed in 1.2.0. You can manually override the GSK renderer +/// using standard environment variables such as `GSK_RENDERER` (from GTK). +@"gtk-gsk-renderer": GtkGskRenderer = .default, /// If `true`, the Ghostty GTK application will run in single-instance mode: /// each new `ghostty` process launched will result in a new window if there is