GNOME Bugzilla – Bug 685463
Implement compositor <-> application frame synchronization
Last modified: 2013-02-13 14:54:05 UTC
This patchset implements the proposal for synchronizing compositor drawing with the application in: https://mail.gnome.org/archives/wm-spec-list/2011-October/msg00006.html Not implemented at the moment are: - The integration with GLX/XSync fences needed for NVIDIA's GL implementation - Timing information sent in _NET_WM_FRAME_DRAWN
Created attachment 225760 [details] [review] MetaWindowActor: Use guint for bitfields A 1-bit boolean (int) bitfield has the values 0 and -1. Use guint instead for bitfield values.
Created attachment 225761 [details] [review] Implement freezing of updates during resize Replace the unused meta_compositor_set_updates() with a reversed-meaning meta_compositor_set_updates_frozen(), and use it to implement freezing application window updates during interactive resizing. This avoids drawing new areas of the window with blank content before the application has a chance to repaint.
Created attachment 225762 [details] [review] Move sync alarms to be per-window and permanent Instead of creating a new alarm each time we resize a window interactively, create an alarm the first time we resize a window and keep it around permanently until we unmanage the window. Doing it this way will be useful when we allow the application to spontaneously generate sync request updates to indicate frames it is drawing.
Created attachment 225763 [details] [review] Support properties with lists of XSyncCounter Add META_PROP_VALUE_SYNC_COUNTER_LIST for a property that contains multiple XSyncCounter values.
Created attachment 225764 [details] [review] Add support for an extended style of _NET_WM_SYNC_REQUEST_COUNTER If an application provides two values in _NET_WM_SYNC_REQUEST_COUNTER, use that as a signal that the applications wants an extended behavior where it can update the counter as well as the window manager. If the application updates the counter to an odd value, updates of the window are frozen until the counter is updated again to an even value.
Created attachment 225765 [details] [review] Send _NET_WM_FRAME_DRAWN messages When the application provides the extended second counter for _NET_WM_SYNC_REQUEST, send a client message with completion information after the next redraw after each counter update by the application.
Created attachment 225766 [details] [review] MetaWindow: always resize the frame first when we have synchronization Resizing the frame triggers creation of a new backing pixmap for the window, so we should do that first before we resize the client window and mess up the contents of the old backing pixmap.
Created attachment 225767 [details] [review] Handle _NET_WM_SYNC_REQUEST_COUNTER updates without redraw It's possible that a client might update the (extended) _NET_WM_SYNC_REQUEST_COUNTER counter twice without actually drawing anything. In that case, we still should send a _NET_WM_FRAME_DRAWN message since it's hard for a client to know every case in which no damage is generated. For now, do it the easy way by forcing a stage repaint.
*** Bug 587255 has been marked as a duplicate of this bug. ***
(In reply to comment #8) > Created an attachment (id=225767) [details] [review] > Handle _NET_WM_SYNC_REQUEST_COUNTER updates without redraw > > It's possible that a client might update the (extended) > _NET_WM_SYNC_REQUEST_COUNTER counter twice without actually drawing > anything. In that case, we still should send a _NET_WM_FRAME_DRAWN > message since it's hard for a client to know every case in which > no damage is generated. For now, do it the easy way by forcing a > stage repaint. Why would a client do this, why should we respect it, and why should we force an unclipped full-stage redraw?
Review of attachment 225766 [details] [review]: Shouldn't we check that we're composited in this case, or do we want to rip out the non-composited code paths in mutter? Code looks fine, comment is good and makes sense.
Review of attachment 225765 [details] [review]: ::: src/compositor/compositor.c @@ +526,3 @@ meta_screen_set_cm_selection (screen); + info->stage = clutter_stage_get_default (); um? It's fairly clear clutter_stage_get_default(); is going away for 2.0. Why the change back? Bad rebase? ::: src/compositor/meta-window-actor.c @@ +2410,3 @@ + ev.format = 32; + ev.data.l[0] = display->atom__NET_WM_FRAME_DRAWN; + ev.data.l[1] = 0; /* timestamp */ Seems fishy? Should this be CurrentTime? @@ +2413,3 @@ + ev.data.l[2] = priv->frame_drawn_serial & G_GUINT64_CONSTANT(0xffffffff); + ev.data.l[3] = priv->frame_drawn_serial >> 32; + ev.data.l[4] = 0; /* vblank estimate */ A more complete comment about this would be nice.
Review of attachment 225760 [details] [review]: Makes sense.
Review of attachment 225761 [details] [review]: Looks good for the most part. ::: src/compositor/meta-window-actor.c @@ +956,3 @@ + /* We ignores resizes on frozen windows */ + meta_window_actor_sync_actor_position (self); "ignores resizes" It's also strange to be talking about resizes when you're calling sync_actor_position. I know that a resize can change the position of a window, but it's still a double-take causer. @@ +1521,3 @@ + meta_window_actor_set_updates_frozen (self, + meta_window_updates_are_frozen (priv->window)); It seems strange to be "pulling" from this property on the MetaWindow, when the only way it can possibly be set is to pass it through the compositor. Is this necessary? @@ +2003,3 @@ * texture regardless of damage event handling. */ + ? ::: src/core/window.c @@ +4444,3 @@ + gboolean updates_frozen) +{ + if (updates_frozen != window->updates_frozen_for_resize) Having the bail-out equality checks in both MetaWindowActor and MetaWindow seems like it's overly cautious. Do you need the state to be tracked in both places?
Review of attachment 225762 [details] [review]: Looks fine.
Review of attachment 225763 [details] [review]: Sure.
Review of attachment 225764 [details] [review]: A link to the appropriate parts of the EWMH, when pushed, would be nice. ::: src/core/window.c @@ +4419,3 @@ + + window->sync_request_serial = + XSyncValueLow32 (init) + ((gint64)XSyncValueHigh32 (init) << 32); Don't you just love Xlib? @@ +4539,3 @@ { +#ifdef HAVE_XSYNC + if (window->sync_request_serial % 2 == 1) Shouldn't this check for extended_sync_request_counter? It doesn't seem like this matches the old style of counter at all -- we don't care about the value at all in that case, only if we've seen an alarm indicating the client has caught up. @@ +9365,3 @@ + meta_window_updates_are_frozen (window)); + + if (window->display->grab_op != META_GRAB_OP_NONE && The meta_grab_op_is_mouse below should take care of this. @@ +9367,3 @@ + if (window->display->grab_op != META_GRAB_OP_NONE && + window == window->display->grab_window && + meta_grab_op_is_mouse (window->display->grab_op) && Why do we need to care about non-resizing grab ops? And why do we not care about keyboard grab ops? The large switch below doesn't care about other kinds of mouse ops, but cares about keyboard resize events.
Review of attachment 225767 [details] [review]: Why would a client do this, why should we respect it, and why should we force an unclipped full-stage redraw?
I just tested this, using part of the wip/frame-synchronization branch from mutter (up to but not including frame timings) + wip/frame-synchronization from gtk+, both rebased on master. I'm not sure where the problem is, but this branch will cause windows to get stuck if they change the maximization/tiling status during a mouse op. Reverting mutter fixes the problem. In the logs I see this: Window manager warning: Log level 8: gdk_x11_window_begin_frame: assertion `GDK_IS_WINDOW (window)' failed Window manager warning: Log level 8: gdk_window_process_updates_with_mode: assertion `GDK_IS_WINDOW (window)' failed Window manager warning: Log level 8: gdk_x11_window_end_frame: assertion `GDK_IS_WINDOW (window)' failed Additionally, but this is a minor bug, if the WM is restarted while the application is painting a frame, gtk will never thaw the paint clock and thus the application will hang. PS: you forgot one file (cogl-output.c) in the cogl branch, so I could not test the cogl and clutter changes.
(In reply to comment #12) > Review of attachment 225765 [details] [review]: > > ::: src/compositor/compositor.c > @@ +526,3 @@ > meta_screen_set_cm_selection (screen); > > + info->stage = clutter_stage_get_default (); > > um? > > It's fairly clear clutter_stage_get_default(); is going away for 2.0. Why the > change back? Bad rebase? Yes, bad rebase. Will be fixed when I post a new set of patches. > ::: src/compositor/meta-window-actor.c > @@ +2410,3 @@ > + ev.format = 32; > + ev.data.l[0] = display->atom__NET_WM_FRAME_DRAWN; > + ev.data.l[1] = 0; /* timestamp */ > > Seems fishy? Should this be CurrentTime? > > @@ +2413,3 @@ > + ev.data.l[2] = priv->frame_drawn_serial & > G_GUINT64_CONSTANT(0xffffffff); > + ev.data.l[3] = priv->frame_drawn_serial >> 32; > + ev.data.l[4] = 0; /* vblank estimate */ > > A more complete comment about this would be nice. Newer patches completely redo what gets sent in FRAME_DRAWN and add a new FRAME_TIMINGS message.
(In reply to comment #10) > (In reply to comment #8) > > Created an attachment (id=225767) [details] [review] [details] [review] > > Handle _NET_WM_SYNC_REQUEST_COUNTER updates without redraw > > > > It's possible that a client might update the (extended) > > _NET_WM_SYNC_REQUEST_COUNTER counter twice without actually drawing > > anything. In that case, we still should send a _NET_WM_FRAME_DRAWN > > message since it's hard for a client to know every case in which > > no damage is generated. For now, do it the easy way by forcing a > > stage repaint. > > Why would a client do this, why should we respect it, and why should we force > an unclipped full-stage redraw? The current language I have in the WM spec (http://fishsoup.net/misc/wm-spec-synchronization.html) The window manager SHOULD redraw the window and send the messages even if no damage events were received. Rational: the reason for redrawing the window even if no damage events have been received is so that there is consistent timing for each frame, even if one frame happens to involve no changes to the window. It is recommended to behave as if a damage event was received containing only a single pixel. Basically, a toolkit that has a corner case that draws nothing shouldn't freeze or go into a tight 100% CPU loop. I was thinking of the ensure_redraw() as a way of getting the pre-paint and post-paint callbacks without actually doing anything, but a) that wouldn't get the right timings b) you are right that it does a full-stage redraw. My next set of patches will have: /* A frame was marked by the client without actually doing any damage; * we need to make sure that the pre_paint/post_paint functions * get called, enabling us to send a _NET_WM_FRAME_DRAWN. We do a * 1-pixel redraw to get consistent timing with non-empty frames. */ if (self->priv->mapped && !self->priv->needs_pixmap) { const cairo_rectangle_int_t clip = { 0, 0, 1, 1 }; clutter_actor_queue_redraw_with_clip (self->priv->actor, &clip); }
(In reply to comment #11) > Review of attachment 225766 [details] [review]: > > Shouldn't we check that we're composited in this case, or do we want to rip out > the non-composited code paths in mutter? > > Code looks fine, comment is good and makes sense. Hmm. * I have no interest in Mutter as a non-composting window manager. I think a basic principle of Mutter is that it is exposing a Clutter scene graph to the library user. * I do have a theoretical interest in enabling the unredirection not just of fullscreen windows but of other windows. If a client wants absolute minimum latency, compositing does not give you that - it has an inherent 1-frame latency penalty. I think, however, this does not involve non-composited code paths - just punching a hole in the COW over the contents (not frame) of the window and suppressing redraws when the only update is in the hole. * Since I think a check for "compositor is running" will always be TRUE, I don't think there's a point in adding it here. * We don't actually have much in the way of "non-composited code paths" in Mutter - maybe about 15 places we do 'if (window->display->compositor) { call_some_compositor_function(); }' We can rip out the checks at some convenient time, but it's not a big burden on the code base.
(In reply to comment #14) > Review of attachment 225761 [details] [review]: > > Looks good for the most part. > > ::: src/compositor/meta-window-actor.c > @@ +956,3 @@ > > + /* We ignores resizes on frozen windows */ > + meta_window_actor_sync_actor_position (self); > > "ignores resizes" Fixed. > It's also strange to be talking about resizes when you're calling > sync_actor_position. I know that a resize can change the position of a window, > but it's still a double-take causer. I'll change the comment to: /* We ignore moves and resizes on frozen windows */ there's a bit of weirdness here that I'm not sure that is right - we're ignoring moves for windows that are frozen for the effect API, but for windows that are frozen because of the frame counter, we really should just ignore the resizes, but allow pure moves - so a window that is stuck in a drawing state is still draggable by the title bar. I don't want to deal with the splitting the frozen state in half, so I'm just going to forget that I noticed that and if it matters it will be noticed again ;-) > @@ +1521,3 @@ > > + meta_window_actor_set_updates_frozen (self, > + meta_window_updates_are_frozen > (priv->window)); > > It seems strange to be "pulling" from this property on the MetaWindow, when the > only way it can possibly be set is to pass it through the compositor. Is this > necessary? This is just the standard thing - to track a property you have to a) get the initial value b) watch changes - done in compositor.c. It would look more natural if change notification were done with signals. > @@ +2003,3 @@ > * texture regardless of damage event handling. > */ > + > > ? Fixed. > ::: src/core/window.c > @@ +4444,3 @@ > + gboolean updates_frozen) > +{ > + if (updates_frozen != window->updates_frozen_for_resize) > > Having the bail-out equality checks in both MetaWindowActor and MetaWindow > seems like it's overly cautious. Do you need the state to be tracked in both > places? I think anywhere you have a setter, it's good to have checks to avoid work on no-op changes - 90% of the time it's a good idea, and the other 10% of the time you need to do global examination of the code to figure that out. The point of having the checks in the setter is so that nobody ever writes the checks *outside* the setter to optimize away problems that occur: if (o.get_bar() != new_bar) o.set_bar(new_bar) is very damaging to code quality. The reason we track a variable in meta-window-actor.c is again a question of writing code that is locally obviously correct. If the code was: void meta_window_actor_set_updates_frozen (MetaWindowActor *self, gboolean updates_frozen) { MetaWindowActorPrivate *priv = self->priv; if (updates_frozen) meta_window_actor_freeze (self); else meta_window_actor_thaw (self); } then someone looking at it would worry about what would happen if set_updates_frozen() was called multiple times in the row with the same value.
(In reply to comment #17) > Review of attachment 225764 [details] [review]: > > A link to the appropriate parts of the EWMH, when pushed, would be nice. The one link that we have currently: http://www.freedesktop.org/standards/wm-spec/1.2/html/x109.html shows the problems - we don't really have stable links. But I'll see what I can do once the wm-spec changes are finalized. > @@ +4539,3 @@ > { > +#ifdef HAVE_XSYNC > + if (window->sync_request_serial % 2 == 1) > > Shouldn't this check for extended_sync_request_counter? It doesn't seem like > this matches the old style of counter at all -- we don't care about the value > at all in that case, only if we've seen an alarm indicating the client has > caught up. Good catch, will fix. > @@ +9365,3 @@ > + meta_window_updates_are_frozen > (window)); > + > + if (window->display->grab_op != META_GRAB_OP_NONE && > > The meta_grab_op_is_mouse below should take care of this. > > @@ +9367,3 @@ > + if (window->display->grab_op != META_GRAB_OP_NONE && > + window == window->display->grab_window && > + meta_grab_op_is_mouse (window->display->grab_op) && > > Why do we need to care about non-resizing grab ops? And why do we not care > about keyboard grab ops? > > The large switch below doesn't care about other kinds of mouse ops, but cares > about keyboard resize events. This looks like leftovers from who knows what. I'll add a patch to the patchset to clean it up.
(In reply to comment #19) > I just tested this, using part of the wip/frame-synchronization branch from > mutter (up to but not including frame timings) + wip/frame-synchronization from > gtk+, both rebased on master. Thanks for testing! > I'm not sure where the problem is, but this branch will cause windows to get > stuck if they change the maximization/tiling status during a mouse op. > Reverting mutter fixes the problem. I put this on my todo list to look at and try to reproduce. Not obvious how this would happen immediately. > In the logs I see this: > Window manager warning: Log level 8: gdk_x11_window_begin_frame: assertion > `GDK_IS_WINDOW (window)' failed > Window manager warning: Log level 8: gdk_window_process_updates_with_mode: > assertion `GDK_IS_WINDOW (window)' failed > Window manager warning: Log level 8: gdk_x11_window_end_frame: assertion > `GDK_IS_WINDOW (window)' failed I have a fix for this (not freeing the paint clock and disconnecting from it) locally, will push it soon. > Additionally, but this is a minor bug, if the WM is restarted while the > application is painting a frame, gtk will never thaw the paint clock and thus > the application will hang. Yeah, there's code that needs to be written. Also added to my TODO. (Last time I looked at this, I got distracted by weirdness in the code to handle WM changes in GTK+) > PS: you forgot one file (cogl-output.c) in the cogl branch, so I could not test > the cogl and clutter changes. Final cogl changes are now landed.
Created attachment 234852 [details] [review] Implement freezing of updates during resize Replace the unused meta_compositor_set_updates() with a reversed-meaning meta_compositor_set_updates_frozen(), and use it to implement freezing application window updates during interactive resizing. This avoids drawing new areas of the window with blank content before the application has a chance to repaint.
Created attachment 234853 [details] [review] Move sync alarms to be per-window and permanent Instead of creating a new alarm each time we resize a window interactively, create an alarm the first time we resize a window and keep it around permanently until we unmanage the window. Doing it this way will be useful when we allow the application to spontaneously generate sync request updates to indicate frames it is drawing.
Created attachment 234854 [details] [review] Support properties with lists of XSyncCounter Add META_PROP_VALUE_SYNC_COUNTER_LIST for a property that contains multiple XSyncCounter values.
Created attachment 234855 [details] [review] Add support for an extended style of _NET_WM_SYNC_REQUEST_COUNTER If an application provides two values in _NET_WM_SYNC_REQUEST_COUNTER, use that as a signal that the applications wants an extended behavior where it can update the counter as well as the window manager. If the application updates the counter to an odd value, updates of the window are frozen until the counter is updated again to an even value.
Created attachment 234856 [details] [review] Send _NET_WM_FRAME_DRAWN messages When the application provides the extended second counter for _NET_WM_SYNC_REQUEST, send a client message with completion information after the next redraw after each counter update by the application.
Created attachment 234857 [details] [review] MetaWindow: always resize the frame first when we have synchronization Resizing the frame triggers creation of a new backing pixmap for the window, so we should do that first before we resize the client window and mess up the contents of the old backing pixmap.
Created attachment 234858 [details] [review] Handle _NET_WM_SYNC_REQUEST_COUNTER updates without redraw It's possible that a client might update the (extended) _NET_WM_SYNC_REQUEST_COUNTER counter twice without actually drawing anything. In that case, we still should send a _NET_WM_FRAME_DRAWN message since it's hard for a client to know every case in which no damage is generated. For now, do it the easy way by forcing a stage repaint.
Created attachment 234859 [details] [review] Enable CLUTTER / COGL_ENABLE_EXPERIMENTAL_API globally Instead of defining CLUTTER_ENABLE_EXPERIMENTAL_API and COGL_ENABLE_EXPERIMENTAL_API in individual source files, enable them on the command line. We weren't tracking exactly what pieces of experimental API we were using and we were using the experimental API in most source files that used Clutter and Cogl, so the local #defines were annoying rather than useful.
Created attachment 234860 [details] [review] Use clutter_stage_set_sync_delay() Using a "sync delay" where we wait for 2 ms after the vblank before starting to draw the next frame provides for much more predictable latency for applications. An application can know that if it completes a frame any time between 8ms before the vblank to the vblank, it will reliably be drawn on the following vblank period, rather than having an unpredictable latency depending on whether the compositor is currently busy drawing a frame or not.
Created attachment 234861 [details] [review] Use XSyncSetPriority() Use XSyncSetPriority() to prioritize the compositor above applications for X server priority. In practice, this makes little difference because the Xorg "smart scheduler" will schedule in a single application for time slices that exceed the frame drawing time, but it's theoretically right and might make a difference if the X server scheduler is improved.
Created attachment 234862 [details] [review] Add meta_compositor_monotonic_time_to_server_time() Add a function to convert from g_get_monotonic_time() to a "high-resolution server timestamp" with microsecond precision. These timestamps will be used when communicating frame timing information to the client.
Created attachment 234863 [details] [review] Send _NET_WM_FRAME_TIMINGS messages We previously had timestamp information stubbed out in _NET_WM_FRAME_DRAWN. Instead of this, add a high-resolution timestamp in _NET_WM_FRAME_DRAWN then send a _NET_WM_FRAME_TIMINGS message after when we have complete frame timing information, representing the "presentation time" of the frame as an offset from the timestamp in _NET_WM_FRAME_DRAWN. To provide maximum space in the messages,_NET_WM_FRAME_DRAWN and _NET_WM_FRAME_TIMINGS are not done as WM_PROTOCOLS messages but have their own message types.
Created attachment 234864 [details] [review] Distinguish "no delay" frames from spontaneous drawing When a client is drawing as hard as possible (without sleeping between frames) we need to draw as soon possible, since sleeping will decrease the effective frame rate shown to the user, and can also result in the system never kicking out of power-saving mode because it doesn't look fully utilized. Use the amount the client increments the counter value by when ending the frame to distinguish these cases: - Increment by 1: a no-delay frame - Increment by more than 1: a non-urgent frame, handle normally
Created attachment 234865 [details] [review] Consistently use meta_grab_op_is_resizing() for _NET_WM_SYNC_REQUEST In different places we checked the grab op differently when determing whether we are using _NET_WM_SYNC_REQUEST. This was somewhat covered up previously by the fact that we only had a sync alarm when using _NET_WM_SYNC_REQUEST, but that is no longer the case, so consistently use meta_grab_op_is_resizing() everywhere.
Review of attachment 234857 [details] [review]: Marking ACN since I didn't change this after review comments.
Review of attachment 234853 [details] [review]: An earlier version was marked ACN, wasn't not changed in meaning, some changes from rebasing. I'll mark ACN but feel free to give it another look over.
Review of attachment 234852 [details] [review]: OK.
Review of attachment 234855 [details] [review]: Mostly fine, a few nits. ::: src/core/window-private.h @@ +350,3 @@ /* XSync update counter */ XSyncCounter sync_request_counter; + gint64 sync_request_serial; I believe this was fixed upstream at some point? Either that or I have a patch somewhere. ::: src/core/window.c @@ +4450,3 @@ + /* In the old style, we're responsible for setting the initial + * value of the counter. In the new (extended style), the counter + * value is initialized by the client before mapping the window Comment: In the old style, X, in the new style, Y. Code: if (new_style) Y else X Would appreciate making the order match. @@ +4538,3 @@ + * that we received from the client; the same code still works + * for the old style */ + wait_serial = window->sync_request_serial + 240; Comment about why 240 was chosen would be nice, even if it explains it's an arbitrary guess. @@ +4554,3 @@ ev.data.l[1] = meta_display_get_current_time (window->display); + ev.data.l[2] = wait_serial & G_GUINT64_CONSTANT(0xffffffff); + ev.data.l[3] = wait_serial >> 32; Is there any specific reason for ditching the XSync helper functions? Does it fix a bug, or do you just not like them?
Review of attachment 234856 [details] [review]: OK.
Review of attachment 234858 [details] [review]: Commit message needs updating.
Review of attachment 234859 [details] [review]: Feel free to move to the front of the stack and push now.
Review of attachment 234860 [details] [review]: ::: src/compositor/compositor.c @@ +550,3 @@ G_CALLBACK (after_stage_paint), info); + /* Wait 6-ms after vblank before starting to draw next frame */ Commit message says 2ms, code says "2", comment says 6?
Review of attachment 234861 [details] [review]: OK.
Review of attachment 234862 [details] [review]: OK. ::: src/compositor/compositor.c @@ +1438,3 @@ + if (compositor->server_time_query_time == 0 || + (!compositor->server_time_is_monotonic_time && + monotonic_time > compositor->server_time_query_time + 10000000)) /* 10 seconds */ I think using a constant like: 10*1000*1000 makes it a bit more readable. @@ +1452,3 @@ + */ + if (server_time_usec > current_monotonic_time - 1000000 && + server_time_usec < current_monotonic_time + 1000000) Same for here, too: 1*1000*1000
Review of attachment 234863 [details] [review]: This relies on new Clutter/Cogl API. Should you bump the req in configure.ac? ::: src/compositor/compositor.c @@ +552,2 @@ /* Wait 6-ms after vblank before starting to draw next frame */ + clutter_stage_set_sync_delay (CLUTTER_STAGE (info->stage), META_SYNC_DELAY); Perhaps this and the addition of META_SYNC_DELAY should be squashed in the patches before? @@ +1224,3 @@ + CoglContext *context = cogl_framebuffer_get_context (COGL_FRAMEBUFFER (onscreen)); + gint64 current_time_cogl = cogl_get_clock_time (context); + gint64 now = g_get_monotonic_time (); "current_time_monotonic"? Some words about the clocks of Cogl and the system monotonic time would be nice. @@ +1227,3 @@ + + presentation_time = + now + (presentation_time_cogl - current_time_cogl) / 1000; now - (current_time_cogl - presentation_time_cogl) is a bit easier to read IMO. ::: src/compositor/meta-window-actor.c @@ +2439,3 @@ + refresh_rate = cogl_frame_info_get_refresh_rate (frame_info); + if (refresh_rate != 0.0) + refresh_interval = (int) (0.5 + 1000000 / refresh_rate); I forget what C's semantics for cast-to-int are like this, but won't this truncate to 32-bits? @@ +2455,3 @@ + } + + if (refresh_interval != 0 && (gint32)refresh_interval == refresh_interval) Any reason the refresh-interval is calculated as a 64-bit integer if it's unused if it goes above that limit?
Review of attachment 234864 [details] [review]: OK.
Review of attachment 234865 [details] [review]: OK.
(In reply to comment #47) > Review of attachment 234860 [details] [review]: > > ::: src/compositor/compositor.c > @@ +550,3 @@ > G_CALLBACK (after_stage_paint), info); > > + /* Wait 6-ms after vblank before starting to draw next frame */ > > Commit message says 2ms, code says "2", comment says 6? Fixed locally.
Review of attachment 234855 [details] [review]: ::: src/core/window-private.h @@ +350,3 @@ /* XSync update counter */ XSyncCounter sync_request_counter; + gint64 sync_request_serial; This patch set was rebased to master before I posted it. But this one isn't a bug in the old code, since before this patch, the sync request serial was determined by Mutter, so it could be known to be a guint. ::: src/core/window.c @@ +4450,3 @@ + /* In the old style, we're responsible for setting the initial + * value of the counter. In the new (extended style), the counter + * value is initialized by the client before mapping the window Good point. Fixed (locally for now) @@ +4538,3 @@ + * that we received from the client; the same code still works + * for the old style */ + wait_serial = window->sync_request_serial + 240; I added: "The increment of 240 is specified by the EWMH and is (1 second) * (60fps) * (an increment of 4 per frame)." @@ +4554,3 @@ ev.data.l[1] = meta_display_get_current_time (window->display); + ev.data.l[2] = wait_serial & G_GUINT64_CONSTANT(0xffffffff); + ev.data.l[3] = wait_serial >> 32; XSync was written without assuming the existence of 64-bit values: typedef struct _XSyncValue { int hi; unsigned int lo; } XSyncValue; Since 64-bit values are mandatory at the glib level, we make our life easier by using real integers (which we can do arithmetic on) for X sync values and only converting at the interfaces to the XSync library.
(In reply to comment #49) > > ::: src/compositor/compositor.c > @@ +1438,3 @@ > + if (compositor->server_time_query_time == 0 || > + (!compositor->server_time_is_monotonic_time && > + monotonic_time > compositor->server_time_query_time + 10000000)) /* 10 > seconds */ > > I think using a constant like: > > 10*1000*1000 > > makes it a bit more readable. > > @@ +1452,3 @@ > + */ > + if (server_time_usec > current_monotonic_time - 1000000 && > + server_time_usec < current_monotonic_time + 1000000) > > Same for here, too: > > 1*1000*1000 Sure, changed.
(In reply to comment #50) > Review of attachment 234863 [details] [review]: > > This relies on new Clutter/Cogl API. Should you bump the req in configure.ac? I'll do that once I get the clutter patch committed. Until then this branch is un-landable > ::: src/compositor/compositor.c > @@ +552,2 @@ > /* Wait 6-ms after vblank before starting to draw next frame */ > + clutter_stage_set_sync_delay (CLUTTER_STAGE (info->stage), META_SYNC_DELAY); > > Perhaps this and the addition of META_SYNC_DELAY should be squashed in the > patches before? I can see the argument, but I think it's fine like this - every step is meaningful in itself, so I don't feel like I need to play rebase tracks to eradicate the last bit of incrementalism from the patch set :-) > @@ +1224,3 @@ > + CoglContext *context = cogl_framebuffer_get_context > (COGL_FRAMEBUFFER (onscreen)); > + gint64 current_time_cogl = cogl_get_clock_time (context); > + gint64 now = g_get_monotonic_time (); > > "current_time_monotonic"? Switched it to current_cogl_time and current_monotonic_time to match variable names elsewhere in the source file. > > Some words about the clocks of Cogl and the system monotonic time would be > nice. > @@ +1227,3 @@ > + > + presentation_time = > + now + (presentation_time_cogl - current_time_cogl) / 1000; > > now - (current_time_cogl - presentation_time_cogl) is a bit easier to read IMO. presentation_time is not *necessarily* in the past. Cogl could predict a future time. For a time that has either sign, I'd rather stick to the idiom "base_value_in_units_a + (value_in_units_b - base_value_in_units_b)" > ::: src/compositor/meta-window-actor.c > @@ +2439,3 @@ > + refresh_rate = cogl_frame_info_get_refresh_rate (frame_info); > + if (refresh_rate != 0.0) > + refresh_interval = (int) (0.5 + 1000000 / refresh_rate); > > I forget what C's semantics for cast-to-int are like this, but won't this > truncate to 32-bits? I don't think it's defined - could truncate - could clamp. I put in a sanity check of 'refresh_rate >= 1.0' though I'm not really worried about geting a refresh rate of 0.0001 reported from Cogl. > @@ +2455,3 @@ > + } > + > + if (refresh_interval != 0 && (gint32)refresh_interval == refresh_interval) > > Any reason the refresh-interval is calculated as a 64-bit integer if it's > unused if it goes above that limit? This is a left-over from an old version of my Cogl patches that had refresh_interval as a 64-bit value rather than 'float refresh_rate'. Removed the check in favor of a simple. ev.data.l[3] = refresh_interval;
Attachment 225760 [details] pushed as 3abaf50 - MetaWindowActor: Use guint for bitfields Attachment 234852 [details] pushed as c9343e3 - Implement freezing of updates during resize Attachment 234853 [details] pushed as 7743c70 - Move sync alarms to be per-window and permanent Attachment 234854 [details] pushed as 7d43bde - Support properties with lists of XSyncCounter Attachment 234855 [details] pushed as 70c0d39 - Add support for an extended style of _NET_WM_SYNC_REQUEST_COUNTER Attachment 234856 [details] pushed as fbfab93 - Send _NET_WM_FRAME_DRAWN messages Attachment 234857 [details] pushed as 790bfca - MetaWindow: always resize the frame first when we have synchronization Attachment 234858 [details] pushed as 04ef448 - Handle _NET_WM_SYNC_REQUEST_COUNTER updates without redraw Attachment 234859 [details] pushed as b07aea4 - Enable CLUTTER / COGL_ENABLE_EXPERIMENTAL_API globally Attachment 234860 [details] pushed as d8696c1 - Use clutter_stage_set_sync_delay() Attachment 234861 [details] pushed as fcc178e - Use XSyncSetPriority() Attachment 234862 [details] pushed as 74b1a9e - Add meta_compositor_monotonic_time_to_server_time() Attachment 234863 [details] pushed as 2d9b8bb - Send _NET_WM_FRAME_TIMINGS messages Attachment 234864 [details] pushed as 87fe968 - Distinguish "no delay" frames from spontaneous drawing Attachment 234865 [details] pushed as 0503f6b - Consistently use meta_grab_op_is_resizing() for _NET_WM_SYNC_REQUEST
Giovanni: > Additionally, but this is a minor bug, if the WM is restarted while the > application is painting a frame, gtk will never thaw the paint clock and thus > the application will hang. Did you have a specific test case for this? The theoretical possibility is there in the GTK+ code, but generally the window manager restart / switch is going to almost always imply a unmap/map cycle which will clear the frame pending flag. I'd like to be able to reproduce the problem rather than just speculatively change the GTK+ code, so I'm interested that you were able see this.