Mir

Merge lp:~afrantzis/mir/fix-out-of-order-buffers-1216472 into lp:~mir-team/mir/trunk

Proposed by Alexandros Frantzis
Status: Superseded
Proposed branch: lp:~afrantzis/mir/fix-out-of-order-buffers-1216472
Merge into: lp:~mir-team/mir/trunk
Diff against target: 187 lines (+78/-29)
3 files modified
include/server/mir/frontend/session_mediator.h (+7/-2)
src/server/frontend/session_mediator.cpp (+33/-27)
tests/unit-tests/frontend/test_session_mediator.cpp (+38/-0)
To merge this branch: bzr merge lp:~afrantzis/mir/fix-out-of-order-buffers-1216472
Reviewer Review Type Date Requested Status
Mir development team Pending
Review via email: mp+187840@code.launchpad.net

This proposal has been superseded by a proposal from 2013-09-26.

Commit message

frontend: Properly track buffers per-surface (not per session)

Description of the change

frontend: Properly track buffers per-surface (not per session)

To post a comment you must log in.

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/server/mir/frontend/session_mediator.h'
2--- include/server/mir/frontend/session_mediator.h 2013-08-28 03:41:48 +0000
3+++ include/server/mir/frontend/session_mediator.h 2013-09-26 16:30:51 +0000
4@@ -21,8 +21,9 @@
5 #define MIR_FRONTEND_SESSION_MEDIATOR_H_
6
7 #include "mir_protobuf.pb.h"
8+#include "mir/frontend/surface_id.h"
9
10-#include <map>
11+#include <unordered_map>
12 #include <memory>
13 #include <mutex>
14
15@@ -108,6 +109,10 @@
16 google::protobuf::Closure* done) override;
17
18 private:
19+ void pack_protobuf_buffer(protobuf::Buffer& protobuf_buffer,
20+ std::shared_ptr<graphics::Buffer> const& graphics_buffer,
21+ bool need_full_ipc);
22+
23 std::shared_ptr<Shell> const shell;
24 std::shared_ptr<graphics::Platform> const graphics_platform;
25
26@@ -119,7 +124,7 @@
27 std::shared_ptr<EventSink> const event_sink;
28 std::shared_ptr<ResourceCache> const resource_cache;
29
30- std::shared_ptr<graphics::Buffer> client_buffer_resource;
31+ std::unordered_map<SurfaceId,std::shared_ptr<graphics::Buffer>> client_buffer_resource;
32
33 std::mutex session_mutex;
34 std::weak_ptr<Session> weak_session;
35
36=== modified file 'src/server/frontend/session_mediator.cpp'
37--- src/server/frontend/session_mediator.cpp 2013-09-24 07:02:12 +0000
38+++ src/server/frontend/session_mediator.cpp 2013-09-26 16:30:51 +0000
39@@ -126,15 +126,15 @@
40
41 report->session_create_surface_called(session->name());
42
43- auto const id = session->create_surface(msh::SurfaceCreationParameters()
44+ auto const surf_id = session->create_surface(msh::SurfaceCreationParameters()
45 .of_name(request->surface_name())
46 .of_size(request->width(), request->height())
47 .of_buffer_usage(static_cast<graphics::BufferUsage>(request->buffer_usage()))
48 .of_pixel_format(static_cast<geometry::PixelFormat>(request->pixel_format()))
49 .with_output_id(graphics::DisplayConfigurationOutputId(request->output_id())));
50 {
51- auto surface = session->get_surface(id);
52- response->mutable_id()->set_value(id.as_value());
53+ auto surface = session->get_surface(surf_id);
54+ response->mutable_id()->set_value(surf_id.as_value());
55 response->set_width(surface->size().width.as_uint32_t());
56 response->set_height(surface->size().height.as_uint32_t());
57 response->set_pixel_format((int)surface->pixel_format());
58@@ -144,17 +144,11 @@
59 response->add_fd(surface->client_input_fd());
60
61 bool need_full_ipc;
62- client_buffer_resource = surface->advance_client_buffer(need_full_ipc);
63- auto const& id = client_buffer_resource->id();
64+ auto client_buffer = surface->advance_client_buffer(need_full_ipc);
65+ client_buffer_resource[surf_id] = client_buffer;
66
67 auto buffer = response->mutable_buffer();
68- buffer->set_buffer_id(id.as_uint32_t());
69-
70- if (need_full_ipc)
71- {
72- auto packer = std::make_shared<mfd::ProtobufBufferPacker>(buffer);
73- graphics_platform->fill_ipc_package(packer, client_buffer_resource);
74- }
75+ pack_protobuf_buffer(*buffer, client_buffer, need_full_ipc);
76 }
77
78 // TODO: NOTE: We use the ordering here to ensure the shell acts on the surface after the surface ID is sent over the wire.
79@@ -171,7 +165,10 @@
80 ::mir::protobuf::Buffer* response,
81 ::google::protobuf::Closure* done)
82 {
83- bool needs_full_ipc;
84+ bool need_full_ipc;
85+ SurfaceId const surf_id{request->value()};
86+ std::shared_ptr<graphics::Buffer> client_buffer;
87+
88 {
89 std::unique_lock<std::mutex> lock(session_mutex);
90
91@@ -188,20 +185,15 @@
92 // outputs.
93 display_changer->ensure_display_powered(session);
94
95- auto surface = session->get_surface(SurfaceId(request->value()));
96-
97- client_buffer_resource.reset();
98- client_buffer_resource = surface->advance_client_buffer(needs_full_ipc);
99- }
100-
101- auto const& id = client_buffer_resource->id();
102- response->set_buffer_id(id.as_uint32_t());
103-
104- if (needs_full_ipc)
105- {
106- auto packer = std::make_shared<mfd::ProtobufBufferPacker>(response);
107- graphics_platform->fill_ipc_package(packer, client_buffer_resource);
108- }
109+ auto surface = session->get_surface(surf_id);
110+
111+ client_buffer_resource[surf_id].reset();
112+ client_buffer = surface->advance_client_buffer(need_full_ipc);
113+ client_buffer_resource[surf_id] = client_buffer;
114+ }
115+
116+ pack_protobuf_buffer(*response, client_buffer, need_full_ipc);
117+
118 done->Run();
119 }
120
121@@ -316,3 +308,17 @@
122 }
123 done->Run();
124 }
125+
126+void mf::SessionMediator::pack_protobuf_buffer(
127+ protobuf::Buffer& protobuf_buffer,
128+ std::shared_ptr<graphics::Buffer> const& graphics_buffer,
129+ bool need_full_ipc)
130+{
131+ protobuf_buffer.set_buffer_id(graphics_buffer->id().as_uint32_t());
132+
133+ if (need_full_ipc)
134+ {
135+ auto packer = std::make_shared<mfd::ProtobufBufferPacker>(&protobuf_buffer);
136+ graphics_platform->fill_ipc_package(packer, graphics_buffer);
137+ }
138+}
139
140=== modified file 'tests/unit-tests/frontend/test_session_mediator.cpp'
141--- tests/unit-tests/frontend/test_session_mediator.cpp 2013-09-12 21:36:55 +0000
142+++ tests/unit-tests/frontend/test_session_mediator.cpp 2013-09-26 16:30:51 +0000
143@@ -545,6 +545,44 @@
144 mediator.disconnect(nullptr, nullptr, nullptr, null_callback.get());
145 }
146
147+TEST_F(SessionMediatorTest, buffer_resource_for_surface_held_over_operations_on_other_surfaces)
148+{
149+ using namespace testing;
150+
151+ auto stub_buffer1 = std::make_shared<mtd::StubBuffer>();
152+
153+ mp::ConnectParameters connect_parameters;
154+ mp::Connection connection;
155+
156+ mediator.connect(nullptr, &connect_parameters, &connection, null_callback.get());
157+ mp::SurfaceParameters surface_request;
158+ mp::Surface surface_response;
159+
160+ /*
161+ * Note that the surface created by the first create_surface() call is
162+ * the pre-created stubbed_session->mock_surface. Further create_surface()
163+ * invocations create new surfaces in stubbed_session->mock_surfaces[].
164+ */
165+ EXPECT_CALL(*stubbed_session->mock_surface, advance_client_buffer())
166+ .WillOnce(Return(stub_buffer1));
167+
168+ mediator.create_surface(nullptr, &surface_request, &surface_response, null_callback.get());
169+ auto refcount = stub_buffer1.use_count();
170+
171+ /* Creating a new surface should not affect other surfaces' buffers */
172+ mediator.create_surface(nullptr, &surface_request, &surface_response, null_callback.get());
173+ EXPECT_EQ(refcount, stub_buffer1.use_count());
174+
175+ mp::SurfaceId buffer_request{surface_response.id()};
176+ mp::Buffer buffer_response;
177+
178+ /* Getting the next buffer of a surface should not affect other surfaces' buffers */
179+ mediator.next_buffer(nullptr, &buffer_request, &buffer_response, null_callback.get());
180+ EXPECT_EQ(refcount, stub_buffer1.use_count());
181+
182+ mediator.disconnect(nullptr, nullptr, nullptr, null_callback.get());
183+}
184+
185 TEST_F(SessionMediatorTest, display_config_request)
186 {
187 using namespace testing;

Subscribers

People subscribed via source and target branches