Code review comment for lp:~vanvugt/unity/regionalDamage

I've noticed that on multiple monitors there is definitely a feedback effect, where compizDamageNux causes something on the second screen to redraw, and then that redraw region is fed right back to us in glPaintOutput which causes that thing to be queued for redraw again.

I'll let you come to your own ideas on this, but one "solution" I've found (and I say "solution" because it has known problems which I think are fixable, more on that on a bit) is to use damageRegion to compute the dirty regions that need redrawing so that we can use the wrap handlers to avoid feedback. For example:

=== modified file 'plugins/unityshell/src/unityshell.cpp'
--- plugins/unityshell/src/unityshell.cpp 2012-06-17 14:17:45 +0000
+++ plugins/unityshell/src/unityshell.cpp 2012-06-17 14:18:38 +0000
@@ -1306,34 +1306,35 @@
 {
   bool ret;
+
+ bool requiring_repaint = false;
+
   // A few cases where we want to force the shell to be painted
- if (forcePaintOnTop() ||
- PluginAdapter::Default()->IsExpoActive() ||
+ if ((GetProhibitedPaintMasks () & ShellPaintRequestorInterface::WindowPaintRequestor) ||
+ PluginAdapter::Default ()->IsExpoActive() ||
       (mask & (PAINT_SCREEN_TRANSFORMED_MASK |
                PAINT_SCREEN_WITH_TRANSFORMED_WINDOWS_MASK)))
   {
- compizDamageNux(region);
- doShellRepaint = true;
+ requiring_repaint = true;
   }
   else if (shellIsHidden(*output))
   {
     // Don't ever waste GPU and CPU rendering the shell in games/benchmarks!
- doShellRepaint = false;
+ requiring_repaint = false;
   }
   else
   {
- compizDamageNux(region);
- doShellRepaint = wt->GetDrawList().size() > 0 ||
- BackgroundEffectHelper::HasDirtyHelpers();
+ bool isDamaged = RegionIsNuxDamaged (CompRegion (*output));
+ requiring_repaint = isDamaged &&
+ (wt->GetDrawList().size() > 0 ||
+ BackgroundEffectHelper::HasDirtyHelpers());
   }

   /* glPaintOutput is part of the opengl plugin, so we need the GLScreen base class. */
@@ -1419,6 +1416,19 @@

 }

+bool UnityScreen::RegionIsNuxDamaged (const CompRegion &r)
+{
+ static CompRegion tmp = emptyRegion;
+
+ tmp &= emptyRegion;
+ tmp |= r;
+ tmp &= nux_damage_;
+
+ nux_damage_ -= tmp;
+
+ return !tmp.isEmpty ();
+}
+

^ Checks if a particular output region has damaged nux regions intersecting the output region that came from damageRegion

 void UnityScreen::donePaint()
 {
   std::list <ShowdesktopHandlerWindowInterface *> remove_windows;
@@ -1491,8 +1501,6 @@
 /* Grab changed nux regions and add damage rects for them */
 void UnityScreen::nuxDamageCompiz()
 {
- CompRegion nux_damage;
-
   if (damaged)
     return;

@@ -1503,7 +1511,7 @@
        it != end; ++it)
   {
     nux::Geometry const& geo = *it;
- nux_damage += CompRegion(geo.x, geo.y, geo.width, geo.height);
+ nux_damage_ += CompRegion(geo.x, geo.y, geo.width, geo.height);
   }

   // launcher_controller_ will still be null on startup
@@ -1518,22 +1526,22 @@
         if (!tooltip.IsNull())
         {
           const nux::Geometry &g = tooltip->GetAbsoluteGeometry();
- nux_damage += CompRegion(g.x, g.y, g.width, g.height);
+ nux_damage_ += CompRegion(g.x, g.y, g.width, g.height);
         }
       }
     }
   }

   nux::Geometry geo = lastTooltipArea;
- nux_damage += CompRegion(lastTooltipArea.x, lastTooltipArea.y,
- lastTooltipArea.width, lastTooltipArea.height);
+ nux_damage_ += CompRegion(lastTooltipArea.x, lastTooltipArea.y,
+ lastTooltipArea.width, lastTooltipArea.height);

   /*
    * Avoid Nux damaging Nux as recommended by smspillaz. Though I don't
    * believe it would be harmful or significantly expensive right now.
    */
   cScreen->damageRegionSetEnabled(this, false);
- cScreen->damageRegion(nux_damage);
+ cScreen->damageRegion(nux_damage_);
   cScreen->damageRegionSetEnabled(this, true);

   lastTooltipArea = geo;
@@ -1703,6 +1711,7 @@
     BackgroundEffectHelper::ProcessDamage(geo);
   }

+ compizDamageNux(region);
   cScreen->damageRegion(region);
 }

^ Buffer up nux damage, mark the nux bits as dirty if need be.

=== modified file 'plugins/unityshell/src/unityshell.h'
--- plugins/unityshell/src/unityshell.h 2012-06-17 14:17:45 +0000
+++ plugins/unityshell/src/unityshell.h 2012-06-17 14:18:38 +0000
@@ -246,6 +246,9 @@
   void RequireShellRepaint ();
   bool RepaintPending ();

+ /* Clears the nux damage region */
+ bool RegionIsNuxDamaged (const CompRegion &reg);
+
   ShellPaintDispatch paint_dispatch_;

   Settings dash_settings_;
@@ -327,6 +330,8 @@
   glib::SourceManager sources_;
   unsigned int no_paint_allowed_mask_;

+ CompRegion nux_damage_;
+
   friend class UnityWindow;
 };

(Note: diff not directly applicable, I had it merged with other stuff, but the behaviour is the same).

A small thing to note with this possible approach is that it will require some changes to nux. Specifically, since we call wt->RenderInterfaceFromForeignCmd in glPaintOutput, this is done N output times per paint pass, so the draw list will be cleared completely on the first output, and the second output won't be drawn at all. So nux needs to only release things from the draw list if they intersect the output geometry provided in RenderInterfaceFromForeignCmd.

Just a thought I guess.