Merge lp:~vanvugt/unity/fix-861061-trunk into lp:unity

[Daniel van Vugt]

Fix major performance regressions due to unnecessary UnityFBO binding (LP: #861061) (LP: #880707)

UnityFBO was being bound even when not required. This caused major lag in glPaintOutput, which slowed down all rendering. This was seen in reduced framerates in apps (LP: #861061) and significantly worse screen tearing with Unity 4.x compared to 3.x (LP: #880707).

[Mikkel Kamstrup Erlandsen (kamstrup)]

(please excuse me, I don't know this code very well and I only looked
at the diff)

Does the ref handling not break now that bind() is conditionalized? It
would seem that you always unbind, but only conditionally bind the
fbo.

[Daniel van Vugt (vanvugt)]

unityshell does not "always unbind". It only unbinds if it is bound:
http://bazaar.launchpad.net/~vanvugt/unity/fix-861061-trunk/view/head:/plugins/unityshell/src/unityshell.cpp#L552

[Marco Trevisan (Treviño) (3v1n0)]

I guess that this merge request should be placed also for lp:unity/4.0 in case that Sam confirms it's fine.

[Daniel van Vugt (vanvugt)]

I already planned doing a proposal for oneiric after it's been approved for lp:unity.

Sam tells me he will get around to looking at this in the near future.

[Daniel van Vugt (vanvugt)]

This fix has now been verified by the two users who reported bug 861061 (and 1 duplicate). They both report it works for them using ppa:vanvugt/unity. More details in bug 861061.

[Daniel van Vugt (vanvugt)]

Of course, I too have verified the fix and tested it on i915, nvidia, nouveau, radeon and fglrx.

[Marco Trevisan (Treviño) (3v1n0)]

Ok, it looks good to me and user testing seems to confirm that is fine, however I guess that you need at least a manual test, so please add one as documented on Unity trunk

[Daniel van Vugt (vanvugt)]

The test case is documented at the top of bug 861061.

[Sam Spilsbury (smspillaz)]

(Will add a standalone version of the UnityFBO stuff very soon, along with lp:~smspillaz/unity/unity.big_fbo)

[Sam Spilsbury (smspillaz)]

(This branch has been merged into a larger work-branch to do with unityfbo in https://code.launchpad.net/~smspillaz/unity/unity.fix_864784_868120_872625v2/+merge/86064)

[Daniel van Vugt (vanvugt)]

I thought it might overlap with you ongoing FBO work in precise. But should I also target this simple fix in a new proposal for oneiric?...

I'm a little concerned that by delaying the fix in precise, it means a fix for oneiric won't get approved until even later than that.

[Sam Spilsbury (smspillaz)]

Well, the testcases for all of them need to come at the same time basically, so its easier to roll it all into one branch.

The plan is to SRU the framebuffer object work in oneiric once its tested. Namely because it fixes a boost::shared_ptr related abort () when hotplugging monitors and it also fixes noticable visual glitches with the expo plugin. Plus, I had planned to incorporate some of the same changes you had made in this branch which improve performance when no framebuffer objects need to be updated.

Unfortunately getting people to actually test this is difficult since everybody is on holiday. Additionally, I'm waiting on a way that we can automate the testcase that was made in https://code.launchpad.net/~smspillaz/unity/unity.fix_864784_868120_872625v2/+merge/86064 using gmock, and I think that's being done now or very soon in the future.

Considering that we finally got someone to test it on fglrx, I'm quite keen on getting all of this landed at the same time.

[Daniel van Vugt (vanvugt)]

I thought it was obvious already, but your branch looks way to complicated to be considered for an SRU. I was assuming this one would be used in the SRU.

[Sam Spilsbury (smspillaz)]

On Tue, 20 Dec 2011, Daniel van Vugt wrote:

> I thought it was obvious already, but your branch looks way to complicated to be considered for an SRU. I was assuming this one would be used in the SRU.

It may look noisy, but the majority of the noise comes from the fact that
I needed to rip out the fbo classes into something that could be tested
independently and that a few variable names were changed.

The actual *behavioural* change is the move from using one framebuffer
object per monitor to using one framebuffer object per screen. (eg, one
framebuffer object).

I'm confident that we can SRU it.

> --
> https://code.launchpad.net/~vanvugt/unity/fix-861061-trunk/+merge/82861
> You are requested to review the proposed merge of lp:~vanvugt/unity/fix-861061-trunk into lp:unity.
>

[Sam Spilsbury (smspillaz)]

review: Approve

[Daniel van Vugt (vanvugt)]

Please fix! The critical part of this bug fix did not get merged:

 - mFbos[output]->bind ();
 + if (BackgroundEffectHelper::HasDirtyHelpers())
 + mFbos[output]->bind ();

[Daniel van Vugt (vanvugt)]

Please "re-merge", I mean :)

[Daniel van Vugt (vanvugt)]

Sorry, my mistake. The fix is present and looks complete in lp:unity. Merge done.

[Sam Spilsbury (smspillaz)]

+void unity::ScreenEffectFramebufferObject::bind (const nux::Geometry &output)
+{
+ if (!BackgroundEffectHelper::HasDirtyHelpers())
+ return;

Confirmed.

=== modified file 'plugins/unityshell/src/BackgroundEffectHelper.cpp'
--- plugins/unityshell/src/BackgroundEffectHelper.cpp	2011-09-28 05:19:39 +0000
+++ plugins/unityshell/src/BackgroundEffectHelper.cpp	2011-11-21 10:41:26 +0000
@@ -91,6 +91,15 @@
   return false;
 }
 
+bool BackgroundEffectHelper::HasDirtyHelpers()
+{
+  for (BackgroundEffectHelper * bg_effect_helper : registered_list_)
+    if (bg_effect_helper->enabled && bg_effect_helper->cache_dirty)
+      return true;
+
+  return false;
+}
+
 void BackgroundEffectHelper::Register(BackgroundEffectHelper* self)
 {
   registered_list_.push_back(self);

=== modified file 'plugins/unityshell/src/BackgroundEffectHelper.h'
--- plugins/unityshell/src/BackgroundEffectHelper.h	2011-10-19 22:13:57 +0000
+++ plugins/unityshell/src/BackgroundEffectHelper.h	2011-11-21 10:41:26 +0000
@@ -53,7 +53,7 @@
   void DirtyCache();
 
   static void ProcessDamage(nux::Geometry geo);
-
+  static bool HasDirtyHelpers();
   static bool HasEnabledHelpers();
 
   static nux::Property<unity::BlurType> blur_type;

=== modified file 'plugins/unityshell/src/PanelView.cpp'
--- plugins/unityshell/src/PanelView.cpp	2011-10-25 16:08:30 +0000
+++ plugins/unityshell/src/PanelView.cpp	2011-11-21 10:41:26 +0000
@@ -593,8 +593,7 @@
 
   _opacity = opacity;
 
-  if (_opacity < 1.0f && !_dash_is_open)
-    bg_effect_helper_.enabled = false;
+  bg_effect_helper_.enabled = (_opacity < 1.0f || _dash_is_open);
 
   ForceUpdateBackground();
 }

=== modified file 'plugins/unityshell/src/unityshell.cpp'
--- plugins/unityshell/src/unityshell.cpp	2011-11-14 18:39:33 +0000
+++ plugins/unityshell/src/unityshell.cpp	2011-11-21 10:41:26 +0000
@@ -940,7 +940,8 @@
    *   attempts to bind it will only increment
    *   its bind reference so make sure that
    *   you always unbind as much as you bind */
-  mFbos[output]->bind ();
+  if (BackgroundEffectHelper::HasDirtyHelpers())
+    mFbos[output]->bind ();
 
   doShellRepaint = true;
   allowWindowPaint = true;