Mir

Merge lp:~cemil-azizoglu/mir/fix-perf-report-surface-name-lp1415305 into lp:mir

Proposed by Cemil Azizoglu
Status: Merged
Approved by: Cemil Azizoglu
Approved revision: no longer in the source branch.
Merged at revision: 2291
Proposed branch: lp:~cemil-azizoglu/mir/fix-perf-report-surface-name-lp1415305
Merge into: lp:mir
Diff against target: 340 lines (+81/-44)
12 files modified
src/client/buffer_stream.cpp (+4/-17)
src/client/buffer_stream.h (+2/-1)
src/client/client_buffer_stream_factory.h (+2/-2)
src/client/default_client_buffer_stream_factory.cpp (+26/-4)
src/client/default_client_buffer_stream_factory.h (+3/-3)
src/client/mir_screencast.cpp (+1/-1)
src/client/mir_surface.cpp (+1/-1)
tests/include/mir_test_doubles/mock_client_buffer_stream_factory.h (+4/-4)
tests/include/mir_test_doubles/stub_client_buffer_stream_factory.h (+4/-2)
tests/unit-tests/client/test_client_buffer_stream.cpp (+32/-7)
tests/unit-tests/client/test_client_mir_surface.cpp (+1/-1)
tests/unit-tests/client/test_mir_screencast.cpp (+1/-1)
To merge this branch: bzr merge lp:~cemil-azizoglu/mir/fix-perf-report-surface-name-lp1415305
Reviewer Review Type Date Requested Status
Alan Griffiths Approve
PS Jenkins bot (community) continuous-integration Approve
Alberto Aguirre (community) Approve
Kevin DuBois (community) Approve
Daniel van Vugt Approve
Review via email: mp+248034@code.launchpad.net

Commit message

client: Sets the client perf report surface name correctly. (LP: #1415305)

Description of the change

Sets the client perf report surface name correctly. (LP: #1415305)

E.g. multiwin example :

Without the fix:
[1422471924.629395] perf: 1: ...
[1422471924.630274] perf: 2: ...
[1422471924.631150] perf: 0: ...

With the fix:
[1422556976.376920] perf: green: ...
[1422556976.377852] perf: blue: ...
[1422556976.378705] perf: red: ...

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

failure is lp:1416317 (Alexandros has landed fix)

There really ought to be an automated test that the expected surface name is passed to the report.

review: Needs Fixing
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Confirmed by manual testing the bug is fixed.

I'm open to finding a nicer solution in future too. Although if we look to the future and find that a window title string is separate to the surface "name" then maybe this is the proper right answer to give a buffer stream a name.

If it's possible to test without major trouble then do. But if this 200 line diff starts to approach 1000 then it's probably not worth the effort (present and future maintenance) to test the surface naming bit.

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Kevin DuBois (kdub) wrote :

okay by me

nit:
105 + if (report_target && !strcmp(report_target, "log"))
may as well switch to strncmp if we're moving it.

308 + const char* const name = "a_unique_surface_name";
std::string would avoid the implicit conversion.

review: Approve
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

LGTM.

review: Approve
Revision history for this message
Cemil Azizoglu (cemil-azizoglu) wrote :

> okay by me
>
> nit:
> 105 + if (report_target && !strcmp(report_target, "log"))
> may as well switch to strncmp if we're moving it.
>
> 308 + const char* const name = "a_unique_surface_name";
> std::string would avoid the implicit conversion.

Addressed.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/client/buffer_stream.cpp'
2--- src/client/buffer_stream.cpp 2015-01-20 22:23:20 +0000
3+++ src/client/buffer_stream.cpp 2015-02-04 08:46:38 +0000
4@@ -77,20 +77,6 @@
5 }
6 }
7
8-std::shared_ptr<mcl::PerfReport>
9-make_perf_report(std::shared_ptr<ml::Logger> const& logger)
10-{
11- // TODO: It seems strange that this directly uses getenv
12- const char* report_target = getenv("MIR_CLIENT_PERF_REPORT");
13- if (report_target && !strcmp(report_target, "log"))
14- {
15- return std::make_shared<mir::client::logging::PerfReport>(logger);
16- }
17- else
18- {
19- return std::make_shared<mir::client::NullPerfReport>();
20- }
21-}
22 }
23
24 // TODO: It seems like a bit of a wart that we have to pass the Logger specifically here...perhaps
25@@ -101,14 +87,15 @@
26 std::shared_ptr<mcl::ClientBufferFactory> const& buffer_factory,
27 std::shared_ptr<mcl::EGLNativeWindowFactory> const& native_window_factory,
28 mp::BufferStream const& protobuf_bs,
29- std::shared_ptr<ml::Logger> const& logger)
30+ std::shared_ptr<mcl::PerfReport> const& perf_report,
31+ std::string const& surface_name)
32 : display_server(server),
33 mode(mode),
34 native_window_factory(native_window_factory),
35 protobuf_bs(protobuf_bs),
36 buffer_depository{buffer_factory, mir::frontend::client_buffer_cache_size},
37 swap_interval_(1),
38- perf_report(make_perf_report(logger))
39+ perf_report(perf_report)
40
41 {
42 if (!protobuf_bs.has_id() || protobuf_bs.has_error())
43@@ -119,7 +106,7 @@
44 process_buffer(protobuf_bs.buffer());
45 egl_native_window_ = native_window_factory->create_egl_native_window(this);
46
47- perf_report->name_surface(std::to_string(protobuf_bs.id().value()).c_str());
48+ perf_report->name_surface(surface_name.c_str());
49 }
50
51 mcl::BufferStream::~BufferStream()
52
53=== modified file 'src/client/buffer_stream.h'
54--- src/client/buffer_stream.h 2015-01-20 22:23:20 +0000
55+++ src/client/buffer_stream.h 2015-02-04 08:46:38 +0000
56@@ -62,7 +62,8 @@
57 std::shared_ptr<ClientBufferFactory> const& buffer_factory,
58 std::shared_ptr<EGLNativeWindowFactory> const& native_window_factory,
59 protobuf::BufferStream const& protobuf_bs,
60- std::shared_ptr<logging::Logger> const& logger);
61+ std::shared_ptr<PerfReport> const& perf_report,
62+ std::string const& surface_name);
63 virtual ~BufferStream();
64
65 MirWaitHandle* next_buffer(std::function<void()> const& done) override;
66
67=== modified file 'src/client/client_buffer_stream_factory.h'
68--- src/client/client_buffer_stream_factory.h 2015-01-13 23:20:12 +0000
69+++ src/client/client_buffer_stream_factory.h 2015-02-04 08:46:38 +0000
70@@ -32,9 +32,9 @@
71 {
72 public:
73 virtual std::shared_ptr<ClientBufferStream> make_consumer_stream(protobuf::DisplayServer& server,
74- protobuf::BufferStream const& protobuf_bs) = 0;
75+ protobuf::BufferStream const& protobuf_bs, std::string const& surface_name) = 0;
76 virtual std::shared_ptr<ClientBufferStream> make_producer_stream(protobuf::DisplayServer& server,
77- protobuf::BufferStream const& protobuf_bs) = 0;
78+ protobuf::BufferStream const& protobuf_bs, std::string const& surface_name) = 0;
79
80 protected:
81 ClientBufferStreamFactory() = default;
82
83=== modified file 'src/client/default_client_buffer_stream_factory.cpp'
84--- src/client/default_client_buffer_stream_factory.cpp 2015-01-15 20:27:16 +0000
85+++ src/client/default_client_buffer_stream_factory.cpp 2015-02-04 08:46:38 +0000
86@@ -17,11 +17,33 @@
87 */
88 #include "default_client_buffer_stream_factory.h"
89 #include "buffer_stream.h"
90+#include "perf_report.h"
91+#include "logging/perf_report.h"
92
93 namespace mcl = mir::client;
94 namespace ml = mir::logging;
95 namespace mp = mir::protobuf;
96
97+namespace
98+{
99+
100+std::shared_ptr<mcl::PerfReport>
101+make_perf_report(std::shared_ptr<ml::Logger> const& logger)
102+{
103+ // TODO: It seems strange that this directly uses getenv
104+ const char* report_target = getenv("MIR_CLIENT_PERF_REPORT");
105+ if (report_target && !strncmp(report_target, "log", strlen(report_target)))
106+ {
107+ return std::make_shared<mcl::logging::PerfReport>(logger);
108+ }
109+ else
110+ {
111+ return std::make_shared<mcl::NullPerfReport>();
112+ }
113+}
114+
115+}
116+
117 mcl::DefaultClientBufferStreamFactory::DefaultClientBufferStreamFactory(std::shared_ptr<mcl::ClientBufferFactory> const& client_buffer_factory,
118 std::shared_ptr<mcl::EGLNativeWindowFactory> const& native_window_factory,
119 std::shared_ptr<ml::Logger> const& logger)
120@@ -32,15 +54,15 @@
121 }
122
123 std::shared_ptr<mcl::ClientBufferStream> mcl::DefaultClientBufferStreamFactory::make_consumer_stream(mp::DisplayServer& server,
124- mp::BufferStream const& protobuf_bs)
125+ mp::BufferStream const& protobuf_bs, std::string const& surface_name)
126 {
127 return std::make_shared<mcl::BufferStream>(server, mcl::BufferStreamMode::Consumer, client_buffer_factory,
128- native_window_factory, protobuf_bs, logger);
129+ native_window_factory, protobuf_bs, make_perf_report(logger), surface_name);
130 }
131
132 std::shared_ptr<mcl::ClientBufferStream> mcl::DefaultClientBufferStreamFactory::make_producer_stream(mp::DisplayServer& server,
133- mp::BufferStream const& protobuf_bs)
134+ mp::BufferStream const& protobuf_bs, std::string const& surface_name)
135 {
136 return std::make_shared<mcl::BufferStream>(server, mcl::BufferStreamMode::Producer, client_buffer_factory,
137- native_window_factory, protobuf_bs, logger);
138+ native_window_factory, protobuf_bs, make_perf_report(logger), surface_name);
139 }
140
141=== modified file 'src/client/default_client_buffer_stream_factory.h'
142--- src/client/default_client_buffer_stream_factory.h 2015-01-14 22:16:09 +0000
143+++ src/client/default_client_buffer_stream_factory.h 2015-02-04 08:46:38 +0000
144@@ -41,15 +41,15 @@
145 virtual ~DefaultClientBufferStreamFactory() = default;
146
147 std::shared_ptr<ClientBufferStream> make_consumer_stream(protobuf::DisplayServer& server,
148- protobuf::BufferStream const& protobuf_bs);
149+ protobuf::BufferStream const& protobuf_bs, std::string const& surface_name);
150 std::shared_ptr<ClientBufferStream> make_producer_stream(protobuf::DisplayServer& server,
151- protobuf::BufferStream const& protobuf_bs);
152+ protobuf::BufferStream const& protobuf_bs, std::string const& surface_name);
153
154 private:
155 std::shared_ptr<ClientBufferFactory> const client_buffer_factory;
156 std::shared_ptr<EGLNativeWindowFactory> const native_window_factory;
157 std::shared_ptr<logging::Logger> const logger;
158-
159+
160 };
161 }
162 }
163
164=== modified file 'src/client/mir_screencast.cpp'
165--- src/client/mir_screencast.cpp 2015-01-27 03:02:22 +0000
166+++ src/client/mir_screencast.cpp 2015-02-04 08:46:38 +0000
167@@ -143,7 +143,7 @@
168 if (!protobuf_screencast.has_error())
169 {
170 buffer_stream = buffer_stream_factory->make_consumer_stream(server,
171- protobuf_screencast.buffer_stream());
172+ protobuf_screencast.buffer_stream(), "MirScreencast");
173 }
174
175 callback(this, context);
176
177=== modified file 'src/client/mir_surface.cpp'
178--- src/client/mir_surface.cpp 2015-02-02 12:18:18 +0000
179+++ src/client/mir_surface.cpp 2015-02-04 08:46:38 +0000
180@@ -269,7 +269,7 @@
181 std::lock_guard<decltype(mutex)> lock(mutex);
182
183 buffer_stream = buffer_stream_factory->
184- make_producer_stream(*server, surface.buffer_stream());
185+ make_producer_stream(*server, surface.buffer_stream(), name);
186
187 for(int i = 0; i < surface.attributes_size(); i++)
188 {
189
190=== modified file 'tests/include/mir_test_doubles/mock_client_buffer_stream_factory.h'
191--- tests/include/mir_test_doubles/mock_client_buffer_stream_factory.h 2015-01-15 21:45:48 +0000
192+++ tests/include/mir_test_doubles/mock_client_buffer_stream_factory.h 2015-02-04 08:46:38 +0000
193@@ -30,10 +30,10 @@
194
195 struct MockClientBufferStreamFactory : public client::ClientBufferStreamFactory
196 {
197- MOCK_METHOD2(make_consumer_stream, std::shared_ptr<client::ClientBufferStream>(protobuf::DisplayServer&,
198- protobuf::BufferStream const&));
199- MOCK_METHOD2(make_producer_stream, std::shared_ptr<client::ClientBufferStream>(protobuf::DisplayServer&,
200- protobuf::BufferStream const&));
201+ MOCK_METHOD3(make_consumer_stream, std::shared_ptr<client::ClientBufferStream>(protobuf::DisplayServer&,
202+ protobuf::BufferStream const&, std::string const&));
203+ MOCK_METHOD3(make_producer_stream, std::shared_ptr<client::ClientBufferStream>(protobuf::DisplayServer&,
204+ protobuf::BufferStream const&, std::string const&));
205 };
206
207 }
208
209=== modified file 'tests/include/mir_test_doubles/stub_client_buffer_stream_factory.h'
210--- tests/include/mir_test_doubles/stub_client_buffer_stream_factory.h 2015-01-13 23:20:12 +0000
211+++ tests/include/mir_test_doubles/stub_client_buffer_stream_factory.h 2015-02-04 08:46:38 +0000
212@@ -32,14 +32,16 @@
213 {
214 std::shared_ptr<client::ClientBufferStream> make_consumer_stream(
215 protobuf::DisplayServer& /* server */,
216- protobuf::BufferStream const& /* protobuf_bs */)
217+ protobuf::BufferStream const& /* protobuf_bs */,
218+ std::string const& /* surface_name */)
219 {
220 return nullptr;
221 }
222
223 std::shared_ptr<client::ClientBufferStream> make_producer_stream(
224 protobuf::DisplayServer& /* server */,
225- protobuf::BufferStream const& /* protobuf_bs */)
226+ protobuf::BufferStream const& /* protobuf_bs */,
227+ std::string const& /* surface_name */)
228 {
229 return nullptr;
230 }
231
232=== modified file 'tests/unit-tests/client/test_client_buffer_stream.cpp'
233--- tests/unit-tests/client/test_client_buffer_stream.cpp 2015-01-20 22:27:26 +0000
234+++ tests/unit-tests/client/test_client_buffer_stream.cpp 2015-02-04 08:46:38 +0000
235@@ -17,6 +17,7 @@
236 */
237
238 #include "src/client/buffer_stream.h"
239+#include "src/client/perf_report.h"
240
241 #include "mir/egl_native_window_factory.h"
242
243@@ -69,6 +70,13 @@
244 static EGLNativeWindowType egl_native_window;
245 };
246
247+struct MockPerfReport : public mcl::PerfReport
248+{
249+ MOCK_METHOD1(name_surface, void(char const*));
250+ MOCK_METHOD1(begin_frame, void(int));
251+ MOCK_METHOD1(end_frame, void(int));
252+};
253+
254 struct MockClientBuffer : public mtd::NullClientBuffer
255 {
256 MOCK_METHOD0(secure_for_cpu_write, std::shared_ptr<mcl::MemoryRegion>());
257@@ -88,8 +96,8 @@
258 MirPixelFormat const default_pixel_format = mir_pixel_format_argb_8888;
259 MirBufferUsage const default_buffer_usage = mir_buffer_usage_hardware;
260
261- std::shared_ptr<ml::Logger> const logger = std::make_shared<mtd::NullLogger>();
262-
263+ std::shared_ptr<mcl::PerfReport> const perf_report = std::make_shared<mcl::NullPerfReport>();
264+
265 std::shared_ptr<mcl::BufferStream> make_buffer_stream(mp::BufferStream const& protobuf_bs,
266 mcl::BufferStreamMode mode=mcl::BufferStreamMode::Producer)
267 {
268@@ -100,7 +108,7 @@
269 mcl::BufferStreamMode mode=mcl::BufferStreamMode::Producer)
270 {
271 return std::make_shared<mcl::BufferStream>(mock_protobuf_server, mode, mt::fake_shared(buffer_factory),
272- mt::fake_shared(stub_native_window_factory), protobuf_bs, logger);
273+ mt::fake_shared(stub_native_window_factory), protobuf_bs, perf_report, "");
274 }
275 };
276
277@@ -116,13 +124,13 @@
278
279 /* assemble buffers */
280 mb->set_fds_on_side_channel(buffer_package.fd_items);
281- for (int i=0; i< buffer_package.data_items; i++)
282+ for (int i=0; i<buffer_package.data_items; i++)
283 {
284- mb->add_data(buffer_package.data[i]);
285+ mb->add_data(buffer_package.data[i]);
286 }
287- for (int i=0; i< buffer_package.fd_items; i++)
288+ for (int i=0; i<buffer_package.fd_items; i++)
289 {
290- mb->add_fd(buffer_package.fd[i]);
291+ mb->add_fd(buffer_package.fd[i]);
292 }
293 mb->set_stride(buffer_package.stride);
294 mb->set_width(buffer_package.width);
295@@ -402,3 +410,20 @@
296
297 EXPECT_EQ(&expected_memory_region, bs->secure_for_cpu_write().get());
298 }
299+
300+TEST_F(ClientBufferStreamTest, passes_name_to_perf_report)
301+{
302+ using namespace ::testing;
303+
304+ MirBufferPackage buffer_package = a_buffer_package();
305+ auto protobuf_bs = a_protobuf_buffer_stream(default_pixel_format, default_buffer_usage, buffer_package);
306+
307+ NiceMock<MockPerfReport> mock_perf_report;
308+ std::string const name = "a_unique_surface_name";
309+
310+ EXPECT_CALL(mock_perf_report, name_surface(StrEq(name))).Times(1);
311+
312+ auto bs = std::make_shared<mcl::BufferStream>(mock_protobuf_server, mcl::BufferStreamMode::Producer,
313+ mt::fake_shared(stub_client_buffer_factory), mt::fake_shared(stub_native_window_factory),
314+ protobuf_bs, mt::fake_shared(mock_perf_report), name);
315+}
316
317=== modified file 'tests/unit-tests/client/test_client_mir_surface.cpp'
318--- tests/unit-tests/client/test_client_mir_surface.cpp 2015-02-03 15:07:44 +0000
319+++ tests/unit-tests/client/test_client_mir_surface.cpp 2015-02-04 08:46:38 +0000
320@@ -450,7 +450,7 @@
321 EXPECT_CALL(mock_bs, next_buffer(_)).Times(1);
322
323 mtd::MockClientBufferStreamFactory bs_factory;
324- EXPECT_CALL(bs_factory, make_producer_stream(_,_))
325+ EXPECT_CALL(bs_factory, make_producer_stream(_,_,_))
326 .Times(1).WillOnce(Return(mt::fake_shared(mock_bs)));
327
328 auto const surface = create_and_wait_for_surface_with(*client_comm_channel, mt::fake_shared(bs_factory));
329
330=== modified file 'tests/unit-tests/client/test_mir_screencast.cpp'
331--- tests/unit-tests/client/test_mir_screencast.cpp 2015-01-27 03:02:22 +0000
332+++ tests/unit-tests/client/test_mir_screencast.cpp 2015-02-04 08:46:38 +0000
333@@ -169,7 +169,7 @@
334 using namespace ::testing;
335
336 ON_CALL(*mock_buffer_stream_factory,
337- make_consumer_stream(_,_)).WillByDefault(
338+ make_consumer_stream(_,_,_)).WillByDefault(
339 Return(mt::fake_shared(mock_bs)));
340 }
341

Subscribers

People subscribed via source and target branches