Mir

Merge lp:~albaguirre/mir/fix-1499039 into lp:mir

fix-1499039
Merge into development-branch

Proposed by Alberto Aguirre on 2015-10-29

Status:

Merged

Approved by:

Alexandros Frantzis on 2015-11-03

Approved revision:

no longer in the source branch.

Merged at revision:

3073

Proposed branch:

lp:~albaguirre/mir/fix-1499039

Merge into:

lp:mir

Diff against target:

143 lines (+69/-9)

4 files modified

src/server/scene/rendering_tracker.cpp (+12/-0)
src/server/scene/rendering_tracker.h (+2/-1)
src/server/scene/surface_stack.cpp (+11/-8)
tests/unit-tests/scene/test_surface_stack.cpp (+44/-0)

To merge this branch:

bzr merge lp:~albaguirre/mir/fix-1499039

Related bugs:

Bug #1499039: compositing never stops when external monitor is connected

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2015-11-03
Kevin DuBois (community)			Approve on 2015-10-30
Alan Griffiths		2015-10-29	Approve on 2015-10-30
Review via email: mp+276197@code.launchpad.net

Commit message

compositor: do not count pending frames from partially exposed surfaces

When querying the pending frames for a specific compositor, do not count frames in surfaces that have only been exposed in different compositor.

Description of the change

compositor: do not count pending frames from partially exposed surfaces

When querying the pending frames for a specific compositor, do not count frames in surfaces that have only been exposed in different compositor.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-10-29:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-10-30:

Nits:

Seems to upset vivid builds. Because I don't think you mean this:

+ auto comp1{reinterpret_cast<mc::CompositorID>(0)};
+ auto comp2{reinterpret_cast<mc::CompositorID>(1)};

But

auto const comp1 = reinterpret_cast<mc::CompositorID>(0);
auto const comp2 = reinterpret_cast<mc::CompositorID>(1);

Using "auto" with initializer lists is a Bad Idea as it rarely does what you intend.

~~~~

+ mutable std::mutex guard;

cv-qualifiers should follow the type

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-10-30:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-10-30:

plausible

review: Approve

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-10-30:

lgtm

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) on 2015-11-03:

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Alberto Aguirre

Emanuele Antonio Faraone

Gerry Boland

Mir development team

@@ 2 / --- src/server/scene/rendering_tracker.cpp 2015-02-22 07:46:25 +0000 @@
@@ 3 / +++ src/server/scene/rendering_tracker.cpp 2015-10-30 14:37:35 +0000 @@
@@ 4 / @@ -68,6 +68,18 @@ @@
         configure_visibility(mir_surface_visibility_occluded);
+}
+bool ms::RenderingTracker::is_exposed_in(mc::CompositorID cid) const
+{
+    std::lock_guard<std::mutex> lock{guard};
+    ensure_is_active_compositor(cid);
+    if (occlusions.size() == 0)
+        return true;
+    return occlusions.find(cid) == occlusions.end();
+}
 bool ms::RenderingTracker::occluded_in_all_active_compositors()
+{
     return occlusions == active_compositors_;
@@ 25 / --- src/server/scene/rendering_tracker.h 2015-02-22 07:46:25 +0000 @@
@@ 26 / +++ src/server/scene/rendering_tracker.h 2015-10-30 14:37:35 +0000 @@
@@ 27 / @@ -42,6 +42,7 @@ @@
     void rendered_in(compositor::CompositorID cid);
     void occluded_in(compositor::CompositorID cid);
     void active_compositors(std::set<compositor::CompositorID> const& cids);
+    bool is_exposed_in(compositor::CompositorID cid) const;
 private:
     bool occluded_in_all_active_compositors();
@@ 35 / @@ -52,7 +53,7 @@ @@
     std::weak_ptr<Surface> const weak_surface;
     std::set<compositor::CompositorID> occlusions;
     std::set<compositor::CompositorID> active_compositors_;
-    std::mutex guard;
+    std::mutex mutable guard;
 };
+}
@@ 46 / --- src/server/scene/surface_stack.cpp 2015-06-18 02:46:16 +0000 @@
@@ 47 / +++ src/server/scene/surface_stack.cpp 2015-10-30 14:37:35 +0000 @@
@@ 48 / @@ -168,15 +168,18 @@ @@
             // TODO: Rename mir_surface_attrib_visibility as it's obviously
             //       confusing with visible()
             if (surface->visible() &&
-                surface->query(mir_surface_attrib_visibility) ==
+                surface->query(mir_surface_attrib_visibility) == mir_surface_visibility_exposed)
-                    mir_surface_visibility_exposed)
+            {
-                // Note that we ask the surface and not a Renderable.
+                auto const tracker = rendering_trackers.find(surface.get());
-                // This is because we don't want to waste time and resources
+                if (tracker != rendering_trackers.end() && tracker->second->is_exposed_in(id))
-                // on a snapshot till we're sure we need it...
+                {
-                int ready = surface->buffers_ready_for_compositor(id);
+                    // Note that we ask the surface and not a Renderable.
-                if (ready > result)
+                    // This is because we don't want to waste time and resources
-                    result = ready;
+                    // on a snapshot till we're sure we need it...
+                    int ready = surface->buffers_ready_for_compositor(id);
+                    if (ready > result)
+                        result = ready;
+                }
+            }
+        }
+    }
@@ 77 / --- tests/unit-tests/scene/test_surface_stack.cpp 2015-06-26 08:00:59 +0000 @@
@@ 78 / +++ tests/unit-tests/scene/test_surface_stack.cpp 2015-10-30 14:37:35 +0000 @@
@@ 79 / @@ -339,6 +339,7 @@ @@
+{
     using namespace testing;
     ms::SurfaceStack stack{report};
+    stack.register_compositor(this);
     mtd::StubBuffer stub_buffer;
     int ready = 0;
     auto mock_queue = std::make_shared<testing::NiceMock<mtd::MockBufferBundle>>();
@@ 87 / @@ -384,6 +385,7 @@ @@
     using namespace testing;
     ms::SurfaceStack stack{report};
+    stack.register_compositor(this);
     auto surface = std::make_shared<ms::BasicSurface>(
         std::string("stub"),
         geom::Rectangle{{},{}},
@@ 95 / @@ -405,6 +407,48 @@ @@
     EXPECT_EQ(0, stack.frames_pending(this));
+}
+TEST_F(SurfaceStack, scene_doesnt_count_pending_frames_from_partially_exposed_surfaces)
+{  // Regression test for LP: #1499039
+    using namespace testing;
+    // Partially exposed means occluded in one compositor but not another
+    ms::SurfaceStack stack{report};
+    auto const comp1 = reinterpret_cast<mc::CompositorID>(0);
+    auto const comp2 = reinterpret_cast<mc::CompositorID>(1);
+    stack.register_compositor(comp1);
+    stack.register_compositor(comp2);
+    auto surface = std::make_shared<ms::BasicSurface>(
+        std::string("stub"),
+        geom::Rectangle{{},{}},
+        false,
+        std::make_shared<mtd::StubBufferStream>(),
+        std::shared_ptr<mir::input::InputChannel>(),
+        std::shared_ptr<mir::input::InputSender>(),
+        std::shared_ptr<mg::CursorImage>(),
+        report);
+    stack.add_surface(surface, default_params.depth, default_params.input_mode);
+    post_a_frame(*surface);
+    post_a_frame(*surface);
+    post_a_frame(*surface);
+    EXPECT_EQ(3, stack.frames_pending(comp2));
+    auto elements = stack.scene_elements_for(comp1);
+    for (auto const& elem : elements)
+    {
+        elem->rendered();
+    }
+    elements = stack.scene_elements_for(comp2);
+    for (auto const& elem : elements)
+    {
+        elem->occluded();
+    }
+    EXPECT_EQ(0, stack.frames_pending(comp2));
+}
 TEST_F(SurfaceStack, surfaces_are_emitted_by_layer)
+{
     using namespace testing;