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

[Daniel van Vugt]

[Daniel van Vugt (vanvugt)]

There's an intermittent failure in MirSurfaceSwapBuffersTest.swap_buffers_does_not_block_when_surface_is_not_composited due to stub class changes in r1567. That needs fixing.

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

This reintroduces eglSwapBuffers() blocking for buffers that are not rendered by the real compositors (i.e. for surfaces that are either occluded or off screen). That is, it only keeps eglSwapBuffers() running when all monitors are off. This can't be a long term solution if we want eglSwapBuffers() to be non-blocking in general, although perhaps it can serve as a workaround for our Qt 5.2 issues.

For a proper solution we need a more fine-grained approach, making decisions per renderable. Chris' approach https://code.launchpad.net/~raof/mir/1hz-rendering-always/+merge/216246 does this an the SwitchingBundle level, but we can achieve the same results at the MultiThreadedCompositor level. I will work on a proposal for that and we can decided on the best approach.

review: Needs Information

[Alexandros Frantzis (afrantzis)]

> Chris' approach https://code.launchpad.net/~raof/mir/1hz-rendering-always/+merge/216246
> does this an the SwitchingBundle level, but we can achieve the same results at the
> MultiThreadedCompositor level

You can find it here: https://code.launchpad.net/~afrantzis/mir/consume-only-not-rendered-buffers/+merge/216725

[Daniel van Vugt (vanvugt)]

Alexandros:

I know. Hence https://code.launchpad.net/~vanvugt/mir/unocclude/+merge/216690

I think these two proposals are both safer and more elegant than the 1Hz branch.

[Chris Halse Rogers (raof)]

I disagree that adding a fake display is more elegant than dropping frames after a timeout; the former is a hack, the latter is a direct implementation of the conceptual behaviour we desire.

I suspect that the 1Hz branch will be safer over time as code changes; it implements the client behaviour we care about in the code that implements client behaviour, rather than as a side-effect of the default compositor.

Hm. How does this interact with the Qt scenegraph thingy? Isn't that replacing the compositor?

That said, it might well be that this is safer than the 1Hz branch *right now*.

[Daniel van Vugt (vanvugt)]

review: Abstain

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1572
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1371/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1665
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1665/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1239
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1103/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1108
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1108/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1241
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1241/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1145
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/6251

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1574
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1379/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1675
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1673
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1248
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1111
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1111/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1116
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1116/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1250
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1250/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1154
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/6299

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

While this seems to have the desired effect it isn't an elegant solution. (I don't have an elegant solution [yet], but there are a lot of MPs around this area pulling in different directions. Maybe there's a nice solution trying to get out.)

review: Abstain

[Alan Griffiths (alan-griffiths)]

After today's standup the consensus is that when SwitchingBundle cannot supply a buffer (before a timeout) we should to be explicitly dropping frames to provide one and not faking it by consuming the frames in the compositor. In broad terms that's Chris's approach - but we need to review the detail.

So, while I think the tests included here are valid, the changes are not the way we want to go forward.

review: Disapprove

[lp:~vanvugt/mir/judder]

1586. By Daniel van Vugt on 2014-05-29

Merge latest development-branch

1587. By Daniel van Vugt on 2014-05-30

Merge latest development-branch

Unmerged revisions

1587. By Daniel van Vugt on 2014-05-30

Merge latest development-branch

1586. By Daniel van Vugt on 2014-05-29

Merge latest development-branch

1585. By Daniel van Vugt on 2014-05-28

Adjust sleep to yield 10Hz client wakeups

1584. By Daniel van Vugt on 2014-05-28

Tidy up

1583. By Daniel van Vugt on 2014-05-28

Fix missing brace, FTBFS

1582. By Daniel van Vugt on 2014-05-28

Merge latest development-branch

1581. By Daniel van Vugt on 2014-05-27

Clean up

1580. By Daniel van Vugt on 2014-05-27

Remember to set the continuation flag correctly... even on frames when
we're not consuming frames.

1579. By Daniel van Vugt on 2014-05-27

Remember to keep fake compositing as required

1578. By Daniel van Vugt on 2014-05-27

Remove more tabs

=== modified file 'src/server/compositor/multi_threaded_compositor.cpp'
--- src/server/compositor/multi_threaded_compositor.cpp	2014-05-28 07:43:51 +0000
+++ src/server/compositor/multi_threaded_compositor.cpp	2014-05-28 16:49:25 +0000
@@ -32,6 +32,7 @@
 #include <condition_variable>
 #include <cstdint>
 #include <chrono>
+#include <atomic>
 
 namespace mc = mir::compositor;
 namespace mg = mir::graphics;
@@ -86,6 +87,8 @@
     mg::DisplayBuffer& buffer;
 };
 
+std::atomic<int> real_frames(0);
+
 class CompositingFunctor
 {
 public:
@@ -180,7 +183,12 @@
                               r.top_left.x.as_int(), r.top_left.y.as_int(),
                               report_id);
 
-        run_compositing_loop([&] { return display_buffer_compositor->composite();});
+        run_compositing_loop([&]
+        {
+            bool ret = display_buffer_compositor->composite();
+            real_frames.fetch_add(1);
+            return ret;
+        });
     }
     catch (...)
     {
@@ -207,19 +215,31 @@
         run_compositing_loop(
             [this]
             {
-                std::vector<std::shared_ptr<mg::Buffer>> saved_resources;
-
+                int old_real_frames = real_frames;
+
+                // Emulate a moderately slow fake display.
+                // Note that due to feedback from consuming a buffer,
+                // this will trigger alternation between consuming a real
+                // and fake frame. So sleeping 50ms here actually results
+                // in a client frame rate of 10Hz.
+
+                std::this_thread::sleep_for(std::chrono::milliseconds(50));
+                bool consumed = real_frames != old_real_frames;
+
+                int more = 0;
                 auto const& renderables = scene->renderable_list_for(this);
-
                 for (auto const& r : renderables)
                 {
-                    if (r->buffers_ready_for_compositor() > 0)
-                        saved_resources.push_back(r->buffer());
+                    int ready = r->buffers_ready_for_compositor();
+                    more += ready;
+
+                    // Now only composite a fake frame if no real display has
+                    // composited during the sleep.
+                    if (ready && !consumed)
+                        (void)r->buffer();
                 }
 
-                wait_until_next_fake_vsync();
-
-                return false;
+                return more > 0;
             });
     }
     catch (...)
@@ -228,23 +248,6 @@
     }
 
 private:
-    void wait_until_next_fake_vsync()
-    {
-        using namespace std::chrono;
-        typedef duration<int64_t, std::ratio<1, 60>> vsync_periods;
-
-        /* Truncate to vsync periods */
-        auto const previous_vsync =
-            duration_cast<vsync_periods>(steady_clock::now().time_since_epoch());
-        /* Convert back to a timepoint */
-        auto const previous_vsync_tp =
-            time_point<steady_clock, vsync_periods>{previous_vsync};
-        /* Next vsync time point */
-        auto const next_vsync = previous_vsync_tp + vsync_periods(1);
-
-        std::this_thread::sleep_until(next_vsync);
-    }
-
     std::shared_ptr<mc::Scene> const scene;
 };
 

=== modified file 'tests/acceptance-tests/test_client_surface_swap_buffers.cpp'
--- tests/acceptance-tests/test_client_surface_swap_buffers.cpp	2014-05-22 09:56:17 +0000
+++ tests/acceptance-tests/test_client_surface_swap_buffers.cpp	2014-05-28 16:49:25 +0000
@@ -21,7 +21,7 @@
 #include "mir_test_framework/stubbed_server_configuration.h"
 #include "mir_test_framework/in_process_server.h"
 #include "mir_test_framework/using_stub_client_platform.h"
-#include "mir_test_doubles/null_display_buffer_compositor_factory.h"
+#include "mir_test_doubles/null_display.h"
 #include "mir_test/spin_wait.h"
 
 #include <gtest/gtest.h>
@@ -37,10 +37,9 @@
 
 struct StubServerConfig : mtf::StubbedServerConfiguration
 {
-    auto the_display_buffer_compositor_factory()
-        -> std::shared_ptr<mc::DisplayBufferCompositorFactory> override
+    std::shared_ptr<mir::graphics::Display> the_display()
     {
-        return std::make_shared<mtd::NullDisplayBufferCompositorFactory>();
+        return std::make_shared<mtd::NullDisplay>();
     }
 };
 

=== modified file 'tests/unit-tests/compositor/test_multi_threaded_compositor.cpp'
--- tests/unit-tests/compositor/test_multi_threaded_compositor.cpp	2014-05-28 07:43:51 +0000
+++ tests/unit-tests/compositor/test_multi_threaded_compositor.cpp	2014-05-28 16:49:25 +0000
@@ -150,6 +150,55 @@
     bool throw_on_add_observer_;
 };
 
+class StubDisplayBufferCompositor : public mc::DisplayBufferCompositor
+{
+public:
+    StubDisplayBufferCompositor(std::shared_ptr<mc::Scene> scene,
+                                std::chrono::milliseconds frame_time)
+        : scene{scene}, frame_time{frame_time}
+    {
+    }
+
+    bool composite() override
+    {
+        auto const& all = scene->renderable_list_for(this);
+        for (auto const& r : all)
+            r->buffer();  // Consume a frame
+
+        std::this_thread::sleep_for(frame_time);
+        return false;
+    }
+
+private:
+    std::shared_ptr<mc::Scene> const scene;
+    std::chrono::milliseconds const frame_time;
+};
+
+
+class StubDisplayBufferCompositorFactory :
+    public mc::DisplayBufferCompositorFactory
+{
+public:
+    StubDisplayBufferCompositorFactory(std::shared_ptr<mc::Scene> scene,
+                                       int hz)
+        : scene{scene},
+          frame_time{std::chrono::milliseconds(1000 / hz)}
+    {
+    }
+
+    std::unique_ptr<mc::DisplayBufferCompositor>
+        create_compositor_for(mg::DisplayBuffer&) override
+    {
+        std::unique_ptr<mc::DisplayBufferCompositor> ret(
+            new StubDisplayBufferCompositor(scene, frame_time));
+        return ret;
+    }
+
+private:
+    std::shared_ptr<mc::Scene> const scene;
+    std::chrono::milliseconds const frame_time;
+};
+
 class RecordingDisplayBufferCompositor : public mc::DisplayBufferCompositor
 {
 public:
@@ -323,7 +372,7 @@
 class BufferCountingRenderable : public mtd::StubRenderable
 {
 public:
-    BufferCountingRenderable(RenderableVisibility visibility)
+    BufferCountingRenderable(RenderableVisibility visibility = RenderableVisibility::visible)
         : buffers_requested_{0}, visibility{visibility}
     {
     }
@@ -627,12 +676,12 @@
 {
     using namespace testing;
 
-    unsigned int const nbuffers{2};
     auto renderable = std::make_shared<BufferCountingRenderable>(GetParam());
-    auto display = std::make_shared<StubDisplay>(nbuffers);
+
+    // Defining zero outputs is the simplest way to emulate what would
+    // happen if all your displays were blocked in swapping...
+    auto display = std::make_shared<StubDisplay>(0);
     auto stub_scene = std::make_shared<StubScene>(mg::RenderableList{renderable});
-    // We use NullDisplayBufferCompositors to simulate DisplayBufferCompositors
-    // not rendering a renderable.
     auto db_compositor_factory = std::make_shared<mtd::NullDisplayBufferCompositorFactory>();
 
     mc::MultiThreadedCompositor compositor{
@@ -657,6 +706,86 @@
     compositor.stop();
 }
 
+TEST(MultiThreadedCompositor, never_steals_frames_from_real_displays)
+{
+    /*
+     * Verify dummy frames are consumed slower than any physical display would
+     * consume them, so that the two types of consumers don't race and don't
+     * visibly skip (LP: #1308843)
+     */
+    using namespace testing;
+
+    unsigned int const nbuffers{2};
+    auto renderable = std::make_shared<BufferCountingRenderable>();
+    auto display = std::make_shared<StubDisplay>(nbuffers);
+    auto stub_scene = std::make_shared<StubScene>(mg::RenderableList{renderable});
+    // We use NullDisplayBufferCompositors to simulate DisplayBufferCompositors
+    // not rendering a renderable.
+    auto db_compositor_factory = std::make_shared<mtd::NullDisplayBufferCompositorFactory>();
+
+    mc::MultiThreadedCompositor compositor{
+        display, stub_scene, db_compositor_factory, null_report, true};
+
+    compositor.start();
+
+    // Realistically I've only ever seen LCDs go as low as 40Hz. But some TV
+    // outputs will go as low as 24Hz (traditional movie frame rate). So we
+    // need to ensure the dummy consumer is slower than that...
+    int const min_real_refresh_rate = 20;
+
+    int const secs = 5;
+    auto const duration = std::chrono::seconds{secs};
+    mir::test::spin_wait_for_condition_or_timeout(
+        [&] { return renderable->buffers_requested() <= 1; },
+        duration);
+
+    auto const end = std::chrono::steady_clock::now() + duration;
+    while (std::chrono::steady_clock::now() < end)
+    {
+        stub_scene->emit_change_event();
+        std::this_thread::yield();
+    }
+
+    int const max_dummy_frames = min_real_refresh_rate * secs;
+    ASSERT_GT(max_dummy_frames, renderable->buffers_requested());
+
+    compositor.stop();
+}
+
+TEST(MultiThreadedCompositor, only_real_displays_limit_consumption_rate)
+{
+    using namespace testing;
+
+    auto display = std::make_shared<StubDisplay>(1);
+    auto renderable = std::make_shared<BufferCountingRenderable>();
+    auto stub_scene = std::make_shared<StubScene>(mg::RenderableList{renderable});
+
+    int const framerate = 75;   // Simulate a nice fast frame rate
+    auto db_compositor_factory =
+        std::make_shared<StubDisplayBufferCompositorFactory>(stub_scene,
+                                                             framerate);
+
+    mc::MultiThreadedCompositor compositor{
+        display, stub_scene, db_compositor_factory, null_report, false};
+
+    compositor.start();
+
+    int const secs = 5;
+    auto const duration = std::chrono::seconds{secs};
+    auto const end = std::chrono::steady_clock::now() + duration;
+    while (std::chrono::steady_clock::now() < end)
+    {
+        stub_scene->emit_change_event();
+        std::this_thread::yield();
+    }
+
+    int const expected_frames = framerate * secs;
+    ASSERT_GT(expected_frames * 1.2f, renderable->buffers_requested());
+    ASSERT_LT(expected_frames * 0.8f, renderable->buffers_requested());
+
+    compositor.stop();
+}
+
 INSTANTIATE_TEST_CASE_P(
     MultiThreadedCompositor, BufferConsumption,
     ::testing::Values(RenderableVisibility::hidden, RenderableVisibility::visible));