Mir

Merge lp:~vanvugt/mir/save-resources-in-the-renderer into lp:mir

  1. save-resources-in-the-renderer
  2. Merge into development-branch
Proposed by Daniel van Vugt
Status: Merged
Approved by: Daniel van Vugt
Approved revision: no longer in the source branch.
Merged at revision: 1567
Proposed branch: lp:~vanvugt/mir/save-resources-in-the-renderer
Merge into: lp:mir
Diff against target: 578 lines (+111/-106)
9 files modified
include/server/mir/compositor/gl_renderer.h (+3/-2)
include/server/mir/compositor/renderer.h (+1/-5)
include/test/mir_test_doubles/mock_renderer.h (+1/-1)
include/test/mir_test_doubles/stub_renderer.h (+4/-2)
src/server/compositor/default_display_buffer_compositor.cpp (+2/-10)
src/server/compositor/gl_renderer.cpp (+8/-3)
tests/integration-tests/test_surface_first_frame_sync.cpp (+1/-1)
tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp (+29/-42)
tests/unit-tests/compositor/test_gl_renderer.cpp (+62/-40)
To merge this branch: bzr merge lp:~vanvugt/mir/save-resources-in-the-renderer
Reviewer Review Type Date Requested Status
Daniel van Vugt Approve
PS Jenkins bot (community) continuous-integration Approve
Robert Carr (community) Abstain
Alexandros Frantzis (community) Abstain
Alberto Aguirre (community) Approve
Kevin DuBois (community) Approve
Review via email: mp+215197@code.launchpad.net

Commit message

Move saved_resources into GLRenderer, which is more logical since the
requirement to save resources is a GL one, and it simplifies parameters
too.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:1554
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1300/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1560
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1558
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1133
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1032
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1032/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1037
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1037/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1134
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1134/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1050
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/5918

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

review: Approve (continuous-integration)
Revision history for this message
Kevin DuBois (kdub) wrote :

The renderable functions still being const seem questionable, but not a blocker.
I've often had the though that mg::Buffer::bind_to_texture is acquiring a resource (a gl texture binding) and the subtlety of why we have to hold the buffer to the end of post_update has to do with the fact that RAII isn't present around mg::Buffer::bind_to_texture. Looks good to me though!

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

~~~
108+ renderer->render(*renderable);
~~~

Try pronouncing that fast 10 times :)

review: Approve
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

111 + display_buffer.post_update();
112 renderer->end();
113
114 - display_buffer.post_update();

I am torn. My main issue is that the usual order is to have distinct phases: render, post, render, post etc, so the need to reorder post and renderer->end() is not intuitive. If I was given the interfaces without any knowledge of the internals I would use them like we did before (first end then post).

I understand the motivation, but this now makes the interface tricky to use. We should at least clearly document in the Renderer interface how operations should be ordered, but ideally we should find a way to make correct use more intuitive at the interface level.

review: Abstain
Revision history for this message
Robert Carr (robertcarr) wrote :

Lots of feedback on this already so not doign an indepth review.

I think Alexandros observation is a clear sign that in fact the resources should not be saved in the renderer, as it creates a coupling (to the timing of the display buffer update) which cant be adequately expressed in the interface, i.e. hidden complexity and a difficult to use interface.

Resource saving is not really GL specific is it? So far we have a GL specific implementation but any graphic system I can imagine has some concept of needing to keep buffers locked until they have passed a certain stage in some sort of pipeline like thing (even software rendering to image files).

review: Abstain
Revision history for this message
Kevin DuBois (kdub) wrote :

> 111 + display_buffer.post_update();
> 112 renderer->end();
> 113
> 114 - display_buffer.post_update();
>
> I am torn. My main issue is that the usual order is to have distinct phases:
> render, post, render, post etc, so the need to reorder post and
> renderer->end() is not intuitive. If I was given the interfaces without any
> knowledge of the internals I would use them like we did before (first end then
> post).

I was a bit torn, too, but approved because its no worse than before, and in a vague sense, we're 'cleaning up the render' up all in one area now.

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

> I think Alexandros observation is a clear sign that in fact the resources
> should not be saved in the renderer, as it creates a coupling (to the timing
> of the display buffer update) which cant be adequately expressed in the
> interface, i.e. hidden complexity and a difficult to use interface.
>

There's already timing coupling just currently coupled in display buffer compositor.
In fact, the display buffer compositor doesn't even "know" why it has to save resources - there's an implicit
assumption that GLRenderer being used - so moving the requirement to GLRenderer cleans up that assumption.

> Resource saving is not really GL specific is it? So far we have a GL specific
> implementation but any graphic system I can imagine has some concept of
> needing to keep buffers locked until they have passed a certain stage in some
> sort of pipeline like thing (even software rendering to image files).

Currently yes it's specific to GLRenderer. I would argue the components you mention should be responsible for holding
the resources they need.

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

> I understand the motivation, but this now makes the interface tricky to use.
> We should at least clearly document in the Renderer interface how operations
> should be ordered, but ideally we should find a way to make correct use more
> intuitive at the interface level.

Maybe a renderer->posted() method? But I would be fine if that comes in a separate MP.

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

kdub:
I agree about Renderer const'ness. It's a pain, although const would seem correct looking from the top down. In future proposals I think I would like to return more info from bind_to_texture. Already thinking about that.

Alberto:
Tongue definitely twisted. But I think it's a sign we're moving toward a more logical design;
renderer->render(*renderable);

Alexandros:
post-then-end made me a bit torn too. However;
  1. We never defined what end() means so are free to redefine it.
  2. There is a test checking that end() does indeed come after post(). You can't easily regress it without also deleting the test case.

Robert:
save_resources is most definitely GL-specific. In fact it's specifically only required (arguably and vaguely) due to GL_OES_EGL_image (although the spec [1] does not make that clear. We've lived without saved_resources working for a couple of months now and no one reported issues).
[1] https://www.khronos.org/registry/gles/extensions/OES/OES_EGL_image.txt
The need to save resources is specific to the GL_OES_EGL_image extension, and specific to the pipelined architecture of OpenGL. It is GL-specific. See also bug 1264934!

Alberto:
Yeah, renaming end() to something else wouldn't hurt, if it clarifies any confusion.

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

-"We've lived without saved_resources working for a couple of months now and no one reported issues"

Sorry, that's incorrect. It was working and still is. For several days last week I thought it wasn't but now I remember I was wrong. </jetlag>

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

Top approval is deferred so we don't interfere with the pending release of 0.1.9

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:1558
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1352/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1642
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1640
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1215
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1084
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1084/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1089
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1089/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1216
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1216/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1125
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/6154

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

review: Approve (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/server/mir/compositor/gl_renderer.h'
2--- include/server/mir/compositor/gl_renderer.h 2014-04-16 21:26:27 +0000
3+++ include/server/mir/compositor/gl_renderer.h 2014-04-22 03:42:08 +0000
4@@ -26,6 +26,7 @@
5 #include <mir/graphics/renderable.h>
6 #include <GLES2/gl2.h>
7 #include <unordered_map>
8+#include <unordered_set>
9 #include <vector>
10
11 namespace mir
12@@ -43,8 +44,7 @@
13 void set_viewport(geometry::Rectangle const& rect) override;
14 void set_rotation(float degrees) override;
15 void begin() const override;
16- void render(graphics::Renderable const& renderable,
17- graphics::Buffer& buffer) const override;
18+ void render(graphics::Renderable const& renderable) const override;
19 void end() const override;
20
21 // This is called _without_ a GL context:
22@@ -116,6 +116,7 @@
23 bool used;
24 };
25 mutable std::unordered_map<graphics::Renderable::ID, Texture> textures;
26+ mutable std::unordered_set<std::shared_ptr<graphics::Buffer>> saved_resources;
27 mutable bool skipped = false;
28
29 };
30
31=== modified file 'include/server/mir/compositor/renderer.h'
32--- include/server/mir/compositor/renderer.h 2014-03-26 05:48:59 +0000
33+++ include/server/mir/compositor/renderer.h 2014-04-22 03:42:08 +0000
34@@ -39,11 +39,7 @@
35 virtual void set_viewport(geometry::Rectangle const& rect) = 0;
36 virtual void set_rotation(float degrees) = 0;
37 virtual void begin() const = 0;
38-
39- // XXX The buffer parameter here could now be replaced with
40- // renderable::buffer().
41- virtual void render(graphics::Renderable const& renderable,
42- graphics::Buffer& buffer) const = 0;
43+ virtual void render(graphics::Renderable const& renderable) const = 0;
44 virtual void end() const = 0;
45
46 virtual void suspend() = 0; // called when begin/render/end skipped
47
48=== modified file 'include/test/mir_test_doubles/mock_renderer.h'
49--- include/test/mir_test_doubles/mock_renderer.h 2014-03-26 05:48:59 +0000
50+++ include/test/mir_test_doubles/mock_renderer.h 2014-04-22 03:42:08 +0000
51@@ -34,7 +34,7 @@
52 MOCK_METHOD1(set_viewport, void(geometry::Rectangle const&));
53 MOCK_METHOD1(set_rotation, void(float));
54 MOCK_CONST_METHOD0(begin, void());
55- MOCK_CONST_METHOD2(render, void(graphics::Renderable const&, graphics::Buffer&));
56+ MOCK_CONST_METHOD1(render, void(graphics::Renderable const&));
57 MOCK_CONST_METHOD0(end, void());
58 MOCK_METHOD0(suspend, void());
59
60
61=== modified file 'include/test/mir_test_doubles/stub_renderer.h'
62--- include/test/mir_test_doubles/stub_renderer.h 2014-03-26 05:48:59 +0000
63+++ include/test/mir_test_doubles/stub_renderer.h 2014-04-22 03:42:08 +0000
64@@ -20,6 +20,7 @@
65 #define MIR_TEST_DOUBLES_STUB_RENDERER_H_
66
67 #include "mir/compositor/renderer.h"
68+#include "mir/graphics/renderable.h"
69
70 namespace mir
71 {
72@@ -43,9 +44,10 @@
73 {
74 }
75
76- void render(graphics::Renderable const&,
77- graphics::Buffer&) const override
78+ void render(graphics::Renderable const& r) const override
79 {
80+ auto buffer = r.buffer(this); // We need to consume a buffer to
81+ // unblock tests with clients.
82 }
83
84 void end() const override
85
86=== modified file 'src/server/compositor/default_display_buffer_compositor.cpp'
87--- src/server/compositor/default_display_buffer_compositor.cpp 2014-04-17 01:34:35 +0000
88+++ src/server/compositor/default_display_buffer_compositor.cpp 2014-04-22 03:42:08 +0000
89@@ -88,9 +88,6 @@
90
91 if (!bypassed)
92 {
93- //preserves buffers backing GL textures until after post_update
94- std::vector<std::shared_ptr<mg::Buffer>> saved_resources;
95-
96 display_buffer.make_current();
97
98 mc::filter_occlusions_from(renderable_list, view_area);
99@@ -101,17 +98,12 @@
100 for(auto const& renderable : renderable_list)
101 {
102 uncomposited_buffers |= (renderable->buffers_ready_for_compositor() > 1);
103-
104- //'renderer.get()' serves as an ID to distinguish itself from other compositors
105- auto buffer = renderable->buffer(renderer.get());
106- renderer->render(*renderable, *buffer);
107- saved_resources.push_back(buffer);
108+ renderer->render(*renderable);
109 }
110
111+ display_buffer.post_update();
112 renderer->end();
113
114- display_buffer.post_update();
115-
116 // This is a frig to avoid lp:1286190
117 if (last_pass_rendered_anything && renderable_list.empty())
118 uncomposited_buffers = true;
119
120=== modified file 'src/server/compositor/gl_renderer.cpp'
121--- src/server/compositor/gl_renderer.cpp 2014-04-16 21:26:27 +0000
122+++ src/server/compositor/gl_renderer.cpp 2014-04-22 03:42:08 +0000
123@@ -128,8 +128,11 @@
124 vertices[3] = {{right, bottom, 0.0f}, {tex_right, tex_bottom}};
125 }
126
127-void mc::GLRenderer::render(mg::Renderable const& renderable, mg::Buffer& buffer) const
128+void mc::GLRenderer::render(mg::Renderable const& renderable) const
129 {
130+ auto buffer = renderable.buffer(this);
131+ saved_resources.insert(buffer);
132+
133 glUseProgram(program);
134
135 if (renderable.shaped() || renderable.alpha() < 1.0f)
136@@ -154,14 +157,14 @@
137 glm::value_ptr(renderable.transformation()));
138 glUniform1f(alpha_uniform_loc, renderable.alpha());
139
140- GLuint surface_tex = load_texture(renderable, buffer);
141+ GLuint surface_tex = load_texture(renderable, *buffer);
142
143 /* Draw */
144 glEnableVertexAttribArray(position_attr_loc);
145 glEnableVertexAttribArray(texcoord_attr_loc);
146
147 std::vector<Primitive> primitives;
148- tessellate(primitives, renderable, buffer.size());
149+ tessellate(primitives, renderable, buffer->size());
150
151 for (auto const& p : primitives)
152 {
153@@ -299,6 +302,8 @@
154 }
155 }
156 skipped = false;
157+
158+ saved_resources.clear();
159 }
160
161 void mc::GLRenderer::suspend()
162
163=== modified file 'tests/integration-tests/test_surface_first_frame_sync.cpp'
164--- tests/integration-tests/test_surface_first_frame_sync.cpp 2014-04-15 05:31:19 +0000
165+++ tests/integration-tests/test_surface_first_frame_sync.cpp 2014-04-22 03:42:08 +0000
166@@ -97,7 +97,7 @@
167 {
168 }
169
170- void render(mg::Renderable const&, mg::Buffer&) const override
171+ void render(mg::Renderable const&) const override
172 {
173 while (write(render_operations_fd, "a", 1) != 1) continue;
174 }
175
176=== modified file 'tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp'
177--- tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp 2014-04-15 05:31:19 +0000
178+++ tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp 2014-04-22 03:42:08 +0000
179@@ -101,7 +101,7 @@
180 using namespace testing;
181 mtd::MockScene scene;
182
183- EXPECT_CALL(mock_renderer, render(_,_))
184+ EXPECT_CALL(mock_renderer, render(_))
185 .Times(0);
186 EXPECT_CALL(display_buffer, view_area())
187 .Times(AtLeast(1));
188@@ -129,14 +129,6 @@
189 auto mock_renderable2 = std::make_shared<NiceMock<mtd::MockRenderable>>();
190 auto mock_renderable3 = std::make_shared<NiceMock<mtd::MockRenderable>>();
191
192- auto buf = std::make_shared<mtd::StubBuffer>();
193- EXPECT_CALL(*mock_renderable1, buffer(_))
194- .WillOnce(Return(buf));
195- EXPECT_CALL(*mock_renderable2, buffer(_))
196- .Times(0);
197- EXPECT_CALL(*mock_renderable3, buffer(_))
198- .WillOnce(Return(buf));
199-
200 glm::mat4 simple;
201 EXPECT_CALL(*mock_renderable1, transformation())
202 .WillOnce(Return(simple));
203@@ -176,11 +168,11 @@
204 };
205 FakeScene scene(list);
206
207- EXPECT_CALL(mock_renderer, render(Ref(*mock_renderable1),_))
208+ EXPECT_CALL(mock_renderer, render(Ref(*mock_renderable1)))
209 .Times(1);
210- EXPECT_CALL(mock_renderer, render(Ref(*mock_renderable2),_))
211+ EXPECT_CALL(mock_renderer, render(Ref(*mock_renderable2)))
212 .Times(0);
213- EXPECT_CALL(mock_renderer, render(Ref(*mock_renderable3),_))
214+ EXPECT_CALL(mock_renderer, render(Ref(*mock_renderable3)))
215 .Times(1);
216
217 mc::DefaultDisplayBufferCompositor compositor(
218@@ -207,9 +199,9 @@
219 .Times(1);
220 EXPECT_CALL(mock_renderer, begin())
221 .Times(0);
222- EXPECT_CALL(mock_renderer, render(Ref(*small),_))
223+ EXPECT_CALL(mock_renderer, render(Ref(*small)))
224 .Times(0);
225- EXPECT_CALL(mock_renderer, render(Ref(*fullscreen),_))
226+ EXPECT_CALL(mock_renderer, render(Ref(*fullscreen)))
227 .Times(0);
228 EXPECT_CALL(mock_renderer, end())
229 .Times(0);
230@@ -252,14 +244,14 @@
231 .InSequence(render_seq);
232 EXPECT_CALL(mock_renderer, begin())
233 .InSequence(render_seq);
234- EXPECT_CALL(mock_renderer, render(Ref(*big),_))
235- .InSequence(render_seq);
236- EXPECT_CALL(mock_renderer, render(Ref(*small),_))
237+ EXPECT_CALL(mock_renderer, render(Ref(*big)))
238+ .InSequence(render_seq);
239+ EXPECT_CALL(mock_renderer, render(Ref(*small)))
240+ .InSequence(render_seq);
241+ EXPECT_CALL(display_buffer, post_update())
242 .InSequence(render_seq);
243 EXPECT_CALL(mock_renderer, end())
244 .InSequence(render_seq);
245- EXPECT_CALL(display_buffer, post_update())
246- .InSequence(render_seq);
247
248 mc::DefaultDisplayBufferCompositor compositor(
249 display_buffer,
250@@ -287,9 +279,9 @@
251 .Times(1);
252 EXPECT_CALL(display_buffer, make_current())
253 .Times(1);
254- EXPECT_CALL(mock_renderer, render(Ref(*fullscreen),_))
255+ EXPECT_CALL(mock_renderer, render(Ref(*fullscreen)))
256 .Times(1);
257- EXPECT_CALL(mock_renderer, render(Ref(*small),_))
258+ EXPECT_CALL(mock_renderer, render(Ref(*small)))
259 .Times(1);
260 EXPECT_CALL(display_buffer, post_update())
261 .Times(1);
262@@ -323,9 +315,9 @@
263 .Times(1);
264 EXPECT_CALL(display_buffer, can_bypass())
265 .WillRepeatedly(Return(false));
266- EXPECT_CALL(mock_renderer, render(Ref(*small),_))
267+ EXPECT_CALL(mock_renderer, render(Ref(*small)))
268 .Times(0); // zero due to occlusion detection
269- EXPECT_CALL(mock_renderer, render(Ref(*fullscreen),_))
270+ EXPECT_CALL(mock_renderer, render(Ref(*fullscreen)))
271 .Times(1);
272
273 mc::DefaultDisplayBufferCompositor compositor(
274@@ -357,9 +349,9 @@
275 };
276 FakeScene scene(list);
277
278- EXPECT_CALL(mock_renderer, render(Ref(*small),_))
279+ EXPECT_CALL(mock_renderer, render(Ref(*small)))
280 .Times(0); // zero due to occlusion detection
281- EXPECT_CALL(mock_renderer, render(Ref(*fullscreen),_))
282+ EXPECT_CALL(mock_renderer, render(Ref(*fullscreen)))
283 .Times(1);
284
285 auto nonbypassable = std::make_shared<mtd::MockBuffer>();
286@@ -400,9 +392,9 @@
287 .InSequence(seq);
288 EXPECT_CALL(mock_renderer, begin())
289 .InSequence(seq);
290- EXPECT_CALL(mock_renderer, render(Ref(*fullscreen),_))
291+ EXPECT_CALL(mock_renderer, render(Ref(*fullscreen)))
292 .InSequence(seq);
293- EXPECT_CALL(mock_renderer, render(Ref(*small),_))
294+ EXPECT_CALL(mock_renderer, render(Ref(*small)))
295 .InSequence(seq);
296 EXPECT_CALL(display_buffer, post_update())
297 .InSequence(seq);
298@@ -422,7 +414,7 @@
299 .InSequence(seq);
300 EXPECT_CALL(mock_renderer, begin())
301 .InSequence(seq);
302- EXPECT_CALL(mock_renderer, render(Ref(*small),_))
303+ EXPECT_CALL(mock_renderer, render(Ref(*small)))
304 .InSequence(seq);
305 EXPECT_CALL(display_buffer, post_update())
306 .InSequence(seq);
307@@ -488,9 +480,9 @@
308 Sequence seq;
309 EXPECT_CALL(display_buffer, make_current())
310 .InSequence(seq);
311- EXPECT_CALL(mock_renderer, render(Ref(*window0),_))
312+ EXPECT_CALL(mock_renderer, render(Ref(*window0)))
313 .InSequence(seq);
314- EXPECT_CALL(mock_renderer, render(Ref(*window3),_))
315+ EXPECT_CALL(mock_renderer, render(Ref(*window3)))
316 .InSequence(seq);
317 EXPECT_CALL(display_buffer, post_update())
318 .InSequence(seq);
319@@ -523,12 +515,12 @@
320 EXPECT_CALL(*mock_renderable, buffers_ready_for_compositor())
321 .InSequence(seq)
322 .WillOnce(Return(2));
323- EXPECT_CALL(*mock_renderable, buffer(_))
324+ EXPECT_CALL(mock_renderer, render(Ref(*mock_renderable)))
325 .InSequence(seq);
326 EXPECT_CALL(*mock_renderable, buffers_ready_for_compositor())
327 .InSequence(seq)
328 .WillOnce(Return(1));
329- EXPECT_CALL(*mock_renderable, buffer(_))
330+ EXPECT_CALL(mock_renderer, render(Ref(*mock_renderable)))
331 .InSequence(seq);
332
333 mg::RenderableList list({mock_renderable});
334@@ -544,7 +536,7 @@
335 EXPECT_FALSE(compositor.composite());
336 }
337
338-TEST_F(DefaultDisplayBufferCompositor, buffers_held_until_post_update_is_done)
339+TEST_F(DefaultDisplayBufferCompositor, renderer_ends_after_post_update)
340 {
341 using namespace testing;
342 auto mock_renderable1 = std::make_shared<NiceMock<mtd::MockRenderable>>();
343@@ -568,16 +560,11 @@
344 };
345 FakeScene scene(list);
346
347- long test_use_count1{buf1.use_count()};
348- long test_use_count2{buf2.use_count()};
349-
350+ Sequence seq;
351 EXPECT_CALL(display_buffer, post_update())
352- .Times(1)
353- .WillOnce(Invoke([&test_use_count1, &test_use_count2, &buf1, &buf2]()
354- {
355- EXPECT_GT(buf1.use_count(), test_use_count1);
356- EXPECT_GT(buf2.use_count(), test_use_count2);
357- }));
358+ .InSequence(seq);
359+ EXPECT_CALL(mock_renderer, end())
360+ .InSequence(seq);
361
362 mc::DefaultDisplayBufferCompositor compositor(
363 display_buffer,
364
365=== modified file 'tests/unit-tests/compositor/test_gl_renderer.cpp'
366--- tests/unit-tests/compositor/test_gl_renderer.cpp 2014-04-17 09:13:13 +0000
367+++ tests/unit-tests/compositor/test_gl_renderer.cpp 2014-04-22 03:42:08 +0000
368@@ -254,6 +254,13 @@
369 EXPECT_CALL(mock_gl, glDrawArrays(_, _, _)).Times(AnyNumber());
370 EXPECT_CALL(mock_gl, glDisableVertexAttribArray(_)).Times(AnyNumber());
371
372+ mock_buffer = std::make_shared<mtd::MockBuffer>();
373+ EXPECT_CALL(*mock_buffer, bind_to_texture()).Times(AnyNumber());
374+ EXPECT_CALL(*mock_buffer, size())
375+ .WillRepeatedly(Return(mir::geometry::Size{123, 456}));
376+
377+ EXPECT_CALL(renderable, id()).WillRepeatedly(Return(&renderable));
378+ EXPECT_CALL(renderable, buffer(_)).WillRepeatedly(Return(mock_buffer));
379 EXPECT_CALL(renderable, shaped()).WillRepeatedly(Return(false));
380 EXPECT_CALL(renderable, alpha()).WillRepeatedly(Return(1.0f));
381 EXPECT_CALL(renderable, transformation()).WillRepeatedly(Return(trans));
382@@ -280,7 +287,7 @@
383 }
384
385 testing::NiceMock<mtd::MockGL> mock_gl;
386- testing::NiceMock<mtd::MockBuffer> mock_buffer;
387+ std::shared_ptr<mtd::MockBuffer> mock_buffer;
388 mir::geometry::Rectangle display_area;
389 glm::mat4 trans;
390 testing::NiceMock<mtd::MockRenderable> renderable;
391@@ -313,7 +320,7 @@
392 .WillOnce(Return(0.0f));
393 EXPECT_CALL(mock_gl, glUniform1f(alpha_uniform_location, _));
394
395- EXPECT_CALL(mock_buffer, id())
396+ EXPECT_CALL(*mock_buffer, id())
397 .WillOnce(Return(mg::BufferID(123)));
398 EXPECT_CALL(mock_gl, glGenTextures(1, _))
399 .WillOnce(SetArgPointee<1>(stub_texture));
400@@ -323,14 +330,9 @@
401 EXPECT_CALL(mock_gl, glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, _));
402 EXPECT_CALL(mock_gl, glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, _));
403
404- EXPECT_CALL(mock_buffer, bind_to_texture());
405-
406 EXPECT_CALL(mock_gl, glEnableVertexAttribArray(position_attr_location));
407 EXPECT_CALL(mock_gl, glEnableVertexAttribArray(texcoord_attr_location));
408
409- EXPECT_CALL(mock_buffer, size())
410- .WillOnce(Return(mir::geometry::Size{123, 456}));
411-
412 EXPECT_CALL(mock_gl, glBindTexture(GL_TEXTURE_2D, stub_texture));
413 EXPECT_CALL(mock_gl, glVertexAttribPointer(position_attr_location, 3,
414 GL_FLOAT, GL_FALSE, _, _));
415@@ -345,7 +347,7 @@
416 EXPECT_CALL(mock_gl, glDeleteTextures(1, Pointee(stub_texture)));
417
418 renderer->begin();
419- renderer->render(renderable, mock_buffer);
420+ renderer->render(renderable);
421 renderer->end();
422 }
423
424@@ -358,7 +360,7 @@
425 .WillOnce(Return(false));
426 EXPECT_CALL(mock_gl, glDisable(GL_BLEND));
427
428- EXPECT_CALL(mock_buffer, id())
429+ EXPECT_CALL(*mock_buffer, id())
430 .WillOnce(Return(mg::BufferID(123)));
431 EXPECT_CALL(mock_gl, glGenTextures(1, _))
432 .WillOnce(SetArgPointee<1>(stub_texture));
433@@ -368,15 +370,11 @@
434 EXPECT_CALL(mock_gl, glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, _));
435 EXPECT_CALL(mock_gl, glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, _));
436
437- EXPECT_CALL(mock_buffer, bind_to_texture());
438- EXPECT_CALL(mock_buffer, size())
439- .WillOnce(Return(mir::geometry::Size{123, 456}));
440-
441 EXPECT_CALL(mock_gl, glBindTexture(GL_TEXTURE_2D, stub_texture));
442 EXPECT_CALL(mock_gl, glDeleteTextures(1, Pointee(stub_texture)));
443
444 renderer->begin();
445- renderer->render(renderable, mock_buffer);
446+ renderer->render(renderable);
447 renderer->end();
448 }
449
450@@ -384,13 +382,10 @@
451 {
452 auto renderer = gl_renderer_factory.create_renderer_for(display_area);
453
454- EXPECT_CALL(mock_buffer, size())
455- .WillRepeatedly(Return(mir::geometry::Size{123, 456}));
456-
457 InSequence seq;
458
459 // First render() - texture generated and uploaded
460- EXPECT_CALL(mock_buffer, id())
461+ EXPECT_CALL(*mock_buffer, id())
462 .WillOnce(Return(mg::BufferID(123)));
463 EXPECT_CALL(mock_gl, glGenTextures(1, _))
464 .WillOnce(SetArgPointee<1>(stub_texture));
465@@ -401,13 +396,12 @@
466 _));
467 EXPECT_CALL(mock_gl, glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER,
468 _));
469- EXPECT_CALL(mock_buffer, bind_to_texture());
470 EXPECT_CALL(mock_gl, glBindTexture(GL_TEXTURE_2D, stub_texture));
471 EXPECT_CALL(mock_gl, glDeleteTextures(_, _))
472 .Times(0);
473
474 // Second render() - texture found in cache and not re-uploaded
475- EXPECT_CALL(mock_buffer, id())
476+ EXPECT_CALL(*mock_buffer, id())
477 .WillOnce(Return(mg::BufferID(123)));
478 EXPECT_CALL(mock_gl, glBindTexture(GL_TEXTURE_2D, stub_texture));
479 EXPECT_CALL(mock_gl, glBindTexture(GL_TEXTURE_2D, stub_texture));
480@@ -415,25 +409,23 @@
481 .Times(0);
482
483 // Third render() - texture found in cache but refreshed with new buffer
484- EXPECT_CALL(mock_buffer, id())
485+ EXPECT_CALL(*mock_buffer, id())
486 .WillOnce(Return(mg::BufferID(456)));
487 EXPECT_CALL(mock_gl, glBindTexture(GL_TEXTURE_2D, stub_texture));
488- EXPECT_CALL(mock_buffer, bind_to_texture());
489 EXPECT_CALL(mock_gl, glBindTexture(GL_TEXTURE_2D, stub_texture));
490 EXPECT_CALL(mock_gl, glDeleteTextures(1, Pointee(stub_texture)))
491 .Times(0);
492
493 // Forth render() - stale texture reuploaded following bypass
494- EXPECT_CALL(mock_buffer, id())
495+ EXPECT_CALL(*mock_buffer, id())
496 .WillOnce(Return(mg::BufferID(456)));
497 EXPECT_CALL(mock_gl, glBindTexture(GL_TEXTURE_2D, stub_texture));
498- EXPECT_CALL(mock_buffer, bind_to_texture());
499 EXPECT_CALL(mock_gl, glBindTexture(GL_TEXTURE_2D, stub_texture));
500 EXPECT_CALL(mock_gl, glDeleteTextures(1, Pointee(stub_texture)))
501 .Times(0);
502
503 // Fifth render() - texture found in cache and not re-uploaded
504- EXPECT_CALL(mock_buffer, id())
505+ EXPECT_CALL(*mock_buffer, id())
506 .WillOnce(Return(mg::BufferID(456)));
507 EXPECT_CALL(mock_gl, glBindTexture(GL_TEXTURE_2D, stub_texture));
508 EXPECT_CALL(mock_gl, glBindTexture(GL_TEXTURE_2D, stub_texture));
509@@ -443,24 +435,54 @@
510 EXPECT_CALL(mock_gl, glDeleteTextures(1, Pointee(stub_texture)));
511
512 renderer->begin();
513- renderer->render(renderable, mock_buffer);
514- renderer->end();
515-
516- renderer->begin();
517- renderer->render(renderable, mock_buffer);
518- renderer->end();
519-
520- renderer->begin();
521- renderer->render(renderable, mock_buffer);
522+ renderer->render(renderable);
523+ renderer->end();
524+
525+ renderer->begin();
526+ renderer->render(renderable);
527+ renderer->end();
528+
529+ renderer->begin();
530+ renderer->render(renderable);
531 renderer->end();
532
533 renderer->suspend();
534
535 renderer->begin();
536- renderer->render(renderable, mock_buffer);
537- renderer->end();
538-
539- renderer->begin();
540- renderer->render(renderable, mock_buffer);
541- renderer->end();
542+ renderer->render(renderable);
543+ renderer->end();
544+
545+ renderer->begin();
546+ renderer->render(renderable);
547+ renderer->end();
548+}
549+
550+TEST_F(GLRenderer, holds_buffers_till_the_end)
551+{
552+ auto renderer = gl_renderer_factory.create_renderer_for(display_area);
553+
554+ InSequence seq;
555+
556+ EXPECT_CALL(*mock_buffer, id())
557+ .WillOnce(Return(mg::BufferID(123)));
558+ EXPECT_CALL(mock_gl, glGenTextures(1, _))
559+ .WillOnce(SetArgPointee<1>(stub_texture));
560+ EXPECT_CALL(mock_gl, glBindTexture(GL_TEXTURE_2D, stub_texture));
561+ EXPECT_CALL(mock_gl, glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, _));
562+ EXPECT_CALL(mock_gl, glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, _));
563+ EXPECT_CALL(mock_gl, glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER,
564+ _));
565+ EXPECT_CALL(mock_gl, glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER,
566+ _));
567+ EXPECT_CALL(mock_gl, glBindTexture(GL_TEXTURE_2D, stub_texture));
568+
569+ EXPECT_CALL(mock_gl, glDeleteTextures(1, Pointee(stub_texture)));
570+
571+ long old_use_count = mock_buffer.use_count();
572+
573+ renderer->begin();
574+ renderer->render(renderable);
575+ EXPECT_EQ(old_use_count+1, mock_buffer.use_count());
576+ renderer->end();
577+ EXPECT_EQ(old_use_count, mock_buffer.use_count());
578 }

Subscribers

People subscribed via source and target branches