Bug 161683 - [GTK] Clarify frame callbacks behaviour in Wayland compositor
Summary: [GTK] Clarify frame callbacks behaviour in Wayland compositor
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2016-09-07 02:11 PDT by Emanuele Aina
Modified: 2016-09-07 08:56 PDT (History)
4 users (show)

See Also:

Patch (1.88 KB, patch)
2016-09-07 02:12 PDT, Emanuele Aina
no flags Details | Formatted Diff | Diff
Patch (1.92 KB, patch)
2016-09-07 06:20 PDT, Emanuele Aina
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emanuele Aina 2016-09-07 02:11:48 PDT
The way we fire frame callbacks in the nested Wayland compositor can be puzzling to developers expecting Wayland semantics, but since we have our own mechanism to handle synchronization we don't care much
about them. Adding a comment would avoid surprised Wayland developers.
Comment 1 Emanuele Aina 2016-09-07 02:12:50 PDT
Created attachment 288124 [details]
Comment 2 Carlos Garcia Campos 2016-09-07 02:33:03 PDT
Comment on attachment 288124 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=288124&action=review

> w/Source/WebKit2/UIProcess/gtk/WaylandCompositor.cpp:237
> +    // From a Wayland point-of-view firing frame callbacks here is very weird,
> +    // but our WebProcess clients don't rely on them for synchronization and
> +    // set the EGL swap interval to zero, relying on the RenderNextFrame events
> +    // in the WebKit IPC instead

The comment adds more confusion to me. It says that the firing frame callbacks here is weird, but it doesn't say why or where they should be fired. The only reason why I didn't remove all frame callbacks handling from the nested compositor is because I was not sure we can rely that eglSwapInterval is always going to work for all drivers. But if it works, frame callbacks are never emitted, so this list is always empty. The reason why we do this, is not because we rely on RenderNextFrame events, adn there isn't any WebKit IPC involved. The reason is that we are rendering to an offscreen context always in the web process, and when we have to render a new frame, we schedule a redraw in the web view, and then we render in the screen. So syncing to vblank in the web process doesn't help, because we will not render at that time to the screen, we will render into the offscreen context, and the  schedule a redraw on the widget. It's at that point where we want to vblank sync, and that's already done by GTK+. This avoids unnecessary throttling in the web process. When there's a new frame rendered in the web process, eglSwapBuffers is what triggers the repaint in the nested compositor using the wayland protocol, not the WebKit IPC.
Comment 3 Emanuele Aina 2016-09-07 02:48:03 PDT
Frame callbacks should be fired where you know the compositor has used the committed content. E.g. in Weston it the place where it has queued the GL commands to update the screen and so the next screen update contents have been locked in place.

To be specific, it should not just be any screen update, it should be the particular update that first uses the content from the same commit as where the frame callback was filed. Frame callbacks should also not be fired if the surface is not shown.

What I was trying to say is that despite frame callbacks being the main throttling mechanism in normal Wayland usage, we don't need them for that purpose as throttling is already handled elsewhere by WebKit.
Comment 4 Emanuele Aina 2016-09-07 06:20:13 PDT
Created attachment 288131 [details]
Comment 5 Carlos Garcia Campos 2016-09-07 06:37:33 PDT
Comment on attachment 288131 [details]

Much clearer, thanks!
Comment 6 WebKit Commit Bot 2016-09-07 08:56:38 PDT
Comment on attachment 288131 [details]

Clearing flags on attachment: 288131

Committed r205547: <http://trac.webkit.org/changeset/205547>
Comment 7 WebKit Commit Bot 2016-09-07 08:56:41 PDT
All reviewed patches have been landed.  Closing bug.