Merge lp:~vanvugt/unity/fix-865006 into lp:unity

[Daniel van Vugt]

Fixed: The panel blur was not always updating when something behind it
changed (LP: #865006)

The reason for this bug was because we were tracking damage from the
XDamage extension. However XDamage only provides a subset of the total
damage applied to the compiz screen. For example, redrawing/moving window
decorations does not necessarily generate any XDamage. Nor do OpenGL
operations from compiz plugins.

[Daniel van Vugt (vanvugt)]

Note: If you're having trouble testing panel opacity and can't get it to work at all, then that's bug 833727.

[Daniel van Vugt (vanvugt)]

Hmm, I think this could be more efficient still.

[Sam Spilsbury (smspillaz)]

This code doesn't work the way you might expect it to. The regions passed to glPaintOutput won't always be the damage region compiz is using.

I don't know why it was never merged in before, but in any case, you can wrap CompositeScreen::damageRegion and mark dirty regions from there.

[Sam Spilsbury (smspillaz)]

review: Needs Fixing

[Sam Spilsbury (smspillaz)]

(Just remember to disable the wrap for damageRegion whenever we post damage to compiz. Eg, anywhere in unityshell where you might see something like:

cScreen->damageRegion (blah);

do something like

cScreen->damageRegionSetEnabled (UnityScreen::get (screen), false);
cScreen->damageRegion ();
cScreen->damageRegionSetEnabled (UnityScreen::get (screen), true);

)

[Daniel van Vugt (vanvugt)]

Sam, I've reviewed the damage logic through composite and opengl and I can't find any reason why glPaintOutput's region would ever exclude part of the output that has been damaged. Except for overlays, but we don't intend to draw blurs on top of overlays.

Please point me to the code that could cause glPaintOutput's region to be incomplete. I still don't see any reason for wrapping damage functions, which would be both uglier and less efficient.

[Daniel van Vugt (vanvugt)]

And a manual test case would be excellent. If there's something broken that I haven't tested with this fix then I'd like to know.

[Daniel van Vugt (vanvugt)]

If anyone can prove or suggest why this code might not work, please provide either steps to reproduce a bug, or a video or something showing problems.

[Sam Spilsbury (smspillaz)]

> Sam, I've reviewed the damage logic through composite and opengl and I can't
> find any reason why glPaintOutput's region would ever exclude part of the
> output that has been damaged. Except for overlays, but we don't intend to draw
> blurs on top of overlays.
>

> Please point me to the code that could cause glPaintOutput's region to be
> incomplete.

EvilPluginScreen::glPaintOutput (...,
                                 const CompRegion &region,
                                 ...)
{
    CompRegion emptyRegion;

    gScreen->glPaintOutput (..., emptyRegion, ...);
}

Once EvilPluginScreen calls glPaintOutput with a region that's not the damage region, its game over. In addition, the whole point of using XDamage in the first place was so that we could accumulate damage that happened during paints before the blurs were updated.

In addition:

http://bazaar.launchpad.net/~compiz-team/compiz/0.9.8/view/head:/plugins/composite/src/screen.cpp

 priv->tmpRegion = priv->damage & screen->region ();

 if (priv->damageMask & COMPOSITE_SCREEN_DAMAGE_REGION_MASK)
 {
     if (priv->tmpRegion == screen->region ())
  damageScreen ();
 }

...

    if (priv->pHnd)
 priv->pHnd->paintOutputs (outputs, mask, priv->tmpRegion);

http://bazaar.launchpad.net/~compiz-team/compiz/0.9.8/view/head:/plugins/opengl/src/screen.cpp#L1123

    CompRegion tmpRegion (region);

...

     outputRegion = tmpRegion & CompRegion (*output);

     if (!gScreen->glPaintOutput (defaultScreenPaintAttrib,
      identity,
      outputRegion, output,
      PAINT_SCREEN_REGION_MASK))

Say for example, I called in my wrapped CompositeScreen::paint (some plugins do this). Or if I called damageRegion inside of glPaintOutput *after* UnityScreen::glPaintOutput was called (remember, UnityScreen::glPaintOutput gets called *first* because it is loaded *last*)

For example

NormalPluginWindow::glDraw (..., const CompRegion &region, ...)
{
    GLMatrix sTransform (matrix);
    sTransform.toScreenSpace (output, -DEFAULT_Z_CAMERA);
    glPushMatrix ();
    glLoadMatrixf (sTransform.getMatrix ());
    draw_some_stuff ();
    damage_areas_where_i_drew_stuff ();
    glPopMatrix ();

    gWindow->glDraw (..., region, ...); // region is unchanged here, since we still wish to clip the window
};

In this case, your draw stack will be like this:

Unity Shell > Some Random Plugin Drawing (think unitymtgrabhandles, decor, unitydialog, group etc etc) > Some Window > The rest of the screen.

The blur will lag behind the random plugin drawing because you didn't get the updates in time (since you got the updates at the time your copy of glPaintOutput was called, which didn't even include the recently updated region anyways).

> I still don't see any reason for wrapping damage functions, which
> would be both uglier and less efficient.

I would deny both of those.

I made CompositeScreen::damageRegion wrapable for this reason. There is very little penalty (in fact, smaller than what core is even doing on CompositeScreen::damageRegion) in updating the nux dirty regions on CompositeScreen::damageRegion, and since the nux windows use full redraws, you can just return immediately (after passing the call chain) if...

review: Needs Fixing

[Sam Spilsbury (smspillaz)]

(Btw, the reason I say this is because we used to use a similar implementation before, and then Jason and I noticed that it was possible for blur updates to lag screen updates for this reason)

[Sam Spilsbury (smspillaz)]

(Second Note: for now, doing updates immidiately on damage will be a little less useful than we might think because we check BackgroundEffectHelper::HasDirtyHelpers (); at the start of ScreenEffectFramebufferObject::bind (). As such, damage updates are always processed on the next frame. Once GLES is merged we will always be binding the fbo (as it is required) and there will be no performance penalty from doing so since it is done immediately after glXSwapBuffers)

[Sam Spilsbury (smspillaz)]

Another case:

StupidPlugin::glPaintOutput (..., const CompRegion &region, ...)
{
    gScreen->glPaintOutput (..., infiniteRegion, ...);
}

Now we will be updating blurs all the time.

[Sam Spilsbury (smspillaz)]

Correction on my previous example:

NormalPluginWindow::glDraw (..., const CompRegion &region, ...)
{
    gWindow->glDraw (..., region, ...); // region is unchanged here, since we still wish to clip the window

    GLMatrix sTransform (matrix);
    sTransform.toScreenSpace (output, -DEFAULT_Z_CAMERA);
    glPushMatrix ();
    glLoadMatrixf (sTransform.getMatrix ());
    draw_some_stuff ();
    damage_areas_where_i_drew_stuff ();
    glPopMatrix ();
};

[Daniel van Vugt (vanvugt)]

Whoa, far too many words. I stopped reading a while ago.

Thanks. I can now see the possibility of the blur lagging by one frame. Although that's a much smaller issue than the blur's generally poor performance. I'm happy to make the change...

[Sam Spilsbury (smspillaz)]

Cool, looks good, you'll need to do 2 more things.

At any point where we call damageRegion () from within unity, you need to disable our handler so as to not get the "feedback effect" (such that we don't update damage on our own updates)

cScreen->damageRegionSetEnabled (this, false);
cScreen->damageRegion (reg);
cScreen->damageRegionSetEnabled (this, true);

Also bonus points for disabling damage region updates after UnityScreen::paintDisplay is called and re-enabling them just before ScreenEffectFramebufferObject::bind () is called.

[Sam Spilsbury (smspillaz)]

+ CompRect::vector rects(region.rects());

Also use const CompRect::vector & there

[Daniel van Vugt (vanvugt)]

Fixed.

[Sam Spilsbury (smspillaz)]

review: Approve

=== modified file 'plugins/unityshell/src/unityshell.cpp'
--- plugins/unityshell/src/unityshell.cpp	2012-06-13 03:54:37 +0000
+++ plugins/unityshell/src/unityshell.cpp	2012-06-14 05:27:19 +0000
@@ -1333,7 +1333,14 @@
   geo = lastTooltipArea;
   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->damageRegionSetEnabled(this, true);
 
   wt->ClearDrawList();
 
@@ -1493,44 +1500,18 @@
   {
     wt->ProcessForeignEvent(event, NULL);
   }
+}
 
-  if (event->type == cScreen->damageEvent() + XDamageNotify)
+void UnityScreen::damageRegion(const CompRegion &region)
+{
+  const CompRect::vector &rects(region.rects());
+  for (const CompRect &r : rects)
   {
-    XDamageNotifyEvent *de = (XDamageNotifyEvent *) event;
-    CompWindow* w = screen->findWindow (de->drawable);
-    std::vector<Window> const& xwns = nux::XInputWindow::NativeHandleList();
-    CompWindow* lastNWindow = screen->findWindow (xwns.back ());
-    bool        processDamage = true;
-
-    if (w)
-    {
-      if (!w->overrideRedirect () &&
-          w->isViewable () &&
-          !w->invisible ())
-      {
-
-        for (; lastNWindow != NULL; lastNWindow = lastNWindow->next)
-        {
-          if (lastNWindow == w)
-          {
-            processDamage = false;
-            break;
-          }
-        }
-
-        if (processDamage)
-        {
-          nux::Geometry damage (de->area.x, de->area.y, de->area.width, de->area.height);
-
-          const CompWindow::Geometry &geom = w->geometry ();
-          damage.x += geom.x () + geom.border ();
-          damage.y += geom.y () + geom.border ();
-
-          BackgroundEffectHelper::ProcessDamage(damage);
-        }
-      }
-    }
+    nux::Geometry geo(r.x(), r.y(), r.width(), r.height());
+    BackgroundEffectHelper::ProcessDamage(geo);
   }
+
+  cScreen->damageRegion(region);
 }
 
 void UnityScreen::handleCompizEvent(const char* plugin,

=== modified file 'plugins/unityshell/src/unityshell.h'
--- plugins/unityshell/src/unityshell.h	2012-05-30 18:43:17 +0000
+++ plugins/unityshell/src/unityshell.h	2012-06-14 05:27:19 +0000
@@ -111,6 +111,7 @@
                      const char         *eventName,
                      CompOption::Vector &o);
 
+  void damageRegion(const CompRegion &region);
 
   /* paint on top of all windows if we could not find a window
    * to paint underneath */