Merge lp:~alan-griffiths/mir/only-visible-surfaces-change-scene into lp:mir

[Alan Griffiths]

scene: surfaces that are not (and were not recently) visible shouldn't trigger compositing

scene: surfaces that are not (and were not recently) visible shouldn't trigger compositing

This is a significant part of the resolution to the discussion on the mir-devel mailing list "When should a nested server first post a buffer?"

We still need a test case for the nested server behaviour but this fixes the root cause (spurious "damage" triggering compositing).

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2606
http://jenkins.qa.ubuntu.com/job/mir-ci/3961/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2649
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/160
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2597
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/117
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/117/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2597
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2597/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5447
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20874

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

This proposal makes sense for improved efficiency at least, but I'm not sure if it improves correctness at all...

Theoretically the compositor should be treated as something that may sample the scene at any time, at any frequency. So briefly drawing a frame when there's not really any new changes to show shouldn't be an error. Unless the shell is broken and fails to draw itself on start-up when there are no active clients(?).

Feels like we're missing the mark on addressing the root cause. But also feels like a worthy change to have...

review: Needs Information

[Alan Griffiths (alan-griffiths)]

> This proposal makes sense for improved efficiency at least, but I'm not sure
> if it improves correctness at all...
>
> Theoretically the compositor should be treated as something that may sample
> the scene at any time, at any frequency. So briefly drawing a frame when
> there's not really any new changes to show shouldn't be an error. Unless the
> shell is broken and fails to draw itself on start-up when there are no active
> clients(?).

Sure the compositor is permitted to sample the scene "when it wants". But it needs to decide how to respond to scene changes. The LegacySceneChangeNotification class is part of this decision mechanism. (It is called "Legacy" because the mechanism is knowingly unsophisticated - this MP makes it a little more sophisticated.)

> Feels like we're missing the mark on addressing the root cause. But also feels
> like a worthy change to have...

The "root cause" depends on what you think the problem is.

From the POV of USC the problem is that U8 posts a frame before it has drawn anything.

The reason U8 posts before it has drawn anything is that nested Mir posts a frame when the surface is created, not when it is drawn.

And that is fixed as a side-effect of the optimization proposed here.

[Alexandros Frantzis (afrantzis)]

Looks good.

review: Approve

[Daniel van Vugt (vanvugt)]

Sounds OK, I think...

In the "already running" case you have to consider that there's possibly another client running that could cause you to get composited on-screen slightly sooner than you expect (or are ready for). Like in bug 1447896.

In Compiz I solved this by giving 1ms of grace time to the scene change notification so that temporally near-by changes in other windows make it into the same frame. Although the latency penalty is sub-optimal then, and my predictive-bypass branch will solve the problem completely.

Even on start-up though, if your shell is comprised of multiple surfaces there's no guarantee that the first visible frame will contain all of them. Because they race each other. But our logic does ensure in that case at least a second frame will replace it soon enough.

So we're potentially re-introducing the problem of having an incomplete shell visible on the first frame. But that's non-trivial to fix without the "wait for second frame" hack. Surfaces are asynchronous with each other and that's a very good thing, most of the time.

review: Approve

=== modified file 'src/server/scene/legacy_scene_change_notification.cpp'
--- src/server/scene/legacy_scene_change_notification.cpp	2015-05-12 06:58:52 +0000
+++ src/server/scene/legacy_scene_change_notification.cpp	2015-06-01 16:35:23 +0000
@@ -41,8 +41,15 @@
 
 void ms::LegacySceneChangeNotification::add_surface_observer(ms::Surface* surface)
 {
+    auto notifier = [surface, this, was_visible = false] () mutable
+        {
+            if (surface->visible() || was_visible)
+                scene_notify_change();
+            was_visible = surface->visible();
+        };
+
     auto observer = std::make_shared<ms::LegacySurfaceChangeNotification>(
-        scene_notify_change, buffer_notify_change);
+        notifier, buffer_notify_change);
     surface->add_observer(observer);
     
     {
@@ -54,7 +61,6 @@
 void ms::LegacySceneChangeNotification::surface_added(ms::Surface* surface)
 {
     add_surface_observer(surface);
-    scene_notify_change();
 }
 
 void ms::LegacySceneChangeNotification::surface_exists(ms::Surface* surface)
@@ -73,7 +79,9 @@
             surface_observers.erase(it);
         }
     }
-    scene_notify_change();
+
+    if (surface->visible())
+        scene_notify_change();
 }
 
 void ms::LegacySceneChangeNotification::surfaces_reordered()

=== modified file 'tests/integration-tests/test_surface_stack_with_compositor.cpp'
--- tests/integration-tests/test_surface_stack_with_compositor.cpp	2015-05-19 21:34:34 +0000
+++ tests/integration-tests/test_surface_stack_with_compositor.cpp	2015-06-01 16:35:23 +0000
@@ -183,7 +183,7 @@
     EXPECT_TRUE(stub_secondary_db.has_posted_at_least(0, timeout));
 }
 
-TEST_F(SurfaceStackCompositor, adding_a_surface_that_has_been_swapped_triggers_a_composition)
+TEST_F(SurfaceStackCompositor, swapping_a_surface_that_has_been_added_triggers_a_composition)
 {
     mc::MultiThreadedCompositor mt_compositor(
         mt::fake_shared(stub_display),
@@ -193,8 +193,8 @@
         null_comp_report, false);
     mt_compositor.start();
 
+    stack.add_surface(stub_surface, default_params.depth, default_params.input_mode);
     stub_surface->primary_buffer_stream()->swap_buffers(&stubbuf, [](mg::Buffer*){});
-    stack.add_surface(stub_surface, default_params.depth, default_params.input_mode);
 
     EXPECT_TRUE(stub_primary_db.has_posted_at_least(1, timeout));
     EXPECT_TRUE(stub_secondary_db.has_posted_at_least(1, timeout));

=== modified file 'tests/unit-tests/scene/test_basic_surface.cpp'
--- tests/unit-tests/scene/test_basic_surface.cpp	2015-05-19 21:34:34 +0000
+++ tests/unit-tests/scene/test_basic_surface.cpp	2015-06-01 16:35:23 +0000
@@ -171,6 +171,7 @@
         .Times(1);
 
     surface.add_observer(observer);
+    post_a_frame(surface);
 
     EXPECT_EQ(rect.top_left, surface.top_left());
 
@@ -187,6 +188,7 @@
         .Times(1);
 
     surface.add_observer(observer);
+    post_a_frame(surface);
 
     EXPECT_EQ(rect.size, surface.size());
     EXPECT_NE(new_size, surface.size());
@@ -223,6 +225,7 @@
         .Times(1);
 
     surface.add_observer(observer);
+    post_a_frame(surface);
 
     auto original_transformation = surface.compositor_snapshot(compositor_id)->transformation();
     glm::mat4 trans{0.1f, 0.5f, 0.9f, 1.3f,
@@ -243,6 +246,7 @@
         .Times(1);
 
     surface.add_observer(observer);
+    post_a_frame(surface);
 
     float alpha = 0.5f;
     surface.set_alpha(0.5f);
@@ -301,6 +305,7 @@
         .Times(1);
 
     surface.add_observer(observer);
+    post_a_frame(surface);
 
     surface.set_hidden(true);
 }

=== modified file 'tests/unit-tests/scene/test_legacy_scene_change_notification.cpp'
--- tests/unit-tests/scene/test_legacy_scene_change_notification.cpp	2015-04-09 08:57:24 +0000
+++ tests/unit-tests/scene/test_legacy_scene_change_notification.cpp	2015-06-01 16:35:23 +0000
@@ -42,6 +42,10 @@
 
 struct LegacySceneChangeNotificationTest : public testing::Test
 {
+    void SetUp() override
+    {
+        ON_CALL(surface,visible()).WillByDefault(testing::Return(true));
+    }
     testing::NiceMock<MockSceneCallback> scene_callback;
     testing::NiceMock<MockBufferCallback> buffer_callback;
     std::function<void(int)> buffer_change_callback{[this](int arg){buffer_callback.invoke(arg);}};
@@ -52,8 +56,7 @@
 
 TEST_F(LegacySceneChangeNotificationTest, fowards_all_observations_to_callback)
 {
-    EXPECT_CALL(scene_callback, invoke())
-        .Times(3);
+    EXPECT_CALL(scene_callback, invoke()).Times(2);
 
     ms::LegacySceneChangeNotification observer(scene_change_callback, buffer_change_callback);
     observer.surface_added(&surface);
@@ -87,10 +90,8 @@
         .WillOnce(SaveArg<0>(&surface_observer));
    
     int buffer_num{3}; 
-    EXPECT_CALL(scene_callback, invoke())
-        .Times(1);
-    EXPECT_CALL(buffer_callback, invoke(buffer_num))
-        .Times(1);
+    EXPECT_CALL(scene_callback, invoke()).Times(0);
+    EXPECT_CALL(buffer_callback, invoke(buffer_num)).Times(1);
 
     ms::LegacySceneChangeNotification observer(scene_change_callback, buffer_change_callback);
     observer.surface_added(&surface);
@@ -105,8 +106,7 @@
 
     EXPECT_CALL(surface, add_observer(_)).Times(1)
         .WillOnce(SaveArg<0>(&surface_observer));
-    EXPECT_CALL(scene_callback, invoke())
-        .Times(2);
+    EXPECT_CALL(scene_callback, invoke()).Times(1);
 
     ms::LegacySceneChangeNotification observer(scene_change_callback,
                                                buffer_change_callback);