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

[Alan Griffiths]

compositor, scene: track which outputs are "damaged" by buffer posts

compositor, scene: track which outputs are "damaged" by buffer posts

This addresses the scenario in the linked bug, but there are still issues when using the AdorningDisplayBufferCompositor from the examples and opportunities to optimize the display updates in the (android) system compositor.

[Mir CI Bot (mir-ci-bot)]

PASSED: Continuous integration, rev:3251
https://mir-jenkins.ubuntu.com/job/mir-ci/87/
Executed test runs:
    None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/87/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/87/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3251
http://jenkins.qa.ubuntu.com/job/mir-ci/6059/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5590
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4497
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5546
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/300
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/383
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/383/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/383
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/383/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5543
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5543/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/8002
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26789
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/296
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/296/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-xenial-touch/152
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26801

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Sounds like a handy optimization to have. Although I can't help but think we're missing the main issue. The main issue is probably related to the fact that you mentioned the system was mostly idle while we were failing to meet frame deadlines. So it's not necessarily a hard performance limit we hit as much as inefficient blocking/waiting somewhere.

[Alan Griffiths (alan-griffiths)]

> Sounds like a handy optimization to have. Although I can't help but think
> we're missing the main issue. The main issue is probably related to the fact
> that you mentioned the system was mostly idle while we were failing to meet
> frame deadlines. So it's not necessarily a hard performance limit we hit as
> much as inefficient blocking/waiting somewhere.

Yes, there's an iceberg of issues here. We should return to these and I've logged lp:1535894 to cover that.

[Daniel van Vugt (vanvugt)]

Wasn't the issue Unity8? I recall QtMir is full of TODOs for multimonitor support, particularly around correct usage of compositor acquire IDs.

[Daniel van Vugt (vanvugt)]

Aside from the above comment, my reason for not liking regional damage is based on previous experience in Compiz and even in mir_proving_server. Accurately tracking damage when you might be painting things using free-form OpenGL (not bound by any surface extents) like shadows is just too much of a pain to maintain. And on low-end systems can become a CPU bottleneck.

Put another way, given two monitors A and B, your window may only exist on A, but you really have to redraw both A and B in case shadows or some other side-effect creeps across.

review: Abstain

[Alan Griffiths (alan-griffiths)]

Yes, QtMir and Unity8 also have work to do. But this issue was explicitly about Mir - and we ought to get this working as well as HWC allows.

[Alexandros Frantzis (afrantzis)]

Looks good.

No tests?

review: Approve

[Daniel van Vugt (vanvugt)]

Another bigger concern is that this branch seems like a crutch. It lets us only just maintain a good frame rate so long as we only need to redraw one monitor. It is however an admission that we're not capable of redrawing multiple monitors per frame, which sounds like a problem that still needs fixing.

[Alan Griffiths (alan-griffiths)]

Not doing unnecessary rendering isn't "a crutch", especially on a battery powered device.

I don't consider our frame rate problems at an end, but I think this is an increment in the right direction.

[Daniel van Vugt (vanvugt)]

It is a crutch if you've established the system is mostly idle and therefore we're not trying very hard to render sufficiently fast. We've had and still do have other such performance problems where it's a matter of real-time getting wasted in blocking system calls, but no CPU or GPU time getting wasted. Bug 1395421 comes to mind.

Adding a little complexity like this to improve performance seems harmless but is almost certainly missing the main issue.

[Brandon Schaefer (brandontschaefer)]

lgtm

Not 100% sure what:
run_cv.notify_one();

Is actually *running* but does it block on this thread? (Since we block at the top of that function).

review: Approve

[Alan Griffiths (alan-griffiths)]

> lgtm
>
> Not 100% sure what:
> run_cv.notify_one();
>
> Is actually *running* but does it block on this thread? (Since we block at the
> top of that function).

It is an implementation detail of compositor logic: the "run()" function blocks on run_cv awaiting a notification that there's work to do.

[Kevin DuBois (kdub)]

lgtm too, apart from having some MultiThreadedCompositor tests

review: Approve

[Gerry Boland (gerboland)]

Late test, but improves MM performance on N7

review: Approve

=== modified file 'src/include/server/mir/scene/legacy_scene_change_notification.h'
--- src/include/server/mir/scene/legacy_scene_change_notification.h	2015-02-22 07:46:25 +0000
+++ src/include/server/mir/scene/legacy_scene_change_notification.h	2016-01-19 17:44:03 +0000
@@ -27,6 +27,7 @@
 
 namespace mir
 {
+namespace geometry { class Rectangle; }
 namespace scene
 {
 class SurfaceObserver;
@@ -40,6 +41,11 @@
     LegacySceneChangeNotification(
         std::function<void()> const& scene_notify_change,
         std::function<void(int)> const& buffer_notify_change);
+
+    LegacySceneChangeNotification(
+        std::function<void()> const& scene_notify_change,
+        std::function<void(int frames, mir::geometry::Rectangle const& damage)> const& damage_notify_change);
+
     ~LegacySceneChangeNotification();
 
     void surface_added(Surface* surface) override;
@@ -54,7 +60,8 @@
 private:
     std::function<void()> const scene_notify_change;
     std::function<void(int)> const buffer_notify_change;
-    
+    std::function<void(int frames, mir::geometry::Rectangle const& damage)> const damage_notify_change;
+
     std::mutex surface_observers_guard;
     std::map<Surface*, std::weak_ptr<SurfaceObserver>> surface_observers;
     

=== modified file 'src/server/compositor/multi_threaded_compositor.cpp'
--- src/server/compositor/multi_threaded_compositor.cpp	2015-12-14 15:38:23 +0000
+++ src/server/compositor/multi_threaded_compositor.cpp	2016-01-19 17:44:03 +0000
@@ -130,6 +130,7 @@
                  * to ensure all surfaces' queues are fully drained.
                  */
                 frames_scheduled--;
+                not_posted_yet = false;
                 lock.unlock();
 
                 for (auto& tuple : compositors)
@@ -198,6 +199,21 @@
         }
     }
 
+    void schedule_compositing(int num_frames, geometry::Rectangle const& damage)
+    {
+        std::lock_guard<std::mutex> lock{run_mutex};
+        bool took_damage = not_posted_yet;
+
+        group.for_each_display_buffer([&](mg::DisplayBuffer& buffer)
+            { if (damage.overlaps(buffer.view_area())) took_damage = true; });
+
+        if (took_damage && num_frames > frames_scheduled)
+        {
+            frames_scheduled = num_frames;
+            run_cv.notify_one();
+        }
+    }
+
     void stop()
     {
         std::lock_guard<std::mutex> lock{run_mutex};
@@ -224,6 +240,7 @@
     std::shared_ptr<CompositorReport> const report;
     std::promise<void> started;
     std::future<void> started_future;
+    bool not_posted_yet = true;
 };
 
 }
@@ -252,9 +269,9 @@
     {
         schedule_compositing(1);
     },
-    [this](int num)
+    [this](int num, geometry::Rectangle const& damage)
     {
-        schedule_compositing(num);
+        schedule_compositing(num, damage);
     });
 }
 
@@ -270,6 +287,13 @@
         f->schedule_compositing(num);
 }
 
+void mc::MultiThreadedCompositor::schedule_compositing(int num, geometry::Rectangle const& damage) const
+{
+    report->scheduled();
+    for (auto& f : thread_functors)
+        f->schedule_compositing(num, damage);
+}
+
 void mc::MultiThreadedCompositor::start()
 {
     auto stopped = CompositorState::stopped;

=== modified file 'src/server/compositor/multi_threaded_compositor.h'
--- src/server/compositor/multi_threaded_compositor.h	2015-07-16 07:03:19 +0000
+++ src/server/compositor/multi_threaded_compositor.h	2016-01-19 17:44:03 +0000
@@ -31,6 +31,7 @@
 
 namespace mir
 {
+namespace geometry { class Rectangle; }
 namespace graphics
 {
 class Display;
@@ -91,6 +92,7 @@
     bool compose_on_start;
 
     void schedule_compositing(int number_composites);
+    void schedule_compositing(int number_composites, geometry::Rectangle const& damage) const;
 
     std::shared_ptr<mir::scene::Observer> observer;
     mir::thread::BasicThreadPool thread_pool;

=== modified file 'src/server/scene/legacy_scene_change_notification.cpp'
--- src/server/scene/legacy_scene_change_notification.cpp	2015-09-18 15:37:25 +0000
+++ src/server/scene/legacy_scene_change_notification.cpp	2016-01-19 17:44:03 +0000
@@ -34,11 +34,60 @@
 {
 }
 
+ms::LegacySceneChangeNotification::LegacySceneChangeNotification(
+    std::function<void()> const& scene_notify_change,
+    std::function<void(int frames, mir::geometry::Rectangle const& damage)> const& damage_notify_change) :
+    scene_notify_change(scene_notify_change),
+    damage_notify_change(damage_notify_change)
+{
+}
+
 ms::LegacySceneChangeNotification::~LegacySceneChangeNotification()
 {
     end_observation();
 }
 
+namespace
+{
+class NonLegacySurfaceChangeNotification : public ms::LegacySurfaceChangeNotification
+{
+public:
+    NonLegacySurfaceChangeNotification(
+        std::function<void()> const& notify_scene_change,
+        std::function<void(int frames, mir::geometry::Rectangle const& damage)> const& damage_notify_change,
+        ms::Surface* surface);
+
+    void moved_to(mir::geometry::Point const& /*top_left*/) override;
+    void frame_posted(int frames_available, mir::geometry::Size const& size) override;
+
+private:
+    mir::geometry::Point top_left;
+    std::function<void(int frames, mir::geometry::Rectangle const& damage)> const damage_notify_change;
+};
+
+NonLegacySurfaceChangeNotification::NonLegacySurfaceChangeNotification(
+    std::function<void()> const& notify_scene_change,
+    std::function<void(int frames, mir::geometry::Rectangle const& damage)> const& damage_notify_change,
+    ms::Surface* surface) :
+    ms::LegacySurfaceChangeNotification(notify_scene_change, {}),
+    damage_notify_change(damage_notify_change)
+{
+    top_left = surface->top_left();
+}
+
+void NonLegacySurfaceChangeNotification::moved_to(mir::geometry::Point const& top_left)
+{
+    this->top_left = top_left;
+    ms::LegacySurfaceChangeNotification::moved_to(top_left);
+}
+
+void NonLegacySurfaceChangeNotification::frame_posted(int frames_available, mir::geometry::Size const& size)
+{
+    mir::geometry::Rectangle const update_region{top_left, size};
+    damage_notify_change(frames_available, update_region);
+}
+}
+
 void ms::LegacySceneChangeNotification::add_surface_observer(ms::Surface* surface)
 {
     auto notifier = [surface, this, was_visible = false] () mutable
@@ -48,11 +97,19 @@
             was_visible = surface->visible();
         };
 
-    auto observer = std::make_shared<ms::LegacySurfaceChangeNotification>(
-        notifier, buffer_notify_change);
-    surface->add_observer(observer);
-    
-    {
+    if (buffer_notify_change)
+    {
+        auto observer = std::make_shared<LegacySurfaceChangeNotification>(notifier, buffer_notify_change);
+        surface->add_observer(observer);
+
+        std::unique_lock<decltype(surface_observers_guard)> lg(surface_observers_guard);
+        surface_observers[surface] = observer;
+    }
+    else
+    {
+        auto observer = std::make_shared<NonLegacySurfaceChangeNotification>(notifier, damage_notify_change, surface);
+        surface->add_observer(observer);
+
         std::unique_lock<decltype(surface_observers_guard)> lg(surface_observers_guard);
         surface_observers[surface] = observer;
     }

=== modified file 'tests/integration-tests/test_surface_stack_with_compositor.cpp'
--- tests/integration-tests/test_surface_stack_with_compositor.cpp	2015-11-05 11:51:04 +0000
+++ tests/integration-tests/test_surface_stack_with_compositor.cpp	2016-01-19 17:44:03 +0000
@@ -144,7 +144,7 @@
     std::shared_ptr<mtd::MockBufferBundle> mock_buffer_stream;
     std::shared_ptr<ms::BasicSurface> stub_surface;
     ms::SurfaceCreationParameters default_params;
-    mtd::StubBuffer stubbuf;
+    mtd::StubBuffer stubbuf{geom::Size{1,1}};
     CountingDisplaySyncGroup stub_primary_db;
     CountingDisplaySyncGroup stub_secondary_db;
     StubDisplay stub_display{stub_primary_db, stub_secondary_db};