From 32885bd045fa1c8d6a2aebac12044c4c2e5f3bdb Mon Sep 17 00:00:00 2001 From: Ramez Ragaa Date: Tue, 10 Dec 2024 16:34:30 +0200 Subject: [PATCH 1/3] chore(x11): use a Timer instead of a DispatcherTimer for render callbacks --- .../X11WindowWrapper.cs | 5 +- .../X11XamlRootHost.cs | 56 ++++++++++--------- .../X11XamlRootHost.x11events.cs | 14 +++-- 3 files changed, 43 insertions(+), 32 deletions(-) diff --git a/src/Uno.UI.Runtime.Skia.X11/X11WindowWrapper.cs b/src/Uno.UI.Runtime.Skia.X11/X11WindowWrapper.cs index a6d92f4dc483..76068be89e3c 100644 --- a/src/Uno.UI.Runtime.Skia.X11/X11WindowWrapper.cs +++ b/src/Uno.UI.Runtime.Skia.X11/X11WindowWrapper.cs @@ -104,16 +104,17 @@ public override void ExtendContentIntoTitleBar(bool extend) _host.ExtendContentIntoTitleBar(extend); } - private void OnWindowClosing() + private bool OnWindowClosing() { var closingArgs = RaiseClosing(); if (closingArgs.Cancel) { - return; + return false; } // All prerequisites passed, can safely close. Close(); + return true; } private void OnNativeActivated(bool focused) => ActivationState = focused ? CoreWindowActivationState.PointerActivated : CoreWindowActivationState.Deactivated; diff --git a/src/Uno.UI.Runtime.Skia.X11/X11XamlRootHost.cs b/src/Uno.UI.Runtime.Skia.X11/X11XamlRootHost.cs index e02190e9f70f..29a8a0614586 100644 --- a/src/Uno.UI.Runtime.Skia.X11/X11XamlRootHost.cs +++ b/src/Uno.UI.Runtime.Skia.X11/X11XamlRootHost.cs @@ -16,6 +16,7 @@ using SkiaSharp; using Uno.Disposables; using Uno.UI; +using Uno.UI.Dispatching; using Uno.UI.Xaml.Controls; namespace Uno.WinUI.Runtime.Skia.X11; @@ -73,8 +74,7 @@ internal partial class X11XamlRootHost : IXamlRootHost private readonly ApplicationView _applicationView; private readonly X11WindowWrapper _wrapper; private readonly Window _window; - private readonly CompositeDisposable _disposables = new(); - private readonly XamlRoot _xamlRoot; + private readonly IDisposable _windowBackgroundDisposable; private int _synchronizedShutDownTopWindowIdleCounter; @@ -83,14 +83,14 @@ internal partial class X11XamlRootHost : IXamlRootHost private IX11Renderer? _renderer; private bool _renderDirty = true; - private readonly DispatcherTimer _renderTimer; + // ReSharper disable once NotAccessedField.Local + private readonly Timer _timer; // this field is necessary to prevent the timer from being GCed public X11Window RootX11Window => _x11Window!.Value; public X11Window TopX11Window => _x11TopWindow!.Value; - public X11XamlRootHost(X11WindowWrapper wrapper, Window winUIWindow, XamlRoot xamlRoot, Action configureCallback, Action closingCallback, Action focusCallback, Action visibilityCallback) + public X11XamlRootHost(X11WindowWrapper wrapper, Window winUIWindow, XamlRoot xamlRoot, Action configureCallback, Func closingCallback, Action focusCallback, Action visibilityCallback) { - _xamlRoot = xamlRoot; _wrapper = wrapper; _window = winUIWindow; @@ -99,22 +99,32 @@ public X11XamlRootHost(X11WindowWrapper wrapper, Window winUIWindow, XamlRoot xa _visibilityCallback = visibilityCallback; _configureCallback = configureCallback; - _renderTimer = new DispatcherTimer(); - _renderTimer.Interval = TimeSpan.FromSeconds(1.0 / X11ApplicationHost.RenderFrameRate); // we're on the UI thread - _renderTimer.Tick += (sender, o) => + // running XConfigureEvent and rendering callbacks on a timer is necessary to avoid freezing in certain situations + // where we get spammed with these events. Most notably, when resizing a window by dragging an edge, we'll get spammed + // with XConfigureEvents. + _timer = new Timer(static obj => { - if (Interlocked.Exchange(ref _needsConfigureCallback, 0) == 1) - { - _configureCallback(); - } + var @this = (X11XamlRootHost)obj!; + var needsConfigureCallback = @this._needsConfigureCallback; + var needsRender = @this._renderDirty; - if (_renderDirty) + if (needsConfigureCallback || needsRender) { - _renderDirty = false; - _renderer?.Render(); + NativeDispatcher.Main.Enqueue(() => + { + if (needsConfigureCallback) + { + @this._needsConfigureCallback = false; + @this._configureCallback(); + } + if (needsRender) + { + @this._renderDirty = false; + @this._renderer?.Render(); + } + }); } - }; - _renderTimer.Start(); + }, this, TimeSpan.Zero, TimeSpan.FromSeconds(1.0 / X11ApplicationHost.RenderFrameRate)); _applicationView = ApplicationView.GetForWindowId(winUIWindow.AppWindow.Id); _applicationView.PropertyChanged += OnApplicationViewPropertyChanged; @@ -150,7 +160,8 @@ public X11XamlRootHost(X11WindowWrapper wrapper, Window winUIWindow, XamlRoot xa // only start listening to events after we're done setting everything up InitializeX11EventsThread(); - RegisterForBackgroundColor(); + _windowBackgroundDisposable = _window.RegisterBackgroundChangedEvent((_, _) => UpdateRendererBackground()); + UpdateRendererBackground(); } public static X11XamlRootHost? GetHostFromWindow(Window window) @@ -444,7 +455,7 @@ private unsafe static X11Window CreateGLXWindow(IntPtr display, int screen, Size } IntPtr context = GlxInterface.glXCreateNewContext(display, bestFbc, GlxConsts.GLX_RGBA_TYPE, IntPtr.Zero, /* True */ 1); - var _1 = XLib.XSync(display, false); + _ = XLib.XSync(display, false); XSetWindowAttributes attribs = default; attribs.border_pixel = XLib.XBlackPixel(display, screen); @@ -593,13 +604,6 @@ private void SynchronizedShutDown(X11Window x11Window) } } - private void RegisterForBackgroundColor() - { - UpdateRendererBackground(); - - _disposables.Add(_window.RegisterBackgroundChangedEvent((s, e) => UpdateRendererBackground())); - } - private void UpdateRendererBackground() { if (_window.Background is Microsoft.UI.Xaml.Media.SolidColorBrush brush) diff --git a/src/Uno.UI.Runtime.Skia.X11/X11XamlRootHost.x11events.cs b/src/Uno.UI.Runtime.Skia.X11/X11XamlRootHost.x11events.cs index 42d7c90d287b..239a1a46ee94 100644 --- a/src/Uno.UI.Runtime.Skia.X11/X11XamlRootHost.x11events.cs +++ b/src/Uno.UI.Runtime.Skia.X11/X11XamlRootHost.x11events.cs @@ -13,12 +13,12 @@ internal partial class X11XamlRootHost { private static int _threadCount; - private readonly Action _closingCallback; + private readonly Func _closingCallback; private readonly Action _focusCallback; private readonly Action _visibilityCallback; private readonly Action _configureCallback; - private int _needsConfigureCallback; + private bool _needsConfigureCallback; private X11PointerInputSource? _pointerSource; private X11KeyboardInputSource? _keyboardSource; @@ -185,7 +185,13 @@ static IEnumerable GetEvents(IntPtr display) // which, according to the source code, just calls XKillClient // https://gitlab.freedesktop.org/xorg/app/xkill/-/blob/a5f704e4cd30f03859f66bafd609a75aae27cc8c/xkill.c#L234 // In the case of xkill, we can't really do much, it's similar to a SIGKILL but for x connections - QueueAction(this, _closingCallback); + QueueAction(this, () => + { + if (_closingCallback()) + { + _windowBackgroundDisposable.Dispose(); + } + }); } else if (@event.ClientMessageEvent.message_type == X11Helper.GetAtom(x11Window.Display, X11Helper.XdndEnter) || @event.ClientMessageEvent.message_type == X11Helper.GetAtom(x11Window.Display, X11Helper.XdndPosition) || @@ -197,7 +203,7 @@ static IEnumerable GetEvents(IntPtr display) } break; case XEventName.ConfigureNotify: - Interlocked.Exchange(ref _needsConfigureCallback, 1); + _needsConfigureCallback = true; break; case XEventName.FocusIn: QueueAction(this, () => _focusCallback(true)); From 798ffb8fc9246f154210b74e07839463fa7b2b8b Mon Sep 17 00:00:00 2001 From: Ramez Ragaa Date: Tue, 10 Dec 2024 22:35:01 +0200 Subject: [PATCH 2/3] chore: rework fix --- .../X11ApplicationHost.cs | 10 +- .../X11WindowWrapper.cs | 5 +- .../X11XamlRootHost.cs | 127 ++++++++++++------ .../X11XamlRootHost.x11events.cs | 18 +-- 4 files changed, 97 insertions(+), 63 deletions(-) diff --git a/src/Uno.UI.Runtime.Skia.X11/X11ApplicationHost.cs b/src/Uno.UI.Runtime.Skia.X11/X11ApplicationHost.cs index 71376b4296f3..a7a878784d02 100644 --- a/src/Uno.UI.Runtime.Skia.X11/X11ApplicationHost.cs +++ b/src/Uno.UI.Runtime.Skia.X11/X11ApplicationHost.cs @@ -23,7 +23,6 @@ namespace Uno.WinUI.Runtime.Skia.X11; public partial class X11ApplicationHost : SkiaHost, ISkiaApplicationHost, IDisposable { [ThreadStatic] private static bool _isDispatcherThread; - [ThreadStatic] private static int _renderFrameRate; private readonly EventLoop _eventLoop; private readonly Func _appBuilder; @@ -75,19 +74,24 @@ public X11ApplicationHost(Func appBuilder, int renderFrameRate = 60 { _appBuilder = appBuilder; + if (RenderFrameRate != default && renderFrameRate != RenderFrameRate) + { + throw new InvalidOperationException($"X11's render frame rate should only be set once."); + } + RenderFrameRate = renderFrameRate; + _eventLoop = new EventLoop(); _eventLoop.Schedule(() => { Thread.CurrentThread.Name = "Uno Event Loop"; }, UI.Dispatching.NativeDispatcherPriority.Normal); _eventLoop.Schedule(() => { _isDispatcherThread = true; - _renderFrameRate = renderFrameRate; }, UI.Dispatching.NativeDispatcherPriority.Normal); CoreDispatcher.DispatchOverride = _eventLoop.Schedule; CoreDispatcher.HasThreadAccessOverride = () => _isDispatcherThread; } - internal static int RenderFrameRate => _renderFrameRate; + internal static int RenderFrameRate { get; private set; } [LibraryImport("libc", StringMarshallingCustomType = typeof(AnsiStringMarshaller))] private static partial void setlocale(int type, string s); diff --git a/src/Uno.UI.Runtime.Skia.X11/X11WindowWrapper.cs b/src/Uno.UI.Runtime.Skia.X11/X11WindowWrapper.cs index 76068be89e3c..a6d92f4dc483 100644 --- a/src/Uno.UI.Runtime.Skia.X11/X11WindowWrapper.cs +++ b/src/Uno.UI.Runtime.Skia.X11/X11WindowWrapper.cs @@ -104,17 +104,16 @@ public override void ExtendContentIntoTitleBar(bool extend) _host.ExtendContentIntoTitleBar(extend); } - private bool OnWindowClosing() + private void OnWindowClosing() { var closingArgs = RaiseClosing(); if (closingArgs.Cancel) { - return false; + return; } // All prerequisites passed, can safely close. Close(); - return true; } private void OnNativeActivated(bool focused) => ActivationState = focused ? CoreWindowActivationState.PointerActivated : CoreWindowActivationState.Deactivated; diff --git a/src/Uno.UI.Runtime.Skia.X11/X11XamlRootHost.cs b/src/Uno.UI.Runtime.Skia.X11/X11XamlRootHost.cs index 29a8a0614586..aedbdd45082d 100644 --- a/src/Uno.UI.Runtime.Skia.X11/X11XamlRootHost.cs +++ b/src/Uno.UI.Runtime.Skia.X11/X11XamlRootHost.cs @@ -16,7 +16,6 @@ using SkiaSharp; using Uno.Disposables; using Uno.UI; -using Uno.UI.Dispatching; using Uno.UI.Xaml.Controls; namespace Uno.WinUI.Runtime.Skia.X11; @@ -74,7 +73,6 @@ internal partial class X11XamlRootHost : IXamlRootHost private readonly ApplicationView _applicationView; private readonly X11WindowWrapper _wrapper; private readonly Window _window; - private readonly IDisposable _windowBackgroundDisposable; private int _synchronizedShutDownTopWindowIdleCounter; @@ -82,14 +80,20 @@ internal partial class X11XamlRootHost : IXamlRootHost private X11Window? _x11TopWindow; private IX11Renderer? _renderer; - private bool _renderDirty = true; - // ReSharper disable once NotAccessedField.Local - private readonly Timer _timer; // this field is necessary to prevent the timer from being GCed + private static readonly Stopwatch _stopwatch = Stopwatch.StartNew(); + + private readonly DispatcherTimer _renderTimer = new DispatcherTimer(); + private long _lastRenderTime; + private int _renderScheduled; + + private readonly DispatcherTimer _configureTimer = new DispatcherTimer(); + private long _lastConfigureTime; + private int _configureScheduled; public X11Window RootX11Window => _x11Window!.Value; public X11Window TopX11Window => _x11TopWindow!.Value; - public X11XamlRootHost(X11WindowWrapper wrapper, Window winUIWindow, XamlRoot xamlRoot, Action configureCallback, Func closingCallback, Action focusCallback, Action visibilityCallback) + public X11XamlRootHost(X11WindowWrapper wrapper, Window winUIWindow, XamlRoot xamlRoot, Action configureCallback, Action closingCallback, Action focusCallback, Action visibilityCallback) { _wrapper = wrapper; _window = winUIWindow; @@ -99,41 +103,28 @@ public X11XamlRootHost(X11WindowWrapper wrapper, Window winUIWindow, XamlRoot xa _visibilityCallback = visibilityCallback; _configureCallback = configureCallback; - // running XConfigureEvent and rendering callbacks on a timer is necessary to avoid freezing in certain situations - // where we get spammed with these events. Most notably, when resizing a window by dragging an edge, we'll get spammed - // with XConfigureEvents. - _timer = new Timer(static obj => + _closed = new TaskCompletionSource(); + Closed = _closed.Task; + + _renderTimer.Tick += (_, _) => { - var @this = (X11XamlRootHost)obj!; - var needsConfigureCallback = @this._needsConfigureCallback; - var needsRender = @this._renderDirty; + _renderTimer.Stop(); + _renderScheduled = 0; + _renderer?.Render(); + }; - if (needsConfigureCallback || needsRender) - { - NativeDispatcher.Main.Enqueue(() => - { - if (needsConfigureCallback) - { - @this._needsConfigureCallback = false; - @this._configureCallback(); - } - if (needsRender) - { - @this._renderDirty = false; - @this._renderer?.Render(); - } - }); - } - }, this, TimeSpan.Zero, TimeSpan.FromSeconds(1.0 / X11ApplicationHost.RenderFrameRate)); + _configureTimer.Tick += (_, _) => + { + _configureTimer.Stop(); + _configureScheduled = 0; + _configureCallback(); + }; _applicationView = ApplicationView.GetForWindowId(winUIWindow.AppWindow.Id); _applicationView.PropertyChanged += OnApplicationViewPropertyChanged; CoreApplication.GetCurrentView().TitleBar.ExtendViewIntoTitleBarChanged += UpdateWindowPropertiesFromCoreApplication; winUIWindow.AppWindow.TitleBar.ExtendsContentIntoTitleBarChanged += ExtendContentIntoTitleBar; - _closed = new TaskCompletionSource(); - Closed = _closed.Task; - Initialize(); // Note: the timing of XamlRootMap.Register is very fragile. It needs to be early enough @@ -142,6 +133,15 @@ public X11XamlRootHost(X11WindowWrapper wrapper, Window winUIWindow, XamlRoot xa _windowToHost[winUIWindow] = this; X11Manager.XamlRootMap.Register(xamlRoot, this); + UpdateWindowPropertiesFromPackage(); + OnApplicationViewPropertyChanged(this, new PropertyChangedEventArgs(null)); + + // only start listening to events after we're done setting everything up + InitializeX11EventsThread(); + + var windowBackgroundDisposable = _window.RegisterBackgroundChangedEvent((_, _) => UpdateRendererBackground()); + UpdateRendererBackground(); + Closed.ContinueWith(_ => { using (X11Helper.XLock(RootX11Window.Display)) @@ -151,17 +151,9 @@ public X11XamlRootHost(X11WindowWrapper wrapper, Window winUIWindow, XamlRoot xa _applicationView.PropertyChanged -= OnApplicationViewPropertyChanged; CoreApplication.GetCurrentView().TitleBar.ExtendViewIntoTitleBarChanged -= UpdateWindowPropertiesFromCoreApplication; winUIWindow.AppWindow.TitleBar.ExtendsContentIntoTitleBarChanged -= ExtendContentIntoTitleBar; + windowBackgroundDisposable.Dispose(); } }); - - UpdateWindowPropertiesFromPackage(); - OnApplicationViewPropertyChanged(this, new PropertyChangedEventArgs(null)); - - // only start listening to events after we're done setting everything up - InitializeX11EventsThread(); - - _windowBackgroundDisposable = _window.RegisterBackgroundChangedEvent((_, _) => UpdateRendererBackground()); - UpdateRendererBackground(); } public static X11XamlRootHost? GetHostFromWindow(Window window) @@ -545,10 +537,57 @@ private bool IsOpenGLSupported(IntPtr display) } } - void IXamlRootHost.InvalidateRender() => _renderDirty = true; - UIElement? IXamlRootHost.RootElement => _window.RootElement; + // running XConfigureEvent and rendering callbacks on a timer is necessary to avoid freezing in certain situations + // where we get spammed with these events. Most notably, when resizing a window by dragging an edge, we'll get spammed + // with XConfigureEvents. + void IXamlRootHost.InvalidateRender() + { + if (Interlocked.Exchange(ref _renderScheduled, 1) == 0) + { + // Don't use ticks, which seem to mess things up for some reason + var now = _stopwatch.ElapsedMilliseconds; + var delta = now - Interlocked.Exchange(ref _lastRenderTime, now); + if (delta > TimeSpan.FromSeconds(1.0 / X11ApplicationHost.RenderFrameRate).TotalMilliseconds) + { + QueueAction(this, () => + { + _renderScheduled = 0; + _renderer?.Render(); + }); + } + else + { + _renderTimer.Interval = TimeSpan.FromTicks(delta); + _renderTimer.Start(); + } + } + } + + private void RaiseConfigureCallback() + { + if (Interlocked.Exchange(ref _configureScheduled, 1) == 0) + { + // Don't use ticks, which seem to mess things up for some reason + var now = _stopwatch.ElapsedMilliseconds; + var delta = now - Interlocked.Exchange(ref _lastConfigureTime, now); + if (delta > TimeSpan.FromSeconds(1.0 / X11ApplicationHost.RenderFrameRate).TotalMilliseconds) + { + QueueAction(this, () => + { + _configureScheduled = 0; + _configureCallback(); + }); + } + else + { + _configureTimer.Interval = TimeSpan.FromTicks(delta); + _configureTimer.Start(); + } + } + } + public unsafe void AttachSubWindow(IntPtr window) { using var lockDisposable = X11Helper.XLock(RootX11Window.Display); diff --git a/src/Uno.UI.Runtime.Skia.X11/X11XamlRootHost.x11events.cs b/src/Uno.UI.Runtime.Skia.X11/X11XamlRootHost.x11events.cs index 239a1a46ee94..bf92f29da29a 100644 --- a/src/Uno.UI.Runtime.Skia.X11/X11XamlRootHost.x11events.cs +++ b/src/Uno.UI.Runtime.Skia.X11/X11XamlRootHost.x11events.cs @@ -13,13 +13,11 @@ internal partial class X11XamlRootHost { private static int _threadCount; - private readonly Func _closingCallback; + private readonly Action _closingCallback; private readonly Action _focusCallback; private readonly Action _visibilityCallback; private readonly Action _configureCallback; - private bool _needsConfigureCallback; - private X11PointerInputSource? _pointerSource; private X11KeyboardInputSource? _keyboardSource; private X11DragDropExtension? _dragDrop; @@ -87,7 +85,7 @@ private unsafe void Run(X11Window x11Window) { var ret = X11Helper.poll(fds, 1, 1000); // timeout every second to see if the window is closed - if (_closed.Task.IsCompleted) + if (Closed.IsCompleted) { SynchronizedShutDown(x11Window); return; @@ -185,13 +183,7 @@ static IEnumerable GetEvents(IntPtr display) // which, according to the source code, just calls XKillClient // https://gitlab.freedesktop.org/xorg/app/xkill/-/blob/a5f704e4cd30f03859f66bafd609a75aae27cc8c/xkill.c#L234 // In the case of xkill, we can't really do much, it's similar to a SIGKILL but for x connections - QueueAction(this, () => - { - if (_closingCallback()) - { - _windowBackgroundDisposable.Dispose(); - } - }); + QueueAction(this, _closingCallback); } else if (@event.ClientMessageEvent.message_type == X11Helper.GetAtom(x11Window.Display, X11Helper.XdndEnter) || @event.ClientMessageEvent.message_type == X11Helper.GetAtom(x11Window.Display, X11Helper.XdndPosition) || @@ -203,7 +195,7 @@ static IEnumerable GetEvents(IntPtr display) } break; case XEventName.ConfigureNotify: - _needsConfigureCallback = true; + RaiseConfigureCallback(); break; case XEventName.FocusIn: QueueAction(this, () => _focusCallback(true)); @@ -215,7 +207,7 @@ static IEnumerable GetEvents(IntPtr display) QueueAction(this, () => _visibilityCallback(@event.VisibilityEvent.state != /* VisibilityFullyObscured */ 2)); break; case XEventName.Expose: - QueueAction(this, () => ((IXamlRootHost)this).InvalidateRender()); + ((IXamlRootHost)this).InvalidateRender(); break; case XEventName.MotionNotify: _pointerSource?.ProcessMotionNotifyEvent(@event.MotionEvent); From 17a70a9c0b7966f4b8924c69a2ce779f20fe4e47 Mon Sep 17 00:00:00 2001 From: Ramez Ragaa Date: Mon, 16 Dec 2024 14:45:24 +0200 Subject: [PATCH 3/3] chore: formatting --- .../X11XamlRootHost.cs | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/Uno.UI.Runtime.Skia.X11/X11XamlRootHost.cs b/src/Uno.UI.Runtime.Skia.X11/X11XamlRootHost.cs index aedbdd45082d..59b69aca0bd2 100644 --- a/src/Uno.UI.Runtime.Skia.X11/X11XamlRootHost.cs +++ b/src/Uno.UI.Runtime.Skia.X11/X11XamlRootHost.cs @@ -545,24 +545,24 @@ private bool IsOpenGLSupported(IntPtr display) void IXamlRootHost.InvalidateRender() { if (Interlocked.Exchange(ref _renderScheduled, 1) == 0) - { + { // Don't use ticks, which seem to mess things up for some reason var now = _stopwatch.ElapsedMilliseconds; - var delta = now - Interlocked.Exchange(ref _lastRenderTime, now); + var delta = now - Interlocked.Exchange(ref _lastRenderTime, now); if (delta > TimeSpan.FromSeconds(1.0 / X11ApplicationHost.RenderFrameRate).TotalMilliseconds) - { - QueueAction(this, () => - { - _renderScheduled = 0; + { + QueueAction(this, () => + { + _renderScheduled = 0; _renderer?.Render(); - }); - } - else - { - _renderTimer.Interval = TimeSpan.FromTicks(delta); + }); + } + else + { + _renderTimer.Interval = TimeSpan.FromTicks(delta); _renderTimer.Start(); - } - } + } + } } private void RaiseConfigureCallback() @@ -571,7 +571,7 @@ private void RaiseConfigureCallback() { // Don't use ticks, which seem to mess things up for some reason var now = _stopwatch.ElapsedMilliseconds; - var delta = now - Interlocked.Exchange(ref _lastConfigureTime, now); + var delta = now - Interlocked.Exchange(ref _lastConfigureTime, now); if (delta > TimeSpan.FromSeconds(1.0 / X11ApplicationHost.RenderFrameRate).TotalMilliseconds) { QueueAction(this, () =>