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. |