Mir

Merge lp:~vanvugt/mir/fix-buffers_ready_for_compositor into lp:mir

Proposed by Daniel van Vugt on 2015-01-21
Status: Merged
Approved by: Daniel van Vugt on 2015-02-03
Approved revision: 2140
Merged at revision: 2289
Proposed branch: lp:~vanvugt/mir/fix-buffers_ready_for_compositor
Merge into: lp:mir
Diff against target: 874 lines (+297/-63)
34 files modified
examples/render_overlays.cpp (+0/-5)
include/platform/mir/graphics/renderable.h (+0/-1)
include/server/mir/compositor/scene.h (+10/-0)
include/server/mir/scene/surface.h (+1/-0)
platform-ABI-sha1sums (+1/-1)
server-ABI-sha1sums (+3/-3)
src/include/server/mir/compositor/buffer_stream.h (+1/-1)
src/server/compositor/buffer_bundle.h (+1/-1)
src/server/compositor/buffer_queue.cpp (+10/-6)
src/server/compositor/buffer_queue.h (+1/-1)
src/server/compositor/buffer_stream_surfaces.cpp (+2/-2)
src/server/compositor/buffer_stream_surfaces.h (+1/-1)
src/server/compositor/multi_threaded_compositor.cpp (+12/-1)
src/server/graphics/software_cursor.cpp (+0/-5)
src/server/input/touchspot_controller.cpp (+0/-5)
src/server/scene/basic_surface.cpp (+12/-4)
src/server/scene/basic_surface.h (+1/-0)
src/server/scene/surface_stack.cpp (+31/-2)
src/server/scene/surface_stack.h (+2/-0)
src/server/symbols.map (+1/-0)
tests/include/mir_test_doubles/fake_renderable.h (+0/-5)
tests/include/mir_test_doubles/mock_buffer_bundle.h (+1/-1)
tests/include/mir_test_doubles/mock_buffer_stream.h (+3/-3)
tests/include/mir_test_doubles/mock_renderable.h (+0/-3)
tests/include/mir_test_doubles/mock_scene.h (+3/-0)
tests/include/mir_test_doubles/stub_buffer_stream.h (+4/-1)
tests/include/mir_test_doubles/stub_renderable.h (+0/-5)
tests/include/mir_test_doubles/stub_scene.h (+5/-1)
tests/include/mir_test_doubles/stub_scene_surface.h (+1/-0)
tests/integration-tests/test_exchange_buffer.cpp (+1/-1)
tests/integration-tests/test_surface_stack_with_compositor.cpp (+5/-4)
tests/unit-tests/compositor/test_buffer_queue.cpp (+66/-0)
tests/unit-tests/compositor/test_multi_threaded_compositor.cpp (+85/-0)
tests/unit-tests/scene/test_surface_stack.cpp (+33/-0)
To merge this branch: bzr merge lp:~vanvugt/mir/fix-buffers_ready_for_compositor
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Approve on 2015-02-03
Kevin DuBois (community) Approve on 2015-02-02
Alan Griffiths Abstain on 2015-01-27
Alberto Aguirre 2015-01-21 Approve on 2015-01-22
Review via email: mp+247124@code.launchpad.net

Commit Message

Fix dangerous underestimate of buffers_ready_for_compositor(). This is
an immediate problem when you start experimenting with double buffers,
and see occasional random freezes. (LP: #1395581)

It could also possibly affect the current triple buffering used for
multimonitor and bypass/overlays.

To post a comment you must log in.
Alberto Aguirre (albaguirre) wrote :

OK. LGTM.

review: Approve
Daniel van Vugt (vanvugt) wrote :

Ugg. Same tests failing for all proposals :(

Alan Griffiths (alan-griffiths) wrote :

I need to think through what happens when the compositor drops renderables. But this seem promising.

review: Abstain
Kevin DuBois (kdub) wrote :

#1
I don't like mg::Renderables::buffers_ready_for_compositor(), and am happy to see it going away, but it is a bit entrenched in qtmir, so we should have a way to transition the downstream.

http://bazaar.launchpad.net/~mir-team/qtmir/trunk/view/head:/src/modules/Unity/Application/mirsurfaceitem.cpp#L446

#2
This does circumvent the observer architecture by requiring feedback from the BufferQueues. I see how the arch is wrong in the case described, but before we were telling the compositors to composite so many times, but now we're telling them to composite so many times, and then asking if they really meant it. The BufferQueue doesn't have all the information about how many compositions are needed to drain because of things like position-only-updates, and the BasicSurface cant give the information that is needed to drain the queue completely. Maybe we should improve our concept of 'damage' of the stack, and then have every composition reduce the scheduled damage by one.

review: Needs Information
Kevin DuBois (kdub) wrote :

to elaborate a bit further #1 is the bigger concern, #2 is to point out in the review that mc::Scene now is somewhat strange in that we have two routes to observe the damage

//frames-pending method
virtual int frames_pending(CompositorID id) const = 0;

//observer method
virtual void add_observer(std::shared_ptr<scene::Observer> const& observer) = 0;
virtual void remove_observer(std::weak_ptr<scene::Observer> const& observer) = 0;

and that the observer method (public api) is still broken in that it can't drain the queue properly.

Daniel van Vugt (vanvugt) wrote :

#1
Yeah I knew about QtMir already. It will need fixing. Hopefully without reintroducing the old Renderable method but we'll see. It's a simple matter of code but needs the involvement of Unity8 guys more to test the changes.

#2
The observer architecture obviously doesn't solve all problems. Compositors can and will occasionally skip over some renderable and not consume its buffer. Compositors are free to make their own decisions, so we should support any outcome from that.
  I don't think this is strange or inconsistent though. A compositor's default state is to be asleep. We _need_ the observer stuff to wake it up. And we _need_ a separate counter to cover the case of rendering N frames after N were scheduled might not be enough (due to occlusions whatever). Callback-based observers can not solve this problem, but semaphore-based observers could, so read on...
  I did consider a more unified approach that might work. That is a singular counter (like a semaphore) stored in the scene where each surface adds to the counter when it has something new not consumed. And critically, only the surface/producer can then remove its count from the scene total when it's flushed. But that's an architectural rework, bigger than this proposal and possibly could end up more complicated with some new coupling too. So first a fix, and later maybe redesign.

Kevin DuBois (kdub) wrote :

#2: Fair enough, I think the unified approach is something to work towards, but in the meantime, we may as well use this approach to solve the problem. Keeping track of whether the stack is damaged seems a better concept than what's in trunk or the wakeup and keep-running-until-drained approach.

#1: I guess I'd like to pause this branch to avoid breaking downstream in a way that we don't have a fix for yet. Its mostly just a timing issue, but I'd guess that the Belgium sprinters might want to integrate the tip of mir with the tip of the other components.

So anyways, approve from me, but would appreciate pausing the branch until we have a downstream update available.

Daniel van Vugt (vanvugt) wrote :

#1: I'm sitting next to Gerry and talking to him about it :)

Kevin DuBois (kdub) wrote :

As long as we can work towards a more unified approach, and there's a branch forthcoming downstream, lgtm.

review: Approve
Daniel van Vugt (vanvugt) wrote :

Fortunately we've branched 0.11 already so landing this for 0.12 puts us in no hurry to sync downstreams.

Robert Carr (robertcarr) wrote :

UNTaed until qtmir branch is available so we can release tomorrow.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'examples/render_overlays.cpp'
2--- examples/render_overlays.cpp 2015-01-19 09:27:46 +0000
3+++ examples/render_overlays.cpp 2015-02-03 15:18:44 +0000
4@@ -162,11 +162,6 @@
5 return false;
6 }
7
8- int buffers_ready_for_compositor() const override
9- {
10- return 1;
11- }
12-
13 private:
14 std::shared_ptr<DemoOverlayClient> const client;
15 geom::Rectangle const position;
16
17=== modified file 'include/platform/mir/graphics/renderable.h'
18--- include/platform/mir/graphics/renderable.h 2015-01-19 09:27:46 +0000
19+++ include/platform/mir/graphics/renderable.h 2015-02-03 15:18:44 +0000
20@@ -69,7 +69,6 @@
21 virtual glm::mat4 transformation() const = 0;
22
23 virtual bool shaped() const = 0; // meaning the pixel format has alpha
24- virtual int buffers_ready_for_compositor() const = 0;
25
26 protected:
27 Renderable() = default;
28
29=== modified file 'include/server/mir/compositor/scene.h'
30--- include/server/mir/compositor/scene.h 2015-01-14 06:39:13 +0000
31+++ include/server/mir/compositor/scene.h 2015-02-03 15:18:44 +0000
32@@ -55,6 +55,16 @@
33 */
34 virtual SceneElementSequence scene_elements_for(CompositorID id) = 0;
35
36+ /**
37+ * Return the number of additional frames that you need to render to get
38+ * fully up to date with the latest data in the scene. For a generic
39+ * "scene change" this will be just 1. For surfaces that have multiple
40+ * frames queued up however, it could be greater than 1. When the result
41+ * reaches zero, you know you have consumed all the latest data from the
42+ * scene.
43+ */
44+ virtual int frames_pending(CompositorID id) const = 0;
45+
46 virtual void register_compositor(CompositorID id) = 0;
47 virtual void unregister_compositor(CompositorID id) = 0;
48
49
50=== modified file 'include/server/mir/scene/surface.h'
51--- include/server/mir/scene/surface.h 2015-01-22 15:55:33 +0000
52+++ include/server/mir/scene/surface.h 2015-02-03 15:18:44 +0000
53@@ -57,6 +57,7 @@
54 virtual geometry::Size size() const = 0;
55
56 virtual std::unique_ptr<graphics::Renderable> compositor_snapshot(void const* compositor_id) const = 0;
57+ virtual int buffers_ready_for_compositor(void const* compositor_id) const = 0;
58
59 virtual float alpha() const = 0; //only used in examples/
60 virtual MirSurfaceType type() const = 0;
61
62=== modified file 'platform-ABI-sha1sums'
63--- platform-ABI-sha1sums 2015-01-29 04:34:12 +0000
64+++ platform-ABI-sha1sums 2015-02-03 15:18:44 +0000
65@@ -56,6 +56,6 @@
66 d1ba61e687d75103e9e7aa0409080214f56a990b include/platform/mir/graphics/platform_ipc_operations.h
67 1b77fb3290af00dc7d1c11dcc5388972dacb9ec3 include/platform/mir/graphics/platform_ipc_package.h
68 f0db0484b8ccc9091e73c80a2a200cb436b48bfb include/platform/mir/graphics/platform_operation_message.h
69-f18876766861e5d4f5ca999dbd176fe1fc520594 include/platform/mir/graphics/renderable.h
70+48ee8461ccf0bbfc3c6bf2c0fb5de8bc679e857b include/platform/mir/graphics/renderable.h
71 5b3872b04b3686fe9a244572ca0787596431a2cb include/platform/mir/module_properties.h
72 b45f14082c4f8b29efaa1b13de795dcb29deb738 include/platform/mir/options/option.h
73
74=== modified file 'server-ABI-sha1sums'
75--- server-ABI-sha1sums 2015-01-30 14:14:27 +0000
76+++ server-ABI-sha1sums 2015-02-03 15:18:44 +0000
77@@ -56,14 +56,14 @@
78 d1ba61e687d75103e9e7aa0409080214f56a990b include/platform/mir/graphics/platform_ipc_operations.h
79 1b77fb3290af00dc7d1c11dcc5388972dacb9ec3 include/platform/mir/graphics/platform_ipc_package.h
80 f0db0484b8ccc9091e73c80a2a200cb436b48bfb include/platform/mir/graphics/platform_operation_message.h
81-f18876766861e5d4f5ca999dbd176fe1fc520594 include/platform/mir/graphics/renderable.h
82+48ee8461ccf0bbfc3c6bf2c0fb5de8bc679e857b include/platform/mir/graphics/renderable.h
83 5b3872b04b3686fe9a244572ca0787596431a2cb include/platform/mir/module_properties.h
84 b45f14082c4f8b29efaa1b13de795dcb29deb738 include/platform/mir/options/option.h
85 f4030e400baf8baa9c38e7c6ec6b4a5ad7134aeb include/server/mir/compositor/compositor.h
86 a9f284ba4b05d58fd3eeb628d1f56fe4ac188526 include/server/mir/compositor/compositor_id.h
87 98fe6bde01fb8b25b1bacafcdf572f24c3ade3c3 include/server/mir/compositor/display_buffer_compositor_factory.h
88 d49eae4f986645b32e29ce2c1f99f524703ba3c0 include/server/mir/compositor/display_buffer_compositor.h
89-4fcf34e424128b87ddc76733594e32e09ebbd486 include/server/mir/compositor/scene.h
90+5fa944a08d5944503fd10d87a4e57985cfcce379 include/server/mir/compositor/scene.h
91 59d6703def2a0ff840e5eb75390a5af76fd93f58 include/server/mir/frontend/prompt_session.h
92 b729a7710c37d9f1fba56a6f8e8eae1c2559f57a include/server/mir/frontend/session_authorizer.h
93 34ce482df448fd2fc5f0c4ae5ac8b7fecbd228c9 include/server/mir/frontend/session_credentials.h
94@@ -100,7 +100,7 @@
95 bbc9e2e2bb71634cd6f1c5c0430093e10e74fa23 include/server/mir/scene/surface_configurator.h
96 e5e4dd7bcaf186810043fa0f05be42d7e49b0843 include/server/mir/scene/surface_coordinator.h
97 da52304237a348a90fcb56729293d628ceaa351c include/server/mir/scene/surface_creation_parameters.h
98-aceccc0bdeea5c90d6c5ea17ed4ce6b5dc1b0ed4 include/server/mir/scene/surface.h
99+046df34f06aec23ce432c96d1a26b70ee109bfab include/server/mir/scene/surface.h
100 587e22d751656ce2d9536afdf5659276ff9bbc46 include/server/mir/scene/surface_observer.h
101 7ef3e99901168cda296d74d05a979f47bf9c3ff1 include/server/mir/server_action_queue.h
102 82adb52457bf9caf913940e8eeddec4e116dd862 include/server/mir/server.h
103
104=== modified file 'src/include/server/mir/compositor/buffer_stream.h'
105--- src/include/server/mir/compositor/buffer_stream.h 2015-01-14 06:39:13 +0000
106+++ src/include/server/mir/compositor/buffer_stream.h 2015-02-03 15:18:44 +0000
107@@ -52,7 +52,7 @@
108 virtual void resize(geometry::Size const& size) = 0;
109 virtual void allow_framedropping(bool) = 0;
110 virtual void force_requests_to_complete() = 0;
111- virtual int buffers_ready_for_compositor() const = 0;
112+ virtual int buffers_ready_for_compositor(void const* user_id) const = 0;
113 virtual void drop_old_buffers() = 0;
114 virtual void drop_client_requests() = 0;
115 };
116
117=== modified file 'src/server/compositor/buffer_bundle.h'
118--- src/server/compositor/buffer_bundle.h 2015-01-14 06:39:13 +0000
119+++ src/server/compositor/buffer_bundle.h 2015-02-03 15:18:44 +0000
120@@ -58,7 +58,7 @@
121 virtual void allow_framedropping(bool dropping_allowed) = 0;
122 virtual void force_requests_to_complete() = 0;
123 virtual void resize(const geometry::Size &newsize) = 0;
124- virtual int buffers_ready_for_compositor() const = 0;
125+ virtual int buffers_ready_for_compositor(void const* user_id) const = 0;
126
127 /**
128 * Return the number of client acquisitions that can be completed
129
130=== modified file 'src/server/compositor/buffer_queue.cpp'
131--- src/server/compositor/buffer_queue.cpp 2015-01-14 06:39:13 +0000
132+++ src/server/compositor/buffer_queue.cpp 2015-02-03 15:18:44 +0000
133@@ -352,15 +352,19 @@
134 the_properties.size = new_size;
135 }
136
137-int mc::BufferQueue::buffers_ready_for_compositor() const
138+int mc::BufferQueue::buffers_ready_for_compositor(void const* user_id) const
139 {
140 std::lock_guard<decltype(guard)> lock(guard);
141
142- /*TODO: this api also needs to know the caller user id
143- * as the number of buffers that are truly ready
144- * vary depending on concurrent compositors.
145- */
146- return ready_to_composite_queue.size();
147+ int count = ready_to_composite_queue.size();
148+ if (!current_buffer_users.empty() && !is_a_current_buffer_user(user_id))
149+ {
150+ // The virtual front of the ready queue isn't actually in the ready
151+ // queue, but is the current_compositor_buffer, so count that too:
152+ ++count;
153+ }
154+
155+ return count;
156 }
157
158 int mc::BufferQueue::buffers_free_for_client() const
159
160=== modified file 'src/server/compositor/buffer_queue.h'
161--- src/server/compositor/buffer_queue.h 2015-01-14 06:39:13 +0000
162+++ src/server/compositor/buffer_queue.h 2015-02-03 15:18:44 +0000
163@@ -58,7 +58,7 @@
164 void allow_framedropping(bool dropping_allowed) override;
165 void force_requests_to_complete() override;
166 void resize(const geometry::Size &newsize) override;
167- int buffers_ready_for_compositor() const override;
168+ int buffers_ready_for_compositor(void const* user_id) const override;
169 int buffers_free_for_client() const override;
170 bool framedropping_allowed() const;
171 bool is_a_current_buffer_user(void const* user_id) const;
172
173=== modified file 'src/server/compositor/buffer_stream_surfaces.cpp'
174--- src/server/compositor/buffer_stream_surfaces.cpp 2015-01-14 06:39:13 +0000
175+++ src/server/compositor/buffer_stream_surfaces.cpp 2015-02-03 15:18:44 +0000
176@@ -85,9 +85,9 @@
177 buffer_bundle->allow_framedropping(allow);
178 }
179
180-int mc::BufferStreamSurfaces::buffers_ready_for_compositor() const
181+int mc::BufferStreamSurfaces::buffers_ready_for_compositor(void const* user_id) const
182 {
183- return buffer_bundle->buffers_ready_for_compositor();
184+ return buffer_bundle->buffers_ready_for_compositor(user_id);
185 }
186
187 void mc::BufferStreamSurfaces::drop_old_buffers()
188
189=== modified file 'src/server/compositor/buffer_stream_surfaces.h'
190--- src/server/compositor/buffer_stream_surfaces.h 2015-01-14 06:39:13 +0000
191+++ src/server/compositor/buffer_stream_surfaces.h 2015-02-03 15:18:44 +0000
192@@ -51,7 +51,7 @@
193 void resize(geometry::Size const& size) override;
194 void allow_framedropping(bool) override;
195 void force_requests_to_complete() override;
196- int buffers_ready_for_compositor() const override;
197+ int buffers_ready_for_compositor(void const* user_id) const override;
198 void drop_old_buffers() override;
199 void drop_client_requests() override;
200
201
202=== modified file 'src/server/compositor/multi_threaded_compositor.cpp'
203--- src/server/compositor/multi_threaded_compositor.cpp 2015-01-14 06:39:13 +0000
204+++ src/server/compositor/multi_threaded_compositor.cpp 2015-02-03 15:18:44 +0000
205@@ -114,6 +114,7 @@
206 CurrentRenderingTarget target{buffer};
207
208 auto display_buffer_compositor = display_buffer_compositor_factory->create_compositor_for(buffer);
209+ auto const comp_id = display_buffer_compositor.get();
210
211 CompositorReport::SubCompositorId report_id =
212 display_buffer_compositor.get();
213@@ -150,9 +151,19 @@
214 lock.unlock();
215
216 display_buffer_compositor->composite(
217- scene->scene_elements_for(display_buffer_compositor.get()));
218+ scene->scene_elements_for(comp_id));
219
220 lock.lock();
221+
222+ /*
223+ * Note the compositor may have chosen to ignore any number
224+ * of renderables and not consumed buffers from them. So it's
225+ * important to re-count number of frames pending, separately
226+ * to the initial scene_elements_for()...
227+ */
228+ int pending = scene->frames_pending(comp_id);
229+ if (pending > frames_scheduled)
230+ frames_scheduled = pending;
231 }
232 }
233 }
234
235=== modified file 'src/server/graphics/software_cursor.cpp'
236--- src/server/graphics/software_cursor.cpp 2015-01-19 09:27:46 +0000
237+++ src/server/graphics/software_cursor.cpp 2015-02-03 15:18:44 +0000
238@@ -92,11 +92,6 @@
239 return true;
240 }
241
242- int buffers_ready_for_compositor() const override
243- {
244- return 1;
245- }
246-
247 void move_to(geom::Point new_position)
248 {
249 std::lock_guard<std::mutex> lock{position_mutex};
250
251=== modified file 'src/server/input/touchspot_controller.cpp'
252--- src/server/input/touchspot_controller.cpp 2015-01-19 09:27:46 +0000
253+++ src/server/input/touchspot_controller.cpp 2015-02-03 15:18:44 +0000
254@@ -76,11 +76,6 @@
255 return true;
256 }
257
258- int buffers_ready_for_compositor() const override
259- {
260- return 1;
261- }
262-
263 // TouchspotRenderable
264 void move_center_to(geom::Point pos)
265 {
266
267=== modified file 'src/server/scene/basic_surface.cpp'
268--- src/server/scene/basic_surface.cpp 2015-01-29 17:18:29 +0000
269+++ src/server/scene/basic_surface.cpp 2015-02-03 15:18:44 +0000
270@@ -223,7 +223,12 @@
271 first_frame_posted = true;
272 }
273
274- observers.frame_posted(surface_buffer_stream->buffers_ready_for_compositor());
275+ /*
276+ * TODO: In future frame_posted() could be made parameterless.
277+ * The new method of catching up on buffer backlogs is to
278+ * query buffers_ready_for_compositor() or Scene::frames_pending
279+ */
280+ observers.frame_posted(1);
281 }
282
283 surface_buffer_stream->acquire_client_buffer(complete);
284@@ -680,9 +685,6 @@
285 {
286 }
287
288- int buffers_ready_for_compositor() const override
289- { return underlying_buffer_stream->buffers_ready_for_compositor(); }
290-
291 std::shared_ptr<mg::Buffer> buffer() const override
292 {
293 if (!compositor_buffer)
294@@ -731,6 +733,12 @@
295 this));
296 }
297
298+int ms::BasicSurface::buffers_ready_for_compositor(void const* id) const
299+{
300+ std::unique_lock<std::mutex> lk(guard);
301+ return surface_buffer_stream->buffers_ready_for_compositor(id);
302+}
303+
304 void ms::BasicSurface::consume(MirEvent const& event)
305 {
306 input_sender->send_event(event, server_input_channel);
307
308=== modified file 'src/server/scene/basic_surface.h'
309--- src/server/scene/basic_surface.h 2015-01-29 17:18:29 +0000
310+++ src/server/scene/basic_surface.h 2015-02-03 15:18:44 +0000
311@@ -137,6 +137,7 @@
312 bool visible() const override;
313
314 std::unique_ptr<graphics::Renderable> compositor_snapshot(void const* compositor_id) const override;
315+ int buffers_ready_for_compositor(void const* compositor_id) const override;
316
317 void with_most_recent_buffer_do(
318 std::function<void(graphics::Buffer&)> const& exec) override;
319
320=== modified file 'src/server/scene/surface_stack.cpp'
321--- src/server/scene/surface_stack.cpp 2015-01-19 09:27:46 +0000
322+++ src/server/scene/surface_stack.cpp 2015-02-03 15:18:44 +0000
323@@ -124,6 +124,8 @@
324 mc::SceneElementSequence ms::SurfaceStack::scene_elements_for(mc::CompositorID id)
325 {
326 std::lock_guard<decltype(guard)> lg(guard);
327+
328+ scene_changed = false;
329 mc::SceneElementSequence elements;
330 for (auto const& layer : layers_by_depth)
331 {
332@@ -146,6 +148,29 @@
333 return elements;
334 }
335
336+int ms::SurfaceStack::frames_pending(mc::CompositorID id) const
337+{
338+ std::lock_guard<decltype(guard)> lg(guard);
339+
340+ int result = scene_changed ? 1 : 0;
341+ for (auto const& layer : layers_by_depth)
342+ {
343+ for (auto const& surface : layer.second)
344+ {
345+ if (surface->visible())
346+ {
347+ // Note that we ask the surface and not a Renderable.
348+ // This is because we don't want to waste time and resources
349+ // on a snapshot till we're sure we need it...
350+ int ready = surface->buffers_ready_for_compositor(id);
351+ if (ready > result)
352+ result = ready;
353+ }
354+ }
355+ }
356+ return result;
357+}
358+
359 void ms::SurfaceStack::register_compositor(mc::CompositorID cid)
360 {
361 std::lock_guard<decltype(guard)> lg(guard);
362@@ -171,7 +196,7 @@
363 std::lock_guard<decltype(guard)> lg(guard);
364 overlays.push_back(overlay);
365 }
366- observers.scene_changed();
367+ emit_scene_changed();
368 }
369
370 void ms::SurfaceStack::remove_input_visualization(
371@@ -188,11 +213,15 @@
372 overlays.erase(p);
373 }
374
375- observers.scene_changed();
376+ emit_scene_changed();
377 }
378
379 void ms::SurfaceStack::emit_scene_changed()
380 {
381+ {
382+ std::lock_guard<decltype(guard)> lg(guard);
383+ scene_changed = true;
384+ }
385 observers.scene_changed();
386 }
387
388
389=== modified file 'src/server/scene/surface_stack.h'
390--- src/server/scene/surface_stack.h 2015-01-14 06:39:13 +0000
391+++ src/server/scene/surface_stack.h 2015-02-03 15:18:44 +0000
392@@ -73,6 +73,7 @@
393
394 // From Scene
395 compositor::SceneElementSequence scene_elements_for(compositor::CompositorID id) override;
396+ int frames_pending(compositor::CompositorID) const override;
397 void register_compositor(compositor::CompositorID id) override;
398 void unregister_compositor(compositor::CompositorID id) override;
399
400@@ -116,6 +117,7 @@
401 std::vector<std::shared_ptr<graphics::Renderable>> overlays;
402
403 Observers observers;
404+ bool scene_changed = false;
405 };
406
407 }
408
409=== modified file 'src/server/symbols.map'
410--- src/server/symbols.map 2015-01-30 14:41:33 +0000
411+++ src/server/symbols.map 2015-02-03 15:18:44 +0000
412@@ -25,6 +25,7 @@
413 mir::compositor::Scene::remove_observer*;
414 mir::compositor::Scene::Scene*;
415 mir::compositor::Scene::scene_elements_for*;
416+ mir::compositor::Scene::frames_pending*;
417 mir::compositor::Scene::unregister_compositor*;
418 mir::frontend::PromptSession::operator*;
419 mir::frontend::PromptSession::?PromptSession*;
420
421=== modified file 'tests/include/mir_test_doubles/fake_renderable.h'
422--- tests/include/mir_test_doubles/fake_renderable.h 2015-01-19 09:27:46 +0000
423+++ tests/include/mir_test_doubles/fake_renderable.h 2015-02-03 15:18:44 +0000
424@@ -95,11 +95,6 @@
425 return rect;
426 }
427
428- int buffers_ready_for_compositor() const override
429- {
430- return 1;
431- }
432-
433 private:
434 std::shared_ptr<graphics::Buffer> buf;
435 mir::geometry::Rectangle rect;
436
437=== modified file 'tests/include/mir_test_doubles/mock_buffer_bundle.h'
438--- tests/include/mir_test_doubles/mock_buffer_bundle.h 2015-01-14 06:39:13 +0000
439+++ tests/include/mir_test_doubles/mock_buffer_bundle.h 2015-02-03 15:18:44 +0000
440@@ -49,7 +49,7 @@
441 MOCK_METHOD0(force_requests_to_complete, void());
442 MOCK_METHOD1(resize, void(const geometry::Size &));
443 MOCK_METHOD0(drop_old_buffers, void());
444- int buffers_ready_for_compositor() const override { return 1; }
445+ int buffers_ready_for_compositor(void const*) const override { return 1; }
446 int buffers_free_for_client() const override { return 1; }
447 void drop_client_requests() override {}
448 };
449
450=== modified file 'tests/include/mir_test_doubles/mock_buffer_stream.h'
451--- tests/include/mir_test_doubles/mock_buffer_stream.h 2015-01-14 06:39:13 +0000
452+++ tests/include/mir_test_doubles/mock_buffer_stream.h 2015-02-03 15:18:44 +0000
453@@ -32,7 +32,7 @@
454 struct MockBufferStream : public compositor::BufferStream
455 {
456 int buffers_ready_{0};
457- int buffers_ready()
458+ int buffers_ready(void const*)
459 {
460 if (buffers_ready_)
461 return buffers_ready_--;
462@@ -41,7 +41,7 @@
463
464 MockBufferStream()
465 {
466- ON_CALL(*this, buffers_ready_for_compositor())
467+ ON_CALL(*this, buffers_ready_for_compositor(::testing::_))
468 .WillByDefault(testing::Invoke(this, &MockBufferStream::buffers_ready));
469 }
470 MOCK_METHOD1(acquire_client_buffer, void(std::function<void(graphics::Buffer* buffer)>));
471@@ -56,7 +56,7 @@
472 MOCK_METHOD0(force_client_completion, void());
473 MOCK_METHOD1(allow_framedropping, void(bool));
474 MOCK_METHOD0(force_requests_to_complete, void());
475- MOCK_CONST_METHOD0(buffers_ready_for_compositor, int());
476+ MOCK_CONST_METHOD1(buffers_ready_for_compositor, int(void const*));
477 MOCK_METHOD0(drop_old_buffers, void());
478 MOCK_METHOD0(drop_client_requests, void());
479 };
480
481=== modified file 'tests/include/mir_test_doubles/mock_renderable.h'
482--- tests/include/mir_test_doubles/mock_renderable.h 2015-01-14 06:39:13 +0000
483+++ tests/include/mir_test_doubles/mock_renderable.h 2015-02-03 15:18:44 +0000
484@@ -37,8 +37,6 @@
485 .WillByDefault(testing::Return(geometry::Rectangle{{},{}}));
486 ON_CALL(*this, buffer())
487 .WillByDefault(testing::Return(std::make_shared<StubBuffer>()));
488- ON_CALL(*this, buffers_ready_for_compositor())
489- .WillByDefault(testing::Return(1));
490 ON_CALL(*this, alpha())
491 .WillByDefault(testing::Return(1.0f));
492 ON_CALL(*this, transformation())
493@@ -54,7 +52,6 @@
494 MOCK_CONST_METHOD0(transformation, glm::mat4());
495 MOCK_CONST_METHOD0(visible, bool());
496 MOCK_CONST_METHOD0(shaped, bool());
497- MOCK_CONST_METHOD0(buffers_ready_for_compositor, int());
498 };
499 }
500 }
501
502=== modified file 'tests/include/mir_test_doubles/mock_scene.h'
503--- tests/include/mir_test_doubles/mock_scene.h 2015-01-14 06:39:13 +0000
504+++ tests/include/mir_test_doubles/mock_scene.h 2015-02-03 15:18:44 +0000
505@@ -36,9 +36,12 @@
506 {
507 ON_CALL(*this, scene_elements_for(testing::_))
508 .WillByDefault(testing::Return(compositor::SceneElementSequence{}));
509+ ON_CALL(*this, frames_pending(testing::_))
510+ .WillByDefault(testing::Return(0));
511 }
512
513 MOCK_METHOD1(scene_elements_for, compositor::SceneElementSequence(compositor::CompositorID));
514+ MOCK_CONST_METHOD1(frames_pending, int(compositor::CompositorID));
515 MOCK_METHOD1(register_compositor, void(compositor::CompositorID));
516 MOCK_METHOD1(unregister_compositor, void(compositor::CompositorID));
517
518
519=== modified file 'tests/include/mir_test_doubles/stub_buffer_stream.h'
520--- tests/include/mir_test_doubles/stub_buffer_stream.h 2015-01-14 06:39:13 +0000
521+++ tests/include/mir_test_doubles/stub_buffer_stream.h 2015-02-03 15:18:44 +0000
522@@ -45,10 +45,12 @@
523
524 void release_client_buffer(graphics::Buffer*) override
525 {
526+ ++nready;
527 }
528
529 std::shared_ptr<graphics::Buffer> lock_compositor_buffer(void const*) override
530 {
531+ --nready;
532 return stub_compositor_buffer;
533 }
534
535@@ -79,13 +81,14 @@
536 {
537 }
538
539- int buffers_ready_for_compositor() const override { return 1; }
540+ int buffers_ready_for_compositor(void const*) const override { return nready; }
541
542 void drop_old_buffers() override {}
543 void drop_client_requests() override {}
544
545 StubBuffer stub_client_buffer;
546 std::shared_ptr<graphics::Buffer> stub_compositor_buffer;
547+ int nready = 0;
548 };
549
550 }
551
552=== modified file 'tests/include/mir_test_doubles/stub_renderable.h'
553--- tests/include/mir_test_doubles/stub_renderable.h 2015-01-19 09:27:46 +0000
554+++ tests/include/mir_test_doubles/stub_renderable.h 2015-02-03 15:18:44 +0000
555@@ -86,11 +86,6 @@
556 return false;
557 }
558
559- int buffers_ready_for_compositor() const override
560- {
561- return 1;
562- }
563-
564 private:
565 std::shared_ptr<graphics::Buffer> make_stub_buffer(geometry::Rectangle const& rect)
566 {
567
568=== modified file 'tests/include/mir_test_doubles/stub_scene.h'
569--- tests/include/mir_test_doubles/stub_scene.h 2015-01-14 06:39:13 +0000
570+++ tests/include/mir_test_doubles/stub_scene.h 2015-02-03 15:18:44 +0000
571@@ -34,7 +34,11 @@
572 public:
573 compositor::SceneElementSequence scene_elements_for(compositor::CompositorID) override
574 {
575- return {};
576+ return {};
577+ }
578+ int frames_pending(compositor::CompositorID) const override
579+ {
580+ return 0;
581 }
582 void register_compositor(compositor::CompositorID) override
583 {
584
585=== modified file 'tests/include/mir_test_doubles/stub_scene_surface.h'
586--- tests/include/mir_test_doubles/stub_scene_surface.h 2015-01-22 03:10:13 +0000
587+++ tests/include/mir_test_doubles/stub_scene_surface.h 2015-02-03 15:18:44 +0000
588@@ -63,6 +63,7 @@
589 bool input_area_contains(mir::geometry::Point const&) const override { return false; }
590
591 virtual std::unique_ptr<graphics::Renderable> compositor_snapshot(void const*) const override { return nullptr; }
592+ int buffers_ready_for_compositor(void const*) const override { return 0; }
593
594 float alpha() const override { return 0.0f;}
595 MirSurfaceType type() const override { return mir_surface_type_normal; }
596
597=== modified file 'tests/integration-tests/test_exchange_buffer.cpp'
598--- tests/integration-tests/test_exchange_buffer.cpp 2015-01-14 06:39:13 +0000
599+++ tests/integration-tests/test_exchange_buffer.cpp 2015-02-03 15:18:44 +0000
600@@ -83,7 +83,7 @@
601 void resize(geom::Size const&) {};
602 void allow_framedropping(bool) {};
603 void force_requests_to_complete() {};
604- int buffers_ready_for_compositor() const { return -5; }
605+ int buffers_ready_for_compositor(void const*) const override { return -5; }
606 void drop_old_buffers() {}
607 void drop_client_requests() override {}
608
609
610=== modified file 'tests/integration-tests/test_surface_stack_with_compositor.cpp'
611--- tests/integration-tests/test_surface_stack_with_compositor.cpp 2015-01-29 17:18:29 +0000
612+++ tests/integration-tests/test_surface_stack_with_compositor.cpp 2015-02-03 15:18:44 +0000
613@@ -208,7 +208,7 @@
614 TEST_F(SurfaceStackCompositor, compositor_runs_until_all_surfaces_buffers_are_consumed)
615 {
616 using namespace testing;
617- ON_CALL(*mock_buffer_stream, buffers_ready_for_compositor())
618+ ON_CALL(*mock_buffer_stream, buffers_ready_for_compositor(_))
619 .WillByDefault(Return(5));
620
621 mc::MultiThreadedCompositor mt_compositor(
622@@ -228,7 +228,7 @@
623 TEST_F(SurfaceStackCompositor, bypassed_compositor_runs_until_all_surfaces_buffers_are_consumed)
624 {
625 using namespace testing;
626- ON_CALL(*mock_buffer_stream, buffers_ready_for_compositor())
627+ ON_CALL(*mock_buffer_stream, buffers_ready_for_compositor(_))
628 .WillByDefault(Return(5));
629
630 stub_surface->resize(geom::Size{10,10});
631@@ -250,7 +250,7 @@
632 TEST_F(SurfaceStackCompositor, an_empty_scene_retriggers)
633 {
634 using namespace testing;
635- ON_CALL(*mock_buffer_stream, buffers_ready_for_compositor())
636+ ON_CALL(*mock_buffer_stream, buffers_ready_for_compositor(_))
637 .WillByDefault(Return(0));
638
639 mc::MultiThreadedCompositor mt_compositor(
640@@ -310,7 +310,8 @@
641
642 TEST_F(SurfaceStackCompositor, buffer_updates_trigger_composition)
643 {
644- ON_CALL(*mock_buffer_stream, buffers_ready_for_compositor())
645+ using namespace testing;
646+ ON_CALL(*mock_buffer_stream, buffers_ready_for_compositor(_))
647 .WillByDefault(testing::Return(1));
648 stack.add_surface(stub_surface, default_params.depth, default_params.input_mode);
649 stub_surface->swap_buffers(&stubbuf, [](mg::Buffer*){});
650
651=== modified file 'tests/unit-tests/compositor/test_buffer_queue.cpp'
652--- tests/unit-tests/compositor/test_buffer_queue.cpp 2015-01-14 06:39:13 +0000
653+++ tests/unit-tests/compositor/test_buffer_queue.cpp 2015-02-03 15:18:44 +0000
654@@ -1281,6 +1281,72 @@
655 }
656 }
657
658+TEST_F(BufferQueueTest, buffers_ready_is_not_underestimated)
659+{ // Regression test for LP: #1395581
660+ using namespace testing;
661+
662+ for (int nbuffers = 2; nbuffers <= max_nbuffers_to_test; ++nbuffers)
663+ {
664+ mc::BufferQueue q{nbuffers, allocator, basic_properties, policy_factory};
665+
666+ // Produce frame 1
667+ q.client_release(client_acquire_sync(q));
668+ // Acquire frame 1
669+ auto a = q.compositor_acquire(this);
670+
671+ // Produce frame 2
672+ q.client_release(client_acquire_sync(q));
673+ // Acquire frame 2
674+ auto b = q.compositor_acquire(this);
675+
676+ // Release frame 1
677+ q.compositor_release(a);
678+ // Produce frame 3
679+ q.client_release(client_acquire_sync(q));
680+ // Release frame 2
681+ q.compositor_release(b);
682+
683+ // Verify frame 3 is ready for the first compositor
684+ ASSERT_THAT(q.buffers_ready_for_compositor(this), Ge(1));
685+ auto c = q.compositor_acquire(this);
686+
687+ // Verify frame 3 is ready for a second compositor
688+ int const that = 0;
689+ ASSERT_THAT(q.buffers_ready_for_compositor(&that), Ge(1));
690+
691+ q.compositor_release(c);
692+ }
693+}
694+
695+TEST_F(BufferQueueTest, buffers_ready_eventually_reaches_zero)
696+{
697+ using namespace testing;
698+
699+ for (int nbuffers = 2; nbuffers <= max_nbuffers_to_test; ++nbuffers)
700+ {
701+ mc::BufferQueue q{nbuffers, allocator, basic_properties, policy_factory};
702+
703+ const int nmonitors = 3;
704+ int monitor[nmonitors];
705+
706+ for (int m = 0; m < nmonitors; ++m)
707+ {
708+ ASSERT_EQ(0, q.buffers_ready_for_compositor(&monitor[m]));
709+ }
710+
711+ // Produce a frame
712+ q.client_release(client_acquire_sync(q));
713+
714+ // Consume
715+ for (int m = 0; m < nmonitors; ++m)
716+ {
717+ ASSERT_EQ(1, q.buffers_ready_for_compositor(&monitor[m]));
718+ q.compositor_release(q.compositor_acquire(&monitor[m]));
719+ ASSERT_EQ(0, q.buffers_ready_for_compositor(&monitor[m]));
720+ }
721+ }
722+}
723+
724 /* Regression test for LP: #1306464 */
725 TEST_F(BufferQueueTest, framedropping_client_acquire_does_not_block_when_no_available_buffers)
726 {
727
728=== modified file 'tests/unit-tests/compositor/test_multi_threaded_compositor.cpp'
729--- tests/unit-tests/compositor/test_multi_threaded_compositor.cpp 2015-01-14 06:39:13 +0000
730+++ tests/unit-tests/compositor/test_multi_threaded_compositor.cpp 2015-02-03 15:18:44 +0000
731@@ -128,7 +128,19 @@
732 throw_on_add_observer_ = flag;
733 }
734
735+ int frames_pending(mc::CompositorID) const override
736+ {
737+ return pending;
738+ }
739+
740+ void set_pending(int n)
741+ {
742+ pending = n;
743+ observer->scene_changed();
744+ }
745+
746 private:
747+ int pending = 0;
748 std::mutex observer_mutex;
749 std::shared_ptr<ms::Observer> observer;
750 bool throw_on_add_observer_;
751@@ -480,6 +492,79 @@
752 compositor.stop();
753 }
754
755+TEST(MultiThreadedCompositor, schedules_enough_frames)
756+{
757+ using namespace testing;
758+
759+ unsigned int const nbuffers = 3;
760+
761+ auto display = std::make_shared<StubDisplay>(nbuffers);
762+ auto scene = std::make_shared<StubScene>();
763+ auto factory = std::make_shared<RecordingDisplayBufferCompositorFactory>();
764+ mc::MultiThreadedCompositor compositor{display, scene, factory,
765+ null_report, true};
766+
767+ EXPECT_TRUE(factory->check_record_count_for_each_buffer(nbuffers, 0, 0));
768+
769+ compositor.start();
770+
771+ int const max_retries = 100;
772+ int retry = 0;
773+ while (retry < max_retries &&
774+ !factory->check_record_count_for_each_buffer(nbuffers, 1))
775+ {
776+ std::this_thread::sleep_for(std::chrono::milliseconds(10));
777+ ++retry;
778+ }
779+
780+ ASSERT_LT(retry, max_retries);
781+
782+ // Now we have the initial frame wait a bit
783+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
784+ // ... and make sure the number is still only 1
785+ ASSERT_TRUE(factory->check_record_count_for_each_buffer(nbuffers, 1, 1));
786+
787+ // Trigger scene changes. Only need one frame to cover them all...
788+ scene->emit_change_event();
789+ scene->emit_change_event();
790+ scene->emit_change_event();
791+
792+ // Display buffers should be forced to render another 1, so that's 2
793+ retry = 0;
794+ while (retry < max_retries &&
795+ !factory->check_record_count_for_each_buffer(nbuffers, 2))
796+ {
797+ std::this_thread::sleep_for(std::chrono::milliseconds(10));
798+ ++retry;
799+ }
800+ ASSERT_LT(retry, max_retries);
801+
802+ // Scene unchanged. Expect no new frames:
803+ retry = 0;
804+ while (retry < max_retries &&
805+ !factory->check_record_count_for_each_buffer(nbuffers, 2))
806+ {
807+ std::this_thread::sleep_for(std::chrono::milliseconds(10));
808+ ++retry;
809+ }
810+ ASSERT_LT(retry, max_retries);
811+
812+ // Some surface queues up multiple frames
813+ scene->set_pending(3);
814+
815+ // Make sure they all get composited separately (2 + 3 more)
816+ retry = 0;
817+ while (retry < max_retries &&
818+ !factory->check_record_count_for_each_buffer(nbuffers, 5))
819+ {
820+ std::this_thread::sleep_for(std::chrono::milliseconds(10));
821+ ++retry;
822+ }
823+ ASSERT_LT(retry, max_retries);
824+
825+ compositor.stop();
826+}
827+
828 TEST(MultiThreadedCompositor, when_no_initial_composite_is_needed_there_is_none)
829 {
830 using namespace testing;
831
832=== modified file 'tests/unit-tests/scene/test_surface_stack.cpp'
833--- tests/unit-tests/scene/test_surface_stack.cpp 2015-01-29 17:18:29 +0000
834+++ tests/unit-tests/scene/test_surface_stack.cpp 2015-02-03 15:18:44 +0000
835@@ -227,6 +227,39 @@
836 SceneElementFor(stub_surface2)));
837 }
838
839+TEST_F(SurfaceStack, scene_counts_pending_accurately)
840+{
841+ using namespace testing;
842+
843+ ms::SurfaceStack stack{report};
844+ auto surface = std::make_shared<ms::BasicSurface>(
845+ std::string("stub"),
846+ geom::Rectangle{{},{}},
847+ false,
848+ std::make_shared<mtd::StubBufferStream>(),
849+ std::shared_ptr<mir::input::InputChannel>(),
850+ std::shared_ptr<mir::input::InputSender>(),
851+ std::shared_ptr<mg::CursorImage>(),
852+ report);
853+ stack.add_surface(surface, default_params.depth, default_params.input_mode);
854+
855+ EXPECT_EQ(0, stack.frames_pending(this));
856+ post_a_frame(*surface);
857+ post_a_frame(*surface);
858+ post_a_frame(*surface);
859+ EXPECT_EQ(3, stack.frames_pending(this));
860+
861+ for (int expect = 3; expect >= 0; --expect)
862+ {
863+ ASSERT_EQ(expect, stack.frames_pending(this));
864+ auto snap = stack.scene_elements_for(compositor_id);
865+ for (auto& element : snap)
866+ {
867+ auto consumed = element->renderable()->buffer();
868+ }
869+ }
870+}
871+
872 TEST_F(SurfaceStack, surfaces_are_emitted_by_layer)
873 {
874 using namespace testing;

Subscribers

People subscribed via source and target branches