Mir

Merge lp:~kdub/mir/fix-1577967 into lp:mir

fix-1577967
Merge into development-branch

Proposed by Kevin DuBois on 2016-05-10

Status:

Merged

Approved by:

Kevin DuBois on 2016-06-08

Approved revision:

3509

Merged at revision:

3533

Proposed branch:

lp:~kdub/mir/fix-1577967

Merge into:

lp:mir

Diff against target:

349 lines (+137/-48)

7 files modified

include/client/mir_toolkit/mir_surface.h (+17/-5)
playground/mir_demo_client_prerendered_frames.c (+6/-0)
src/client/mir_connection.cpp (+21/-25)
src/client/mir_surface.cpp (+17/-16)
src/client/mir_surface.h (+7/-2)
tests/acceptance-tests/test_buffer_stream_arrangement.cpp (+45/-0)
tests/unit-tests/client/test_client_mir_surface.cpp (+24/-0)

To merge this branch:

bzr merge lp:~kdub/mir/fix-1577967

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Mir CI Bot	continuous-integration		Approve on 2016-06-08
Daniel van Vugt			Abstain on 2016-06-08
Cemil Azizoglu (community)			Approve on 2016-05-16
Alan Griffiths			Approve on 2016-05-11
Chris Halse Rogers		2016-05-10	Approve on 2016-05-11
Review via email: mp+294283@code.launchpad.net

Commit Message

fix lp: #1577967 by clarifying what mir_surface_get_buffer_stream() should do in a multistream world, and by making mir_surface_is_valid() respond properly when non-default streams are used.

Description of the Change

fix lp: #1577967 by clarifying what mir_surface_get_buffer_stream() should do in a multistream world, and by making mir_surface_is_valid() respond properly when non-default streams are used.

note: the original thought was designate the 1st MirBufferStream as 'the default stream', however, soon it will be possible to have multiple MirPresentationChains, in which case there are no MirBufferStream*'s to be the default.

Mir CI Bot (mir-ci-bot) wrote on 2016-05-10:

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

review: Needs Fixing (continuous-integration)

Chris Halse Rogers (raof) wrote on 2016-05-11:

LGTM. Should we be marking all the mir_surface_* functions which implicitly deal with a default BufferStream as deprecated?

Certainly we should be marking mir_surface_{get,set}_swap_interval as deprecated. I lean towards marking mir_surface_get_buffer_stream as deprecated, also, although there's not a direct replacement.

review: Approve

Alan Griffiths (alan-griffiths) wrote on 2016-05-11:

Triggered rebuild as a quick looks suggested extremely slow test hardware leading to timeouts.

~~~~

+ uint32_t output_id;
+
};

Unnecessary whitespace

review: Approve

Mir CI Bot (mir-ci-bot) wrote on 2016-05-11:

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

review: Needs Fixing (continuous-integration)

Kevin DuBois (kdub) wrote on 2016-05-11:

> Certainly we should be marking mir_surface_{get,set}_swap_interval as
> deprecated. I lean towards marking mir_surface_get_buffer_stream as
> deprecated, also, although there's not a direct replacement.

There is a way in the current API to work without mir_surface_get_buffer_stream()

MirSurfaceSpec* spec = ...;
MirBufferStream* stream = mir_connection_buffer_stream_create_sync(..);
mir_surface_spec_set_streams(spec, {stream, 0, 0}, 1);
MirSurface* surface = mir_surface_create_sync(spec);

now the client knows which stream is in the surface, and could change/swap as it wants to.

Cemil Azizoglu (cemil-azizoglu) wrote on 2016-05-11:

>LGTM. Should we be marking all the mir_surface_* functions which implicitly deal with a default >BufferStream as deprecated?

+1 as the names are really confusing.

Needs fixings :

153 + legacy_default_stream(buffer_stream),

Since the notion of "default stream" is not going away, we should just call it "default_stream".
-------------------------------------------

209 + auto resize_event = mir_event_get_resize_event(&e);
210 + size = { mir_resize_event_get_width(resize_event), mir_resize_event_get_height(resize_event) };
211 + if (legacy_default_stream)
212 + legacy_default_stream->set_size(size);

The if statement should enclose the whole thing as it only makes sense when there is a default stream.

review: Needs Fixing

Kevin DuBois (kdub) wrote on 2016-05-16:

can add some deprecated tags to discourage use in the headers.

re legacy_default_stream, I think tagging legacy_ on front indicates that this bit of functionality is really only around for legacy purposes. Its not going away yet, but it is still around for legacy reasons. OTOH, its an internal member variable, so will change name and add comment to this effect.

The size member variable is used in get_parameters, and should be changed when the resize event occurs.

Cemil Azizoglu (cemil-azizoglu) wrote on 2016-05-16:

LGTM

review: Approve

Mir CI Bot (mir-ci-bot) wrote on 2016-05-19:

review: Needs Fixing (continuous-integration)

Mir CI Bot (mir-ci-bot) wrote on 2016-05-19:

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

review: Needs Fixing (continuous-integration)

Daniel van Vugt (vanvugt) wrote on 2016-05-20:

A few failures:

16:48:22 9: [ FAILED ] 3 tests, listed below:
16:48:22 9: [ FAILED ] ServerStartup.creates_endpoint_on_filesystem
16:48:22 9: [ FAILED ] ServerStartup.after_server_sigkilled_can_start_new_instance
16:48:22 9: [ FAILED ] UnresponsiveClient.does_not_hang_server

They are not pre-existing bugs or known failures...

Daniel van Vugt (vanvugt) wrote on 2016-05-25:

Just a reminder - the CI failures appear to be consistent and unique to this branch. So I guess the branch is faulty.

review: Needs Fixing

lp:~kdub/mir/fix-1577967 updated on 2016-06-08

3507. By Kevin DuBois on 2016-05-20: merge in mir
3508. By Kevin DuBois on 2016-06-08: merge in mir
3509. By Kevin DuBois on 2016-06-08: remember to free spec in BufferStreamArrangement test suite

Mir CI Bot (mir-ci-bot) wrote on 2016-06-08:

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

review: Needs Fixing (continuous-integration)

Daniel van Vugt (vanvugt) on 2016-06-08:

review: Abstain

Kevin DuBois (kdub) wrote on 2016-06-08:

CI was ran against rev3508... rev3509 has fix for valgrind+ci.

Mir CI Bot (mir-ci-bot) wrote on 2016-06-08:

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

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:

Emanuele Antonio Faraone

Gerry Boland

Kevin DuBois

Mir development team

 === modified file 'include/client/mir_toolkit/mir_surface.h'
 --- include/client/mir_toolkit/mir_surface.h	2016-05-03 06:55:25 +0000
 +++ include/client/mir_toolkit/mir_surface.h	2016-06-08 13:06:12 +0000
@@ -538,7 +538,14 @@
  /**
   * Retrieve the primary MirBufferStream associated with a surface (to advance buffers,
   * obtain EGLNativeWindow, etc...)
-- *
++ *
++ *   \deprecated Users should use mir_surface_spec_set_streams() to arrange
++ *               the content of a surface, instead of relying on a stream
++ *               being created by default.
++ *   \warning If the surface was created with, or modified to have a
++ *            MirSurfaceSpec containing streams added through
++ *            mir_surface_spec_set_streams(), the default stream will
++ *            be removed, and this function will return NULL.
   *   \param[in] surface The surface
   */
  MirBufferStream* mir_surface_get_buffer_stream(MirSurface *surface);
@@ -618,9 +625,13 @@
  MirSurfaceState mir_surface_get_state(MirSurface *surface);
  /**
-- * Set the swapinterval for mir_surface_swap_buffers. EGL users should use
-- * eglSwapInterval directly.
-- * At the time being, only swapinterval of 0 or 1 is supported.
++ * Set the swapinterval for the default stream.
++ *   \warning EGL users should use eglSwapInterval directly.
++ *   \warning Only swapinterval of 0 or 1 is supported.
++ *   \warning If the surface was created with, or modified to have a
++ *            MirSurfaceSpec containing streams added through
++ *            mir_surface_spec_set_streams(), the default stream will
++ *            be removed, and this function will return NULL.
   *   \param [in] surface  The surface to operate on
   *   \param [in] interval The number of vblank signals that
   *                        mir_surface_swap_buffers will wait for
@@ -634,7 +645,8 @@
   * The default interval is 1.
   *   \param [in] surface  The surface to operate on
   *   \return              The swapinterval value that the client is operating with.
-- *                        Returns -1 if surface is invalid.
++ *                        Returns -1 if surface is invalid, or if the default stream
++ *                        was removed by use of mir_surface_spec_set_streams().
   */
  int mir_surface_get_swapinterval(MirSurface* surface);
 === modified file 'playground/mir_demo_client_prerendered_frames.c'
 --- playground/mir_demo_client_prerendered_frames.c	2016-06-02 05:33:50 +0000
 +++ playground/mir_demo_client_prerendered_frames.c	2016-06-08 13:06:12 +0000
@@ -161,6 +161,12 @@
      mir_surface_spec_add_presentation_chain(
          spec, width, height, displacement_x, displacement_y, chain);
      MirSurface* surface = mir_surface_create_sync(spec);
++    if (!mir_surface_is_valid(surface))
++    {
++        printf("could not create MirSurface\n");
++        return -1;
++    }
++
      mir_surface_spec_release(spec);
      int num_prerendered_frames = 20;
 === modified file 'src/client/mir_connection.cpp'
 --- src/client/mir_connection.cpp	2016-06-02 05:33:50 +0000
 +++ src/client/mir_connection.cpp	2016-06-08 13:06:12 +0000
@@ -363,8 +363,6 @@
  void MirConnection::surface_created(SurfaceCreationRequest* request)
+ {
      std::unique_lock<decltype(mutex)> lock(mutex);
--    std::shared_ptr<MirSurface> surf {nullptr};
--    std::shared_ptr<mcl::ClientBufferStream> stream {nullptr};
      //make sure this request actually was made.
      auto request_it = std::find_if(surface_requests.begin(), surface_requests.end(),
          [&request](std::shared_ptr<MirConnection::SurfaceCreationRequest> r){ return request == r.get(); });
@@ -375,32 +373,34 @@
      auto callback = request->cb;
      auto context = request->context;
      auto const& spec = request->spec;
++    std::string name{spec.surface_name.is_set() ?
++                     spec.surface_name.value() : ""};
--    try
++    std::shared_ptr<mcl::ClientBufferStream> default_stream {nullptr};
++    if (surface_proto->buffer_stream().has_id())
+     {
--        std::string name{spec.surface_name.is_set() ?
--                         spec.surface_name.value() : ""};
--
--        if (surface_proto->has_buffer_stream())
++        try
+         {
--            stream = std::make_shared<mcl::BufferStream>(
++            default_stream = std::make_shared<mcl::BufferStream>(
                  this, request->wh, server, platform, surface_map, buffer_factory,
                  surface_proto->buffer_stream(), make_perf_report(logger), name,
                  mir::geometry::Size{surface_proto->width(), surface_proto->height()}, nbuffers);
--        }
--    }
--    catch (std::exception const& error)
--    {
--        if (!surface_proto->has_error())
--            surface_proto->set_error(error.what());
--        // failed to create buffer_stream, so clean up FDs it doesn't own
--        for (auto i = 0; i < surface_proto->fd_size(); i++)
--            ::close(surface_proto->fd(i));
--        if (surface_proto->has_buffer_stream() && surface_proto->buffer_stream().has_buffer())
--            for (int i = 0; i < surface_proto->buffer_stream().buffer().fd_size(); i++)
--                ::close(surface_proto->buffer_stream().buffer().fd(i));
++            surface_map->insert(default_stream->rpc_id(), default_stream);
++        }
++        catch (std::exception const& error)
++        {
++            if (!surface_proto->has_error())
++                surface_proto->set_error(error.what());
++            // failed to create buffer_stream, so clean up FDs it doesn't own
++            for (auto i = 0; i < surface_proto->fd_size(); i++)
++                ::close(surface_proto->fd(i));
++            if (surface_proto->has_buffer_stream() && surface_proto->buffer_stream().has_buffer())
++                for (int i = 0; i < surface_proto->buffer_stream().buffer().fd_size(); i++)
++                    ::close(surface_proto->buffer_stream().buffer().fd(i));
++        }
+     }
++    std::shared_ptr<MirSurface> surf {nullptr};
      if (surface_proto->has_error() || !surface_proto->has_id())
+     {
          std::string reason;
@@ -417,12 +417,8 @@
      else
+     {
          surf = std::make_shared<MirSurface>(
--            this, server, &debug, stream, input_platform, spec, *surface_proto, request->wh);
--
++            this, server, &debug, default_stream, input_platform, spec, *surface_proto, request->wh);
          surface_map->insert(mf::SurfaceId{surface_proto->id().value()}, surf);
--
--        if (stream)
--            surface_map->insert(mf::BufferStreamId{surface_proto->buffer_stream().id().value()}, stream);
+     }
      callback(surf.get(), context);
 === modified file 'src/client/mir_surface.cpp'
 --- src/client/mir_surface.cpp	2016-05-03 06:55:25 +0000
 +++ src/client/mir_surface.cpp	2016-06-08 13:06:12 +0000
@@ -122,11 +122,15 @@
        void_response{mcl::make_protobuf_object<mir::protobuf::Void>()},
        modify_result{mcl::make_protobuf_object<mir::protobuf::Void>()},
        connection_(allocating_connection),
--      buffer_stream(buffer_stream),
++      default_stream(buffer_stream),
        input_platform(input_platform),
        keymapper(std::make_shared<mircv::XKBMapper>()),
        configure_result{mcl::make_protobuf_object<mir::protobuf::SurfaceSetting>()},
--      creation_handle(handle)
++      creation_handle(handle),
++      size(spec.width.is_set() && spec.height.is_set() ? geom::Size{spec.width.value(), spec.height.value()} : geom::Size{0,0}),
++      format(spec.pixel_format.is_set() ? spec.pixel_format.value() : mir_pixel_format_invalid),
++      usage(spec.buffer_usage.is_set() ? spec.buffer_usage.value() : mir_buffer_usage_hardware),
++      output_id(spec.output_id.is_set() ? spec.output_id.value() : static_cast<uint32_t>(mir_display_output_id_invalid))
+ {
      for(int i = 0; i < surface_proto.attributes_size(); i++)
+     {
@@ -172,7 +176,7 @@
+ {
      std::lock_guard<decltype(mutex)> lock(mutex);
--    return buffer_stream->get_parameters();
++    return {name.c_str(), size.width.as_int(), size.height.as_int(), format, usage, output_id};
+ }
  char const * MirSurface::get_error_message()
@@ -281,9 +285,9 @@
      // possible to eliminate it in the second phase of buffer
      // stream where the existing MirSurface swap interval functions
      // may be deprecated in terms of mir_buffer_stream_ alternatives
--    if (at == mir_surface_attrib_swapinterval)
++    if ((at == mir_surface_attrib_swapinterval) && default_stream)
+     {
--        buffer_stream->set_swap_interval(value);
++        default_stream->set_swap_interval(value);
          return &configure_wait_handle;
+     }
@@ -387,8 +391,8 @@
      if (at == mir_surface_attrib_swapinterval)
+     {
--        if (buffer_stream)
--            return buffer_stream->swap_interval();
++        if (default_stream)
++            return default_stream->swap_interval();
          else // Surface creation is not finalized
              return 1;
+     }
@@ -448,13 +452,10 @@
+     }
      case mir_event_type_resize:
+     {
--        if (auto_resize_stream)
--        {
--            auto resize_event = mir_event_get_resize_event(&e);
--            buffer_stream->set_size(geom::Size{
--                mir_resize_event_get_width(resize_event),
--                mir_resize_event_get_height(resize_event)});
--        }
++        auto resize_event = mir_event_get_resize_event(&e);
++        size = { mir_resize_event_get_width(resize_event), mir_resize_event_get_height(resize_event) };
++        if (default_stream)
++            default_stream->set_size(size);
          break;
+     }
      default:
@@ -507,7 +508,7 @@
+ {
      std::lock_guard<decltype(mutex)> lock(mutex);
--    return buffer_stream.get();
++    return default_stream.get();
+ }
  void MirSurface::on_modified()
@@ -601,7 +602,7 @@
      if (spec.streams.is_set())
+     {
--        auto_resize_stream = false;
++        default_stream = nullptr;
          for(auto const& stream : spec.streams.value())
+         {
              auto const new_stream = surface_specification->add_stream();
 === modified file 'src/client/mir_surface.h'
 --- src/client/mir_surface.h	2016-05-03 06:55:25 +0000
 +++ src/client/mir_surface.h	2016-06-08 13:06:12 +0000
@@ -227,7 +227,8 @@
      MirWaitHandle configure_cursor_wait_handle;
      MirWaitHandle persistent_id_wait_handle;
--    std::shared_ptr<mir::client::ClientBufferStream> buffer_stream;
++    //Deprecated functions can cause MirSurfaces to be created with a default stream
++    std::shared_ptr<mir::client::ClientBufferStream> default_stream;
      std::shared_ptr<mir::input::receiver::InputPlatform> const input_platform;
      std::shared_ptr<mir::input::receiver::XKBMapper> const keymapper;
@@ -239,11 +240,15 @@
      std::function<void(MirEvent const*)> handle_event_callback;
      std::shared_ptr<mir::dispatch::ThreadedDispatcher> input_thread;
--    bool auto_resize_stream{true};
      //a bit batty, but the creation handle has to exist for as long as the MirSurface does,
      //as we don't really manage the lifetime of MirWaitHandle sensibly.
      std::shared_ptr<MirWaitHandle> const creation_handle;
++    mir::geometry::Size size;
++    MirPixelFormat format;
++    MirBufferUsage usage;
++    uint32_t output_id;
++
  };
  #endif /* MIR_CLIENT_PRIVATE_MIR_WAIT_HANDLE_H_ */
 === modified file 'tests/acceptance-tests/test_buffer_stream_arrangement.cpp'
 --- tests/acceptance-tests/test_buffer_stream_arrangement.cpp	2016-05-18 07:27:24 +0000
 +++ tests/acceptance-tests/test_buffer_stream_arrangement.cpp	2016-06-08 13:06:12 +0000
@@ -283,3 +283,48 @@
      EXPECT_TRUE(ordering->wait_for_positions_within(positions, 5s))
           << "timed out waiting to see the compositor post the streams in the right arrangement";
+ }
++
++//LP: #1577967
++TEST_F(BufferStreamArrangement, surfaces_can_start_with_non_default_stream)
++{
++    using namespace testing;
++    std::vector<MirBufferStreamInfo> infos(streams.size());
++    auto i = 0u;
++    for (auto const& stream : streams)
++    {
++        infos[i++] = MirBufferStreamInfo{
++            stream->handle(),
++            stream->position().x.as_int(),
++            stream->position().y.as_int()};
++    }
++
++    auto spec = mir_connection_create_spec_for_normal_surface(
++        connection, 100, 100, mir_pixel_format_abgr_8888);
++    mir_surface_spec_set_streams(spec, infos.data(), infos.size());
++    auto surface = mir_surface_create_sync(spec);
++    mir_surface_spec_release(spec);
++    EXPECT_TRUE(mir_surface_is_valid(surface));
++    EXPECT_THAT(mir_surface_get_error_message(surface), StrEq(""));
++}
++
++TEST_F(BufferStreamArrangement, when_non_default_streams_are_set_surface_get_stream_gives_null)
++{
++    using namespace testing;
++    EXPECT_TRUE(mir_buffer_stream_is_valid(mir_surface_get_buffer_stream(surface)));
++
++    std::vector<MirBufferStreamInfo> infos(streams.size());
++    auto i = 0u;
++    for (auto const& stream : streams)
++    {
++        infos[i++] = MirBufferStreamInfo{
++            stream->handle(),
++            stream->position().x.as_int(),
++            stream->position().y.as_int()};
++    }
++    auto change_spec = mir_connection_create_spec_for_changes(connection);
++    mir_surface_spec_set_streams(change_spec, infos.data(), infos.size());
++    mir_surface_apply_spec(surface, change_spec);
++    mir_surface_spec_release(change_spec);
++
++    EXPECT_THAT(mir_surface_get_buffer_stream(surface), Eq(nullptr));
++}
 === modified file 'tests/unit-tests/client/test_client_mir_surface.cpp'
 --- tests/unit-tests/client/test_client_mir_surface.cpp	2016-05-03 06:55:25 +0000
 +++ tests/unit-tests/client/test_client_mir_surface.cpp	2016-06-08 13:06:12 +0000
@@ -542,3 +542,27 @@
      surface.handle_event(*ev);
      surface_map->erase(mir::frontend::BufferStreamId(2));
+ }
++
++TEST_F(MirClientSurfaceTest, parameters_are_unhooked_from_stream_sizes)
++{
++    using namespace testing;
++    auto mock_stream = std::make_shared<mtd::MockClientBufferStream>();
++    auto mock_input_platform = std::make_shared<NiceMock<MockClientInputPlatform>>();
++    ON_CALL(*mock_input_platform, create_input_receiver(_,_,_))
++        .WillByDefault(Return(std::make_shared<mt::TestDispatchable>([]{})));
++    ON_CALL(*mock_stream, rpc_id()).WillByDefault(Return(mir::frontend::BufferStreamId(2)));
++    geom::Size size(120, 124);
++    EXPECT_CALL(*mock_stream, set_size(size));
++    auto ev = mir::events::make_event(mir::frontend::SurfaceId(2), size);
++
++    MirSurface surface{connection.get(), *client_comm_channel, nullptr,
++        mock_stream, mock_input_platform, spec, surface_proto, wh};
++
++    auto params = surface.get_parameters();
++    EXPECT_THAT(params.width, Eq(spec.width.value()));
++    EXPECT_THAT(params.height, Eq(spec.height.value()));
++    surface.handle_event(*ev);
++    params = surface.get_parameters();
++    EXPECT_THAT(params.width, Eq(size.width.as_int()));
++    EXPECT_THAT(params.height, Eq(size.height.as_int()));
++}