Mir

Merge lp:~kdub/mir/no-rendering-operator into lp:mir

Proposed by Kevin DuBois
Status: Merged
Approved by: Kevin DuBois
Approved revision: no longer in the source branch.
Merged at revision: 1536
Proposed branch: lp:~kdub/mir/no-rendering-operator
Merge into: lp:mir
Diff against target: 342 lines (+58/-198)
7 files modified
src/server/compositor/CMakeLists.txt (+0/-1)
src/server/compositor/default_display_buffer_compositor.cpp (+14/-32)
src/server/compositor/rendering_operator.cpp (+0/-37)
src/server/compositor/rendering_operator.h (+0/-49)
tests/unit-tests/compositor/CMakeLists.txt (+0/-1)
tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp (+44/-2)
tests/unit-tests/compositor/test_rendering_operator.cpp (+0/-76)
To merge this branch: bzr merge lp:~kdub/mir/no-rendering-operator
Reviewer Review Type Date Requested Status
Alan Griffiths Approve
Alexandros Frantzis (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Alberto Aguirre (community) Approve
Review via email: mp+213943@code.launchpad.net

Commit message

Remove the RenderingOperator and VisibilityFilter classes in favor of rendering on the generated list of renderables from the scene

Description of the change

Remove the RenderingOperator and VisibilityFilter classes in favor of rendering on the generated list of renderables from the scene

This does not address the Scene locking problem, but puts us in a good position to do so!

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

271 + EXPECT_EQ(buf1.use_count(), use_count1);
272 + EXPECT_EQ(buf2.use_count(), use_count2);

Expectations are reversed (we can use EXPECT_THAT() which is less error prone).

But, taking a step back, I see two issues with the test:

1. It's checking the number of users *before* posting, whereas we care about the number of users *after* posting, but that's OK for a unit test (that is we are modelling a post_update() that doesn't mess with the buffers, which is reasonable).

2. The test is too strict and dependent on the internals. We are checking that the number of users at posting time is the same as the number of users at rendering time, but that's not what we really care about. What we want to check is that the buffer is held alive until after post_update(), i.e., there is at least one user (besides the test function) at that point:

EXPECT_THAT(buf2.use_count() - 1, Ge(1));
EXPECT_THAT(buf2.use_count() - 1, Ge(1));

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

So (1) is not really an issue just something that caught my eye...

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

Much easier to reason about, lgtm.

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

Looks good.

review: Approve
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

84 + //'renderer.get()' serves as an ID to distinguish itself from other compositors
85 + auto buffer = renderable->buffer(renderer.get());

Seems fragile, but not a reason to block this change.

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

> 84 + //'renderer.get()' serves as an ID to distinguish itself from other
> compositors
> 85 + auto buffer = renderable->buffer(renderer.get());
>
> Seems fragile, but not a reason to block this change.

indeed, just preserving what was there before. I'll have to rectify that situation before the android stuff can work.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/server/compositor/CMakeLists.txt'
2--- src/server/compositor/CMakeLists.txt 2014-03-06 06:05:17 +0000
3+++ src/server/compositor/CMakeLists.txt 2014-04-03 23:09:17 +0000
4@@ -6,7 +6,6 @@
5 temporary_buffers.cpp
6 buffer_stream_factory.cpp
7 buffer_stream_surfaces.cpp
8- rendering_operator.cpp
9 gl_renderer.cpp
10 gl_renderer_factory.cpp
11 multi_threaded_compositor.cpp
12
13=== modified file 'src/server/compositor/default_display_buffer_compositor.cpp'
14--- src/server/compositor/default_display_buffer_compositor.cpp 2014-04-01 19:44:01 +0000
15+++ src/server/compositor/default_display_buffer_compositor.cpp 2014-04-03 23:09:17 +0000
16@@ -19,8 +19,8 @@
17
18 #include "default_display_buffer_compositor.h"
19
20-#include "rendering_operator.h"
21 #include "mir/compositor/scene.h"
22+#include "mir/compositor/renderer.h"
23 #include "mir/graphics/renderable.h"
24 #include "mir/graphics/display_buffer.h"
25 #include "mir/graphics/buffer.h"
26@@ -34,32 +34,6 @@
27 namespace mc = mir::compositor;
28 namespace mg = mir::graphics;
29
30-//TODO remove VisibilityFilter once we don't need filters/operators for rendering
31-namespace
32-{
33-struct VisibilityFilter : public mc::FilterForScene
34-{
35-public:
36- VisibilityFilter(
37- mg::RenderableList const& renderable_list)
38- : list(renderable_list)
39- {
40- }
41-
42- bool operator()(mg::Renderable const& r)
43- {
44- auto matcher = [&r](std::shared_ptr<mg::Renderable> const& renderable)
45- {
46- return (renderable.get() == &r);
47- };
48- return (std::find_if(list.begin(), list.end(), matcher) != list.end());
49- }
50-
51-private:
52- mg::RenderableList const& list;
53-};
54-}
55-
56 mc::DefaultDisplayBufferCompositor::DefaultDisplayBufferCompositor(
57 mg::DisplayBuffer& display_buffer,
58 std::shared_ptr<mc::Scene> const& scene,
59@@ -124,20 +98,28 @@
60
61 if (!bypassed)
62 {
63+ //preserves buffers backing GL textures until after post_update
64+ std::vector<std::shared_ptr<mg::Buffer>> saved_resources;
65+
66 display_buffer.make_current();
67
68 auto const& view_area = display_buffer.view_area();
69 auto renderable_list = scene->generate_renderable_list();
70 mc::filter_occlusions_from(renderable_list, view_area);
71
72+ renderer->set_rotation(display_buffer.orientation());
73+ renderer->begin();
74+
75 for(auto const& renderable : renderable_list)
76+ {
77 uncomposited_buffers |= (renderable->buffers_ready_for_compositor() > 1);
78
79- renderer->set_rotation(display_buffer.orientation());
80- renderer->begin();
81- mc::RenderingOperator applicator(*renderer);
82- VisibilityFilter selector(renderable_list);
83- scene->for_each_if(selector, applicator);
84+ //'renderer.get()' serves as an ID to distinguish itself from other compositors
85+ auto buffer = renderable->buffer(renderer.get());
86+ renderer->render(*renderable, *buffer);
87+ saved_resources.push_back(buffer);
88+ }
89+
90 renderer->end();
91
92 display_buffer.post_update();
93
94=== removed file 'src/server/compositor/rendering_operator.cpp'
95--- src/server/compositor/rendering_operator.cpp 2014-03-26 23:41:48 +0000
96+++ src/server/compositor/rendering_operator.cpp 1970-01-01 00:00:00 +0000
97@@ -1,37 +0,0 @@
98-/*
99- * Copyright © 2012 Canonical Ltd.
100- *
101- * This program is free software: you can redistribute it and/or modify it
102- * under the terms of the GNU General Public License version 3,
103- * as published by the Free Software Foundation.
104- *
105- * This program is distributed in the hope that it will be useful,
106- * but WITHOUT ANY WARRANTY; without even the implied warranty of
107- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
108- * GNU General Public License for more details.
109- *
110- * You should have received a copy of the GNU General Public License
111- * along with this program. If not, see <http://www.gnu.org/licenses/>.
112- *
113- * Authored by: Kevin DuBois <kevin.dubois@canonical.com>
114- */
115-
116-#include "rendering_operator.h"
117-#include "mir/compositor/buffer_stream.h"
118-#include "mir/graphics/renderable.h"
119-
120-namespace mc=mir::compositor;
121-
122-mc::RenderingOperator::RenderingOperator(
123- Renderer& renderer)
124- : renderer(renderer)
125-{
126-}
127-
128-void mc::RenderingOperator::operator()(graphics::Renderable const& renderable)
129-{
130- auto compositor_buffer = renderable.buffer(&renderer);
131- // preserves buffers used in rendering until after post_update()
132- saved_resources.push_back(compositor_buffer);
133- renderer.render(renderable, *compositor_buffer);
134-}
135
136=== removed file 'src/server/compositor/rendering_operator.h'
137--- src/server/compositor/rendering_operator.h 2014-03-26 23:41:48 +0000
138+++ src/server/compositor/rendering_operator.h 1970-01-01 00:00:00 +0000
139@@ -1,49 +0,0 @@
140-/*
141- * Copyright © 2012 Canonical Ltd.
142- *
143- * This program is free software: you can redistribute it and/or modify it
144- * under the terms of the GNU General Public License version 3,
145- * as published by the Free Software Foundation.
146- *
147- * This program is distributed in the hope that it will be useful,
148- * but WITHOUT ANY WARRANTY; without even the implied warranty of
149- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
150- * GNU General Public License for more details.
151- *
152- * You should have received a copy of the GNU General Public License
153- * along with this program. If not, see <http://www.gnu.org/licenses/>.
154- *
155- * Authored by: Kevin DuBois <kevin.dubois@canonical.com>
156- */
157-#ifndef MIR_COMPOSITOR_RENDERING_OPERATOR_H_
158-#define MIR_COMPOSITOR_RENDERING_OPERATOR_H_
159-
160-#include "mir/compositor/renderer.h"
161-#include "mir/compositor/scene.h"
162-
163-#include <vector>
164-#include <functional>
165-#include <memory>
166-
167-namespace mir
168-{
169-namespace compositor
170-{
171-
172-class RenderingOperator : public OperatorForScene
173-{
174-public:
175- explicit RenderingOperator(
176- Renderer& renderer);
177- ~RenderingOperator() = default;
178-
179- void operator()(graphics::Renderable const&);
180-
181-private:
182- Renderer& renderer;
183- std::vector<std::shared_ptr<void>> saved_resources;
184-};
185-
186-}
187-}
188-#endif /* MIR_COMPOSITOR_RENDERING_OPERATOR_H_ */
189
190=== modified file 'tests/unit-tests/compositor/CMakeLists.txt'
191--- tests/unit-tests/compositor/CMakeLists.txt 2014-03-06 06:05:17 +0000
192+++ tests/unit-tests/compositor/CMakeLists.txt 2014-04-03 23:09:17 +0000
193@@ -2,7 +2,6 @@
194 ${CMAKE_CURRENT_SOURCE_DIR}/test_default_display_buffer_compositor.cpp
195 ${CMAKE_CURRENT_SOURCE_DIR}/test_buffer_stream.cpp
196 ${CMAKE_CURRENT_SOURCE_DIR}/test_temporary_buffers.cpp
197- ${CMAKE_CURRENT_SOURCE_DIR}/test_rendering_operator.cpp
198 ${CMAKE_CURRENT_SOURCE_DIR}/test_multi_threaded_compositor.cpp
199 ${CMAKE_CURRENT_SOURCE_DIR}/test_switching_bundle.cpp
200 ${CMAKE_CURRENT_SOURCE_DIR}/test_gl_renderer.cpp
201
202=== modified file 'tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp'
203--- tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp 2014-04-01 20:39:59 +0000
204+++ tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp 2014-04-03 23:09:17 +0000
205@@ -119,8 +119,6 @@
206 .Times(1);
207 EXPECT_CALL(scene, generate_renderable_list())
208 .Times(1);
209- EXPECT_CALL(scene, for_each_if(_,_))
210- .Times(1);
211 EXPECT_CALL(display_buffer, post_update())
212 .Times(1);
213
214@@ -555,3 +553,47 @@
215 EXPECT_TRUE(compositor.composite());
216 EXPECT_FALSE(compositor.composite());
217 }
218+
219+TEST_F(DefaultDisplayBufferCompositor, buffers_held_until_post_update_is_done)
220+{
221+ using namespace testing;
222+ auto mock_renderable1 = std::make_shared<NiceMock<mtd::MockRenderable>>();
223+ auto mock_renderable2 = std::make_shared<NiceMock<mtd::MockRenderable>>();
224+ auto buf1 = std::make_shared<mtd::StubBuffer>();
225+ auto buf2 = std::make_shared<mtd::StubBuffer>();
226+ ON_CALL(*mock_renderable1, buffer(_))
227+ .WillByDefault(Return(buf1));
228+ ON_CALL(*mock_renderable2, buffer(_))
229+ .WillByDefault(Return(buf2));
230+ ON_CALL(display_buffer, view_area())
231+ .WillByDefault(Return(geom::Rectangle{{0,0},{14,14}}));
232+ EXPECT_CALL(*mock_renderable1, screen_position())
233+ .WillRepeatedly(Return(geom::Rectangle{{1,2}, {3,4}}));
234+ EXPECT_CALL(*mock_renderable2, screen_position())
235+ .WillRepeatedly(Return(geom::Rectangle{{0,2}, {3,4}}));
236+
237+ mg::RenderableList list{
238+ mock_renderable1,
239+ mock_renderable2
240+ };
241+ FakeScene scene(list);
242+
243+ long test_use_count1{buf1.use_count()};
244+ long test_use_count2{buf2.use_count()};
245+
246+ EXPECT_CALL(display_buffer, post_update())
247+ .Times(1)
248+ .WillOnce(Invoke([&test_use_count1, &test_use_count2, &buf1, &buf2]()
249+ {
250+ EXPECT_GT(buf1.use_count(), test_use_count1);
251+ EXPECT_GT(buf2.use_count(), test_use_count2);
252+ }));
253+
254+ mc::DefaultDisplayBufferCompositor compositor(
255+ display_buffer,
256+ mt::fake_shared(scene),
257+ mt::fake_shared(mock_renderer),
258+ mr::null_compositor_report());
259+ compositor.composite();
260+}
261+
262
263=== removed file 'tests/unit-tests/compositor/test_rendering_operator.cpp'
264--- tests/unit-tests/compositor/test_rendering_operator.cpp 2014-03-26 05:48:59 +0000
265+++ tests/unit-tests/compositor/test_rendering_operator.cpp 1970-01-01 00:00:00 +0000
266@@ -1,76 +0,0 @@
267-/*
268- * Copyright © 2012 Canonical Ltd.
269- *
270- * This program is free software: you can redistribute it and/or modify
271- * it under the terms of the GNU General Public License version 3 as
272- * published by the Free Software Foundation.
273- *
274- * This program is distributed in the hope that it will be useful,
275- * but WITHOUT ANY WARRANTY; without even the implied warranty of
276- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
277- * GNU General Public License for more details.
278- *
279- * You should have received a copy of the GNU General Public License
280- * along with this program. If not, see <http://www.gnu.org/licenses/>.
281- *
282- * Authored by: Kevin DuBois <kevin.dubois@canonical.com>
283- */
284-
285-#include "src/server/compositor/rendering_operator.h"
286-#include "mir/geometry/rectangle.h"
287-#include "mir_test_doubles/mock_renderer.h"
288-#include "mir_test_doubles/mock_buffer_stream.h"
289-#include "mir_test_doubles/mock_renderable.h"
290-#include "mir_test_doubles/stub_buffer.h"
291-
292-#include <gmock/gmock.h>
293-#include <gtest/gtest.h>
294-
295-namespace mc = mir::compositor;
296-namespace mg = mir::graphics;
297-namespace geom = mir::geometry;
298-namespace mtd = mir::test::doubles;
299-namespace mg = mir::graphics;
300-
301-TEST(RenderingOperator, render_operator_saves_resources)
302-{
303- using namespace testing;
304-
305- mtd::MockRenderer mock_renderer;
306- mtd::MockRenderable mock_renderable;
307- auto stub_buffer0 = std::make_shared<mtd::StubBuffer>();
308- auto stub_buffer1 = std::make_shared<mtd::StubBuffer>();
309- auto stub_buffer2 = std::make_shared<mtd::StubBuffer>();
310-
311- EXPECT_CALL(mock_renderable, buffer(_))
312- .Times(3)
313- .WillOnce(Return(stub_buffer0))
314- .WillOnce(Return(stub_buffer1))
315- .WillOnce(Return(stub_buffer2));
316-
317- Sequence seq;
318- EXPECT_CALL(mock_renderer, render(Ref(mock_renderable), Ref(*stub_buffer0)))
319- .InSequence(seq);
320- EXPECT_CALL(mock_renderer, render(Ref(mock_renderable), Ref(*stub_buffer1)))
321- .InSequence(seq);
322- EXPECT_CALL(mock_renderer, render(Ref(mock_renderable), Ref(*stub_buffer2)))
323- .InSequence(seq);
324-
325- auto use_count_before0 = stub_buffer0.use_count();
326- auto use_count_before1 = stub_buffer1.use_count();
327- auto use_count_before2 = stub_buffer2.use_count();
328- {
329- mc::RenderingOperator rendering_operator(mock_renderer);
330- rendering_operator(mock_renderable);
331- rendering_operator(mock_renderable);
332- rendering_operator(mock_renderable);
333-
334- EXPECT_EQ(use_count_before0 + 1, stub_buffer0.use_count());
335- EXPECT_EQ(use_count_before1 + 1, stub_buffer1.use_count());
336- EXPECT_EQ(use_count_before2 + 1, stub_buffer2.use_count());
337- }
338-
339- EXPECT_EQ(use_count_before0, stub_buffer0.use_count());
340- EXPECT_EQ(use_count_before1, stub_buffer1.use_count());
341- EXPECT_EQ(use_count_before2, stub_buffer2.use_count());
342-}

Subscribers

People subscribed via source and target branches