Mir

Merge lp:~kdub/mir/initial-visibility-event into lp:mir

initial-visibility-event
Merge into development-branch

Proposed by Kevin DuBois on 2015-10-26

Status:	Superseded
Proposed branch:	lp:~kdub/mir/initial-visibility-event
Merge into:	lp:mir
Diff against target:	477 lines (+202/-132) 6 files modified src/server/scene/basic_surface.h (+1/-1) src/server/scene/surface_stack.cpp (+1/-4) tests/acceptance-tests/test_client_focus_notification.cpp (+106/-114) tests/acceptance-tests/test_surface_specification.cpp (+44/-10) tests/unit-tests/scene/test_basic_surface.cpp (+44/-1) tests/unit-tests/scene/test_surface_stack.cpp (+6/-2)
To merge this branch:	bzr merge lp:~kdub/mir/initial-visibility-event
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration		Needs Fixing on 2015-11-04
Alexandros Frantzis (community)			Needs Fixing on 2015-10-27
Alan Griffiths		2015-10-26	Approve on 2015-10-26
Review via email: mp+275704@code.launchpad.net

This proposal has been superseded by a proposal from 2015-11-30.

Commit Message

Change the default value for surfaces visibily to occluded. Surfaces don't become visible until they've submitted a buffer, and the shell has placed them somewhere visible. This has the practical effect of sending a startup visibility event.

Description of the Change

nb: interested in this now because SurfaceSpecification test suite relies that a surface is visible. Since NBS will not wait for the submit_buffer() rpc call to return its mp::Void message, we don't have a sync point anymore that guarantees the surface is visible. It seemed appropriate to change the behavior an synchronize around the visibility event in this suite, as a surface isn't visible by default.

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

More reasonable than current behaviour.

review: Approve

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

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

review: Needs Fixing (continuous-integration)

lp:~kdub/mir/initial-visibility-event updated on 2015-10-26

3060. By Kevin DuBois on 2015-10-26: fix problem for clang
3061. By Kevin DuBois on 2015-10-26: merge in mir

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

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

review: Needs Fixing (continuous-integration)

Alexandros Frantzis (afrantzis) wrote on 2015-10-27:

The change in behavior is fine.

[ FAILED ] ClientFocusNotification.a_surface_is_notified_of_receiving_focus

We need to make the tests more resilient to differences in the ordering and quantity of events we expect, since differences in system load and thread scheduling will produce different results.

(The FD leaks seen in the logs are just a consequence of the tests not shutting down cleanly, they are not the core cause of the failures)

review: Needs Fixing

lp:~kdub/mir/initial-visibility-event updated on 2015-10-28

3062. By Kevin DuBois on 2015-10-28: completely rewrite the ClientFocusNotification to be more robust and not to fork off clients

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

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

review: Needs Fixing (continuous-integration)

lp:~kdub/mir/initial-visibility-event updated on 2015-11-04

3063. By Kevin DuBois on 2015-11-04: merge in mir
3064. By Kevin DuBois on 2015-11-04: remove spurious change
3065. By Kevin DuBois on 2015-11-04: roll back small change and accommodate gmock not knowing how to print an object

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

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

review: Needs Fixing (continuous-integration)

Kevin DuBois (kdub) wrote on 2015-11-04:

current logic is slightly improper (visible() and visibility attribute are a bit confusing) -> WIP

lp:~kdub/mir/initial-visibility-event updated on 2015-12-02

3066. By Kevin DuBois on 2015-11-25: merge in mir
3067. By Kevin DuBois on 2015-11-30: merge in mir
3068. By Kevin DuBois on 2015-11-30: merge in mir
3069. By Kevin DuBois on 2015-11-30: revert src
3070. By Kevin DuBois on 2015-11-30: change the default value
3071. By Kevin DuBois on 2015-11-30: test to pass
3072. By Kevin DuBois on 2015-11-30: clean up remaining tests,get them to pass
3073. By Kevin DuBois on 2015-11-30: cleanups
3074. By Kevin DuBois on 2015-11-30: further cleanups
3075. By Kevin DuBois on 2015-11-30: merge in mir
3076. By Kevin DuBois on 2015-11-30: roll back change to test
3077. By Kevin DuBois on 2015-11-30: modify test so that occlusion is always modified via the rendering trackers, as designed
3078. By Kevin DuBois on 2015-12-02: adopt Albertos suggestion to match the sequence of events in the surface
3079. By Kevin DuBois on 2015-12-02: merge in mir

Unmerged revisions

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:

Emanuele Antonio Faraone

Gerry Boland

Kevin DuBois

Mir development team

 === modified file 'src/server/scene/basic_surface.h'
 --- src/server/scene/basic_surface.h	2015-10-20 04:57:13 +0000
 +++ src/server/scene/basic_surface.h	2015-11-30 18:16:24 +0000
@@ -181,7 +181,7 @@
      int swapinterval_ = 1;
      MirSurfaceFocusState focus_ = mir_surface_unfocused;
      int dpi_ = 0;
--    MirSurfaceVisibility visibility_ = mir_surface_visibility_exposed;
++    MirSurfaceVisibility visibility_ = mir_surface_visibility_occluded;
      MirOrientationMode pref_orientation_mode = mir_orientation_mode_any;
      std::unique_ptr<CursorStreamImageAdapter> const cursor_stream_adapter;
 === modified file 'src/server/scene/surface_stack.cpp'
 --- src/server/scene/surface_stack.cpp	2015-11-05 11:51:04 +0000
 +++ src/server/scene/surface_stack.cpp	2015-11-30 18:16:24 +0000
@@ -160,10 +160,7 @@
      int result = scene_changed ? 1 : 0;
      for (auto const& surface : surfaces)
+     {
--        // 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)
++        if (surface->visible())
+         {
              auto const tracker = rendering_trackers.find(surface.get());
              if (tracker != rendering_trackers.end() && tracker->second->is_exposed_in(id))
 === modified file 'tests/acceptance-tests/test_client_focus_notification.cpp'
 --- tests/acceptance-tests/test_client_focus_notification.cpp	2015-10-02 04:27:35 +0000
 +++ tests/acceptance-tests/test_client_focus_notification.cpp	2015-11-30 18:16:24 +0000
@@ -1,5 +1,5 @@
  /*
-- * Copyright © 2013-2014 Canonical Ltd.
++ * Copyright © 2015 Canonical Ltd.
+  *
   * This program is free software: you can redistribute it and/or modify
   * it under the terms of the GNU General Public License version 3 as
@@ -13,7 +13,7 @@
   * You should have received a copy of the GNU General Public License
   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+  *
-- * Authored by: Robert Carr <robert.carr@canonical.com>
++ * Authored by: Kevin DuBois <kevin.dubois@canonical.com>
   */
  #include "mir_toolkit/mir_client_library.h"
@@ -21,7 +21,7 @@
  #include "mir/test/wait_condition.h"
  #include "mir/test/event_matchers.h"
--#include "mir_test_framework/interprocess_client_server_test.h"
++#include "mir_test_framework/connected_client_headless_server.h"
  #include "mir_test_framework/process.h"
  #include "mir/test/cross_process_sync.h"
@@ -35,134 +35,126 @@
  namespace
+ {
--struct MockEventObserver
++struct FocusLogger
+ {
--    MOCK_METHOD1(see, void(MirEvent const*));
++    void log_focus_event(MirSurface* surface, MirSurfaceFocusState state)
++    {
++        std::lock_guard<std::mutex> lk(mutex);
++        focus_events[surface].push_back(state);
++        cv.notify_all();
++    }
++
++    void wait_for_num_focus_events(unsigned int num, MirSurface* surface, std::chrono::seconds until)
++    {
++        std::unique_lock<std::mutex> lk(mutex);
++        if (!cv.wait_for(lk, until, [this, num, surface]
++            {
++                return ((focus_events.find(surface) != focus_events.end()) &&
++                    (focus_events[surface].size() >= num));
++            }))
++        {
++            throw std::logic_error("timeout waiting for events");
++        }
++    }
++
++    std::vector<MirSurfaceFocusState> events_for(MirSurface* surface)
++    {
++        std::lock_guard<std::mutex> lk(mutex);
++        if (focus_events.find(surface) == focus_events.end())
++            throw std::logic_error("no events");
++        return focus_events[surface];
++    }
++
++    static void handle_event(MirSurface* surface, MirEvent const* ev, void* context)
++    {
++        if (mir_event_type_surface == mir_event_get_type(ev))
++        {
++            auto surface_ev = mir_event_get_surface_event(ev);
++            if (mir_surface_attrib_focus == mir_surface_event_get_attribute(surface_ev))
++            {
++                auto self = static_cast<FocusLogger*>(context);
++                self->log_focus_event(surface,
++                    static_cast<MirSurfaceFocusState>(mir_surface_event_get_attribute_value(surface_ev)));
++            }
++        }
++    }
++private:
++    std::mutex mutex;
++    std::condition_variable cv;
++    std::map<MirSurface*, std::vector<MirSurfaceFocusState>> focus_events;
  };
--struct ClientFocusNotification : mtf::InterprocessClientServerTest
++struct FocusSurface
+ {
--    void SetUp() override
--    {
--        mtf::InterprocessClientServerTest::SetUp();
--        run_in_server([]{});
--    }
--
--    MockEventObserver observer;
--    mt::WaitCondition all_events_received;
--
--    void connect_and_create_surface()
--    {
--        MirConnection *connection = mir_connect_sync(mir_test_socket, __PRETTY_FUNCTION__);
--        ASSERT_TRUE(mir_connection_is_valid(connection));
--
++    FocusSurface(MirConnection* connection, std::shared_ptr<FocusLogger> const& logger) :
++        connection(connection)
++    {
          auto spec = mir_connection_create_spec_for_normal_surface(connection, 100, 100, mir_pixel_format_abgr_8888);
++        mir_surface_spec_set_event_handler(spec, FocusLogger::handle_event, logger.get());
--        mir_wait_for(mir_surface_create(spec, surface_created, this));
++        surface = mir_surface_create_sync(spec);
          mir_surface_spec_release(spec);
++
          mir_buffer_stream_swap_buffers_sync(mir_surface_get_buffer_stream(surface));
--
--        all_events_received.wait_for_at_most_seconds(60);
++    }
++
++    ~FocusSurface()
++    {
++        if (!released) release();
++    }
++
++    MirSurface* native_handle() const
++    {
++        return surface;
++    }
++
++    void release()
++    {
          mir_surface_release_sync(surface);
          mir_connection_release(connection);
--    }
--
--private:
--
--    MirSurface *surface;
--
--    static void handle_event(MirSurface* /* surface */, MirEvent const* ev, void* context)
--    {
--        auto self = static_cast<ClientFocusNotification*>(context);
--        self->observer.see(ev);
--    }
--
--    static void surface_created(MirSurface *surface_, void *ctx)
--    {
--        auto self = static_cast<ClientFocusNotification*>(ctx);
--
--        self->surface = surface_;
--        // We need to set the event delegate from the surface_created
--        // callback so we can block the reading of new events
--        // until we are ready
--        mir_surface_set_event_handler(surface_, handle_event, self);
--    }
++        released = true;
++    }
++
++    MirConnection* const connection;
++    MirSurface* surface;
++    bool released{false};
++};
++
++struct ClientFocusNotification : mtf::ConnectedClientHeadlessServer
++{
++    std::shared_ptr<FocusLogger> logger{std::make_shared<FocusLogger>()};
  };
+ }
  TEST_F(ClientFocusNotification, a_surface_is_notified_of_receiving_focus)
+ {
--    run_in_client([&]
--        {
--            InSequence s;
--            EXPECT_CALL(observer, see(_)); //ignore scaling events
--            EXPECT_CALL(observer, see(mt::SurfaceEvent(mir_surface_attrib_focus, mir_surface_focused))).Times(1)
--                .WillOnce(mt::WakeUp(&all_events_received));
--            // We may not see mir_surface_unfocused before connection closes
--            EXPECT_CALL(observer, see(mt::SurfaceEvent(mir_surface_attrib_focus, mir_surface_unfocused))).Times(AtMost(1));
--
--            connect_and_create_surface();
--        });
--}
--
--namespace
--{
--
--ACTION_P(SignalFence, fence)
--{
--    fence->try_signal_ready_for();
--}
--
++    FocusSurface surface(mir_connect_sync(new_connection().c_str(), __PRETTY_FUNCTION__), logger);
++    surface.release();
++
++    logger->wait_for_num_focus_events(2, surface.native_handle(), std::chrono::seconds(5));
++
++    auto log = logger->events_for(surface.native_handle());
++    EXPECT_THAT(log[0], Eq(mir_surface_focused));
++    EXPECT_THAT(log[1], Eq(mir_surface_unfocused));
+ }
  TEST_F(ClientFocusNotification, two_surfaces_are_notified_of_gaining_and_losing_focus)
+ {
--    // We use this for synchronization to ensure the two clients
--    // are launched in a defined order.
--    mt::CrossProcessSync ready_for_second_client;
--
--    auto const client_one = new_client_process([&]
--        {
--            InSequence seq;
--            EXPECT_CALL(observer, see(_)); //ignore scaling events
--            // We should receive focus as we are created
--            EXPECT_CALL(observer, see(mt::SurfaceEvent(mir_surface_attrib_focus,
--                mir_surface_focused))).Times(1)
--                    .WillOnce(SignalFence(&ready_for_second_client));
--
--            // And lose it as the second surface is created
--            EXPECT_CALL(observer, see(mt::SurfaceEvent(mir_surface_attrib_focus,
--                mir_surface_unfocused))).Times(1);
--            // And regain it when the second surface is closed
--            EXPECT_CALL(observer, see(mt::SurfaceEvent(mir_surface_attrib_focus,
--                mir_surface_focused))).Times(1).WillOnce(mt::WakeUp(&all_events_received));
--            // And then lose it as we are closed (but we may not see confirmation before connection closes)
--            EXPECT_CALL(observer, see(mt::SurfaceEvent(mir_surface_attrib_focus,
--                mir_surface_unfocused))).Times(AtMost(1));
--
--            connect_and_create_surface();
--        });
--
--    auto const client_two = new_client_process([&]
--        {
--            client_one->detach();
--            ready_for_second_client.wait_for_signal_ready_for();
--
--            EXPECT_CALL(observer, see(_)); //ignore scaling events
--            EXPECT_CALL(observer, see(
--                mt::SurfaceEvent(mir_surface_attrib_focus, mir_surface_focused)))
--                    .Times(1).WillOnce(mt::WakeUp(&all_events_received));
--            // We may not see mir_surface_unfocused before connection closes
--            EXPECT_CALL(observer, see(
--                mt::SurfaceEvent(mir_surface_attrib_focus, mir_surface_unfocused)))
--                    .Times(AtMost(1));
--
--            connect_and_create_surface();
--        });
--
--    if (is_test_process())
--    {
--        EXPECT_THAT(client_one->wait_for_termination().exit_code, Eq(EXIT_SUCCESS));
--        EXPECT_THAT(client_two->wait_for_termination().exit_code, Eq(EXIT_SUCCESS));
--    }
++    FocusSurface surface1(mir_connect_sync(new_connection().c_str(), __PRETTY_FUNCTION__), logger);
++    logger->wait_for_num_focus_events(1, surface1.native_handle(), std::chrono::seconds(5));
++    FocusSurface surface2(mir_connect_sync(new_connection().c_str(), __PRETTY_FUNCTION__), logger);
++    surface2.release();
++    surface1.release();
++    logger->wait_for_num_focus_events(4, surface1.native_handle(), std::chrono::seconds(5));
++    logger->wait_for_num_focus_events(2, surface2.native_handle(), std::chrono::seconds(5));
++
++    auto log = logger->events_for(surface1.native_handle());
++    EXPECT_THAT(log[0], Eq(mir_surface_focused));
++    EXPECT_THAT(log[1], Eq(mir_surface_unfocused));
++    EXPECT_THAT(log[2], Eq(mir_surface_focused));
++    EXPECT_THAT(log[3], Eq(mir_surface_unfocused));
++
++    log = logger->events_for(surface2.native_handle());
++    EXPECT_THAT(log[0], Eq(mir_surface_focused));
++    EXPECT_THAT(log[1], Eq(mir_surface_unfocused));
+ }
 === modified file 'tests/acceptance-tests/test_surface_specification.cpp'
 --- tests/acceptance-tests/test_surface_specification.cpp	2015-09-23 21:49:39 +0000
 +++ tests/acceptance-tests/test_surface_specification.cpp	2015-11-30 18:16:24 +0000
@@ -46,22 +46,60 @@
  class SurfaceHandle
+ {
  public:
--    explicit SurfaceHandle(MirSurface* surface) : surface{surface}
++    explicit SurfaceHandle(MirSurfaceSpec* spec) :
++        visible{false}
+     {
++        mir_surface_spec_set_event_handler(spec, SurfaceHandle::event_callback, this);
++        surface = mir_surface_create_sync(spec);
          // Swap buffers to ensure surface is visible for event based tests
          if (mir_surface_is_valid(surface))
++        {
              mir_buffer_stream_swap_buffers_sync(mir_surface_get_buffer_stream(surface));
++
++            std::unique_lock<std::mutex> lk(mutex);
++            if (!cv.wait_for(lk, std::chrono::seconds(5), [this] { return visible; }))
++                throw std::runtime_error("timeout waiting for visibility of surface");
++        }
+     }
      ~SurfaceHandle() { if (surface) mir_surface_release_sync(surface); }
++    static void event_callback(MirSurface* surf, MirEvent const* ev, void* context)
++    {
++        if (mir_event_get_type(ev) == mir_event_type_surface)
++        {
++            if (mir_surface_event_get_attribute(mir_event_get_surface_event(ev)) == mir_surface_attrib_visibility)
++            {
++                auto ctx = reinterpret_cast<SurfaceHandle*>(context);
++                ctx->set_visibility(surf, mir_surface_event_get_attribute_value(mir_event_get_surface_event(ev)));
++            }
++        }
++    }
++
++    void set_visibility(MirSurface* surf, bool vis)
++    {
++        std::lock_guard<std::mutex> lk(mutex);
++        if (surf != surface) return;
++        visible = vis;
++        cv.notify_all();
++    }
++
      operator MirSurface*() const { return surface; }
      SurfaceHandle(SurfaceHandle&& that) : surface{that.surface} { that.surface = nullptr; }
  private:
++    std::mutex mutex;
++    std::condition_variable cv;
++
      SurfaceHandle(SurfaceHandle const&) = delete;
      MirSurface* surface;
++    bool visible;
  };
++//gmocks printer has problems with printing this type
++::std::ostream& operator<<(::std::ostream& os, const SurfaceHandle& handle) {
++    return os << static_cast<MirSurface*>(handle);
++}
++
  class MockSurfaceObserver : public ms::NullSurfaceObserver
+ {
  public:
@@ -108,16 +146,12 @@
+     }
      template<typename Specifier>
--    SurfaceHandle create_surface(Specifier const& specifier) const
++    SurfaceHandle create_surface(Specifier const& specifier)
+     {
--        auto const spec = mir_create_surface_spec(connection);
--
--        specifier(spec);
--
--        auto const surface = mir_surface_create_sync(spec);
--        mir_surface_spec_release(spec);
--
--        return SurfaceHandle{surface};
++        auto del = [] (MirSurfaceSpec* spec) { mir_surface_spec_release(spec); };
++        std::unique_ptr<MirSurfaceSpec, decltype(del)> spec(mir_create_surface_spec(connection), del);
++        specifier(spec.get());
++        return SurfaceHandle{spec.get()};
+     }
      NiceMock<MockSurfaceObserver> surface_observer;
 === modified file 'tests/unit-tests/scene/test_basic_surface.cpp'
 --- tests/unit-tests/scene/test_basic_surface.cpp	2015-10-07 12:06:14 +0000
 +++ tests/unit-tests/scene/test_basic_surface.cpp	2015-11-30 18:16:24 +0000
@@ -535,8 +535,8 @@
  AttributeTestParameters const surface_visibility_test_parameters{
      mir_surface_attrib_visibility,
++    mir_surface_visibility_occluded,
      mir_surface_visibility_exposed,
--    mir_surface_visibility_occluded,
      -1
  };
@@ -995,3 +995,46 @@
      EXPECT_THAT(renderables[0], IsRenderableOfSize(size0));
      EXPECT_THAT(renderables[1], IsRenderableOfSize(size1));
+ }
++
++namespace
++{
++struct VisibilityObserver : ms::NullSurfaceObserver
++{
++    void attrib_changed(MirSurfaceAttrib attrib, int value) override
++    {
++        if (attrib == mir_surface_attrib_visibility)
++        {
++            if (value == mir_surface_visibility_occluded)
++                hides_++;
++            else if (value == mir_surface_visibility_exposed)
++                exposes_++;
++        }
++    }
++    unsigned int exposes()
++    {
++        return exposes_;
++    }
++    unsigned int hides()
++    {
++        return hides_;
++    }
++private:
++    unsigned int exposes_{0};
++    unsigned int hides_{0};
++};
++}
++
++TEST_F(BasicSurfaceTest, notifies_when_first_visible)
++{
++    using namespace testing;
++    auto observer = std::make_shared<VisibilityObserver>();
++    surface.add_observer(observer);
++
++    EXPECT_THAT(observer->exposes(), Eq(0));
++    EXPECT_THAT(observer->hides(), Eq(0));
++    post_a_frame(surface);
++    surface.configure(mir_surface_attrib_visibility, mir_surface_visibility_exposed);
++
++    EXPECT_THAT(observer->exposes(), Eq(1));
++    EXPECT_THAT(observer->hides(), Eq(0));
++}
 === modified file 'tests/unit-tests/scene/test_surface_stack.cpp'
 --- tests/unit-tests/scene/test_surface_stack.cpp	2015-11-05 11:51:04 +0000
 +++ tests/unit-tests/scene/test_surface_stack.cpp	2015-11-30 18:16:24 +0000
@@ -397,8 +397,9 @@
          report);
      stack.add_surface(surface, default_params.input_mode);
--    surface->configure(mir_surface_attrib_visibility,
--                       mir_surface_visibility_occluded);
++    auto elements = stack.scene_elements_for(this);
++    for (auto const& elem : elements)
++        elem->occluded();
      EXPECT_EQ(0, stack.frames_pending(this));
      post_a_frame(*surface);
@@ -433,7 +434,9 @@
      post_a_frame(*surface);
      post_a_frame(*surface);
++    EXPECT_EQ(3, stack.frames_pending(comp1));
      EXPECT_EQ(3, stack.frames_pending(comp2));
++
      auto elements = stack.scene_elements_for(comp1);
      for (auto const& elem : elements)
+     {
@@ -446,6 +449,7 @@
          elem->occluded();
+     }
++    EXPECT_EQ(3, stack.frames_pending(comp1));
      EXPECT_EQ(0, stack.frames_pending(comp2));
+ }