BeginFrameSource implementation for Wayland [chromium/src : main]
Mitchell Cohen (Gerrit)
Mitchell Cohen added 1 comment![]( "Open in Gerrit")
@fang...@chromium.org: I don't know exactly what it would take to get this production-ready or if it's even the right approach. Will comment on code where I have some questions I wasn't able to answer.
I based this on the existing classes and factory method and iOS implementation which has similar constraints (needing to go across process boundaries). It does seem to work, and I thought it might be a good starting point to investigate.
Run with `./chrome --enable-features=WaylandExternalBeginFrameSource --vmodule=begin_frame_source_wayland=1`.
Related details
- Kramer Ge
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitchell Cohen (Gerrit)
Mitchell Cohen added 4 comments![]( "Open in Gerrit")
// There won't be a frame callback with no damage so schedule one.is this something that could be changed on the WaylandFrameManager side?
// Safety fallback in case the frame callback doesn't arrive (e.g., windowThis fallback mechanism doesn't feel great to me and it's the part I struggled with the most in this PoC.
- If it runs on every presentation feedback interval, or close to it, we basically end up reinventing a delay-based pacer, and Wayland frame callbacks are useless. - - But if it's on a longer delay, Chrome freezes while waiting for frame callbacks that never come (common when switching tabs).
So the current approach of frequently needing to schedule frames in OnBeginFrameAck conflicts with the idea of frames being event-driven. There's probably a more sophisticated way to deal with missing frame callbacks and/or to make sure fewer are dropped.
std::unique_ptr<ui::BeginFrameSourceWayland> begin_frame_source_;The tree host seems like a strange place to own this driver, and other platforms don't use it this way, but this placement had several advantages: the tree host can pre-configure the compositor to use the external frame source with existing params (that for some reason aren't used by other impls), and it has the earliest access to the compositor, PlatformWindow and Wayland extension.
ui::GetWaylandToplevelExtension(*platform_window()) != nullptr;When I tried to enable the driver for all windows (not just toplevels) I ran into failed DCHECKs in the Viz process. That surprised me because each surface has its own compositor and paces its own frames. I was able to get it to work by removing the check and clearing the frame sink in Viz, but that change seemed too drastic and could not be contained to the PoC.
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Kramer Ge added 3 comments![]( "Open in Gerrit")
Sorry for the delay. Some initial thoughts
// There won't be a frame callback with no damage so schedule one.is this something that could be changed on the WaylandFrameManager side?
I think if you want you can apply an empty commit in `waylandframemanager` in that case to manually trigger next `frame_callback`.
// Safety fallback in case the frame callback doesn't arrive (e.g., windowThis fallback mechanism doesn't feel great to me and it's the part I struggled with the most in this PoC.
- If it runs on every presentation feedback interval, or close to it, we basically end up reinventing a delay-based pacer, and Wayland frame callbacks are useless. - - But if it's on a longer delay, Chrome freezes while waiting for frame callbacks that never come (common when switching tabs).
So the current approach of frequently needing to schedule frames in OnBeginFrameAck conflicts with the idea of frames being event-driven. There's probably a more sophisticated way to deal with missing frame callbacks and/or to make sure fewer are dropped.
Like switch to delay-based beginframe that refers to present time interval, can switch back to callback-based when callback catches up to the most recent frame.
Does chrome freeze currently when switching tabs? I imagine the same issue exists as of now if frame callback does not come in time.
Related details
- Mitchell Cohen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitchell Cohen (Gerrit)
Mitchell Cohen added 1 comment![]( "Open in Gerrit")
// There won't be a frame callback with no damage so schedule one.Kramer Geis this something that could be changed on the WaylandFrameManager side?
I think if you want you can apply an empty commit in `waylandframemanager` in that case to manually trigger next `frame_callback`.
Thanks for this suggestion, I implemented the empty commit in patchset 4 and performance is much more consistent. It rarely falls back to the timer now (usually once when switching tabs).
Related details
- Kramer Ge
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitchell Cohen (Gerrit)
Mitchell Cohen added 2 comments![]( "Open in Gerrit")
@fang...@chromium.org: I don't know exactly what it would take to get this production-ready or if it's even the right approach. Will comment on code where I have some questions I wasn't able to answer.
I based this on the existing classes and factory method and iOS implementation which has similar constraints (needing to go across process boundaries). It does seem to work, and I thought it might be a good starting point to investigate.
Run with `./chrome --enable-features=WaylandExternalBeginFrameSource --vmodule=begin_frame_source_wayland=1`.
@fang...@chromium.org I added no-damage commits and presentation feedback and this works a lot better now.
I'm seeing promising performance in a lightly optimized build. The external frame source clearly performs better on a site with a high refresh rate animation (https://testufo.com/) on my 144hz display. The frequent vsync misses go away completely when I enable the flag:
Current default delay-based timer:
```
❯ ./out/Release/chrome
[2575752:2575752:0308/153547.234354:ERROR:chrome/browser/ui/views/user_education/impl/browser_user_education_interface_impl.cc:163] Attempting to show IPH IPH_ExtensionsMenu before browser initialization complete; IPH will not be shown.
[2575752:2575778:0308/153552.361774:ERROR:google_apis/gcm/engine/registration_request.cc:291] Registration response error message: DEPRECATED_ENDPOINT
[2575802:2575938:0308/153556.645870:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.03 ms
[2575802:2575938:0308/153556.766338:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.195 ms
[2575802:2575938:0308/153557.886113:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.002 ms
[2575802:2575938:0308/153558.406438:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.035 ms
[2575802:2575938:0308/153558.503106:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.082 ms
[2575802:2575938:0308/153559.143282:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.111 ms
[2575802:2575938:0308/153559.783466:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.037 ms
[2575802:2575938:0308/153600.063306:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.043 ms
[2575802:2575938:0308/153600.143264:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.193 ms
[2575802:2575938:0308/153600.663748:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.017 ms
[2575802:2575938:0308/153600.744175:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.113 ms
[2575802:2575938:0308/153601.384445:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.059 ms
[2575802:2575938:0308/153601.984590:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.062 ms
[2575802:2575938:0308/153602.641420:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.082 ms
[2575802:2575938:0308/153603.007525:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.067 ms
[2575802:2575938:0308/153603.144642:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.069 ms
[2575802:2575938:0308/153604.361920:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.002 ms
[2575802:2575938:0308/153604.481820:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.051 ms
[2575802:2575938:0308/153604.881989:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.045 ms
[2575802:2575938:0308/153605.641689:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.008 ms
[2575802:2575938:0308/153605.841712:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.062 ms
[2575802:2575938:0308/153606.043037:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.131 ms
[2575802:2575938:0308/153606.179438:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.011 ms
[2575802:2575938:0308/153606.299371:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.201 ms
[2575802:2575938:0308/153606.379519:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.077 ms
[2575802:2575938:0308/153606.562626:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.009 ms
[2575802:2575938:0308/153606.642455:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.061 ms
[2575802:2575938:0308/153606.802601:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.058 ms
[2575802:2575938:0308/153606.860069:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.003 ms
[2575802:2575938:0308/153607.282455:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.047 ms
[2575802:2575938:0308/153607.363061:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.11 ms
[2575802:2575938:0308/153608.562667:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.035 ms
[2575802:2575938:0308/153608.836783:ERROR:components/viz/service/display/display.cc:286] Frame latency is negative: -0.027 ms
^C[2575752:2575789:0308/153609.257804:ERROR:content/browser/browser_main_loop.cc:275] GLib: g_main_context_pop_thread_default: assertion 'stack != NULL' failed
QThreadStorage: entry 2 destroyed before end of thread 0x39fc026fe940
QThreadStorage: entry 1 destroyed before end of thread 0x39fc026fe940
```
New frame source:
```
❯ ./out/Release/chrome --enable-features=WaylandExternalBeginFrameSource
[2576157:2576157:0308/153617.687014:ERROR:chrome/browser/ui/views/user_education/impl/browser_user_education_interface_impl.cc:163] Attempting to show IPH IPH_ExtensionsMenu before browser initialization complete; IPH will not be shown.
[2576157:2576185:0308/153622.815586:ERROR:google_apis/gcm/engine/registration_request.cc:291] Registration response error message: DEPRECATED_ENDPOINT
[2576157:2576185:0308/153647.310875:ERROR:google_apis/gcm/engine/registration_request.cc:291] Registration response error message: DEPRECATED_ENDPOINT
[2576157:2576237:0308/153647.657540:ERROR:content/browser/browser_main_loop.cc:275] GLib: g_main_context_pop_thread_default: assertion 'stack != NULL' failed
[2576157:2576239:0308/153647.657539:ERROR:content/browser/browser_main_loop.cc:275] GLib: g_main_context_pop_thread_default: assertion 'stack != NULL' failed
[2576157:2576238:0308/153647.657540:ERROR:content/browser/browser_main_loop.cc:275] GLib: g_main_context_pop_thread_default: assertion 'stack != NULL' failed
^C[2576208:2576208:0308/153659.328258:ERROR:gpu/command_buffer/service/shared_image/shared_image_manager.cc:252] SharedImageManager::ProduceSkia: Trying to Produce a Skia representation from a non-existent mailbox.
[2576157:2576195:0308/153659.468110:ERROR:content/browser/browser_main_loop.cc:275] GLib: g_main_context_pop_thread_default: assertion 'stack != NULL' failed
QThreadStorage: entry 2 destroyed before end of thread 0x3d5c004f9da0
QThreadStorage: entry 1 destroyed before end of thread 0x3d5c004f9da0
```
// Safety fallback in case the frame callback doesn't arrive (e.g., windowKramer GeThis fallback mechanism doesn't feel great to me and it's the part I struggled with the most in this PoC.
- If it runs on every presentation feedback interval, or close to it, we basically end up reinventing a delay-based pacer, and Wayland frame callbacks are useless. - - But if it's on a longer delay, Chrome freezes while waiting for frame callbacks that never come (common when switching tabs).
So the current approach of frequently needing to schedule frames in OnBeginFrameAck conflicts with the idea of frames being event-driven. There's probably a more sophisticated way to deal with missing frame callbacks and/or to make sure fewer are dropped.
Like switch to delay-based beginframe that refers to present time interval, can switch back to callback-based when callback catches up to the most recent frame.
Does chrome freeze currently when switching tabs? I imagine the same issue exists as of now if frame callback does not come in time.
Done
Related details
- Kramer Ge
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Kramer Ge added 1 comment![]( "Open in Gerrit")
`Frame latency is negative` is a surprising message to me, it's supposed to mean draw timing is after swap timing but does not mean much to the wayland compositor latch. Unclear to me why this is printed.
Related details
- Mitchell Cohen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitchell Cohen (Gerrit)
Mitchell Cohen added 1 comment![]( "Open in Gerrit")
It only started appearing for me in recent versions of Chromium, so I think it's a new message, but what's interesting is that it occurs much less often when I enable this driver, especially after I added presentation feedback.
Related details
- Kramer Ge
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Kramer Ge added 3 comments![]( "Open in Gerrit")
// Copyright 2026 The Chromium AuthorsI think this should live inside `ui/ozone/platform/wayland/host/`.
begin_frame_source_ = std::make_unique<ui::BeginFrameSourceWayland>(We want to hide the impl detail of the ozone platform.
I think it would better if we ask on the `platform_window()` to produce a `BeginFrameSource` extended by `BeginFrameSourceWayland`. life time is easier to manage if `platform_window()` owns the `begin_frame_source`.
This way you may also control that only toplevel can create the BF source.
Related details
- Mitchell Cohen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Kramer Ge added 2 comments![]( "Open in Gerrit")
base::ObserverList<WaylandFrameTimingObserver> frame_timing_observers_;`WaylandWindow` and `WaylandFrameManager` is 1:1, also 1:1 with `ui::Compositor`, so I did not expect more than 1 here.
May be the `WaylandBeginFrameSource` and `WaylandWindow` can be 1:N, where there is 1 `WBFS` per `WaylandWindow` tree.
if (frame_callback_freeze_detected_ &&Do you know if the beginframessource is behaving ok when browser is being captured by say https://meet.google.com, and goes into minimize/restore states?
Mitchell Cohen (Gerrit)
Mitchell Cohen added 1 comment![]( "Open in Gerrit")
begin_frame_source_ = std::make_unique<ui::BeginFrameSourceWayland>(We want to hide the impl detail of the ozone platform.
I think it would better if we ask on the `platform_window()` to produce a `BeginFrameSource` extended by `BeginFrameSourceWayland`. life time is easier to manage if `platform_window()` owns the `begin_frame_source`.
This way you may also control that only toplevel can create the BF source.
I originally placed `BeginFrameSourceWayland` in the ozone host, but there's a problem: it needs access to `ui::Compositor` to issue frames over mojo. (The iOS frame source works the same way, which is why it lives in `content/browser`.)
So we would need to remove the dependency on `ui/compositor` with a new delegate/interface for this to work. I can try it.
Related details
- Kramer Ge
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Kramer Ge added 1 comment![]( "Open in Gerrit")
begin_frame_source_ = std::make_unique<ui::BeginFrameSourceWayland>(Mitchell CohenWe want to hide the impl detail of the ozone platform.
I think it would better if we ask on the `platform_window()` to produce a `BeginFrameSource` extended by `BeginFrameSourceWayland`. life time is easier to manage if `platform_window()` owns the `begin_frame_source`.
This way you may also control that only toplevel can create the BF source.
I originally placed `BeginFrameSourceWayland` in the ozone host, but there's a problem: it needs access to `ui::Compositor` to issue frames over mojo. (The iOS frame source works the same way, which is why it lives in `content/browser`.)
So we would need to remove the dependency on `ui/compositor` with a new delegate/interface for this to work. I can try it.
Ah that is unfortunate. I guess we should probably put it in `ui/base/wayland/` then.
Related details
- Mitchell Cohen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitchell Cohen (Gerrit)
Mitchell Cohen added 1 comment![]( "Open in Gerrit")
begin_frame_source_ = std::make_unique<ui::BeginFrameSourceWayland>(Mitchell CohenWe want to hide the impl detail of the ozone platform.
I think it would better if we ask on the `platform_window()` to produce a `BeginFrameSource` extended by `BeginFrameSourceWayland`. life time is easier to manage if `platform_window()` owns the `begin_frame_source`.
This way you may also control that only toplevel can create the BF source.
Kramer GeI originally placed `BeginFrameSourceWayland` in the ozone host, but there's a problem: it needs access to `ui::Compositor` to issue frames over mojo. (The iOS frame source works the same way, which is why it lives in `content/browser`.)
So we would need to remove the dependency on `ui/compositor` with a new delegate/interface for this to work. I can try it.
Ah that is unfortunate. I guess we should probably put it in `ui/base/wayland/` then.
It has the same problem, `ui/compositor` depends on `ui/base/wayland`. I don't see a place to move the frame source without increasing complexity.
The Linux tree host already makes decisions based on Wayland vs. X11 and decides whether the compositor should use an external mojo frame source, so I think this placement is more justified than it seems at first. (An abstraction on `PlatformWindow` would also not be used by any other platforms.)
The biggest problem is leaking logic about toplevels right? Maybe the best solution is to get the driver working with all types of windows. ;)
Related details
- Kramer Ge
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitchell Cohen (Gerrit)
Mitchell Cohen added 1 comment![]( "Open in Gerrit")
if (frame_callback_freeze_detected_ &&Do you know if the beginframessource is behaving ok when browser is being captured by say https://meet.google.com, and goes into minimize/restore states?
Good question, it crashes when screen recording so I'll fine tune this more.
Mitchell Cohen (Gerrit)
Mitchell Cohen added 1 comment![]( "Open in Gerrit")
@fang...@chromium.org, it took me a long time to figure out why this was crashing for non-toplevel windows, when screen recording, etc. The solution was really simple: the source ID counter just needed to start at 1 instead of 0. New `ExternalBeginFrameSourceMojo` instances have their `original_source_id_` set to 0 until they receive their first frame, so we have to avoid broadcasting on ID 0 to prevent conflicts.
With the most recent change everything works with screen sharing , and there don't seem to be any conflicts with the freeze logic because the no-damage frame callbacks do not have buffers and are not affected by the issue.
Related details
- Kramer Ge
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitchell Cohen (Gerrit)
Mitchell Cohen added 1 comment![]( "Open in Gerrit")
base::ObserverList<WaylandFrameTimingObserver> frame_timing_observers_;`WaylandWindow` and `WaylandFrameManager` is 1:1, also 1:1 with `ui::Compositor`, so I did not expect more than 1 here.
May be the `WaylandBeginFrameSource` and `WaylandWindow` can be 1:N, where there is 1 `WBFS` per `WaylandWindow` tree.
Done
Related details
- Kramer Ge
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitchell Cohen (Gerrit)
Mitchell Cohen added 1 comment![]( "Open in Gerrit")
Mitchell CohenDo you know if the beginframessource is behaving ok when browser is being captured by say https://meet.google.com, and goes into minimize/restore states?
Good question, it crashes when screen recording so I'll fine tune this more.
Done
Kramer Ge (Gerrit)
Kramer Ge added 1 comment![]( "Open in Gerrit")
begin_frame_source_ = std::make_unique<ui::BeginFrameSourceWayland>(Mitchell CohenWe want to hide the impl detail of the ozone platform.
I think it would better if we ask on the `platform_window()` to produce a `BeginFrameSource` extended by `BeginFrameSourceWayland`. life time is easier to manage if `platform_window()` owns the `begin_frame_source`.
This way you may also control that only toplevel can create the BF source.
Kramer GeI originally placed `BeginFrameSourceWayland` in the ozone host, but there's a problem: it needs access to `ui::Compositor` to issue frames over mojo. (The iOS frame source works the same way, which is why it lives in `content/browser`.)
So we would need to remove the dependency on `ui/compositor` with a new delegate/interface for this to work. I can try it.
Mitchell CohenAh that is unfortunate. I guess we should probably put it in `ui/base/wayland/` then.
It has the same problem, `ui/compositor` depends on `ui/base/wayland`. I don't see a place to move the frame source without increasing complexity.
The Linux tree host already makes decisions based on Wayland vs. X11 and decides whether the compositor should use an external mojo frame source, so I think this placement is more justified than it seems at first. (An abstraction on `PlatformWindow` would also not be used by any other platforms.)
The biggest problem is leaking logic about toplevels right? Maybe the best solution is to get the driver working with all types of windows. ;)
ozone platform specific code best stay inside ozone, which is why I think it's best to have the `platformwindow` create a generic `beginframesource`, so that `desktop_window_tree_host_linux` does not have to include a `wayland` class. I think we should break the direct dependency unfortunately.
Related details
- Mitchell Cohen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitchell Cohen (Gerrit)
Mitchell Cohen added 1 comment![]( "Open in Gerrit")
begin_frame_source_ = std::make_unique<ui::BeginFrameSourceWayland>(Mitchell CohenWe want to hide the impl detail of the ozone platform.
I think it would better if we ask on the `platform_window()` to produce a `BeginFrameSource` extended by `BeginFrameSourceWayland`. life time is easier to manage if `platform_window()` owns the `begin_frame_source`.
This way you may also control that only toplevel can create the BF source.
Kramer GeI originally placed `BeginFrameSourceWayland` in the ozone host, but there's a problem: it needs access to `ui::Compositor` to issue frames over mojo. (The iOS frame source works the same way, which is why it lives in `content/browser`.)
So we would need to remove the dependency on `ui/compositor` with a new delegate/interface for this to work. I can try it.
Mitchell CohenAh that is unfortunate. I guess we should probably put it in `ui/base/wayland/` then.
Kramer GeIt has the same problem, `ui/compositor` depends on `ui/base/wayland`. I don't see a place to move the frame source without increasing complexity.
The Linux tree host already makes decisions based on Wayland vs. X11 and decides whether the compositor should use an external mojo frame source, so I think this placement is more justified than it seems at first. (An abstraction on `PlatformWindow` would also not be used by any other platforms.)
The biggest problem is leaking logic about toplevels right? Maybe the best solution is to get the driver working with all types of windows. ;)
ozone platform specific code best stay inside ozone, which is why I think it's best to have the `platformwindow` create a generic `beginframesource`, so that `desktop_window_tree_host_linux` does not have to include a `wayland` class. I think we should break the direct dependency unfortunately.
OK, I got this working nicely in patchset 13. I chose to implement a separate `PlatformWindow` extension and created an adapter in `ui/compositor` to use as a delegate. This made it possible to move the frame source over to `WaylandWindow` and remove all special code from the Linux tree host, at the cost of two small abstractions.
Related details
- Kramer Ge
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitchell Cohen (Gerrit)
Mitchell Cohen added 1 comment![]( "Open in Gerrit")
Related details
- Kramer Ge
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Kramer Ge added 5 comments![]( "Open in Gerrit")
Thanks! I'm going to run `chrome://tracing` with `viz` and `wayland` categories to check some known bad cases.
if (!feedback.interval.is_zero()) {I think we want to set `vsync_interval_` back to default explicitly, in case a window is moved from a 120hz display to a variable refresh display, it will attempt to consistently stay at 60hz.
deadline = now.SnappedToNextTick(last_presentation_time_, vsync_interval_);I believe if we know the definitive deadline, chrome prefers to set `frame_time = deadline - vsync_interval_`, and treat the period between logical `frame_time` and `now` as delay to propagate the vsync signal.
int frame_callback_freeze_detected_ = false;this should be `bool`.
frame_timing_observer_ = observer;nit: Add a `DCHECK(!frame_timing_observer_);`
Related details
- Mitchell Cohen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitchell Cohen (Gerrit)
Mitchell Cohen added 3 comments![]( "Open in Gerrit")
if (!feedback.interval.is_zero()) {I think we want to set `vsync_interval_` back to default explicitly, in case a window is moved from a 120hz display to a variable refresh display, it will attempt to consistently stay at 60hz.
Done
int frame_callback_freeze_detected_ = false;this should be `bool`.
Done
frame_timing_observer_ = observer;Mitchell Cohennit: Add a `DCHECK(!frame_timing_observer_);`
Done
Related details
- Kramer Ge
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitchell Cohen (Gerrit)
Mitchell Cohen added 8 comments![]( "Open in Gerrit")
Done
// Copyright 2026 The Chromium AuthorsMitchell CohenI think this should live inside `ui/ozone/platform/wayland/host/`.
Done
// There won't be a frame callback with no damage so schedule one.Kramer Geis this something that could be changed on the WaylandFrameManager side?
Mitchell CohenI think if you want you can apply an empty commit in `waylandframemanager` in that case to manually trigger next `frame_callback`.
Thanks for this suggestion, I implemented the empty commit in patchset 4 and performance is much more consistent. It rarely falls back to the timer now (usually once when switching tabs).
Done
// Safety fallback in case the frame callback doesn't arrive (e.g., windowKramer GeThis fallback mechanism doesn't feel great to me and it's the part I struggled with the most in this PoC.
- If it runs on every presentation feedback interval, or close to it, we basically end up reinventing a delay-based pacer, and Wayland frame callbacks are useless. - - But if it's on a longer delay, Chrome freezes while waiting for frame callbacks that never come (common when switching tabs).
So the current approach of frequently needing to schedule frames in OnBeginFrameAck conflicts with the idea of frames being event-driven. There's probably a more sophisticated way to deal with missing frame callbacks and/or to make sure fewer are dropped.
Mitchell CohenLike switch to delay-based beginframe that refers to present time interval, can switch back to callback-based when callback catches up to the most recent frame.
Does chrome freeze currently when switching tabs? I imagine the same issue exists as of now if frame callback does not come in time.
Done
Done
deadline = now.SnappedToNextTick(last_presentation_time_, vsync_interval_);Mitchell CohenI believe if we know the definitive deadline, chrome prefers to set `frame_time = deadline - vsync_interval_`, and treat the period between logical `frame_time` and `now` as delay to propagate the vsync signal.
Good call, now `frame_time` and `deadline` are aligned to presentation time.
std::unique_ptr<ui::BeginFrameSourceWayland> begin_frame_source_;The tree host seems like a strange place to own this driver, and other platforms don't use it this way, but this placement had several advantages: the tree host can pre-configure the compositor to use the external frame source with existing params (that for some reason aren't used by other impls), and it has the earliest access to the compositor, PlatformWindow and Wayland extension.
Done
begin_frame_source_ = std::make_unique<ui::BeginFrameSourceWayland>(Mitchell CohenWe want to hide the impl detail of the ozone platform.
I think it would better if we ask on the `platform_window()` to produce a `BeginFrameSource` extended by `BeginFrameSourceWayland`. life time is easier to manage if `platform_window()` owns the `begin_frame_source`.
This way you may also control that only toplevel can create the BF source.
Kramer GeI originally placed `BeginFrameSourceWayland` in the ozone host, but there's a problem: it needs access to `ui::Compositor` to issue frames over mojo. (The iOS frame source works the same way, which is why it lives in `content/browser`.)
So we would need to remove the dependency on `ui/compositor` with a new delegate/interface for this to work. I can try it.
Mitchell CohenAh that is unfortunate. I guess we should probably put it in `ui/base/wayland/` then.
Kramer GeIt has the same problem, `ui/compositor` depends on `ui/base/wayland`. I don't see a place to move the frame source without increasing complexity.
The Linux tree host already makes decisions based on Wayland vs. X11 and decides whether the compositor should use an external mojo frame source, so I think this placement is more justified than it seems at first. (An abstraction on `PlatformWindow` would also not be used by any other platforms.)
The biggest problem is leaking logic about toplevels right? Maybe the best solution is to get the driver working with all types of windows. ;)
Mitchell Cohenozone platform specific code best stay inside ozone, which is why I think it's best to have the `platformwindow` create a generic `beginframesource`, so that `desktop_window_tree_host_linux` does not have to include a `wayland` class. I think we should break the direct dependency unfortunately.
OK, I got this working nicely in patchset 13. I chose to implement a separate `PlatformWindow` extension and created an adapter in `ui/compositor` to use as a delegate. This made it possible to move the frame source over to `WaylandWindow` and remove all special code from the Linux tree host, at the cost of two small abstractions.
Done
ui::GetWaylandToplevelExtension(*platform_window()) != nullptr;When I tried to enable the driver for all windows (not just toplevels) I ran into failed DCHECKs in the Viz process. That surprised me because each surface has its own compositor and paces its own frames. I was able to get it to work by removing the check and clearing the frame sink in Viz, but that change seemed too drastic and could not be contained to the PoC.
Done
Related details
- Kramer Ge
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitchell Cohen (Gerrit)
Mitchell Cohen added 1 comment![]( "Open in Gerrit")
@fang...@chromium.org, it took me a long time to figure out why this was crashing for non-toplevel windows, when screen recording, etc. The solution was really simple: the source ID counter just needed to start at 1 instead of 0. New `ExternalBeginFrameSourceMojo` instances have their `original_source_id_` set to 0 until they receive their first frame, so we have to avoid broadcasting on ID 0 to prevent conflicts.
With the most recent change everything works with screen sharing , and there don't seem to be any conflicts with the freeze logic because the no-damage frame callbacks do not have buffers and are not affected by the issue.
Done
Kramer Ge (Gerrit)
Kramer Ge added 3 comments![]( "Open in Gerrit")
I checked some traces on real websites on mutter with 60hz monitor, this feature reduces `Graphics.Pipeline.DrawAndSwap` slice from 36.9ms to 28.2ms, 28ms is still kind of long but I notice that might be because mutter can reserve as long as 12ms from latch to vsync for itself. Without sufficient information about latch timing, chrome attempts to submit its frame within the first `4.6ms=16.6ms-12ms` of the `wl_frame.callback`, which results in the frame to be stored by mutter for as long as `12ms+16.6ms=28.6ms`.
Perhaps we can experiment using `wl_frame.callback` as latch timing, but this is a great step in the right direction.
+oshima@ for
ui/base/
ui/compositor/
ui/platform_window/
+thomasanderson@ for
ui/views/
TRACE_EVENT1("ui", "BeginFrameSourceWayland::SetNeedsBeginFrame",nit: I think this is better as "wayland"
DVLOG(1) << "MaybeIssueBeginFrame: interval=" << vsync_interval_
<< " deadline=" << deadline << " presentation_feedback delta="
<< (deadline - (now + vsync_interval_)).InMicroseconds() << "us";nit: can you unify the units used in this logline?
And IMO `deadline` as a generic timestamp does not have as much information, probably more informative as `deadline in = deadline - now`.
Related details
- Mitchell Cohen
- Mitsuru Oshima
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Anderson (Gerrit)
Thomas Anderson voted and added 2 comments![]( "Open in Gerrit")
Votes added by Thomas Anderson
| Code-Review | +1 |
2 comments
ui/views/widget/desktop_aura/desktop_window_tree_host_platform.* lgtm
begin_frame_adapter_.reset();Does this need the following before reset?
```
compositor()->SetExternalBeginFrameControllerClientFactory(nullptr);
```
Related details
- Mitchell Cohen
- Mitsuru Oshima
Mitchell Cohen (Gerrit)
Mitchell Cohen added 3 comments![]( "Open in Gerrit")
TRACE_EVENT1("ui", "BeginFrameSourceWayland::SetNeedsBeginFrame",nit: I think this is better as "wayland"
Done
DVLOG(1) << "MaybeIssueBeginFrame: interval=" << vsync_interval_
<< " deadline=" << deadline << " presentation_feedback delta="
<< (deadline - (now + vsync_interval_)).InMicroseconds() << "us";nit: can you unify the units used in this logline?
And IMO `deadline` as a generic timestamp does not have as much information, probably more informative as `deadline in = deadline - now`.
I improved the logging and units across the file and also split logs that happen normally on every frame into DVLOG(2)
Does this need the following before reset?
```
compositor()->SetExternalBeginFrameControllerClientFactory(nullptr);
```
- Kramer Ge
- Mitsuru Oshima
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitchell Cohen (Gerrit)
Mitchell Cohen added 1 comment![]( "Open in Gerrit")
I checked some traces on real websites on mutter with 60hz monitor, this feature reduces `Graphics.Pipeline.DrawAndSwap` slice from 36.9ms to 28.2ms, 28ms is still kind of long but I notice that might be because mutter can reserve as long as 12ms from latch to vsync for itself. Without sufficient information about latch timing, chrome attempts to submit its frame within the first `4.6ms=16.6ms-12ms` of the `wl_frame.callback`, which results in the frame to be stored by mutter for as long as `12ms+16.6ms=28.6ms`.
Perhaps we can experiment using `wl_frame.callback` as latch timing, but this is a great step in the right direction.
+oshima@ for
ui/base/
ui/compositor/
ui/platform_window/+thomasanderson@ for
ui/views/
@fang...@chromium.org is the idea that frame_time/deadline would be snapped to the callback time instead of presentation feedback? (I'm currently not using the callback timestamp for anything.)
Kramer Ge (Gerrit)
Kramer Ge voted and added 3 comments![]( "Open in Gerrit")
Votes added by Kramer Ge
| Code-Review | +1 |
3 comments
Kramer Ge@fang...@chromium.org would you be able to take another look?
Acknowledged
Mitchell CohenI checked some traces on real websites on mutter with 60hz monitor, this feature reduces `Graphics.Pipeline.DrawAndSwap` slice from 36.9ms to 28.2ms, 28ms is still kind of long but I notice that might be because mutter can reserve as long as 12ms from latch to vsync for itself. Without sufficient information about latch timing, chrome attempts to submit its frame within the first `4.6ms=16.6ms-12ms` of the `wl_frame.callback`, which results in the frame to be stored by mutter for as long as `12ms+16.6ms=28.6ms`.
Perhaps we can experiment using `wl_frame.callback` as latch timing, but this is a great step in the right direction.
+oshima@ for
ui/base/
ui/compositor/
ui/platform_window/+thomasanderson@ for
ui/views/
@fang...@chromium.org is the idea that frame_time/deadline would be snapped to the callback time instead of presentation feedback? (I'm currently not using the callback timestamp for anything.)
Yeah, though I'm not settled on what to do with it, as the callback arrival time is not at a consistent phase offset from the vsync time on mutter, and other compositors may behave differently as well.
Related details
- Mitchell Cohen
- Mitsuru Oshima
- Thomas Anderson
Thomas Anderson (Gerrit)
Thomas Anderson voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Mitchell Cohen
- Mitsuru Oshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitsuru Oshima (Gerrit)
Mitsuru Oshima voted and added 4 comments![]( "Open in Gerrit")
Votes added by Mitsuru Oshima
| Code-Review | +1 |
4 comments
ui/base/, ui/compositor/, ui/platform_window/ lgtm
if (is_linux || is_fuchsia) {Oh does fuchsia support Wayland ?
~ExternalBeginFrameAdapter() override;super nit: move dtor after deletes.
Related details
- Mitchell Cohen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitchell Cohen (Gerrit)
Mitchell Cohen added 1 comment![]( "Open in Gerrit")
if (is_linux || is_fuchsia) {Oh does fuchsia support Wayland ?
Fuchsia builds the platform tree host which owns the adapter. It will only build the interface, not the Wayland impl.
Related details
- Mitsuru Oshima
Mitsuru Oshima (Gerrit)
Mitsuru Oshima added 1 comment![]( "Open in Gerrit")
if (is_linux || is_fuchsia) {Mitchell CohenOh does fuchsia support Wayland ?
Fuchsia builds the platform tree host which owns the adapter. It will only build the interface, not the Wayland impl.
- Mitchell Cohen
Mitsuru Oshima (Gerrit)
Mitsuru Oshima added 1 comment![]( "Open in Gerrit")
if (is_linux || is_fuchsia) {Mitchell CohenOh does fuchsia support Wayland ?
Mitsuru OshimaFuchsia builds the platform tree host which owns the adapter. It will only build the interface, not the Wayland impl.
Hm, is it possible to exclude using buildflag?
Maybe the right question is if this make sense for other OZONE platforms.
Mitchell Cohen (Gerrit)
Mitchell Cohen added 1 comment![]( "Open in Gerrit")
if (is_linux || is_fuchsia) {Mitchell CohenOh does fuchsia support Wayland ?
Mitsuru OshimaFuchsia builds the platform tree host which owns the adapter. It will only build the interface, not the Wayland impl.
Mitsuru OshimaHm, is it possible to exclude using buildflag?
Maybe the right question is if this make sense for other OZONE platforms.
The issue I ran into is that `desktop_window_tree_host_platform` owns the adapter and creates the compositor, and it's compiled (only!) for `is_linux || is_fuchsia` in `ui/views/BUILD.gn`.
(I was surprised to see that this file isn't really cross-platform and the other desktop tree hosts don't inherit from it. Maybe it used to be?)
I could try moving the adapter down to `desktop_window_tree_host_linux` or somewhere else but it's a bit more of refactor.
Related details
- Mitsuru Oshima
Mitsuru Oshima (Gerrit)
Mitsuru Oshima added 1 comment![]( "Open in Gerrit")
if (is_linux || is_fuchsia) {Mitchell CohenOh does fuchsia support Wayland ?
Mitsuru OshimaFuchsia builds the platform tree host which owns the adapter. It will only build the interface, not the Wayland impl.
Mitsuru OshimaHm, is it possible to exclude using buildflag?
Mitchell CohenMaybe the right question is if this make sense for other OZONE platforms.
The issue I ran into is that `desktop_window_tree_host_platform` owns the adapter and creates the compositor, and it's compiled (only!) for `is_linux || is_fuchsia` in `ui/views/BUILD.gn`.
(I was surprised to see that this file isn't really cross-platform and the other desktop tree hosts don't inherit from it. Maybe it used to be?)
I could try moving the adapter down to `desktop_window_tree_host_linux` or somewhere else but it's a bit more of refactor.
The original plan (AFAIK) was Window's will also migrate to OZONE, but it was abandoned. (and CrOS isn't using desktop aura).
You may land this as is, but it'd be nice if we can limit this to the platform that makes sense.
Related details
- Mitchell Cohen
Mitsuru Oshima (Gerrit)
Mitsuru Oshima added 1 comment![]( "Open in Gerrit")
slgtm
Mitchell Cohen (Gerrit)
Mitchell Cohen added 2 comments![]( "Open in Gerrit")
super nit: move dtor after deletes.
- Kramer Ge
- Mitsuru Oshima
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitchell Cohen (Gerrit)
Mitchell Cohen added 1 comment![]( "Open in Gerrit")
Mitchell CohenI checked some traces on real websites on mutter with 60hz monitor, this feature reduces `Graphics.Pipeline.DrawAndSwap` slice from 36.9ms to 28.2ms, 28ms is still kind of long but I notice that might be because mutter can reserve as long as 12ms from latch to vsync for itself. Without sufficient information about latch timing, chrome attempts to submit its frame within the first `4.6ms=16.6ms-12ms` of the `wl_frame.callback`, which results in the frame to be stored by mutter for as long as `12ms+16.6ms=28.6ms`.
Perhaps we can experiment using `wl_frame.callback` as latch timing, but this is a great step in the right direction.
+oshima@ for
ui/base/
ui/compositor/
ui/platform_window/+thomasanderson@ for
ui/views/
Kramer Ge@fang...@chromium.org is the idea that frame_time/deadline would be snapped to the callback time instead of presentation feedback? (I'm currently not using the callback timestamp for anything.)
Yeah, though I'm not settled on what to do with it, as the callback arrival time is not at a consistent phase offset from the vsync time on mutter, and other compositors may behave differently as well.
Acknowledged
Related details
- Kramer Ge
- Mitsuru Oshima
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Anderson (Gerrit)
Thomas Anderson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Thomas Anderson
| Code-Review | +1 |
- Kramer Ge
- Mitchell Cohen
- Mitsuru Oshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Kramer Ge voted Code-Review+1![]( "Open in Gerrit")
- Mitchell Cohen
- Mitsuru Oshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitchell Cohen (Gerrit)
Mitchell Cohen added 1 comment![]( "Open in Gerrit")
Mitchell Cohenslgtm
One more time please, I addressed the other nits. :)
Related details
- Mitsuru Oshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitsuru Oshima (Gerrit)
Mitsuru Oshima voted Code-Review+1![]( "Open in Gerrit")
- Mitchell Cohen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitchell Cohen (Gerrit)
Mitchell Cohen added 1 comment![]( "Open in Gerrit")
Mitchell Cohenslgtm
One more time please, I addressed the other nits. :)
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitchell Cohen (Gerrit)
Mitchell Cohen voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
BeginFrameSource implementation for Wayland
This is a compositor-driven frame source for Wayland which uses frame
callbacks and presentation feedback to pace frames more accurately and
efficiently than the delay-based timer currently used on Linux. It
supports all windows (toplevels, tooltips, popups, etc.) and is behind a
feature flag, kWaylandExternalBeginFrameSource.
- M ui/base/ui_base_features.cc
- M ui/base/ui_base_features.h
- M ui/compositor/BUILD.gn
- A ui/compositor/external_begin_frame_adapter.cc
- A ui/compositor/external_begin_frame_adapter.h
- M ui/ozone/platform/wayland/BUILD.gn
- A ui/ozone/platform/wayland/host/begin_frame_source_wayland.cc
- A ui/ozone/platform/wayland/host/begin_frame_source_wayland.h
- M ui/ozone/platform/wayland/host/wayland_frame_manager.cc
- M ui/ozone/platform/wayland/host/wayland_frame_manager.h
- M ui/ozone/platform/wayland/host/wayland_window.cc
- M ui/ozone/platform/wayland/host/wayland_window.h
- M ui/platform_window/BUILD.gn
- A ui/platform_window/extensions/begin_frame_source_extension.cc
- A ui/platform_window/extensions/begin_frame_source_extension.h
- M ui/views/widget/desktop_aura/desktop_window_tree_host_platform.cc
- M ui/views/widget/desktop_aura/desktop_window_tree_host_platform.h
Code-Review: +1 by Mitsuru Oshima, +1 by Kramer Ge, +1 by Thomas Anderson
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |