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

Proposed by Łukasz Zemczak on 2012-02-22
Status: Merged
Approved by: Daniel van Vugt on 2012-02-27
Approved revision: 121
Merged at revision: 119
Proposed branch: lp:~sil2100/compiz-workarounds-plugin/initial_damage
Merge into: lp:compiz-workarounds-plugin
Diff against target: 69 lines (+23/-0)
3 files modified
src/workarounds.cpp (+14/-0)
src/workarounds.h (+4/-0) (+5/-0)
To merge this branch: bzr merge lp:~sil2100/compiz-workarounds-plugin/initial_damage
Reviewer Review Type Date Requested Status
Daniel van Vugt Approve on 2012-02-27
Sam Spilsbury 2012-02-22 Approve on 2012-02-24
Review via email:

Description of the change

= 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.

To post a comment you must log in.
Daniel van Vugt (vanvugt) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

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

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) wrote :

Fixed the two mentioned issues.

Sam Spilsbury (smspillaz) wrote :

> 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) 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.

Sam Spilsbury (smspillaz) wrote :

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.

> --
> You are reviewing the proposed merge of lp:~sil2100/compiz-workarounds-plugin/initial_damage into lp:compiz-workarounds-plugin.

Daniel van Vugt (vanvugt) wrote :

Revision 121 looks and works OK too.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/workarounds.cpp'
2--- src/workarounds.cpp 2012-02-21 03:51:25 +0000
3+++ src/workarounds.cpp 2012-02-24 08:54:19 +0000
4@@ -299,6 +299,17 @@
5 return gWindow->glPaint (attrib, transform, region, mask);
6 }
9+WorkaroundsWindow::damageRect (bool initial, const CompRect &rect)
11+ if (initial)
12+ cWindow->addDamage (true);
14+ cWindow->damageRectSetEnabled (this, false);
16+ return cWindow->damageRect (initial, rect);
19 void
20 WorkaroundsScreen::checkFunctions (bool checkWindow, bool checkScreen)
21 {
22@@ -1075,6 +1086,9 @@
26+ if (ws->optionGetInitialDamageCompleteRedraw ())
27+ CompositeWindowInterface::setHandler (cWindow);
29 if (ws->optionGetLegacyFullscreen ())
30 {
31 window->getAllowedActionsSetEnabled (this, false);
33=== modified file 'src/workarounds.h'
34--- src/workarounds.h 2012-01-13 00:14:05 +0000
35+++ src/workarounds.h 2012-02-24 08:54:19 +0000
36@@ -122,6 +122,7 @@
37 class WorkaroundsWindow :
38 public PluginClassHandler <WorkaroundsWindow, CompWindow>,
39 public WindowInterface,
40+ public CompositeWindowInterface,
41 public GLWindowInterface
42 {
43 public:
44@@ -200,6 +201,9 @@
45 unsigned int
46 getFixedWindowType ();
48+ bool
49+ damageRect (bool initial, const CompRect &rect);
51 };
53 #define WORKAROUNDS_WINDOW(w) \
55=== modified file ''
56--- 2012-01-13 00:14:05 +0000
57+++ 2012-02-24 08:54:19 +0000
58@@ -83,6 +83,11 @@
59 <_long>Don't wait for the next video sync time to redraw</_long>
60 <default>false</default>
61 </option>
62+ <option type="bool" name="initial_damage_complete_redraw">
63+ <_short>Force complete redraw on initial damage</_short>
64+ <_long>Force complete redraw of the window on initial damage event</_long>
65+ <default>true</default>
66+ </option>
67 <option type="bool" name="force_swap_buffers">
68 <_short>Force full screen redraws (buffer swap) on repaint</_short>
69 <_long>Forces the entire screen to redraw every repaint. Use with care, this will cause a massive increase in GPU and CPU usage</_long>


People subscribed via source and target branches