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

 === modified file 'src/server/scene/rendering_tracker.cpp'
 --- src/server/scene/rendering_tracker.cpp	2015-02-22 07:46:25 +0000
 +++ src/server/scene/rendering_tracker.cpp	2015-10-30 14:37:35 +0000
@@ -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_;
 === modified file 'src/server/scene/rendering_tracker.h'
 --- src/server/scene/rendering_tracker.h	2015-02-22 07:46:25 +0000
 +++ src/server/scene/rendering_tracker.h	2015-10-30 14:37:35 +0000
@@ -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();
@@ -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;
  };
+ }
 === modified file 'src/server/scene/surface_stack.cpp'
 --- src/server/scene/surface_stack.cpp	2015-06-18 02:46:16 +0000
 +++ src/server/scene/surface_stack.cpp	2015-10-30 14:37:35 +0000
@@ -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) ==
--                    mir_surface_visibility_exposed)
++                surface->query(mir_surface_attrib_visibility) == mir_surface_visibility_exposed)
+             {
--                // Note that we ask the surface and not a Renderable.
--                // This is because we don't want to waste time and resources
--                // on a snapshot till we're sure we need it...
--                int ready = surface->buffers_ready_for_compositor(id);
--                if (ready > result)
--                    result = ready;
++                auto const tracker = rendering_trackers.find(surface.get());
++                if (tracker != rendering_trackers.end() && tracker->second->is_exposed_in(id))
++                {
++                    // Note that we ask the surface and not a Renderable.
++                    // This is because we don't want to waste time and resources
++                    // on a snapshot till we're sure we need it...
++                    int ready = surface->buffers_ready_for_compositor(id);
++                    if (ready > result)
++                        result = ready;
++                }
+             }
+         }
+     }
 === modified file 'tests/unit-tests/scene/test_surface_stack.cpp'
 --- tests/unit-tests/scene/test_surface_stack.cpp	2015-06-26 08:00:59 +0000
 +++ tests/unit-tests/scene/test_surface_stack.cpp	2015-10-30 14:37:35 +0000
@@ -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>>();
@@ -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{{},{}},
@@ -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;