Repository navigation
Use vsync in Linux embedder - #192342
Use vsync in Linux embedder#192342seismor-com wants to merge 7 commits into
Conversation
github-actions
Bot
added
engine
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements display-based vsync driving for the Flutter Linux embedder by tracking display refresh rates and updating the engine's frame interval accordingly. It also optimizes Wayland rendering by committing the parent surface of a subsurface instead of triggering a full software redraw. Feedback focuses on improving robustness, specifically by adding null checks for display monitors, parent surfaces, and subsurfaces, as well as correcting a logic bug in the tick-snapping calculation when the current time is less than the phase.
df300c7 to
e57bc61
Compare
The display monitor is released in dispose, but remove_renderable can run from a view's weak-ref callback during teardown. Fall back to the default refresh rate in that case instead of hitting GLib assertions.
Also move a misplaced test comment back onto the Destroy test.
The signed modulo version added an extra interval when the value was earlier than the phase, skipping a tick. Use unsigned arithmetic and return the phase itself in that case. Add a test for it.
gdk_display_get_monitor can return null, so return 0 instead of passing it through to gdk_monitor_get_refresh_rate.
e57bc61 to
fc16fbe
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the Flutter Linux platform embedder to drive the engine's vsync ticks based on display refresh rates and commits the parent Wayland surface to display frames efficiently. Feedback on these changes suggests adding defensive checks in fl_engine.cc to prevent potential crashes, specifically guarding against division-by-zero in snap_to_next_tick, and checking for null pointers in update_frame_interval and fl_engine_vsync_cb during teardown and initialization.
| static uint64_t snap_to_next_tick(uint64_t value, | ||
| uint64_t phase, | ||
| uint64_t interval) { | ||
| if (value <= phase) { | ||
| return phase; | ||
| } | ||
| uint64_t remainder = (value - phase) % interval; | ||
| if (remainder == 0) { | ||
| return value; | ||
| } | ||
| return value + (interval - remainder); | ||
| } |
There was a problem hiding this comment.
To prevent a potential division-by-zero crash, snap_to_next_tick should guard against an interval of 0. This could theoretically happen if the display driver reports an extremely high refresh rate or if a mock returns an unexpected value.
static uint64_t snap_to_next_tick(uint64_t value,
uint64_t phase,
uint64_t interval) {
if (interval == 0) {
return value;
}
if (value <= phase) {
return phase;
}
uint64_t remainder = (value - phase) % interval;
if (remainder == 0) {
return value;
}
return value + (interval - remainder);
}| // used so any view is rendered smoothly. | ||
| static void update_frame_interval(FlEngine* self) { | ||
| double refresh_rate = 0.0; | ||
| if (self->display_monitor != nullptr) { |
There was a problem hiding this comment.
During engine disposal, self->display_ids_by_view_id is cleared to nullptr. To prevent a potential null-pointer dereference crash if update_frame_interval is called during teardown, we should ensure self->display_ids_by_view_id is not null before attempting to iterate over it.
if (self->display_monitor != nullptr && self->display_ids_by_view_id != nullptr) {| static void fl_engine_vsync_cb(void* user_data, intptr_t baton) { | ||
| FlEngine* self = FL_ENGINE(user_data); | ||
|
|
||
| uint64_t frame_interval; |
There was a problem hiding this comment.
The vsync callback can potentially be triggered during engine initialization before self->engine is fully populated. To prevent a crash or failure when calling OnVsync, we should defensively check if self->engine is nullptr and return early.
static void fl_engine_vsync_cb(void* user_data, intptr_t baton) {
FlEngine* self = FL_ENGINE(user_data);
if (self->engine == nullptr) {
return;
}
uint64_t frame_interval;
|
@seismor-com thanks for working on this. Could you split the subsurface change into a separate PR? I'm more concerned about side-effects in that code. |
The subsurface view renderer set the Wayland buffer scale once, when the subsurface was created. GTK only queues a redraw when a widget's scale factor changes, not a resize, so after a window moved to a monitor with a different scale the engine rendered frames at the new pixel size while the subsurface kept its old buffer scale. The compositor then showed the frame at the wrong size: on a 2.4x display next to a 1.2x one the content was 1.5x too large, overflowed the window onto the other display, and pointer events landed on the wrong widgets. Record the widget's scale on the FlSubsurfaceEGL and apply it in present() with the first frame whose size is a multiple of the new scale, so the scale and a matching buffer are committed together; Wayland raises a protocol error for a buffer whose size is not a multiple of the scale. The renderer forwards the scale from notify::scale-factor and size-allocate, and queues a resize on a scale change so FlView sends the engine new window metrics and the frame of the new size is waited for. Co-Authored-By: Claude Fable 5.1 <[email protected]>
|
Good call to hold off on the merge. I found new bug (not my own) that causes the window/ui to render at 2x/3x scale no matter what the monitor dpi is set to (already fixed in my fork). I also found out on linux it refuses to use vulkan properly and falls back to opengl, so I have a fix for that too on my fork. I will try to make new separate pull requests for all my changes, they work great for me! |
Add two fixes for enabling refresh rates / fps to reach above 60hz/fps.
First, the engine never installed/ran a vsync_callback, so it did a fallback to using VsyncWaiterFallback, which is hardcoded to 60hz. This adds a vsync callback and uses values from FlDisplayMonitor, trying values from the fastest display attached. This alone didn't fix the issue.
Second: FlViewRendererSubsurface queued a GTK redraw after every frame to force the parent commit for its synchronized subsurface, which software-repaints the whole window (~25–30ms/frame at 4K/144hz, GPU idle). This fix uses wayland subsurfaces instead via the parent wl_surface, using the GPU (buttery smooth at 144hz at ~50% gpu usage (fast scroll spikes)).
Measured on a 4K/144Hz display with adaptive sync enabled, attached via usb-c to a laptop. Fedora 43, KDE 6.7.4 on Wayland. Kernel 7.1.12-100.fc43.x86_64 (64-bit) with SecureBoot On. CPU: AMD Ryzen 7 7730U with Radeon Graphics & 32GB RAM. I used Claude Fable 5.1 over a ~2 hour session to get to real meat of the problem & fix it for my use-case. I have not tested it on other hardware or distributions.
If a more experienced flutter engine team member can double check the code that would be great; this is my first ever contribution.
Fixes: #49757
Pre-launch Checklist
///).