Merge lp:~vanvugt/mir/flush into lp:mir

[Daniel van Vugt]

When restarting the compositor (including waking from sleep), flush
the scene and wait briefly for fresh frames. Then composite as soon as the
client (nested server) provides a new frame or the timeout of 500ms expires,
whichever happens first. This avoids the user seeing any stale frames on
wakeup.

This feature will also be useful in future as clients adjust to
display config changes seamlessly without visibly lagging behind the new
config (like resolution changes when a window is maximized).

This branch was intended to help with LP: #1340510. Although it's now clear that bug is more to do with Unity8/dbus and less to do with Mir. So this improvement to Mir isn't expected to solve LP: #1340510 by itself.

Still, this proposal is useful to have for other potential shells that respond to wakeup more quickly than Unity8 does right now.

[Kevin DuBois (kdub)]

needs fixing:

166 + void schedule_compositing(int number_composites,
167 + std::chrono::milliseconds delay = std::chrono::milliseconds::zero());

default arguments ( http://unity.ubuntu.com/mir/cppguide/index.html?showone=Default_Arguments#Default_Arguments )

suggestion:
The solution isn't bad, but alternatively, we could have the objects implementing mir::DisplayChanger/mf::DisplayChanger trigger a display composition on wakeup. This could avoid messing with the timeouts.

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

Note that we already have support for dropping older frames when a surface becomes visible (restarting a compositor is a special case of this). The frame dropping supported was added by https://code.launchpad.net/~afrantzis/mir/drop-stale-frames/+merge/227961 as a partial fix for the same bug targetted by this MP.

The existing mechanism ensures that we use the latest client frame instead of rapidly showing stale queued frames. This MP is slightly different in that it tries to force the clients to redraw, then waits for some amount of time hoping that they have produced a newer frame. However, for the time intervals we are using ( > 100ms which is larger than our BufferQueues's framedropping policy timeout), I don't think forcing new frames will make much difference, and I am not sure if the complexity is worth it.

As you note, the real problem is the slow greeter rendering.

review: Needs Information

[Alexandros Frantzis (afrantzis)]

> This MP is slightly different in that it

Well not slightly different, rather just "different".

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1842
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2367/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1287
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1293
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1275
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/889
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/889/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/890
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/890/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/211
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/211/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2373
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/11328

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Yeah, I'm undecided about this branch now. The complexity is a bit ugly. I had a simpler version prepared but intentionally didn't propose it thinking more people would prefer this one.

Anyway, now my ABI problems are apparently resolved today I will do some system testing on the phone.

[Alberto Aguirre (albaguirre)]

Yeah I though this was already handled by the update to BufferQueue from Alexandros.

Do we need more than that?

review: Needs Information

[Daniel van Vugt (vanvugt)]

It's a handy feature. And it is still required (after Unity8 gets fixed) to fully resolve bug 1340510, based on my own experiments inserting delays into clients. Not sure why Alexandros' existing code wasn't enough...? But either way, this solution scales better than putting too much intelligence in BufferQueue (which is instantiated for every surface).

So yes, I think we will still need something like this. But it's not needed right now.

[lp:~vanvugt/mir/flush]

1843. By Daniel van Vugt on 2014-08-13

Merge latest development-branch

1844. By Daniel van Vugt on 2014-08-14

Merge latest development-branch

1845. By Daniel van Vugt on 2014-08-15

Merge latest development-branch

1846. By Daniel van Vugt on 2014-08-18

Merge latest development-branch

1847. By Daniel van Vugt on 2014-08-19

Merge latest development-branch

1848. By Daniel van Vugt on 2014-08-22

Merge latest development-branch

1849. By Daniel van Vugt on 2014-08-25

Merge latest development-branch

1850. By Daniel van Vugt on 2014-08-28

Merge latest development-branch

1851. By Daniel van Vugt on 2014-08-28

Merge latest development-branch

1852. By Daniel van Vugt on 2014-08-29

Merge latest development-branch

1853. By Daniel van Vugt on 2014-09-01

Merge latest development-branch

1854. By Daniel van Vugt on 2014-09-02

Merge latest development-branch

1855. By Daniel van Vugt on 2014-09-03

Merge latest development-branch

1856. By Daniel van Vugt on 2014-09-03

Merge latest development-branch

1857. By Daniel van Vugt on 2014-09-04

Merge latest development-branch

1858. By Daniel van Vugt on 2014-09-05

Merge latest development-branch

1859. By Daniel van Vugt on 2014-09-08

Merge latest development-branch

1860. By Daniel van Vugt on 2014-09-09

Merge latest development-branch and fix conflicts

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1860
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2657/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1665
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1671
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1647
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1179
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1179/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/584
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/584/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2704
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/12928

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

review: Approve (continuous-integration)

Unmerged revisions

1860. By Daniel van Vugt on 2014-09-09

Merge latest development-branch and fix conflicts

1859. By Daniel van Vugt on 2014-09-08

Merge latest development-branch

1858. By Daniel van Vugt on 2014-09-05

Merge latest development-branch

1857. By Daniel van Vugt on 2014-09-04

Merge latest development-branch

1856. By Daniel van Vugt on 2014-09-03

Merge latest development-branch

1855. By Daniel van Vugt on 2014-09-03

Merge latest development-branch

1854. By Daniel van Vugt on 2014-09-02

Merge latest development-branch

1853. By Daniel van Vugt on 2014-09-01

Merge latest development-branch

1852. By Daniel van Vugt on 2014-08-29

Merge latest development-branch

1851. By Daniel van Vugt on 2014-08-28

Merge latest development-branch

=== modified file 'src/server/compositor/multi_threaded_compositor.cpp'
--- src/server/compositor/multi_threaded_compositor.cpp	2014-09-06 14:28:26 +0000
+++ src/server/compositor/multi_threaded_compositor.cpp	2014-09-09 03:47:53 +0000
@@ -22,6 +22,7 @@
 #include "mir/compositor/display_buffer_compositor.h"
 #include "mir/compositor/display_buffer_compositor_factory.h"
 #include "mir/compositor/scene.h"
+#include "mir/compositor/scene_element.h"
 #include "mir/compositor/compositor_report.h"
 #include "mir/scene/legacy_scene_change_notification.h"
 #include "mir/scene/surface_observer.h"
@@ -60,6 +61,26 @@
     std::function<void()> const apply;
 };
 
+void drop_frames(mc::Scene& scene, int nframes)
+{
+    // Consume nframes+1. The +1 is required for force the multi-monitor
+    // frame sync logic to release old buffers. It will only do so on the
+    // second and subsequent calls for a given CompositorID (0).
+
+    for (int f = 0; f < nframes + 1; ++f)
+    {
+        auto const& elements = scene.scene_elements_for(0);
+        for (auto& element : elements)
+            (void)element->renderable()->buffer();
+    }
+}
+
+/*
+ * Annoyingly, we can't use milliseconds::max() because that's actually
+ * negative and will trigger immediate wakeups (!?)
+ */
+std::chrono::milliseconds const forever(INT_MAX);
+
 }
 
 namespace mir
@@ -123,8 +144,14 @@
         std::unique_lock<std::mutex> lock{run_mutex};
         while (running)
         {
-            /* Wait until compositing has been scheduled or we are stopped */
-            run_cv.wait(lock, [&]{ return (frames_scheduled > 0) || !running; });
+            while (running && !frames_scheduled)
+            {
+                auto status = run_cv.wait_for(lock, timeout);
+                if (!frames_scheduled && status == std::cv_status::timeout)
+                    frames_scheduled = deferred_frames_scheduled;
+            }
+
+            timeout = forever;
 
             /*
              * Check if we are running before compositing, since we may have
@@ -153,11 +180,20 @@
         mir::terminate_with_current_exception();
     }
 
-    void schedule_compositing(int num_frames)
+    void schedule_compositing(int num_frames,
+                              std::chrono::milliseconds delay =
+                                  std::chrono::milliseconds::zero())
     {
         std::lock_guard<std::mutex> lock{run_mutex};
 
-        if (num_frames > frames_scheduled)
+        if (delay != std::chrono::milliseconds::zero())
+        {
+            timeout = delay;
+            deferred_frames_scheduled = num_frames;
+            frames_scheduled = 0;
+            run_cv.notify_one();
+        }
+        else if (num_frames > frames_scheduled)
         {
             frames_scheduled = num_frames;
             run_cv.notify_one();
@@ -176,6 +212,8 @@
     mg::DisplayBuffer& buffer;
     bool running;
     int frames_scheduled;
+    int deferred_frames_scheduled = 0;
+    std::chrono::milliseconds timeout = forever;
     std::mutex run_mutex;
     std::condition_variable run_cv;
     std::shared_ptr<CompositorReport> const report;
@@ -189,13 +227,15 @@
     std::shared_ptr<mc::Scene> const& scene,
     std::shared_ptr<DisplayBufferCompositorFactory> const& db_compositor_factory,
     std::shared_ptr<CompositorReport> const& compositor_report,
-    bool compose_on_start)
+    bool compose_on_start,
+    std::chrono::milliseconds restart_delay)
     : display{display},
       scene{scene},
       display_buffer_compositor_factory{db_compositor_factory},
       report{compositor_report},
       state{CompositorState::stopped},
       compose_on_start{compose_on_start},
+      restart_delay{restart_delay},
       thread_pool{1}
 {
     observer = std::make_shared<ms::LegacySceneChangeNotification>(
@@ -214,13 +254,14 @@
     stop();
 }
 
-void mc::MultiThreadedCompositor::schedule_compositing(int num)
+void mc::MultiThreadedCompositor::schedule_compositing(int num,
+                                       std::chrono::milliseconds delay)
 {
     std::unique_lock<std::mutex> lk(state_guard);
 
     report->scheduled();
     for (auto& f : thread_functors)
-        f->schedule_compositing(num);
+        f->schedule_compositing(num, delay);
 }
 
 void mc::MultiThreadedCompositor::start()
@@ -249,8 +290,11 @@
     /* Optional first render */
     if (compose_on_start)
     {
+        int const max_buffer_queue_depth = 3;
+        drop_frames(*scene, max_buffer_queue_depth);
+
         lk.unlock();
-        schedule_compositing(1);
+        schedule_compositing(1, restart_delay);
     }
 }
 

=== modified file 'src/server/compositor/multi_threaded_compositor.h'
--- src/server/compositor/multi_threaded_compositor.h	2014-09-06 14:28:26 +0000
+++ src/server/compositor/multi_threaded_compositor.h	2014-09-09 03:47:53 +0000
@@ -26,6 +26,8 @@
 #include <memory>
 #include <vector>
 #include <future>
+#include <thread>
+#include <chrono>
 
 namespace mir
 {
@@ -61,7 +63,9 @@
                             std::shared_ptr<Scene> const& scene,
                             std::shared_ptr<DisplayBufferCompositorFactory> const& db_compositor_factory,
                             std::shared_ptr<CompositorReport> const& compositor_report,
-                            bool compose_on_start);
+                            bool compose_on_start,
+                            std::chrono::milliseconds restart_delay =
+                                std::chrono::milliseconds(500));
     ~MultiThreadedCompositor();
 
     void start();
@@ -82,8 +86,10 @@
     std::mutex state_guard;
     CompositorState state;
     bool compose_on_start;
+    std::chrono::milliseconds const restart_delay;
 
-    void schedule_compositing(int number_composites);
+    void schedule_compositing(int number_composites,
+          std::chrono::milliseconds delay = std::chrono::milliseconds::zero());
 
     std::shared_ptr<mir::scene::Observer> observer;
     mir::thread::BasicThreadPool thread_pool;

=== modified file 'tests/unit-tests/compositor/test_multi_threaded_compositor.cpp'
--- tests/unit-tests/compositor/test_multi_threaded_compositor.cpp	2014-09-05 03:37:19 +0000
+++ tests/unit-tests/compositor/test_multi_threaded_compositor.cpp	2014-09-09 03:47:53 +0000
@@ -608,6 +608,120 @@
     compositor.stop();
 }
 
+TEST(MultiThreadedCompositor, restart_flushes_the_scene)
+{
+    using namespace testing;
+
+    int const ndisplays = 1;
+    int const buffer_queue_depth = 3;
+    auto display = std::make_shared<StubDisplayWithMockBuffers>(ndisplays);
+    auto mock_scene = std::make_shared<mtd::MockScene>();
+    auto factory = std::make_shared<NullDisplayBufferCompositorFactory>();
+    auto mock_report =
+        std::make_shared<testing::NiceMock<mtd::MockCompositorReport>>();
+
+    int const nstarts = 2;
+    int const nstops = nstarts;
+    EXPECT_CALL(*mock_report, started())
+        .Times(nstarts);
+    EXPECT_CALL(*mock_report, stopped())
+        .Times(nstops);
+    EXPECT_CALL(*mock_scene, add_observer(_))
+        .Times(nstarts);
+    EXPECT_CALL(*mock_scene, remove_observer(_))
+        .Times(nstarts);
+
+    EXPECT_CALL(*mock_scene, scene_elements_for(_))
+        .Times(nstarts * (buffer_queue_depth + 1))
+        .WillRepeatedly(Return(mc::SceneElementSequence{}));
+
+    mc::MultiThreadedCompositor compositor{display, mock_scene, factory,
+                                           mock_report, true};
+
+    compositor.start();
+    compositor.stop();
+    compositor.start();
+    compositor.stop();
+}
+
+TEST(MultiThreadedCompositor, resume_on_idle_scene_is_delayed)
+{
+    using namespace testing;
+
+    unsigned int const nbuffers = 3;
+
+    auto display = std::make_shared<StubDisplay>(nbuffers);
+    auto scene = std::make_shared<StubScene>();
+    auto factory = std::make_shared<RecordingDisplayBufferCompositorFactory>();
+
+    std::chrono::milliseconds const delay(500);
+    mc::MultiThreadedCompositor compositor{display, scene, factory,
+                                           null_report, true, delay};
+
+    EXPECT_TRUE(factory->check_record_count_for_each_buffer(nbuffers, 0, 0));
+
+    compositor.start();
+
+    scene->emit_change_event();
+    while (!factory->check_record_count_for_each_buffer(nbuffers, 1))
+        std::this_thread::sleep_for(std::chrono::milliseconds(10));
+    EXPECT_TRUE(factory->check_record_count_for_each_buffer(nbuffers, 1, 1));
+
+    compositor.stop();
+
+    using namespace std::chrono;
+    auto from = system_clock::now();
+
+    compositor.start();
+    while (!factory->check_record_count_for_each_buffer(nbuffers, 2))
+        std::this_thread::sleep_for(std::chrono::milliseconds(10));
+    auto to = system_clock::now();
+    auto measured_delay = duration_cast<milliseconds>(to - from);
+    EXPECT_THAT(measured_delay, Ge(delay));
+
+    compositor.stop();
+}
+
+TEST(MultiThreadedCompositor, resume_on_busy_scene_is_not_delayed)
+{
+    using namespace testing;
+
+    unsigned int const nbuffers = 3;
+
+    auto display = std::make_shared<StubDisplay>(nbuffers);
+    auto scene = std::make_shared<StubScene>();
+    auto factory = std::make_shared<RecordingDisplayBufferCompositorFactory>();
+    std::chrono::milliseconds const long_delay(10000);
+    std::chrono::milliseconds const max_frame_time(1000); // Valgrind :)
+    ASSERT_LT(max_frame_time, long_delay);
+    mc::MultiThreadedCompositor compositor{display, scene, factory,
+                                           null_report, true, long_delay};
+
+    EXPECT_TRUE(factory->check_record_count_for_each_buffer(nbuffers, 0, 0));
+
+    compositor.start();
+
+    scene->emit_change_event();
+    while (!factory->check_record_count_for_each_buffer(nbuffers, 1))
+        std::this_thread::sleep_for(std::chrono::milliseconds(10));
+    EXPECT_TRUE(factory->check_record_count_for_each_buffer(nbuffers, 1, 1));
+
+    compositor.stop();
+
+    using namespace std::chrono;
+    auto from = system_clock::now();
+
+    compositor.start();
+    scene->emit_change_event();
+    while (!factory->check_record_count_for_each_buffer(nbuffers, 2))
+        std::this_thread::sleep_for(std::chrono::milliseconds(10));
+    auto to = system_clock::now();
+    auto measured_delay = duration_cast<milliseconds>(to - from);
+    EXPECT_THAT(measured_delay, Lt(max_frame_time));
+
+    compositor.stop();
+}
+
 TEST(MultiThreadedCompositor, cleans_up_after_throw_in_start)
 {
     unsigned int const nbuffers{3};