GNOME Bugzilla – Bug 745512
Improve damage tracking and use swap_buffers_with_damage
Last modified: 2015-04-29 21:55:34 UTC
Created attachment 298409 [details] [review] Fix buffer age tracking Currently the implementation of buffer-age doesn't handle varying ages very well, such as latency spikes. And then we don't inform the buffer flip of the current damage, which makes flips very much slower on PRIME systems.
Created attachment 298410 [details] [review] Use swap_buffers_with_damage
Review of attachment 298409 [details] [review]: Looks good to me; something happened when attaching the patch to Bugzilla, because I don't see a proper commit log, though. ::: clutter/cogl/clutter-stage-cogl.h @@ +60,2 @@ /* Stores a list of previous damaged areas */ + cairo_rectangle_int_t damage_history[16]; Do you have any particular reason as to why you chose 16 slots, or is it just a good round number?
Review of attachment 298410 [details] [review]: Looks good. Same as the patch above: I don't see a commit log.
The highest buffer age I saw in testing with the list was 9!! (That's with a lot of server debugging enabled and vnc-like GetImage every frame). Switching over to the damage array, the highest I have seen so far is 5 (the expected value is 4 for the particular setup I am testing) - and no unexpected whole frame redraws due to lack of history. The reason the list went higher was because we would discard damage too early and every so often have to repaint the whole frame, causing the GetImage to also grab the whole frame and so be much slower, causing a latency spike and a vicious circle. I also wonder if there were secondary effects preventing the old back buffers from being cached (and so the returned back buffers being older than expected.) TLDR; no reason other than it was the next power of two greater than I saw in testing. I left the commit logs empty, just so I could have a discussion first :)
(In reply to Chris Wilson from comment #4) > TLDR; no reason other than it was the next power of two greater than I saw > in testing. Sounds like a good reason to me. :-)
Review of attachment 298409 [details] [review]: Seems like Robert's concern wasn't unjustified: https://bugzilla.gnome.org/show_bug.cgi?id=669122#c35 Code looks good to me just small nitpicks. ::: clutter/cogl/clutter-stage-cogl.c @@ +482,3 @@ + *current_damage = *clip_region; + + if (age > 0 && age < 16 && !stage_cogl->dirty_backbuffer) Can we move it to a constant or something so that if we find a reason to change it we don't have to hunt it down and change every "15" as well? @@ +686,3 @@ { cairo_rectangle_int_t *rect; + rect = &stage_cogl->damage_history[(stage_cogl->damage_index-1)&15]; Missing whitespace.
Created attachment 298886 [details] [review] Fix damage tracking with varying buffer ages
Created attachment 298887 [details] [review] Use swap buffers with damage
Created attachment 298892 [details] [review] Fix damage tracking with varying buffer ages v2 With macros to hide the constants.
Review of attachment 298887 [details] [review]: LGTM
Worth doing something like: diff --git a/clutter/cogl/clutter-stage-cogl.c b/clutter/cogl/clutter-stage-cogl.c index 684aa5a..79bae3b 100644 --- a/clutter/cogl/clutter-stage-cogl.c +++ b/clutter/cogl/clutter-stage-cogl.c @@ -392,6 +392,15 @@ clutter_stage_cogl_get_redraw_clip_bounds (ClutterStageWindow *stage_window, return FALSE; } +static inline gboolean +valid_buffer_age (ClutterStageCogl *stage_cogl, unsigned age) +{ + if (!stage_cogl->dirty_backbuffer) + return 0; + + return age < MIN(stage_cogl->damage_index, DAMAGE_HISTORY_MAX); +} + /* XXX: This is basically identical to clutter_stage_glx_redraw */ static void clutter_stage_cogl_redraw (ClutterStageWindow *stage_window) @@ -462,7 +471,7 @@ clutter_stage_cogl_redraw (ClutterStageWindow *stage_window) *current_damage = *clip_region; - if (DAMAGE_HISTORY_VALID(age) && !stage_cogl->dirty_backbuffer) + if (valid_buffer_age (stage_cogl, age)) { for (i = 1; i < age; i++) _clutter_util_rectangle_union (clip_region, diff --git a/clutter/cogl/clutter-stage-cogl.h b/clutter/cogl/clutter-stage-cogl.h index 03e32e2..e13e58d 100644 --- a/clutter/cogl/clutter-stage-cogl.h +++ b/clutter/cogl/clutter-stage-cogl.h @@ -60,7 +60,6 @@ struct _ClutterStageCogl /* Stores a list of previous damaged areas */ #define DAMAGE_HISTORY_MAX 16 #define DAMAGE_HISTORY(x) ((x) & (DAMAGE_HISTORY_MAX - 1)) -#define DAMAGE_HISTORY_VALID(x) ((unsigned)(x) < DAMAGE_HISTORY_MAX) cairo_rectangle_int_t damage_history[DAMAGE_HISTORY_MAX]; unsigned damage_index; };
Review of attachment 298892 [details] [review]: Minimal coding style issues, but the patch looks good as it is. Feel free to commit after fixing. ::: clutter/cogl/clutter-stage-cogl.c @@ +455,3 @@ { + cairo_rectangle_int_t *current_damage = + &stage_cogl->damage_history[DAMAGE_HISTORY(stage_cogl->damage_index++)]; Coding style: missing space between macro/function name and parenthesis. @@ +463,3 @@ + *current_damage = *clip_region; + + { Coding style: missing space between macro/function name and parenthesis. @@ +467,3 @@ + for (i = 1; i < age; i++) + _clutter_util_rectangle_union (clip_region, + Coding style: missing space between macro/function name and parenthesis. @@ +660,3 @@ cairo_rectangle_int_t *rect; + + rect = &stage_cogl->damage_history[DAMAGE_HISTORY(stage_cogl->damage_index-1)]; Coding style: missing space between macro/function and parenthesis.
(In reply to Chris Wilson from comment #11) > Worth doing something like: > +static inline gboolean > +valid_buffer_age (ClutterStageCogl *stage_cogl, unsigned age) > +{ > + if (!stage_cogl->dirty_backbuffer) > + return 0; > + > + return age < MIN(stage_cogl->damage_index, DAMAGE_HISTORY_MAX); > +} Minus the coding style, seems okay. Can be an additional commit, as far as I'm concerned.
Review of attachment 298892 [details] [review]: Some comments below fine to push with them fixed. ::: clutter/cogl/clutter-stage-cogl.c @@ +480,3 @@ + else + { + CLUTTER_NOTE (CLIPPING, "Invalid back buffer(age=%d): Resetting damage history list.\n", age); Sorry didn't notice before but that note is now a lie ;) .. should say "doing full stage redraw". @@ +660,3 @@ cairo_rectangle_int_t *rect; + + rect = &stage_cogl->damage_history[DAMAGE_HISTORY(stage_cogl->damage_index-1)]; Still no whitespace. ::: clutter/cogl/clutter-stage-cogl.h @@ +61,3 @@ +#define DAMAGE_HISTORY_MAX 16 +#define DAMAGE_HISTORY(x) ((x) & (DAMAGE_HISTORY_MAX - 1)) +#define DAMAGE_HISTORY_VALID(x) ((unsigned)(x) < DAMAGE_HISTORY_MAX) Lost the "age > 0" check.
(In reply to drago01 from comment #14) > @@ +660,3 @@ > cairo_rectangle_int_t *rect; > + > + rect = > &stage_cogl->damage_history[DAMAGE_HISTORY(stage_cogl->damage_index-1)]; > > Still no whitespace. I misread and assumed you wanted the whitespace between variable declaration and code.
Created attachment 298898 [details] [review] Fix damage tracking with varying buffer ages
I've started testing this on my machine, and I get artefacts with a simple test case, like examples/basic-actor. The middle actor scales down on mouse-over, and it leaves a trail of pixels in its top-left corner. It does not happen without the patches.
Created attachment 299114 [details] Screenshot with rendering artifacts
For the record, which driver? My testing so far has been with piglit for the basic GLX_EXT_buffer_age and gnome-shell.
I'm using xorg-x11-drv-intel-2.99.916-3.20141117.fc21.x86_64 from Fedora 21, along with the Linux 3.17.7-300.fc21.x86_64 kernel; Mesa is at 10.4.1-1.20141230.fc21.
I am using an fc21 base with clutter-1.20.0-1.fc21.src.rpm + patches, Xorg.rpm + patches and xf86-video-intel.git / xf86-video-intel-2.99.916, I haven't seen the trailing pixels on bare X or under gnome-shell (having double checked for glxinfo -display :0 | grep GLX_EXT_buffer_age). It's definitely an off-by-one, could be anywhere between the history tracking changes in the patches or the rendering through to the ddx doing something questionable. Could you grab the CLUTTER_NOTE(CLIPPING) for unpatched and patched? And teach me how to do the same :) (How to add them to the code is mentioned in HACKING but not how to enable the output at runtime!)
(In reply to Chris Wilson from comment #21) > It's definitely an off-by-one, could be anywhere between the history > tracking changes in the patches or the rendering through to the ddx doing > something questionable. Could you grab the CLUTTER_NOTE(CLIPPING) for > unpatched and patched? And teach me how to do the same :) (How to add them > to the code is mentioned in HACKING but not how to enable the output at > runtime!) CLUTTER_NOTE macros map to the CLUTTER_DEBUG environment variable: https://developer.gnome.org/clutter/unstable/running-clutter.html#clutter-Debug-Flags If you run an example with: $ CLUTTER_DEBUG=clipping ./examples/basic-actor you'll get the debugging messages for the 'CLIPPING' section. I'm attaching the two debug runs, without and with the patch.
Created attachment 299166 [details] CLUTTER_DEBUG=clipping basic-actor from master
Created attachment 299167 [details] CLUTTER_DEBUG=clipping basic-actor with patches applied
At a rough glance, the behaviour looks consistent: both are seeing double-buffered redraws (with buffer-age=1). Ok, I think I spotted it: diff --git a/clutter/cogl/clutter-stage-cogl.c b/clutter/cogl/clutter-stage-cogl.c index 275caaf..8ba9208 100644 --- a/clutter/cogl/clutter-stage-cogl.c +++ b/clutter/cogl/clutter-stage-cogl.c @@ -474,7 +474,7 @@ clutter_stage_cogl_redraw (ClutterStageWindow *stage_window) if (valid_buffer_age (stage_cogl, age)) { - for (i = 1; i < age; i++) + for (i = 1; i <= age; i++) _clutter_util_rectangle_union (clip_region, &stage_cogl->damage_history[DAMAGE_HISTORY (stage_cogl->damage_index - i - 1)], clip_region);
Indeed, that fixes it. Thanks!
Created attachment 299173 [details] [review] Fix damage tracking with varying buffer ages v2 Just to refresh the patch with the bugfix applied.
Pushed to master, so that it's part of the next 1.21 snapshot before the GNOME 3.15 code freeze.
Review of attachment 298887 [details] [review]: ::: clutter/cogl/clutter-stage-cogl.c @@ +597,3 @@ + damage[2] = geom.width; + damage[3] = geom.height; + ndamage = -1; Did you mean for this to be -1? This will cause cogl to call alloca with a bogus amount and completely trash the stack: https://git.gnome.org/browse/cogl/tree/cogl/winsys/cogl-winsys-egl.c#n772 It seems to me like this should be "1" in both cases, but the fact that you have a branch for this at all tells me something else is going on here.
Created attachment 302534 [details] [review] clutter-stage-cogl: Match EGL's behavior of eglSwapBuffersWithDamage -1 is explicitly an invalid value to pass to eglSwapBuffersWithDamage, and the specification admits as much: If eglSwapBuffersWithDamageEXT is called and <n_rects>, is less than zero or <n_rects> is greater than zero but <rects> is NULL, EGL_BAD_PARAMETER is generated. Fix up our usage of SwapBuffersWithDamage to match the behavior in the EGL specification.
The answer, for future reference, is that Chris Wilson was implementing his own extension [0], which has different semantics for the n_rects parameter. In his case, swapping an empty (n_rects = 0) region does nothing but generate a swap event, and passing n_rects = -1 is the equivalent of eglSwapBuffersWithDamage(NULL, 0); in the EGL extension. [0] http://cgit.freedesktop.org/~ickle/mesa/tree/docs/specs/MESA_swap_buffers_with_damage.spec?h=dri2-async-swap My patch fixes things up to be compliant with the currently exposed Cogl API, and with the EGL extension. I'd say that behavior differences like this should either be handled at the Cogl layer, or by fixing the extension to use the same semantics.
Review of attachment 302534 [details] [review]: The Cogl API taking a signed integer with no real error checking on the admissible range is quite useless. Looking at the discussion on #xorg-devel about a GLX extension for glXSwapBuffersWithDamage(), I agree with Chris: using -1 to imply the whole drawable should be invalidated (i.e. behave like glXSwapBuffers()) is better than using 0. The Cogl API sadly encodes 0 as the 'whole region' magic value, which is fairly broken. A zero region is an empty region, not the whole admissible region. Well: stable, meet doors. If the GLX_MESA_swap_buffers_with_damage extension ends up in Mesa for real, we'll either have to change Cogl or have Clutter deal with it. Maybe we should just ignore magic values. ::: clutter/cogl/clutter-stage-cogl.c @@ +608,3 @@ else { + ndamage = 0; What if, instead, we just make a single region the size of the drawable geometry and side-step this whole mess? After all, under the definition of the EGL extension: damage=[], ndamage=0 and under the definition of the proposed GLX extension: damage=[], ndamage=-1 are both strictly equivalent to: damage=[0,0,geom.width,geom.height], ndamage=1
Created attachment 302572 [details] [review] cogl/stage: Do not pass invalid arguments to Cogl The cogl_onscreen_swap_region() and cogl_onscreen_swap_buffers_with_damage() functions in Cogl take an array of rectangles and the number of rectangles as arguments. The magic 0 value is used to mean "use the whole drawable region of the on-screen frame buffer", but that value is not only modelled on a specific EGL extension, but also does not allow expressing an empty region. The changes of commit 55c957267ef241767ebd3891d49f06deb2ff4aa9 are based on an unpublished GLX extension for Mesa that allows passing 0 as a magic value meaning "empty region", and -1 as a magic value meaning "use the whole drawable region": http://cgit.freedesktop.org/~ickle/mesa/commit/?h=dri2-async-swap&id=1c11a5877729ab874c5667616a558e33e0fb805d Cogl does not do validation on either the swap_region() nor the swap_buffers_with_damage() calls, so we end up submitting a -1 value which trashes the memory inside the windowing system implementations of both calls. Since ClutterStageCogl knows exactly what is the region that should be submitted to the driver, and since the Cogl implementation can opt to discard arguments depending on the windowing system and supported extensions, we can always pass a single region to Cogl, and have it decide what to do in a more sensible way than Clutter can ever do without knowing the details that we hid in Cogl in the first place.
Review of attachment 302572 [details] [review]: meh. there's a performance impact i'm worried about but whatever.
(In reply to Jasper St. Pierre from comment #34) > Review of attachment 302572 [details] [review] [review]: > > meh. there's a performance impact i'm worried about but whatever. What kind of performance impact? Are you worried about drivers doing some work with the region? We can check inside Cogl if the rectangle passed is equal to, or larger than, the CoglOnscreen size, and do the right thing depending on the windowing system backend.
In the case of DRI2, we have to construct a region and pass it to XDamageAdd in any case, so the performance impact of passing non-0 rectangles through is simply a malloc/memcpy/free. Not that much. In DRI3, we can avoid constructing a server-side object entirely. (In reply to Emmanuele Bassi (:ebassi) from comment #35) > We can check inside Cogl if the rectangle passed is equal to, or larger > than, the CoglOnscreen size, and do the right thing depending on the > windowing system backend. This couldn't ever possibly go wrong when resizing windows. I mean, sure, we *can* do that and rely on certain heuristics and expect that the values passed to swap_with_onscreen_whatever are up to date in the case of resizing windows, or we can rely on an explicit sentinel value provided to us by the EGL specification to mark "the full surface". I don't really care what the API is, but we have to pick *one*, and Cogl picked its API to match the EGL one, so I'm tempted to stick with it. Note that this behavior is documented API of Cogl's. https://git.gnome.org/browse/cogl/tree/cogl/cogl-onscreen.h#n449
Decided to go with Jasper's approach in the end. Attachment 302534 [details] pushed as 21ce9bc - clutter-stage-cogl: Match EGL's behavior of eglSwapBuffersWithDamage