Merge lp:~sil2100/compiz-workarounds-plugin/initial_damage into lp:compiz-workarounds-plugin

[Łukasz Zemczak]

= Problem description =

Sometimes, X returns strange XDamageNotifyEvent's to our processing damage callbacks. Those damage events seem to have invalid area coordinates, set to the same values as the geometry. One of the resulting bugs is, for instance, #931473 - as because of this sometimes menus on initial popup are not updated as they should.

= The fix =

As a workaround, we prepare a damageRect() callback that forces a complete window redraw for initial damage handling (as advised by Sam, thanks!)

= Test coverage =

It is possible to check if the fix works by trying to reproduce the #931473 bug with the fix applied. Fast titlebar context menu invoking now always shows the menu fully redrawn.

[Daniel van Vugt (vanvugt)]

In testing it seems to work fine. But I do have some concerns:
  1. This workaround is not configurable. It's hard-coded.
  2. I don't think we have found the root cause. It's only a workaround.
  3. Because of #1, we may never notice or solve #2.

review: Approve

[Daniel van Vugt (vanvugt)]

Disclaimer: "seems to work fine" means I couldn't see any regressions. I have never been able to reproduce bug 931473 and so can't fully verify the fix.

[Sam Spilsbury (smspillaz)]

Conditional approve - could you also add

cWindow->damageRectSetEnabled (this, false);

At the bottom of CompositeWindow::damageRect ? Its a small optimization that goes a long way.

review: Approve

[Sam Spilsbury (smspillaz)]

Oh, please also make this an option too, since Workarounds needs to work where composite is not enabled.

(Eg, add it to the xml file, then use optionGetFooWhatever ())

[Sam Spilsbury (smspillaz)]

Also to clarify, indeed this is a workaround, however, what appears to be happening is that on the initial XDamageNotify event when the window is mapped, it is convention that the damaged area covers the entire drawable, but for some reason, it isn't.

I'll be doing some more testing on this in future to see where X bugs need to be logged.

[Daniel van Vugt (vanvugt)]

I would bet it's not an X bug. More likely a race somewhere.

[lp:~sil2100/compiz-workarounds-plugin/initial_damage]

120. By Łukasz Zemczak on 2012-02-24

Added the damageRect fix as an option (is such a check sufficient?).

121. By Łukasz Zemczak on 2012-02-24

As noted by Sam - for optimization, we should call damageRectSetEnabled (false) to skip this function until enabled again. For this dispatch table of course.

[Łukasz Zemczak (sil2100)]

Fixed the two mentioned issues.

[Sam Spilsbury (smspillaz)]

> I would bet it's not an X bug. More likely a race somewhere.

Sending wrong co-ordinates definitely feels like an X bug to me :)

[Daniel van Vugt (vanvugt)]

The coordinates may only be wrong "at the time" and were right at a slightly earlier time for whatever reason. Hence the possibility of it being a race.

[Sam Spilsbury (smspillaz)]

On Sun, 26 Feb 2012, Daniel van Vugt wrote:

> The coordinates may only be wrong "at the time" and were right at a slightly earlier time for whatever reason. Hence the possibility of it being a race.

Damage co-ordinates are relative to the pixmap storage itself. When a
pixmap is first drawn upon, you would expect the entire pixmap region to
be damaged. Eg, it should always be 0x0+w+h. This is not the case.

> --
> https://code.launchpad.net/~sil2100/compiz-workarounds-plugin/initial_damage/+merge/94232
> You are reviewing the proposed merge of lp:~sil2100/compiz-workarounds-plugin/initial_damage into lp:compiz-workarounds-plugin.
>

[Daniel van Vugt (vanvugt)]

Revision 121 looks and works OK too.

review: Approve

=== modified file 'src/workarounds.cpp'
--- src/workarounds.cpp	2012-02-21 03:51:25 +0000
+++ src/workarounds.cpp	2012-02-24 08:54:19 +0000
@@ -299,6 +299,17 @@
     return gWindow->glPaint (attrib, transform, region, mask);
 }
 
+bool
+WorkaroundsWindow::damageRect (bool initial, const CompRect &rect)
+{
+    if (initial)
+	cWindow->addDamage (true);
+
+    cWindow->damageRectSetEnabled (this, false);
+
+    return cWindow->damageRect (initial, rect);
+}
+
 void
 WorkaroundsScreen::checkFunctions (bool checkWindow, bool checkScreen)
 {
@@ -1075,6 +1086,9 @@
 
     WORKAROUNDS_SCREEN (screen);
 
+    if (ws->optionGetInitialDamageCompleteRedraw ())
+	CompositeWindowInterface::setHandler (cWindow);
+
     if (ws->optionGetLegacyFullscreen ())
     {
 	window->getAllowedActionsSetEnabled (this, false);

=== modified file 'src/workarounds.h'
--- src/workarounds.h	2012-01-13 00:14:05 +0000
+++ src/workarounds.h	2012-02-24 08:54:19 +0000
@@ -122,6 +122,7 @@
 class WorkaroundsWindow :
     public PluginClassHandler <WorkaroundsWindow, CompWindow>,
     public WindowInterface,
+    public CompositeWindowInterface,
     public GLWindowInterface
 {
     public:
@@ -200,6 +201,9 @@
 	unsigned int
 	getFixedWindowType ();
 
+	bool
+	damageRect (bool initial, const CompRect &rect);
+
 };
 
 #define WORKAROUNDS_WINDOW(w)						       \

=== modified file 'workarounds.xml.in'
--- workarounds.xml.in	2012-01-13 00:14:05 +0000
+++ workarounds.xml.in	2012-02-24 08:54:19 +0000
@@ -83,6 +83,11 @@
 		<_long>Don't wait for the next video sync time to redraw</_long>
 		<default>false</default>
 	    </option>
+	    <option type="bool" name="initial_damage_complete_redraw">
+		<_short>Force complete redraw on initial damage</_short>
+		<_long>Force complete redraw of the window on initial damage event</_long>
+		<default>true</default>
+	    </option>
 	    <option type="bool" name="force_swap_buffers">
 		<_short>Force full screen redraws (buffer swap) on repaint</_short>
 		<_long>Forces the entire screen to redraw every repaint. Use with care, this will cause a massive increase in GPU and CPU usage</_long>