Merge lp:~alan-griffiths/mir/fix-1288570 into lp:mir

[Alan Griffiths]

compositor: check that client buffers are actually available before completing client_acquire. (fixes 1288570)

compositor: check that client buffers are actually available before completing client_acquire. (fixes 1288570)

[Alexandros Frantzis (afrantzis)]

Without this patch I get a constant 120 FPS when the app is between screens. Applying this patch I get 60FPS for a few seconds, then a couple of seconds of increased frame rate, and so on:

120 FPS
60 FPS
60 FPS
60 FPS
60 FPS
60 FPS
80 FPS
120 FPS
61 FPS
60 FPS
60 FPS
60 FPS
60 FPS
77 FPS
120 FPS
64 FPS
60 FPS
60 FPS
60 FPS
60 FPS
78 FPS
120 FPS
62 FPS
60 FPS
60 FPS
60 FPS
60 FPS
78 FPS

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> Without this patch I get a constant 120 FPS when the app is between screens.
> Applying this patch I get 60FPS for a few seconds, then a couple of seconds of
> increased frame rate, and so on:

Weird. I see a steady 60fps for minutes.

As I can't reproduce your result could you help by trying -r1455 of this branch (which simply reinstates the synchronous behavior).

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1457
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1004/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1152
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1150
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/734
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/736
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/736/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/741
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/741/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/735
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/735/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/696
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4542

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1004/rebuild

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

With r1455 behavior is the same as with r1457:

60 FPS
66 FPS
120 FPS
77 FPS
60 FPS
60 FPS
60 FPS
60 FPS
65 FPS
120 FPS
78 FPS
60 FPS
60 FPS
60 FPS
60 FPS
65 FPS
120 FPS
78 FPS

FWIW, this is on an intel laptop with screen 1280x800, and external screen 1920x1080 connected with VGA.

[Alan Griffiths (alan-griffiths)]

On my intel laptop with screen 1600x900 and external 1920x1080 I see a slight variation in speed. (between 56fps and 62fps)

The animation looks fine throughout.

[Alan Griffiths (alan-griffiths)]

Update - I just saw it hit 90fps

[Daniel van Vugt (vanvugt)]

Seems to solve the problem for me. Though I don't yet understand the new algorithm to understand why.

Some thoughts:

(1) This shouldn't be inline. There's no performance benefit and it will just bloat the code slightly:
8 +inline
9 +bool mc::SwitchingBundle::client_buffers_available(std::unique_lock<std::mutex> const& /*lock*/)

(2) The lock parameter in the above function is unnecessary (except as documentation?), so should be removed. The function is always called inside a lock anyway.

review: Approve

[Alan Griffiths (alan-griffiths)]

The effect Alexandros reports running on his laptop already exists in -r 1397 and clearly has a different root cause to 1288570.

Landing this and reporting a new bug.

=== modified file 'src/server/compositor/switching_bundle.cpp'
--- src/server/compositor/switching_bundle.cpp	2014-03-06 05:29:00 +0000
+++ src/server/compositor/switching_bundle.cpp	2014-03-06 12:38:10 +0000
@@ -182,6 +182,29 @@
     return ring[slot].buf;
 }
 
+inline
+bool mc::SwitchingBundle::client_buffers_available(std::unique_lock<std::mutex> const& /*lock*/)
+{
+    /*
+     * Even if there are free buffers available, we might wish to still
+     * wait. This is so we don't touch (hence allocate) a third or higher
+     * buffer until/unless it's absolutely necessary. It becomes necessary
+     * only when framedropping (above) or with overlapping compositors
+     * (like with bypass).
+     * The performance benefit of triple buffering is usually minimal,
+     * but always uses 50% more memory. So try to avoid it when possible.
+     */
+
+    int min_free =
+#if 0  // FIXME: This memory optimization breaks timing tests
+        (nbuffers > 2 && !overlapping_compositors) ?  nbuffers - 1 : 1;
+#else
+        1;
+#endif
+
+    return nfree() >= min_free;
+}
+
 void mc::SwitchingBundle::client_acquire(std::function<void(graphics::Buffer* buffer)> complete)
 {
     std::unique_lock<std::mutex> lock(guard);
@@ -199,24 +222,7 @@
     }
     else
     {
-        /*
-         * Even if there are free buffers available, we might wish to still
-         * wait. This is so we don't touch (hence allocate) a third or higher
-         * buffer until/unless it's absolutely necessary. It becomes necessary
-         * only when framedropping (above) or with overlapping compositors
-         * (like with bypass).
-         * The performance benefit of triple buffering is usually minimal,
-         * but always uses 50% more memory. So try to avoid it when possible.
-         */
-
-        int min_free =
-#if 0  // FIXME: This memory optimization breaks timing tests
-            (nbuffers > 2 && !overlapping_compositors) ?  nbuffers - 1 : 1;
-#else
-            1;
-#endif
-
-        if (nfree() < min_free)
+        if (!client_buffers_available(lock))
             return;
     }
 
@@ -362,7 +368,8 @@
             ncompositors--;
         }
 
-        if (client_acquire_todo) complete_client_acquire(std::move(lock));
+        if (client_buffers_available(lock) && client_acquire_todo)
+            complete_client_acquire(std::move(lock));
     }
 }
 

=== modified file 'src/server/compositor/switching_bundle.h'
--- src/server/compositor/switching_bundle.h	2014-03-06 05:29:00 +0000
+++ src/server/compositor/switching_bundle.h	2014-03-06 12:38:10 +0000
@@ -84,6 +84,7 @@
 
     const std::shared_ptr<graphics::Buffer> &alloc_buffer(int slot);
     void complete_client_acquire(std::unique_lock<std::mutex> lock);
+    bool client_buffers_available(std::unique_lock<std::mutex> const& lock);
     struct SharedBuffer
     {
         std::shared_ptr<graphics::Buffer> buf;