Mir

Merge lp:~alan-griffiths/mir/BufferStreamArrangement.can_be_specified_when_creating_surface into lp:mir

BufferStreamArrangement.can_be_specified_when_creating_surface
Merge into development-branch

Proposed by Alan Griffiths on 2016-05-11

Status:	Merged
Approved by:	Alan Griffiths on 2016-05-16
Approved revision:	3509
Merged at revision:	3508
Proposed branch:	lp:~alan-griffiths/mir/BufferStreamArrangement.can_be_specified_when_creating_surface
Merge into:	lp:mir
Diff against target:	130 lines (+41/-21) 3 files modified src/client/mir_connection.cpp (+10/-5) src/server/frontend/session_mediator.cpp (+4/-5) tests/acceptance-tests/test_buffer_stream_arrangement.cpp (+27/-11)
To merge this branch:	bzr merge lp:~alan-griffiths/mir/BufferStreamArrangement.can_be_specified_when_creating_surface
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Mir CI Bot	continuous-integration		Approve on 2016-05-16
Kevin DuBois (community)		2016-05-11	Approve on 2016-05-16
Review via email: mp+294407@code.launchpad.net

Commit Message

Add BufferStreamArrangement.can_be_specified_when_creating_surface test and fix implementation to pass.

Description of the Change

In looking to fix lp:1580555 I found the code I wanted to change ins SessionMediator::create_surface() wasn't under test. Here is the missing test.

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/976/rebuild

review: Needs Fixing (continuous-integration)

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

Difficult to review right now as mir_connection.cpp changes are conflicting with those in https://code.launchpad.net/~kdub/mir/fix-1577967/+merge/294283.

lp:~alan-griffiths/mir/BufferStreamArrangement.can_be_specified_when_creating_surface updated on 2016-05-13

3508. By Alan Griffiths on 2016-05-13: merge lp:mir
3509. By Alan Griffiths on 2016-05-13: Avoid conflict with ~kdub/mir/fix-1577967 in tests/acceptance-tests/test_buffer_stream_arrangement.cpp

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

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

review: Needs Fixing (continuous-integration)

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

> Difficult to review right now as mir_connection.cpp changes are conflicting
> with those in https://code.launchpad.net/~kdub/mir/fix-1577967/+merge/294283.

The mir_connection.cpp changes here are not needed with that branch. So you can try:

$ bzr resolve --take-this src/client/mir_connection.cpp

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

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

review: Needs Fixing (continuous-integration)

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

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

review: Approve (continuous-integration)

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

lgtm, more-or-less a subset of what is needed by fix-1577967 (which tacks on some further clarifications)

review: Approve

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

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/986/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:

Alan Griffiths

Emanuele Antonio Faraone

Gerry Boland

Mir development team

 === modified file 'src/client/mir_connection.cpp'
 --- src/client/mir_connection.cpp	2016-05-09 14:27:58 +0000
 +++ src/client/mir_connection.cpp	2016-05-13 08:57:27 +0000
@@ -381,10 +381,13 @@
          std::string name{spec.surface_name.is_set() ?
                           spec.surface_name.value() : ""};
--        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);
++        if (surface_proto->has_buffer_stream())
++        {
++            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)
+     {
@@ -417,7 +420,9 @@
              this, server, &debug, stream, input_platform, spec, *surface_proto, request->wh);
          surface_map->insert(mf::SurfaceId{surface_proto->id().value()}, surf);
--        surface_map->insert(mf::BufferStreamId{surface_proto->buffer_stream().id().value()}, stream);
++
++        if (stream)
++            surface_map->insert(mf::BufferStreamId{surface_proto->buffer_stream().id().value()}, stream);
+     }
      callback(surf.get(), context);
 === modified file 'src/server/frontend/session_mediator.cpp'
 --- src/server/frontend/session_mediator.cpp	2016-05-10 10:48:28 +0000
 +++ src/server/frontend/session_mediator.cpp	2016-05-13 08:57:27 +0000
@@ -335,19 +335,15 @@
      auto const surf_id = shell->create_surface(session, params, sink);
      auto surface = session->get_surface(surf_id);
--    auto stream = session->get_buffer_stream(buffer_stream_id);
      auto const& client_size = surface->client_size();
      response->mutable_id()->set_value(surf_id.as_value());
      response->set_width(client_size.width.as_uint32_t());
      response->set_height(client_size.height.as_uint32_t());
      // TODO: Deprecate
--    response->set_pixel_format(stream->pixel_format());
++    response->set_pixel_format(request->pixel_format());
      response->set_buffer_usage(request->buffer_usage());
--    response->mutable_buffer_stream()->set_pixel_format(stream->pixel_format());
--    response->mutable_buffer_stream()->set_buffer_usage(request->buffer_usage());
--
      if (surface->supports_input())
          response->add_fd(surface->client_input_fd());
@@ -364,6 +360,9 @@
+     {
          buffer_stream_tracker.set_default_stream(surf_id, buffer_stream_id);
          response->mutable_buffer_stream()->mutable_id()->set_value(buffer_stream_id.as_value());
++        response->mutable_buffer_stream()->set_pixel_format(legacy_stream->pixel_format());
++        response->mutable_buffer_stream()->set_buffer_usage(request->buffer_usage());
++
          advance_buffer(buffer_stream_id, *legacy_stream, buffer_stream_tracker.last_buffer(buffer_stream_id),
              [this, buffering_sender, response, done, session]
              (graphics::Buffer* client_buffer, graphics::BufferIpcMsgType msg_type)
 === modified file 'tests/acceptance-tests/test_buffer_stream_arrangement.cpp'
 --- tests/acceptance-tests/test_buffer_stream_arrangement.cpp	2016-01-29 08:18:22 +0000
 +++ tests/acceptance-tests/test_buffer_stream_arrangement.cpp	2016-05-13 08:57:27 +0000
@@ -66,15 +66,6 @@
  struct Stream
+ {
--    Stream(MirBufferStream* stream, geom::Point pt, geom::Size sz) :
--        stream(stream),
--        pos{pt},
--        stream_size{sz},
--        needs_release{false}
--    {
--        mir_buffer_stream_swap_buffers_sync(stream);
--    }
--
      Stream(MirConnection* connection, geom::Rectangle rect) :
          stream(mir_connection_create_buffer_stream_sync(
              connection,
@@ -212,8 +203,7 @@
          ConnectedClientWithASurface::SetUp();
          server.the_cursor()->hide();
--        streams.emplace_back(
--            std::make_unique<Stream>(mir_surface_get_buffer_stream(surface), geom::Point{0,0}, surface_size));
++        streams.emplace_back(std::make_unique<Stream>(connection, geom::Rectangle{geom::Point{0,0}, surface_size}));
          int const additional_streams{3};
          for (auto i = 0; i < additional_streams; i++)
+         {
@@ -235,6 +225,32 @@
  };
+ }
++TEST_F(BufferStreamArrangement, can_be_specified_when_creating_surface)
++{
++    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()};
++    }
++
++    mir_surface_release_sync(surface);
++
++    auto const spec = mir_connection_create_spec_for_normal_surface(
++        connection, surface_size.width.as_int(), surface_size.height.as_int(), mir_pixel_format_abgr_8888);
++    mir_surface_spec_set_name(spec, "BufferStreamArrangement.can_be_specified_when_creating_surface");
++    mir_surface_spec_set_buffer_usage(spec, mir_buffer_usage_hardware);
++    mir_surface_spec_set_streams(spec, infos.data(), infos.size());
++
++    surface = mir_surface_create_sync(spec);
++    mir_surface_spec_release(spec);
++    EXPECT_TRUE(mir_surface_is_valid(surface)) << mir_surface_get_error_message(surface);
++}
++
  TEST_F(BufferStreamArrangement, arrangements_are_applied)
+ {
      using namespace testing;