GNOME Bugzilla – Bug 669122
Clipped redraws and tearing
Last modified: 2015-03-09 14:59:01 UTC
As seen in https://bugzilla.gnome.org/show_bug.cgi?id=651312#c37 there is a problem on how clipped redraws are handled causing tearing. If the GPU does not complete the blit in a vblank period it tearing might become visible. We had a guard against this in the distinct past but we removed it because we could not spot any problem with it in testing. See: http://git.gnome.org/browse/clutter/commit/?id=56315d92bd9e961450746d9f9c973f937be20aa6 Apparently this isn't always the case as a sufficient large blit can cause such tearing on some GPUs. Having a hardcoded value like we had prior to that commit is not really a solution and I can't think of any other heuristic that makes sense so I though about doing it dynamically ie. Clutter calls cogl_framebuffer_swap_region which does the blit. cogl_framebuffer_swap_region (onscreen_swap_region to be exact) records the vblank counter prior to the blit and compares it with the value it gets afterwards. If the two did not match it means that the blit did not complete in time and has caused a tear. ClutterStage computes the relative area of the blit area to the stage size i.e blit area / (stage_height * stage_width) and stores the result. Next time it uses that value to fall back to a full stage redraw (if the blit area is greater then or equal the previously computed area don't do a clipped redraw). I have no idea how this would fit into the API though as cogl_framebuffer_swap_region would have to leak implementation details to the caller. Toughs?
My current feeling here is that we should look at a different approach for incremental redraws that avoids the nightmare of synchronizing and throttling when using blits to present and also be much more efficient overall by using flips to present instead of copying data. I propose that we see if we can get the following extensions implemented for the drivers we care about: https://github.com/rib/gl-extensions/blob/master/GLX_EXT_buffer_age.txt https://github.com/rib/gl-extensions/blob/master/GLX_EXT_start_frame.txt as well as a GLX equivalent for this: https://github.com/rib/gl-extensions/blob/master/EGL_EXT_swap_buffers_with_damage.txt Recently I implemented EGL equivalents of these extensions for Mesa as well as corresponding Cogl and Clutter support since this is what we are aiming to use on Wayland. These extension were easy to implement for EGL so my expectation is that they would also be relatively easy to support with a GLX driver and with non Mesa based drivers too. The patch for "egl: Implements EGL_EXT_start_frame" was the most involved just because it touches all the different chip-set drivers in Mesa but those changes should also be re-usable for GLX support too. My Mesa patches can be seen for reference here: http://cgit.freedesktop.org/~rib/mesa/log/?h=wip/egl-extensions
Based on some early feedback from Nvidia two changes have been made to the previous proposal: 1) Instead of requiring a separate GLX_EXT_start_frame extension the GLX_EXT_buffer_age semantics were changed so that querying the age implicitly starts a new frame and the age remains valid until the end of the frame. 2) To consider drivers that might share back buffers between contexts it was clarified that drivers implementing the GLX_EXT_buffer_age extension should discount buffers whose contents are not fully owned by the drawing context (i.e. all the pixels would have passed the pixel ownership test). The changes have been pushed to the github repo linked to before.
*** Bug 651312 has been marked as a duplicate of this bug. ***
Gnome 3.3.90 fixed this for me.
Not for me. Tears horribly here unless I disable clipped redraws. Thinkpad X220 with Sandy Bridge, Arch Linux mutter 3.3.90 clutter 1.9.14 cogl 1.9.8 mesa 8.0.1 xorg-server 1.12.0 xf86-video-intel 2.18.0 linux 3.2.10
Ok. No more tearing when moving windows close the top of screen. However for fullscreen videos the tearing persists.
(In reply to comment #6) > Ok. No more tearing when moving windows close the top of screen. However for > fullscreen videos the tearing persists. We did not implement the fixes described in this bug yet ... so there is no point in telling us that's not fixed.
Created attachment 209689 [details] [review] winsys-glx: Don't assume that copy_sub_buffer is synchronized We initially assumed that copy_sub_buffer is synchronized on which is only the case for a subset of GPUs for example it is not synchronized on INTEL gen6 and gen7, so we remove this assumption for now. We should have a specific driver / GPU whitelist if we want to enable this. --- This should make tearing on Sandybridge and Ivybridge less frequent. (Forces sub_buffer copies to be always synchronized).
yeah that makes sense, I've pushed the patch to master as commit fd31da45e22c276a thanks
Reopening for the the implementation of comment 2
*** Bug 688552 has been marked as a duplicate of this bug. ***
Comment on attachment 209689 [details] [review] winsys-glx: Don't assume that copy_sub_buffer is synchronized Has been in git for a while.
*** Bug 689314 has been marked as a duplicate of this bug. ***
Created attachment 232207 [details] [review] cogl-onscreen: Add buffer_age support Add a new BUFFER_AGE winsys feature and a get_buffer_age method to cogl-onscreen that allows to query the value.
Created attachment 232208 [details] [review] clutter-stage: Use a separate buffer for picking Use an FBO for picking to allow the backbuffer contents to recycled to minimize drawing and save memory bandwith.
Created attachment 232209 [details] [review] stage-cogl: Reuse backbuffer contents Use the buffer_age extension when available to recycle backbuffer contents instead of blitting from the back to front buffer when doing clipped redraws. This should improve performance and avoid tearing.
(In reply to comment #15) > Created an attachment (id=232208) [details] [review] > clutter-stage: Use a separate buffer for picking > > Use an FBO for picking to allow the backbuffer contents to recycled to minimize > drawing and save memory bandwith. Emmanuele asked for performance numbers for this patch on IRC. The picking tests from test/performance do not show any difference with or without this patch. The reason for this patch is to not touch the back buffer contents to allow them to be resued for clipped redraws.
Created attachment 232210 [details] [review] cogl-onscreen: Add buffer_age support Add a new BUFFER_AGE winsys feature and a get_buffer_age method to cogl-onscreen that allows to query the value.
(In reply to comment #15) > Created an attachment (id=232208) [details] [review] > clutter-stage: Use a separate buffer for picking > > Use an FBO for picking to allow the backbuffer contents to recycled to minimize > drawing and save memory bandwith. I think it could be worth taking a look at my old wip/clip-with-swap-buffers branch and at 5c548ad9bd95bcec0 in particular which also added a dedicated pick buffer in clutter but only a 1x1 buffer. I think the important things to consider here are the amount of memory required (a full stage size buffer can be pretty huge considering embedded devices) and making sure we preserve the optimization that the pick buffer can be reused with no rendering while the scene is static.
(In reply to comment #18) > Created an attachment (id=232210) [details] [review] > cogl-onscreen: Add buffer_age support > > Add a new BUFFER_AGE winsys feature and a get_buffer_age method to > cogl-onscreen that allows to query the value. This patch looks good to me, although I think it would be good to give some more detail in the api documentation if possible please. I think as a rule of thumb a developer shouldn't have to read the EGL_EXT_buffer_age spec to figure out the semantics of this api. I think it could be good to provide an explanation of what the api can be useful for. We should be careful to say it returns the age of the buffer contents not just the age of the buffer. I think it could be good to explain that pixel ownership changes are not necessarily reflected in the reported age and applications are required to listen to window system specific events if this affects them. We can note that for many window systems pixel ownership changes are not an issue but that with some X11 drivers it is a problem for non-composited X11 windows. Applications can listen to Damage, Expose and ConfigureNotify events to track pixel ownership changes.
Created attachment 232523 [details] [review] cogl-onscreen: Add buffer_age support Add a new BUFFER_AGE winsys feature and a get_buffer_age method to cogl-onscreen that allows to query the value. --- Improve documentation.
Created attachment 232541 [details] [review] cogl-onscreen: Add buffer_age support Add a new BUFFER_AGE winsys feature and a get_buffer_age method to cogl-onscreen that allows to query the value. --- More clarifictions after IRC discussion.
(In reply to comment #15) I don't think it's strictly necessary to use a separate buffer for picking. If Clutter is going to have to track dirty regions for the buffers anyway, wouldn't it be quite simple to just add the single pixel of the pick to the dirty region so that it will get repainted next time? The pick pixel doesn't actually have to be where the mouse is positioned if we twiddle the viewport. For example we could make it always paint the pick pixel in the top left corner. It could even try to check if there is already a dirty pixel somewhere in the buffer before picking and always use that. It seems like a shame to make Clutter require FBO support for picking if it's not really necessary.
(In reply to comment #23) > (In reply to comment #15) > > I don't think it's strictly necessary to use a separate buffer for picking. If > Clutter is going to have to track dirty regions for the buffers anyway, > wouldn't it be quite simple to just add the single pixel of the pick to the > dirty region so that it will get repainted next time? Well if the pick (mouse movement or whatever) happens in an area far away from the damaged region it would increase the area that we have to repair significantly. > The pick pixel doesn't > actually have to be where the mouse is positioned if we twiddle the viewport. > For example we could make it always paint the pick pixel in the top left > corner. It could even try to check if there is already a dirty pixel somewhere > in the buffer before picking and always use that. Yes we could do stuff like that but ... are they any meaningful platforms that we want to support that don't have FBOs ?
(In reply to comment #24) > Yes we could do stuff like that but ... are they any meaningful platforms that > we want to support that don't have FBOs ? strictly speaking, indirect GLX does not support anything outside of GL 1.5 (if not even earlier) so requiring FBOs would remove our support for indirect GLX. not that I care *that* much about that, and I'm pretty sure support is moderately broken already.
Review of attachment 232541 [details] [review]: ::: cogl/cogl-onscreen.c @@ +200,3 @@ + + _COGL_RETURN_VAL_IF_FAIL (winsys->onscreen_get_buffer_age != NULL, 0); + I think it could be good to make this just return 0 without using the _COGL_RETURN_VAL_IF_FAIL macro when the feature is unavailable. Then the application wouldn't have to check for the feature and the code will just naturally redraw the entire framebuffer without having to have a special case. It'd probably be worth documenting that behaviour too.
Comment on attachment 232541 [details] [review] cogl-onscreen: Add buffer_age support For reference this patch has now been landed on master and on the cogl-1.14 branch, thanks.
Created attachment 235096 [details] [review] stage-window: make it possible to damage the back buffer This allows us to report to the backend that the stage's back buffer has been trashed while handling picking. If the backend is keeping track of the contents of back buffers so it can minimize how much of the stage is redrawn then it needs to know when we do pick renders so it can invalidate the back buffer. Based on patch from Robert Bragg <robert@linux.intel.com>
Created attachment 235097 [details] [review] stage: add dedicated pixel pick buffer This gives each stage a dedicated 1 pixel framebuffer for handling picking when we are only reading back a single point under the cursor. Having a dedicated pick buffer means we can avoid damaging the contents of the back buffer used for painting so we can re-use the contents of old back buffers to reduce how much is drawn each frame. Note that we don't create a full stage size pick buffer because we want to avoid the large allocations of memory that would imply. Note when a clutter scene is static then we will still use the back buffer for picking so that we can do a full stage size pick render without requiring the allocation of a dedicated buffer.
Created attachment 235098 [details] [review] stage-cogl: Reuse backbuffer contents Use the buffer_age extension when available to recycle backbuffer contents instead of blitting from the back to front buffer when doing clipped redraws. This should improve performance and avoid tearing.
Created attachment 235099 [details] [review] clutter-stage: Fix typo
(In reply to comment #27) > (From update of attachment 232541 [details] [review]) > For reference this patch has now been landed on master and on the cogl-1.14 > branch, thanks. OK, thanks. Sorry for the delay have been a busy lately. I went with your 1x1 pixel approach instead of a full sized FBO (based on your patches / rebased one of them). Review would be appreciated. (Last patch is just a typo I have found while reading the code).
Switching to clutter as there are no cogl changes left here.
Created attachment 235299 [details] [review] stage-cogl: Reuse backbuffer contents Use the buffer_age extension when available to recycle backbuffer contents instead of blitting from the back to front buffer when doing clipped redraws. The picking is now done in a pixel that is going to be repaired during the next redraw cycle for non static scences. This should improve performance and avoid tearing. --- As discussed on IRC here is a patch that uses a pixel in a dirty region instead of a 1x1 pixel FBO. So with this patch we don't need any FBOs for picking. Regarding the 2 picks threshold we talked about yesterday ... turned out that my debugging code was incorrect it seems to be sufficent.
Initially on reading this I had some concerns about using a list to track the history instead of a circular buffer since I can think of a few scenarios where that will result in some redundant full-stage redraws, but I think it's also possible to think of contrived examples where a circular buffer isn't ideal. I think for now we shouldn't worry to much about the pros and cons of using a list vs a circular buffer since this can be changed later if we have a specific use case which doesn't map well to using a list. One minor nit-pick would be that I think the ClutterStageCogl::damage member might warrant being renamed to 'damage_history' since I didn't think it was immediately obvious what was held in that member when reading some parts of the patch in isolation. So in summary I think this patch looks good to me: Reviewed-by: Robert Bragg <robert@linux.intel.com>
(In reply to comment #35) > Initially on reading this I had some concerns about using a list to track the > history instead of a circular buffer since I can think of a few scenarios where > that will result in some redundant full-stage redraws, but I think it's also > possible to think of contrived examples where a circular buffer isn't ideal. I > think for now we shouldn't worry to much about the pros and cons of using a > list vs a circular buffer since this can be changed later if we have a specific > use case which doesn't map well to using a list. > > One minor nit-pick would be that I think the ClutterStageCogl::damage member > might warrant being renamed to 'damage_history' since I didn't think it was > immediately obvious what was held in that member when reading some parts of the > patch in isolation. Yeah that makes sense. Thanks for the review.
Created attachment 235316 [details] [review] stage-cogl: Reuse backbuffer contents Use the buffer_age extension when available to recycle backbuffer contents instead of blitting from the back to front buffer when doing clipped redraws. The picking is now done in a pixel that is going to be repaired during the next redraw cycle for non static scences. This should improve performance and avoid tearing. Reviewed-by: Robert Bragg <robert@linux.intel.com>
Attachment 235096 [details] pushed as 60f20e8 - stage-window: make it possible to damage the back buffer Attachment 235099 [details] pushed as 683f15b - clutter-stage: Fix typo Attachment 235316 [details] pushed as b9ad93a - stage-cogl: Reuse backbuffer contents
Isn't this supposed to be fixed in gnome 3.8? I'm using gnome 3.8 final in arch linux, and I still get tearing at the top of the screen when watching fullscreen video on intel ivybridge...
Right, I'm getting tearing for NVIDIA proprietary drivers too. And Nouveau is just a mess: https://bugzilla.redhat.com/show_bug.cgi?id=949355
It needs the buffer_age OpenGL extension, which is not yet provided by any released mesa version.
(In reply to comment #39) > Isn't this supposed to be fixed in gnome 3.8? Yes but it requires driver support. > I'm using gnome 3.8 final in arch > linux, and I still get tearing at the top of the screen when watching > fullscreen video on intel ivybridge... Mesa only supports the required extension when using EGL. (In reply to comment #40) > Right, I'm getting tearing for NVIDIA proprietary drivers too. Which driver version? Number of monitors? Can you attach / pastebin the output of glxinfo? (In reply to comment #41) > It needs the buffer_age OpenGL extension, which is not yet provided by any > released mesa version. Yes because this requires DRI3000 which is still WIP.