Merge pull request #93684 from Riteo/explicitly-catastrophic

Wayland: minimize surface commits and limit them to the main thread

platform/linuxbsd/wayland/display_server_wayland.cpp

@@ -1111,6 +1111,28 @@ Key DisplayServerWayland::keyboard_get_keycode_from_physical(Key p_keycode) cons
 	return key;
 }
 
+void DisplayServerWayland::try_suspend() {
+	// Due to various reasons, we manually handle display synchronization by
+	// waiting for a frame event (request to draw) or, if available, the actual
+	// window's suspend status. When a window is suspended, we can avoid drawing
+	// altogether, either because the compositor told us that we don't need to or
+	// because the pace of the frame events became unreliable.
+	if (emulate_vsync) {
+		bool frame = wayland_thread.wait_frame_suspend_ms(1000);
+		if (!frame) {
+			suspended = true;
+		}
+	} else {
+		if (wayland_thread.is_suspended()) {
+			suspended = true;
+		}
+	}
+
+	if (suspended) {
+		DEBUG_LOG_WAYLAND("Window suspended.");
+	}
+}
+
 void DisplayServerWayland::process_events() {
 	wayland_thread.mutex.lock();
 
@@ -1193,30 +1215,32 @@ void DisplayServerWayland::process_events() {
 	wayland_thread.keyboard_echo_keys();
 
 	if (!suspended) {
-		if (emulate_vsync) {
-			// Due to various reasons, we manually handle display synchronization by
-			// waiting for a frame event (request to draw) or, if available, the actual
-			// window's suspend status. When a window is suspended, we can avoid drawing
-			// altogether, either because the compositor told us that we don't need to or
-			// because the pace of the frame events became unreliable.
-			bool frame = wayland_thread.wait_frame_suspend_ms(1000);
-			if (!frame) {
-				suspended = true;
+		// Due to the way legacy suspension works, we have to treat low processor
+		// usage mode very differently than the regular one.
+		if (OS::get_singleton()->is_in_low_processor_usage_mode()) {
+			// NOTE: We must avoid committing a surface if we expect a new frame, as we
+			// might otherwise commit some inconsistent data (e.g. buffer scale). Note
+			// that if a new frame is expected it's going to be committed by the renderer
+			// soon anyways.
+			if (!RenderingServer::get_singleton()->has_changed()) {
+				// We _can't_ commit in a different thread (such as in the frame callback
+				// itself) because we would risk to step on the renderer's feet, which would
+				// cause subtle but severe issues, such as crashes on setups with explicit
+				// sync. This isn't normally a problem, as the renderer commits at every
+				// frame (which is what we need for atomic surface updates anyways), but in
+				// low processor usage mode that expectation is broken. When it's on, our
+				// frame rate stops being constant. This also reflects in the frame
+				// information we use for legacy suspension. In order to avoid issues, let's
+				// manually commit all surfaces, so that we can get fresh frame data.
+				wayland_thread.commit_surfaces();
+				try_suspend();
 			}
 		} else {
-			if (wayland_thread.is_suspended()) {
-				suspended = true;
-			}
-		}
-
-		if (suspended) {
-			DEBUG_LOG_WAYLAND("Window suspended.");
-		}
-	} else {
-		if (wayland_thread.get_reset_frame()) {
-			// At last, a sign of life! We're no longer suspended.
-			suspended = false;
+			try_suspend();
 		}
+	} else if (wayland_thread.get_reset_frame()) {
+		// At last, a sign of life! We're no longer suspended.
+		suspended = false;
 	}
 
 #ifdef DBUS_ENABLED

platform/linuxbsd/wayland/display_server_wayland.h

@@ -154,6 +154,8 @@ class DisplayServerWayland : public DisplayServer {
 
 	virtual void _show_window();
 
+	void try_suspend();
+
 public:
 	virtual bool has_feature(Feature p_feature) const override;
 

platform/linuxbsd/wayland/wayland_thread.cpp

@@ -968,7 +968,6 @@ void WaylandThread::_frame_wl_callback_on_done(void *data, struct wl_callback *w
 
 	ws->frame_callback = wl_surface_frame(ws->wl_surface),
 	wl_callback_add_listener(ws->frame_callback, &frame_wl_callback_listener, ws);
-	wl_surface_commit(ws->wl_surface);
 
 	if (ws->wl_surface && ws->buffer_scale_changed) {
 		// NOTE: We're only now setting the buffer scale as the idea is to get this
@@ -980,11 +979,6 @@ void WaylandThread::_frame_wl_callback_on_done(void *data, struct wl_callback *w
 		// rendering if needed.
 		wl_surface_set_buffer_scale(ws->wl_surface, window_state_get_preferred_buffer_scale(ws));
 	}
-
-	// NOTE: Remember to set here also other buffer-dependent states (e.g. opaque
-	// region) if used, to be as close as possible to an atomic surface update.
-	// Ideally we'd only have one surface commit, but it's not really doable given
-	// the current state of things.
 }
 
 void WaylandThread::_wl_surface_on_leave(void *data, struct wl_surface *wl_surface, struct wl_output *wl_output) {
@@ -3241,10 +3235,6 @@ void WaylandThread::window_create(DisplayServer::WindowID p_window_id, int p_wid
 	ws.frame_callback = wl_surface_frame(ws.wl_surface);
 	wl_callback_add_listener(ws.frame_callback, &frame_wl_callback_listener, &ws);
 
-	// NOTE: This commit is only called once to start the whole frame callback
-	// "loop".
-	wl_surface_commit(ws.wl_surface);
-
 	if (registry.xdg_exporter) {
 		ws.xdg_exported = zxdg_exporter_v1_export(registry.xdg_exporter, ws.wl_surface);
 		zxdg_exported_v1_add_listener(ws.xdg_exported, &xdg_exported_listener, &ws);
@@ -4120,6 +4110,10 @@ void WaylandThread::primary_set_text(const String &p_text) {
 	wl_display_roundtrip(wl_display);
 }
 
+void WaylandThread::commit_surfaces() {
+	wl_surface_commit(main_window.wl_surface);
+}
+
 void WaylandThread::set_frame() {
 	frame = true;
 }

platform/linuxbsd/wayland/wayland_thread.h

@@ -992,6 +992,8 @@ class WaylandThread {
 
 	void primary_set_text(const String &p_text);
 
+	void commit_surfaces();
+
 	void set_frame();
 	bool get_reset_frame();
 	bool wait_frame_suspend_ms(int p_timeout);