Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

VSync callback support for Linux #28465

Closed
wants to merge 1 commit into from

Conversation

ishitatsuyuki
Copy link

This change adds support for VSync callbacks on Linux, making it possible
to run at high refresh rates. The heavy lifting is done by GDK; and so the
implementation just wires the Flutter internals to it.

Unresolved: the implementation conflicts with "smooth resizing" support
(#25884). Smooth resizing blocks the main thread until a frame is rendered,
but this also blocks the VSync callbacks necessary to make progress in
rendering, resulting in a livelock. The smooth resizing is commented out
for now.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on
    writing and running engine tests.
  • N/A I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

@ishitatsuyuki
Copy link
Author

@knopp Your original attempt at smooth resizing simply ran GMainLoop iterations while it blocked the main thread, but it "caused problems with Wayland where some events were prematurely processed while resize callback was still in progress". Could you elaborate on what kind of events was that? The nature of this PR requires the main GLib event loop to be not be jammed while waiting for a frame, hence I wonder if we can make the original approach of running GMainLoop iterations work.

@knopp
Copy link
Member

knopp commented Sep 5, 2021

@knopp Your original attempt at smooth resizing simply ran GMainLoop iterations while it blocked the main thread, but it "caused problems with Wayland where some events were prematurely processed while resize callback was still in progress". Could you elaborate on what kind of events was that? The nature of this PR requires the main GLib event loop to be not be jammed while waiting for a frame, hence I wonder if we can make the original approach of running GMainLoop iterations work.

IIRC you would get resize notification while another resize notification was in progress (held until frame is available). What do you need vsync callback for during resizing? During resizing you want to produce frame as fast as possible and the rasterization is driven from viewport size change, that shouldn't be dependend on vsync.

@ishitatsuyuki
Copy link
Author

What do you need vsync callback for during resizing? During resizing you want to produce frame as fast as possible and the rasterization is driven from viewport size change, that shouldn't be dependent on vsync.

I don't really know, but it just "jams" and gets stuck inside fl_task_runner_block_main_thread.

@knopp
Copy link
Member

knopp commented Sep 5, 2021

What do you need vsync callback for during resizing? During resizing you want to produce frame as fast as possible and the rasterization is driven from viewport size change, that shouldn't be dependent on vsync.

I don't really know, but it just "jams" and gets stuck inside fl_task_runner_block_main_thread.

It's likely stuck there, because it's waiting for frame with appropriate size that never comes. I'm not entirely sure if engine attempts to wait for vsync when presenting frame triggered from viewport change size, but if it does I can imagine this could cause the behavior you're seeing.

FlTaskRunner could be easily extended to post any task on main thread, not just FlutterTask. This would solve the issue of g_idle_add not working while main loop is blocked. I'm not sure GdkFrameClock's update callback would be working in this scenario, but during resizing it shouldn't matter, it could be short-circuited to call embedder_api.OnVsync as soon as possible.

@ishitatsuyuki
Copy link
Author

ishitatsuyuki commented Sep 6, 2021

OK, turns out that the smooth resizing support in GTK has been refactored to use frame clock (commit), and if we follow GTK's frame cycle, then GTK should nicely handle smooth resizing for us without the need to block the event loop. I'll try to replace the current blocking code with an approach that follows the frame clock.

Edit: The blocking code actually can't go away as we still need to block, but we would be blocking in the "draw" phase as opposed to the "layout" phase.

@knopp
Copy link
Member

knopp commented Sep 6, 2021

I don't think you can get rid of blocking. It is necessary due to flutter threading model.

@ishitatsuyuki
Copy link
Author

ishitatsuyuki commented Sep 6, 2021

Forget what I said in the previous comment, that was partially wrong.

I ended up implementing the approach you described in #28465 (comment), and now it is theoretically compatible with smooth resizing. In practice, resizing gives visual artifacts both before and after the change, which might need to be investigated. I run Arch with GTK 3.24, with KDE running on X11.

There is also issues with certain animations getting skipped, I suspect the timestamps passed to the callbacks are not quite correct. I'll look into this.

Edit: pushed the commit I forgot to push. The issues above remains.

This change adds support for VSync callbacks on Linux, making it possible
to run at high refresh rates. The heavy lifting is done by GDK; and so the
implementation just wires the Flutter internals to it.

FlTaskRunner is also modified to allow the VSync callback to get through
while the main thread is blocked.

v2: Fix compatibility with smooth resizing.
* @target_time_nanos: absolute time in nanoseconds
*
* Posts a Flutter task to be executed on main thread. This function is thread
* safe and may be called from any thread.
*/
void fl_task_runner_post_task(FlTaskRunner* task_runner,
FlutterTask task,
std::function<void()> delegate,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the embedder code is complied as C++, the actual code is C. So I don't think using std::function here is a way to go. You can perhaps replace it with a callback that takes gpointer and a gpointer value.

@@ -6,6 +6,7 @@

#include <gmodule.h>

#include <atomic>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this for?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftover, will remove it later.

@@ -24,7 +24,7 @@ struct _FlTaskRunner {
typedef struct _FlTaskRunnerTask {
// absolute time of task (based on g_get_monotonic_time)
gint64 task_time_micros;
FlutterTask task;
std::function<void()> delegate;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FlTaskRunnerTask is freed using g_free. The implicit c++ destructor is not called at any point. So you'll leak any heap allocation that std::function might have. Again, it would be better to just have a callback and gpointer instead.

@ishitatsuyuki
Copy link
Author

Status update: If you test the PR then you would see, but right now high refresh rate works in neither X11 nor Wayland. It seems that the frame clock interacts in a weird way with GL and/or child GdkWindow.

I'm doing some investigation by myself as well as asking on #gtk. It might be the case that GTK 3 doesn't really deal well with this and in that case I'm going to drop this PR until we get a port to GTK 4.

@ishitatsuyuki
Copy link
Author

Closing as I have lost momentum on this for now. I'll open another when I get this into a working state some day.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants