Merge lp:~vanvugt/mir/fix-compositor-lag into lp:~mir-team/mir/trunk

[Daniel van Vugt]

Fix the compositor side of lag observed between input events and the screen.
This is half the fix for LP: #1199450.

The other half of the fix is to resolve client buffers arriving out of order,
which has not been fully diagnosed but is known to be resolved by the
"switch" branch.

I think this is still a bit of a kludge. It would be nice if we could work a more elegant solution into the architecture over time.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:948
http://jenkins.qa.ubuntu.com/job/mir-ci/1255/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1660
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1545
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/493/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1255/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:950
http://jenkins.qa.ubuntu.com/job/mir-ci/1256/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1661
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1546
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/494
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/494/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1256/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

It isn't entirely a new problem, but I don't like seeing tests that are dependent on timing intervals.

[Daniel van Vugt (vanvugt)]

Alan,

I understand. Aside from longer execution times, it creates the risk of spurious failures. On the other hand, testing code in its "natural environment" without mocking (where you can) is arguably better practice.

Also, the test in question was already timing-dependent.

[Alan Griffiths (alan-griffiths)]

> Alan,
>
> I understand. Aside from longer execution times, it creates the risk of
> spurious failures. On the other hand, testing code in its "natural
> environment" without mocking (where you can) is arguably better practice.

At the risk of straying off the topic of this MP...

Unit tests are about testing code in isolation. That is, with test doubles to replace its "natural environment".

It is best practice to have tests at unit, integration and system level.

> Also, the test in question was already timing-dependent.

Already noted. ;)

[Alexandros Frantzis (afrantzis)]

I am not set up to test the lag issue, but otherwise looks good, doesn't seem to cause any unexpected effects.

review: Approve

[Kevin DuBois (kdub)]

lgtm

review: Approve

=== modified file 'include/server/mir/compositor/multi_threaded_compositor.h'
--- include/server/mir/compositor/multi_threaded_compositor.h	2013-07-26 05:12:14 +0000
+++ include/server/mir/compositor/multi_threaded_compositor.h	2013-08-08 08:53:04 +0000
@@ -34,6 +34,8 @@
 namespace compositor
 {
 
+enum {max_client_buffers = 3};
+
 class DisplayBufferCompositorFactory;
 class CompositingFunctor;
 class Scene;

=== modified file 'src/server/compositor/multi_threaded_compositor.cpp'
--- src/server/compositor/multi_threaded_compositor.cpp	2013-07-26 05:12:14 +0000
+++ src/server/compositor/multi_threaded_compositor.cpp	2013-08-08 08:53:04 +0000
@@ -60,7 +60,7 @@
         : display_buffer_compositor_factory{db_compositor_factory},
           buffer(buffer),
           running{true},
-          compositing_scheduled{false}
+          frames_scheduled{0}
     {
     }
 
@@ -79,10 +79,10 @@
         while (running)
         {
             /* Wait until compositing has been scheduled or we are stopped */
-            while (!compositing_scheduled && running)
+            while (!frames_scheduled && running)
                 run_cv.wait(lock);
 
-            compositing_scheduled = false;
+            frames_scheduled--;
 
             /*
              * Check if we are running before compositing, since we may have
@@ -100,11 +100,17 @@
     void schedule_compositing()
     {
         std::lock_guard<std::mutex> lock{run_mutex};
-        if (!compositing_scheduled)
-        {
-            compositing_scheduled = true;
-            run_cv.notify_one();
-        }
+
+        /*
+         * Each surface could have a number of frames ready in its buffer
+         * queue. And we need to ensure that we render all of them so that
+         * none linger in the queue indefinitely (seen as input lag). So while
+         * there's no API support for finding out queue lengths, assume the
+         * worst and schedule enough frames to ensure all surfaces' queues
+         * are fully drained.
+         */
+        frames_scheduled = max_client_buffers;
+        run_cv.notify_one();
     }
 
     void stop()
@@ -118,7 +124,7 @@
     std::shared_ptr<mc::DisplayBufferCompositorFactory> const display_buffer_compositor_factory;
     mg::DisplayBuffer& buffer;
     bool running;
-    bool compositing_scheduled;
+    int frames_scheduled;
     std::mutex run_mutex;
     std::condition_variable run_cv;
 };

=== modified file 'tests/unit-tests/compositor/test_multi_threaded_compositor.cpp'
--- tests/unit-tests/compositor/test_multi_threaded_compositor.cpp	2013-07-26 05:12:14 +0000
+++ tests/unit-tests/compositor/test_multi_threaded_compositor.cpp	2013-08-08 08:53:04 +0000
@@ -202,17 +202,18 @@
 
     bool check_record_count_for_each_buffer(
             unsigned int nbuffers,
-            std::function<bool(unsigned int)> const& check)
+            unsigned int min,
+            unsigned int max = ~0u)
     {
         std::lock_guard<std::mutex> lk{m};
 
         if (records.size() < nbuffers)
-            return false;
+            return (min == 0 && max == 0);
 
         for (auto const& e : records)
         {
             Record const& r = e.second;
-            if (!check(r.first))
+            if (r.first < min || r.first > max)
                 return false;
         }
 
@@ -327,36 +328,60 @@
 {
     using namespace testing;
 
-    unsigned int const nbuffers{3};
+    unsigned int const nbuffers{mc::max_client_buffers};
 
     auto display = std::make_shared<StubDisplay>(nbuffers);
     auto scene = std::make_shared<StubScene>();
     auto db_compositor_factory = std::make_shared<RecordingDisplayBufferCompositorFactory>();
     mc::MultiThreadedCompositor compositor{display, scene, db_compositor_factory};
 
-    auto at_least_one = [](unsigned int n){return n >= 1;};
-    auto at_least_two = [](unsigned int n){return n >= 2;};
-    auto exactly_two = [](unsigned int n){return n == 2;};
+    // Verify we're actually starting at zero frames
+    EXPECT_TRUE(db_compositor_factory->check_record_count_for_each_buffer(nbuffers, 0, 0));
 
     compositor.start();
 
-    /* Wait until buffers have been rendered to once (initial render) */
-    while (!db_compositor_factory->check_record_count_for_each_buffer(nbuffers, at_least_one))
+    // Initial render: 3 frames should be composited at least
+    while (!db_compositor_factory->check_record_count_for_each_buffer(nbuffers, 3))
         std::this_thread::sleep_for(std::chrono::milliseconds(10));
 
+    // Now we have 3 redraws, pause for a while
+    std::this_thread::sleep_for(std::chrono::milliseconds(1000));
+
+    // ... and make sure the number is still only 3
+    EXPECT_TRUE(db_compositor_factory->check_record_count_for_each_buffer(nbuffers, nbuffers, nbuffers));
+
+    // Trigger more surface changes
     scene->emit_change_event();
 
-    /* Wait until buffers have been rendered to twice (initial render + change) */
-    while (!db_compositor_factory->check_record_count_for_each_buffer(nbuffers, at_least_two))
-        std::this_thread::sleep_for(std::chrono::milliseconds(10));
-
-    /* Give some more time for other renders (if any) to happen */
-    std::this_thread::sleep_for(std::chrono::milliseconds(10));
+    // Display buffers should be forced to render another 3, so that's 6
+    while (!db_compositor_factory->check_record_count_for_each_buffer(nbuffers, 2*nbuffers))
+        std::this_thread::sleep_for(std::chrono::milliseconds(10));
+
+    // Now pause without any further surface changes
+    std::this_thread::sleep_for(std::chrono::milliseconds(1000));
+
+    // Verify we never triggered more than 6 compositions
+    EXPECT_TRUE(db_compositor_factory->check_record_count_for_each_buffer(nbuffers, 2*nbuffers, 2*nbuffers));
+
+    compositor.stop();  // Pause the compositor so we don't race
+
+    // Now trigger many surfaces changes close together
+    for (int i = 0; i < 10; i++)
+        scene->emit_change_event();
+
+    compositor.start();
+
+    // Display buffers should be forced to render another 3, so that's 9
+    while (!db_compositor_factory->check_record_count_for_each_buffer(nbuffers, 3*nbuffers))
+        std::this_thread::sleep_for(std::chrono::milliseconds(10));
+
+    // Now pause without any further surface changes
+    std::this_thread::sleep_for(std::chrono::milliseconds(1000));
+
+    // Verify we never triggered more than 9 compositions
+    EXPECT_TRUE(db_compositor_factory->check_record_count_for_each_buffer(nbuffers, 3*nbuffers, 3*nbuffers));
 
     compositor.stop();
-
-    /* Only two renders should have happened */
-    EXPECT_TRUE(db_compositor_factory->check_record_count_for_each_buffer(nbuffers, exactly_two));
 }
 
 TEST(MultiThreadedCompositor, surface_update_from_render_doesnt_deadlock)