Mir

Merge lp:~vanvugt/mir/single-buffer into lp:mir

single-buffer
Merge into development-branch

Proposed by Daniel van Vugt on 2014-04-29

Status:

Rejected

Rejected by:

Daniel van Vugt on 2014-05-06

Proposed branch:

lp:~vanvugt/mir/single-buffer

Merge into:

lp:mir

Diff against target:

287 lines (+59/-22)

4 files modified

src/server/compositor/gl_renderer.cpp (+3/-0)
src/server/compositor/switching_bundle.cpp (+38/-4)
src/server/compositor/switching_bundle.h (+1/-1)
tests/unit-tests/compositor/test_switching_bundle.cpp (+17/-17)

To merge this branch:

bzr merge lp:~vanvugt/mir/single-buffer

Low

Triaged

Link a bug report

Reviewer	Review Type	Date Requested	Status
Daniel van Vugt			Needs Fixing on 2014-05-06
PS Jenkins bot (community)	continuous-integration		Approve on 2014-05-05
Alexandros Frantzis (community)		2014-04-29	Abstain on 2014-04-30
Review via email: mp+217555@code.launchpad.net

Commit message

Add basic support for single buffering at the lowest level. It won't be
turned on yet, but you can try it out by changing the "3" to "1" in
buffer_stream_factory.cpp. (LP: #1194333)

Note that single buffered surfaces are nonblocking (no frame rate control).
This is suitable for most use cases of single buffering. But it doesn't
necessarily always have to be that way.

Description of the change

Why?
Because some applications like painting (fingerpaint) and raytracing etc are rendered cumulatively/additively. The buffer is built up slowly over time, but may get composited many times before it's finished (if ever). Single buffering also ensures such static image surfaces never waste more memory than a single buffer.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-29:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1425/rebuild

review: Approve (continuous-integration)

Revision history for this message

Chris Halse Rogers (raof) wrote on 2014-04-30:

I think this rather neatly demonstrates my opposition to supporting single buffering:

*) It complicates SwitchingBundle by requiring a bunch of explicit “support single buffering” code
*) It complicates code *outside of* switching bundle - see the gl_renderer TODO
*) It invokes undefined GL behaviour - applications are required to do their own synchronisation, and this does none. Best case is we present unfinished rendering, likely worst-case is rendering never makes it to the display thanks to various cache layers. (Obviously the true worst case is that it hacks NORAD, launches nuclear armageddon, and then sets the machine on fire, but that seems pretty remote ☺).

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-04-30:

>> (Obviously the true worst case is that it hacks NORAD, launches nuclear armageddon, and then sets the
>> machine on fire, but that seems pretty remote ☺).

Who are we to say whats right and wrong?

Haha seriously though. I mostly agree with RAOFs concerns...meant to post something this morning.

If we do need single buffering, maybe the way it special cases all of SwitchingBundle means its in fact a separate concrete class.

If we have single buffering GL should probably not be enabled for such surfaces (perhaps no EGLNativeWindowType).

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-04-30:

1. That new complication is just my rush in proving the concept quickly, and proving the assertion that it is easy to support. It's probably possible to reuse the existing logic more elegantly.

2. I knew when I wrote that part of GLRenderer that it would only work with multi-buffering. It's a trivial issue of invalidating the texture cache. Set the bool to true at the right time.

3. Synchronization is pretty irrelevant in single buffering use cases. This is because they all involve either building up an image over a long period of time (so it's slow anyway). Or that they involve never or hardly redrawing the image. So synchronization/throttling is generally irrelevant to single buffering.

But like #1, lack of any synchronization is just from the rushed proof of concept. It's probably possible to implement decent synchronization with some more changes in SwitchingBundle. But not important to single-buffering use cases.

If these are blocking issues, I will work to implement the various items later.

Revision history for this message

Chris Halse Rogers (raof) wrote on 2014-04-30:

As to 3, I dislike intentionally invoking undefined behaviour in one of our foundation libraries that's deeply involved in hardware twiddling.

Synchronisation is not a nice-to-have, it's a blocking issue. We must ensure exclusivity between the compositor and the client.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-04-30:

Well, the purpose of this proposal was to prove it's possible and not complicated. Not to perfect the idea immediately :) Then again, what have we truly perfected?

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-04-30:

Since there is ongoing work for an alternative implementation that is hopefully going to land in the not too distant future (and also supports a single buffer), I don't see the purpose of adding this functionality to the current version.

review: Abstain

lp:~vanvugt/mir/single-buffer updated on 2014-05-01

1586. By Daniel van Vugt on 2014-04-29: Merge latest development-branch
1587. By Daniel van Vugt on 2014-04-30: Merge latest development-branch
1588. By Daniel van Vugt on 2014-05-01: Merge latest development-branch

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-05-01:

The purpose, per discussion on mir-devel was only to ensure the bug does not get closed. Because we can implement it. Which branch implements it is not so important.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-01:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1462/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Chris Halse Rogers (raof) wrote on 2014-05-01:

I'm still not convinced that we *can* implement it reasonably. This sample implementation ignores all the hard problems; of course it's simple.

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-05-01:

> I'm still not convinced that we *can* implement it reasonably. This sample
> implementation ignores all the hard problems; of course it's simple.

Like, android's fencing should probably be disabled if we're doing this, the "exclusivity between the compositor and the client" is baked into the native window type. I think the worst it would do now is glitch/tear/wait (but not deadlock) in various ways if we haven't disabled the fencing, but it is a 'new rock' to turn over that android hasn't.

lp:~vanvugt/mir/single-buffer updated on 2014-05-02

1589. By Daniel van Vugt on 2014-05-02: Merge latest development-branch

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-05-02:

In the case of bypass for example (which does work well with this branch) you have to explicitly avoid exclusivity. Because the compositor needs to hold two buffers (which are the same one in single buffering) at the same time as allowing the client to render to a buffer (again the same one).

Single buffering is not a new or hard problem. I spent the first few years of my graphics career dealing only with single buffering. The only thing actually missing in this branch is the ability to schedule client rendering at a time closest to vertical retrace (compositing in our case) such that you should not usually see any tearing.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-02:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1490/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Chris Halse Rogers (raof) wrote on 2014-05-05:

You don't seem to be responding to any of the concerns raised here?

Specifically:
*) it seems that this will require code outside the BufferBundle implementation to support cases that aren't necessary in multi-buffering.
*) unless you ensure exclusivity we are at best invoking undefined rendering and at worst undefined behaviour in libGL
+) Relatedly, Android has no GL-accessible frontbuffer; if we do this we're attempting to use behaviour unsupported by the drivers' major target.
*) if you ensure exclusivity, how does that interact with the compositor loop and client API?

In return for all these problems and potential problems, we gain a feature with at best a niche use-case.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-05-05:

I think I did respond to them. It's just not obvious which bullet point is which now...

*) it seems that this will require code outside the BufferBundle implementation to support cases that aren't necessary in multi-buffering.

Yes, the change you refer to is the texture caching logic in GL renderer. That's a simple issue of flagging I was aware would happen when I introduced the texture cache, and I'm confident could be solved in a variety of ways. Set the bool differently based on the expected buffer semantics...

*) unless you ensure exclusivity we are at best invoking undefined rendering and at worst undefined behaviour in libGL

Synchronization is conceivably possible, but exclusivity is not important for the primary use cases, as described above. Single buffering is not a new problem, and the worst-case undefined behaviour you refer to is either (a) irrelevant because of the expected use cases not doing complex (many-pixel) rendering in which tearing on artefacts could ever be visible, or (b) solved for all use cases by the later implementation of careful timing (which does not guarantee exclusivity but makes it highly likely on most frames).

+) Relatedly, Android has no GL-accessible frontbuffer; if we do this we're attempting to use behaviour unsupported by the drivers' major target.

Not every feature in Mir needs to be relevant to Android. If it doesn't work on Android, we can just disallow it on that platform.

*) if you ensure exclusivity, how does that interact with the compositor loop and client API?

You can "pulse" the availability of client buffers on vblank boundaries. Or as an analog of that, pulse on compositor frame completion. Again, it's not "exclusivity" because that word is not appropriate here, but it does give you synchronization to minimize the risk of writing to the buffer while it's being read from. Thus minimizing the risk of tearing or other artefacts.

lp:~vanvugt/mir/single-buffer updated on 2014-05-05

1590. By Daniel van Vugt on 2014-05-05: Merge latest development-branch

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-05:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1512/rebuild

review: Approve (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-05-06:

Ugh, everything has changed since BufferQueue landed. This needs rewriting.

review: Needs Fixing

Unmerged revisions

1590. By Daniel van Vugt on 2014-05-05: Merge latest development-branch
1589. By Daniel van Vugt on 2014-05-02: Merge latest development-branch
1588. By Daniel van Vugt on 2014-05-01: Merge latest development-branch
1587. By Daniel van Vugt on 2014-04-30: Merge latest development-branch
1586. By Daniel van Vugt on 2014-04-29: Merge latest development-branch
1585. By Daniel van Vugt on 2014-04-29: Add a TODO comment to gl_renderer.cpp describing future work required.
1584. By Daniel van Vugt on 2014-04-29: Fix failing resize test case (resize support missing for single buffers).
1583. By Daniel van Vugt on 2014-04-29: First prototype of single buffering.
1582. By Daniel van Vugt on 2014-04-29: test_switching_bundle.cpp: Modified a bundle of tests to include single
buffering. They presently fail but should be passable in future.
1581. By Daniel van Vugt on 2014-04-29: Remove the symbolic constant "min_buffers". Making it easy to change could
result in test cases regressing without anyone noticing.

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:

Daniel van Vugt

Emanuele Antonio Faraone

Gerry Boland

Mir development team

1	=== modified file 'src/server/compositor/gl_renderer.cpp'
2	--- src/server/compositor/gl_renderer.cpp 2014-05-02 03:56:16 +0000
3	+++ src/server/compositor/gl_renderer.cpp 2014-05-05 03:15:36 +0000
4	@@ -208,6 +208,9 @@
5	else
6	{
7	glBindTexture(GL_TEXTURE_2D, tex.id);
8	+
9	+ // TODO: If single buffered, ensure changed is always true. Or else
10	+ // the image will appear to freeze never updating the texture.
11	changed = (tex.origin != buf_id) \|\| skipped;
12	}
13	tex.origin = buf_id;
14
15	=== modified file 'src/server/compositor/switching_bundle.cpp'
16	--- src/server/compositor/switching_bundle.cpp 2014-04-15 05:31:19 +0000
17	+++ src/server/compositor/switching_bundle.cpp 2014-05-05 03:15:36 +0000
18	@@ -80,12 +80,10 @@
19	overlapping_compositors{false},
20	framedropping{false}, force_drop{0}
21	{
22	- if (nbuffers < min_buffers \|\| nbuffers > max_buffers)
23	+ if (nbuffers < 1 \|\| nbuffers > max_buffers)
24	{
25	BOOST_THROW_EXCEPTION(std::logic_error("SwitchingBundle only supports "
26	- "nbuffers between " +
27	- std::to_string(min_buffers) +
28	- " and " +
29	+ "nbuffers between 1 and " +
30	std::to_string(max_buffers)));
31	}
32	}
33	@@ -208,6 +206,19 @@
34	void mc::SwitchingBundle::client_acquire(std::function<void(graphics::Buffer* buffer)> complete)
35	{
36	std::unique_lock<std::mutex> lock(guard);
37	+
38	+ if (nbuffers == 1)
39	+ {
40	+ auto buf = alloc_buffer(0);
41	+ if (buf->size() != bundle_properties.size)
42	+ {
43	+ buf = gralloc->alloc_buffer(bundle_properties);
44	+ ring[0].buf = buf;
45	+ }
46	+ complete(buf.get());
47	+ return;
48	+ }
49	+
50	client_acquire_todo = std::move(complete);
51
52	if ((framedropping \|\| force_drop) && nbuffers > 1)
53	@@ -292,6 +303,13 @@
54	{
55	std::unique_lock<std::mutex> lock(guard);
56
57	+ if (nbuffers == 1)
58	+ {
59	+ nready = 1;
60	+ cond.notify_all();
61	+ return;
62	+ }
63	+
64	if (nclients <= 0 \|\| ring[first_client].buf.get() != released_buffer)
65	{
66	BOOST_THROW_EXCEPTION(std::logic_error(
67	@@ -308,6 +326,10 @@
68	void const* user_id)
69	{
70	std::unique_lock<std::mutex> lock(guard);
71	+
72	+ if (nbuffers == 1)
73	+ return alloc_buffer(0);
74	+
75	int compositor;
76
77	// Multi-monitor acquires close to each other get the same frame:
78	@@ -354,6 +376,9 @@
79
80	void mc::SwitchingBundle::compositor_release(std::shared_ptr<mg::Buffer> const& released_buffer)
81	{
82	+ if (nbuffers == 1)
83	+ return;
84	+
85	std::unique_lock<std::mutex> lock(guard);
86	int compositor = -1;
87
88	@@ -390,6 +415,9 @@
89	{
90	std::unique_lock<std::mutex> lock(guard);
91
92	+ if (nbuffers == 1)
93	+ return alloc_buffer(0);
94	+
95	/*
96	* Note that "nsnapshotters" is a separate counter to ring[x].users.
97	* This is because snapshotters should be completely passive and should
98	@@ -416,6 +444,9 @@
99
100	void mc::SwitchingBundle::snapshot_release(std::shared_ptr<mg::Buffer> const& released_buffer)
101	{
102	+ if (nbuffers == 1)
103	+ return;
104	+
105	std::unique_lock<std::mutex> lock(guard);
106
107	if (nsnapshotters <= 0 \|\| ring[snapshot].buf != released_buffer)
108	@@ -434,6 +465,9 @@
109
110	void mc::SwitchingBundle::force_requests_to_complete()
111	{
112	+ if (nbuffers == 1)
113	+ return;
114	+
115	std::unique_lock<std::mutex> lock(guard);
116	if (client_acquire_todo)
117	{
118
119	=== modified file 'src/server/compositor/switching_bundle.h'
120	--- src/server/compositor/switching_bundle.h 2014-03-26 05:48:59 +0000
121	+++ src/server/compositor/switching_bundle.h 2014-05-05 03:15:36 +0000
122	@@ -40,7 +40,7 @@
123	class SwitchingBundle : public BufferBundle
124	{
125	public:
126	- enum {min_buffers = 1, max_buffers = 5};
127	+ enum {max_buffers = 5};
128
129	SwitchingBundle(int nbuffers,
130	const std::shared_ptr<graphics::GraphicBufferAllocator> &,
131
132	=== modified file 'tests/unit-tests/compositor/test_switching_bundle.cpp'
133	--- tests/unit-tests/compositor/test_switching_bundle.cpp 2014-04-15 05:31:19 +0000
134	+++ tests/unit-tests/compositor/test_switching_bundle.cpp 2014-05-05 03:15:36 +0000
135	@@ -87,7 +87,7 @@
136	mir_pixel_format_argb_8888,
137	mg::BufferUsage::software};
138
139	- for (int nbuffers = mc::SwitchingBundle::min_buffers;
140	+ for (int nbuffers = 1;
141	nbuffers <= mc::SwitchingBundle::max_buffers;
142	++nbuffers)
143	{
144	@@ -123,7 +123,7 @@
145
146	TEST_F(SwitchingBundleTest, client_acquire_basic)
147	{
148	- for (int nbuffers = mc::SwitchingBundle::min_buffers;
149	+ for (int nbuffers = 1;
150	nbuffers <= mc::SwitchingBundle::max_buffers;
151	++nbuffers)
152	{
153	@@ -147,7 +147,7 @@
154
155	TEST_F(SwitchingBundleTest, is_really_synchronous)
156	{
157	- for (int nbuffers = mc::SwitchingBundle::min_buffers;
158	+ for (int nbuffers = 1;
159	nbuffers <= mc::SwitchingBundle::max_buffers;
160	++nbuffers)
161	{
162	@@ -184,7 +184,7 @@
163
164	TEST_F(SwitchingBundleTest, framedropping_clients_never_block)
165	{
166	- for (int nbuffers = 2;
167	+ for (int nbuffers = 1;
168	nbuffers <= mc::SwitchingBundle::max_buffers;
169	++nbuffers)
170	{
171	@@ -256,7 +256,7 @@
172
173	TEST_F(SwitchingBundleTest, compositor_acquire_basic)
174	{
175	- for (int nbuffers = mc::SwitchingBundle::min_buffers;
176	+ for (int nbuffers = 1;
177	nbuffers <= mc::SwitchingBundle::max_buffers;
178	++nbuffers)
179	{
180	@@ -274,7 +274,7 @@
181
182	TEST_F(SwitchingBundleTest, multimonitor_frame_sync)
183	{
184	- for (int nbuffers = mc::SwitchingBundle::min_buffers;
185	+ for (int nbuffers = 1;
186	nbuffers <= mc::SwitchingBundle::max_buffers;
187	++nbuffers)
188	{
189	@@ -319,7 +319,7 @@
190
191	TEST_F(SwitchingBundleTest, compositor_acquire_never_blocks)
192	{
193	- for (int nbuffers = mc::SwitchingBundle::min_buffers;
194	+ for (int nbuffers = 1;
195	nbuffers <= mc::SwitchingBundle::max_buffers;
196	++nbuffers)
197	{
198	@@ -339,7 +339,7 @@
199
200	TEST_F(SwitchingBundleTest, compositor_acquire_recycles_latest_ready_buffer)
201	{
202	- for (int nbuffers = mc::SwitchingBundle::min_buffers;
203	+ for (int nbuffers = 1;
204	nbuffers <= mc::SwitchingBundle::max_buffers;
205	++nbuffers)
206	{
207	@@ -454,7 +454,7 @@
208
209	TEST_F(SwitchingBundleTest, snapshot_acquire_basic)
210	{
211	- for (int nbuffers = mc::SwitchingBundle::min_buffers;
212	+ for (int nbuffers = 1;
213	nbuffers <= mc::SwitchingBundle::max_buffers;
214	++nbuffers)
215	{
216	@@ -470,7 +470,7 @@
217
218	TEST_F(SwitchingBundleTest, snapshot_acquire_never_blocks)
219	{
220	- for (int nbuffers = mc::SwitchingBundle::min_buffers;
221	+ for (int nbuffers = 1;
222	nbuffers <= mc::SwitchingBundle::max_buffers;
223	++nbuffers)
224	{
225	@@ -567,7 +567,7 @@
226
227	TEST_F(SwitchingBundleTest, stress)
228	{
229	- for (int nbuffers = 2;
230	+ for (int nbuffers = 1;
231	nbuffers <= mc::SwitchingBundle::max_buffers;
232	++nbuffers)
233	{
234	@@ -613,7 +613,7 @@
235	* unique ones while it makes sense to do so. Buffers are big things and
236	* should be allocated sparingly...
237	*/
238	- for (int nbuffers = mc::SwitchingBundle::min_buffers;
239	+ for (int nbuffers = 1;
240	nbuffers <= mc::SwitchingBundle::max_buffers;
241	++nbuffers)
242	{
243	@@ -683,7 +683,7 @@
244
245	TEST_F(SwitchingBundleTest, framedropping_clients_get_all_buffers)
246	{
247	- for (int nbuffers = 2;
248	+ for (int nbuffers = 1;
249	nbuffers <= mc::SwitchingBundle::max_buffers;
250	++nbuffers)
251	{
252	@@ -718,7 +718,7 @@
253
254	TEST_F(SwitchingBundleTest, waiting_clients_unblock_on_shutdown)
255	{
256	- for (int nbuffers = 2;
257	+ for (int nbuffers = 1;
258	nbuffers <= mc::SwitchingBundle::max_buffers;
259	++nbuffers)
260	{
261	@@ -741,7 +741,7 @@
262
263	TEST_F(SwitchingBundleTest, waiting_clients_unblock_on_vt_switch_not_permanent)
264	{ // Regression test for LP: #1207226
265	- for (int nbuffers = 2;
266	+ for (int nbuffers = 1;
267	nbuffers <= mc::SwitchingBundle::max_buffers;
268	++nbuffers)
269	{
270	@@ -860,7 +860,7 @@
271
272	TEST_F(SwitchingBundleTest, resize_affects_client_acquires_immediately)
273	{
274	- for (int nbuffers = mc::SwitchingBundle::min_buffers;
275	+ for (int nbuffers = 1;
276	nbuffers <= mc::SwitchingBundle::max_buffers;
277	++nbuffers)
278	{
279	@@ -887,7 +887,7 @@
280
281	TEST_F(SwitchingBundleTest, compositor_acquires_resized_frames)
282	{
283	- for (int nbuffers = mc::SwitchingBundle::min_buffers;
284	+ for (int nbuffers = 1;
285	nbuffers <= mc::SwitchingBundle::max_buffers;
286	++nbuffers)
287	{