Mir

Merge lp:~vanvugt/mir/rotate-renderer into lp:mir

  1. rotate-renderer
  2. Merge into development-branch
Proposed by Daniel van Vugt
Status: Merged
Approved by: Alan Griffiths
Approved revision: no longer in the source branch.
Merged at revision: 1345
Proposed branch: lp:~vanvugt/mir/rotate-renderer
Merge into: lp:mir
Prerequisite: lp:~vanvugt/mir/rotate-display-buffer
Diff against target: 288 lines (+47/-18)
11 files modified
include/test/mir_test_doubles/mock_surface_renderer.h (+1/-1)
src/server/compositor/default_display_buffer_compositor.cpp (+1/-1)
src/server/compositor/gl_renderer.cpp (+15/-2)
src/server/compositor/gl_renderer.h (+2/-1)
src/server/compositor/renderer.h (+1/-1)
tests/acceptance-tests/test_server_shutdown.cpp (+1/-1)
tests/integration-tests/test_session.cpp (+1/-1)
tests/integration-tests/test_surface_first_frame_sync.cpp (+1/-1)
tests/mir_test_framework/stubbed_server_configuration.cpp (+1/-1)
tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp (+20/-4)
tests/unit-tests/compositor/test_gl_renderer.cpp (+3/-4)
To merge this branch: bzr merge lp:~vanvugt/mir/rotate-renderer
Reviewer Review Type Date Requested Status
Alan Griffiths Approve
PS Jenkins bot (community) continuous-integration Approve
Alexandros Frantzis (community) Approve
Review via email: mp+202425@code.launchpad.net

Commit message

Implement screen rotation in GLRenderer, for platforms which can't do it
natively in DisplayBuffer.

Description of the change

I'm intentionally not testing the matrix used in GLRenderer::begin(). Because rotation is a very visual thing. Just verifying that the matrix has expected contents does not in any way validate that anything will look right on screen. It would just add more unnecessary coupling.

As it is, most of the relevant calculations are done in the vertex shader. And we're not about to write automated tests for that any time soon.

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

PASSED: Continuous integration, rev:1365
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/695/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/684
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/680
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/286
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/425
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/425/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/429
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/429/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/286
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/286/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/299
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/3218

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

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

73 + glUniformMatrix4fv(display_transform_uniform_loc, 1, GL_TRUE, rot);

glUniform* functions need an active shader program. We should move glUseProgram() from render() to begin() (which makes sense anyway).

94 + mutable GLuint display_transform_uniform_loc;

No need for mutable.

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

Thanks Alexandros. All fixed.

Instead of moving glUseProgram, I just inserted a new one. Because I imagine in the not-too-distant future shells will get some control of the rendering loop. And when that happens we can no longer assume there's only one GL program between begin() and render().

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

> Thanks Alexandros. All fixed.
>
> Instead of moving glUseProgram, I just inserted a new one. Because I imagine
> in the not-too-distant future shells will get some control of the rendering
> loop. And when that happens we can no longer assume there's only one GL
> program between begin() and render().

Fair enough. I am somewhat concerned about the multiple glUseProgram() calls, but I have no hard numbers to back these concerns.

I would only argue against clearing the current program:

75 + glUseProgram(0);

which doesn't really seem to offer anything. I would expect drivers to be able to better handle glUseProgram(N); glUseProgram(N);... than glUseProgram(N); glUseProgram(0); glUseProgram(N);...

Anyway, looks good in general, and this is not reason enough to block.

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

Unless there is a solid performance reason for avoiding glUseProgram(0), I think it's useful to keep. Because if honoured correctly (which now looks doubtful for Mesa), it will prevent stray calls (like glUniform) from affecting other peoples' shader programs by accident.

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

PASSED: Continuous integration, rev:1368
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/706/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/698
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/694
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/300
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/436
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/436/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/440
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/440/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/300
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/300/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/313
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/3248

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

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

OK

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/test/mir_test_doubles/mock_surface_renderer.h'
2--- include/test/mir_test_doubles/mock_surface_renderer.h 2014-01-13 06:12:33 +0000
3+++ include/test/mir_test_doubles/mock_surface_renderer.h 2014-01-22 08:54:26 +0000
4@@ -31,7 +31,7 @@
5
6 struct MockSurfaceRenderer : public compositor::Renderer
7 {
8- MOCK_CONST_METHOD0(begin, void());
9+ MOCK_CONST_METHOD1(begin, void(float));
10 MOCK_CONST_METHOD2(render, void(compositor::CompositingCriteria const&, graphics::Buffer&));
11 MOCK_CONST_METHOD0(end, void());
12 MOCK_METHOD0(suspend, void());
13
14=== modified file 'src/server/compositor/default_display_buffer_compositor.cpp'
15--- src/server/compositor/default_display_buffer_compositor.cpp 2014-01-21 03:39:15 +0000
16+++ src/server/compositor/default_display_buffer_compositor.cpp 2014-01-22 08:54:26 +0000
17@@ -148,7 +148,7 @@
18 mc::OcclusionMatch occlusion_match;
19 scene->reverse_for_each_if(occlusion_search, occlusion_match);
20
21- renderer->begin();
22+ renderer->begin(display_buffer.orientation());
23 mc::RenderingOperator applicator(*renderer, save_resource, local_frameno);
24 FilterForVisibleSceneInRegion selector(view_area, occlusion_match);
25 scene->for_each_if(selector, applicator);
26
27=== modified file 'src/server/compositor/gl_renderer.cpp'
28--- src/server/compositor/gl_renderer.cpp 2014-01-13 06:12:33 +0000
29+++ src/server/compositor/gl_renderer.cpp 2014-01-22 08:54:26 +0000
30@@ -25,6 +25,7 @@
31
32 #include <boost/throw_exception.hpp>
33 #include <stdexcept>
34+#include <cmath>
35
36 namespace mg = mir::graphics;
37 namespace mc = mir::compositor;
38@@ -38,10 +39,11 @@
39 "attribute vec3 position;\n"
40 "attribute vec2 texcoord;\n"
41 "uniform mat4 screen_to_gl_coords;\n"
42+ "uniform mat4 display_transform;\n"
43 "uniform mat4 transform;\n"
44 "varying vec2 v_texcoord;\n"
45 "void main() {\n"
46- " gl_Position = screen_to_gl_coords * transform * vec4(position, 1.0);\n"
47+ " gl_Position = display_transform * screen_to_gl_coords * transform * vec4(position, 1.0);\n"
48 " v_texcoord = texcoord;\n"
49 "}\n"
50 };
51@@ -172,6 +174,7 @@
52 /* Set up program variables */
53 GLint mat_loc = glGetUniformLocation(program, "screen_to_gl_coords");
54 GLint tex_loc = glGetUniformLocation(program, "tex");
55+ display_transform_uniform_loc = glGetUniformLocation(program, "display_transform");
56 transform_uniform_loc = glGetUniformLocation(program, "transform");
57 alpha_uniform_loc = glGetUniformLocation(program, "alpha");
58 position_attr_loc = glGetAttribLocation(program, "position");
59@@ -280,8 +283,18 @@
60 glDisableVertexAttribArray(position_attr_loc);
61 }
62
63-void mc::GLRenderer::begin() const
64+void mc::GLRenderer::begin(float rotation) const
65 {
66+ float rad = rotation * M_PI / 180.0f;
67+ GLfloat cos = cosf(rad);
68+ GLfloat sin = sinf(rad);
69+ GLfloat rot[16] = {cos, -sin, 0.0f, 0.0f,
70+ sin, cos, 0.0f, 0.0f,
71+ 0.0f, 0.0f, 1.0f, 0.0f,
72+ 0.0f, 0.0f, 0.0f, 1.0f};
73+ glUseProgram(program);
74+ glUniformMatrix4fv(display_transform_uniform_loc, 1, GL_TRUE, rot);
75+ glUseProgram(0);
76 glClear(GL_COLOR_BUFFER_BIT);
77 }
78
79
80=== modified file 'src/server/compositor/gl_renderer.h'
81--- src/server/compositor/gl_renderer.h 2014-01-13 06:12:33 +0000
82+++ src/server/compositor/gl_renderer.h 2014-01-22 08:54:26 +0000
83@@ -37,7 +37,7 @@
84 virtual ~GLRenderer() noexcept;
85
86 // These are called with a valid GL context:
87- void begin() const override;
88+ void begin(float rotation) const override;
89 void render(CompositingCriteria const& info, graphics::Buffer& buffer) const override;
90 void end() const override;
91
92@@ -50,6 +50,7 @@
93 GLuint program;
94 GLuint position_attr_loc;
95 GLuint texcoord_attr_loc;
96+ GLuint display_transform_uniform_loc;
97 GLuint transform_uniform_loc;
98 GLuint alpha_uniform_loc;
99 GLuint vertex_attribs_vbo;
100
101=== modified file 'src/server/compositor/renderer.h'
102--- src/server/compositor/renderer.h 2014-01-13 06:12:33 +0000
103+++ src/server/compositor/renderer.h 2014-01-22 08:54:26 +0000
104@@ -34,7 +34,7 @@
105 public:
106 virtual ~Renderer() = default;
107
108- virtual void begin() const = 0;
109+ virtual void begin(float rotation = 0.0f) const = 0;
110 virtual void render(CompositingCriteria const& info, graphics::Buffer& buffer) const = 0;
111 virtual void end() const = 0;
112
113
114=== modified file 'tests/acceptance-tests/test_server_shutdown.cpp'
115--- tests/acceptance-tests/test_server_shutdown.cpp 2014-01-13 06:12:33 +0000
116+++ tests/acceptance-tests/test_server_shutdown.cpp 2014-01-22 08:54:26 +0000
117@@ -48,7 +48,7 @@
118 class NullRenderer : public mc::Renderer
119 {
120 public:
121- void begin() const override
122+ void begin(float) const override
123 {
124 }
125
126
127=== modified file 'tests/integration-tests/test_session.cpp'
128--- tests/integration-tests/test_session.cpp 2014-01-13 06:12:33 +0000
129+++ tests/integration-tests/test_session.cpp 2014-01-22 08:54:26 +0000
130@@ -87,7 +87,7 @@
131 {
132 struct StubRenderer : public mc::Renderer
133 {
134- void begin() const override
135+ void begin(float) const override
136 {
137 }
138 void render(mc::CompositingCriteria const&, mg::Buffer&) const override
139
140=== modified file 'tests/integration-tests/test_surface_first_frame_sync.cpp'
141--- tests/integration-tests/test_surface_first_frame_sync.cpp 2014-01-13 06:12:33 +0000
142+++ tests/integration-tests/test_surface_first_frame_sync.cpp 2014-01-22 08:54:26 +0000
143@@ -96,7 +96,7 @@
144 {
145 }
146
147- void begin() const override
148+ void begin(float) const override
149 {
150 }
151
152
153=== modified file 'tests/mir_test_framework/stubbed_server_configuration.cpp'
154--- tests/mir_test_framework/stubbed_server_configuration.cpp 2014-01-13 06:12:33 +0000
155+++ tests/mir_test_framework/stubbed_server_configuration.cpp 2014-01-22 08:54:26 +0000
156@@ -203,7 +203,7 @@
157 class StubRenderer : public mc::Renderer
158 {
159 public:
160- void begin() const override
161+ void begin(float) const override
162 {
163 }
164
165
166=== modified file 'tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp'
167--- tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp 2014-01-13 06:12:33 +0000
168+++ tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp 2014-01-22 08:54:26 +0000
169@@ -108,9 +108,9 @@
170 {
171 }
172
173- void begin() const override
174+ void begin(float rotation) const override
175 {
176- renderer->begin();
177+ renderer->begin(rotation);
178 }
179
180 void render(mc::CompositingCriteria const& criteria, mg::Buffer& buffer) const override
181@@ -157,6 +157,9 @@
182 MockScene scene;
183 NiceMock<mtd::MockDisplayBuffer> display_buffer;
184
185+ ON_CALL(display_buffer, orientation())
186+ .WillByDefault(Return(mir_orientation_normal));
187+
188 EXPECT_CALL(renderer_factory.mock_renderer, render(_,_)).Times(0);
189
190 EXPECT_CALL(display_buffer, view_area())
191@@ -257,7 +260,7 @@
192
193 EXPECT_CALL(renderer_factory.mock_renderer, suspend())
194 .Times(1);
195- EXPECT_CALL(renderer_factory.mock_renderer, begin())
196+ EXPECT_CALL(renderer_factory.mock_renderer, begin(_))
197 .Times(0);
198 EXPECT_CALL(renderer_factory.mock_renderer, render(Ref(small),_))
199 .Times(0);
200@@ -316,7 +319,10 @@
201 .Times(0);
202 EXPECT_CALL(display_buffer, make_current())
203 .InSequence(render_seq);
204- EXPECT_CALL(renderer_factory.mock_renderer, begin())
205+ EXPECT_CALL(display_buffer, orientation())
206+ .InSequence(render_seq)
207+ .WillOnce(Return(mir_orientation_normal));
208+ EXPECT_CALL(renderer_factory.mock_renderer, begin(_))
209 .InSequence(render_seq);
210 EXPECT_CALL(renderer_factory.mock_renderer, render(Ref(big),_))
211 .InSequence(render_seq);
212@@ -356,6 +362,8 @@
213 .WillRepeatedly(Return(screen));
214 EXPECT_CALL(display_buffer, make_current())
215 .Times(1);
216+ EXPECT_CALL(display_buffer, orientation())
217+ .WillOnce(Return(mir_orientation_normal));
218 EXPECT_CALL(display_buffer, post_update())
219 .Times(1);
220 EXPECT_CALL(display_buffer, can_bypass())
221@@ -406,6 +414,8 @@
222 .WillRepeatedly(Return(screen));
223 EXPECT_CALL(display_buffer, make_current())
224 .Times(1);
225+ EXPECT_CALL(display_buffer, orientation())
226+ .WillOnce(Return(mir_orientation_normal));
227 EXPECT_CALL(display_buffer, post_update())
228 .Times(1);
229 EXPECT_CALL(display_buffer, can_bypass())
230@@ -452,6 +462,8 @@
231 .WillRepeatedly(Return(screen));
232 EXPECT_CALL(display_buffer, make_current())
233 .Times(1);
234+ EXPECT_CALL(display_buffer, orientation())
235+ .WillOnce(Return(mir_orientation_normal));
236 EXPECT_CALL(display_buffer, post_update())
237 .Times(1);
238 EXPECT_CALL(display_buffer, can_bypass())
239@@ -500,6 +512,8 @@
240 .WillRepeatedly(Return(screen));
241 EXPECT_CALL(display_buffer, make_current())
242 .Times(1);
243+ EXPECT_CALL(display_buffer, orientation())
244+ .WillRepeatedly(Return(mir_orientation_normal));
245 EXPECT_CALL(display_buffer, post_update())
246 .Times(1);
247 EXPECT_CALL(display_buffer, can_bypass())
248@@ -582,6 +596,8 @@
249 .WillRepeatedly(Return(screen));
250 EXPECT_CALL(display_buffer, make_current())
251 .Times(1);
252+ EXPECT_CALL(display_buffer, orientation())
253+ .WillOnce(Return(mir_orientation_normal));
254 EXPECT_CALL(display_buffer, post_update())
255 .Times(1);
256 EXPECT_CALL(display_buffer, can_bypass())
257
258=== modified file 'tests/unit-tests/compositor/test_gl_renderer.cpp'
259--- tests/unit-tests/compositor/test_gl_renderer.cpp 2014-01-13 06:12:33 +0000
260+++ tests/unit-tests/compositor/test_gl_renderer.cpp 2014-01-22 08:54:26 +0000
261@@ -59,6 +59,7 @@
262 const GLint texcoord_attr_location = 4;
263 const GLint screen_to_gl_coords_uniform_location = 5;
264 const GLint tex_uniform_location = 6;
265+const GLint display_transform_uniform_location = 7;
266 const std::string stub_info_log = "something failed!";
267 const size_t stub_info_log_length = stub_info_log.size();
268
269@@ -127,6 +128,8 @@
270 EXPECT_CALL(mock_gl, glGetUniformLocation(stub_program, _))
271 .WillOnce(Return(tex_uniform_location));
272 EXPECT_CALL(mock_gl, glGetUniformLocation(stub_program, _))
273+ .WillOnce(Return(display_transform_uniform_location));
274+ EXPECT_CALL(mock_gl, glGetUniformLocation(stub_program, _))
275 .WillOnce(Return(transform_uniform_location));
276 EXPECT_CALL(mock_gl, glGetUniformLocation(stub_program, _))
277 .WillOnce(Return(alpha_uniform_location));
278@@ -145,10 +148,6 @@
279 .WillOnce(SetArgPointee<1>(stub_vbo));
280 EXPECT_CALL(mock_gl, glBindBuffer(GL_ARRAY_BUFFER, stub_vbo));
281 EXPECT_CALL(mock_gl, glBufferData(GL_ARRAY_BUFFER, _, _, GL_STATIC_DRAW));
282-
283- /* These should go away */
284- EXPECT_CALL(mock_gl, glBindBuffer(GL_ARRAY_BUFFER, 0));
285- EXPECT_CALL(mock_gl, glUseProgram(0));
286 }
287
288 class GLRendererSetupProcess :

Subscribers

People subscribed via source and target branches