Mir

Merge lp:~andreas-pokorny/mir/configure-clear-color into lp:mir

  1. configure-clear-color
  2. Merge into development-branch
Proposed by Andreas Pokorny
Status: Rejected
Rejected by: Andreas Pokorny
Proposed branch: lp:~andreas-pokorny/mir/configure-clear-color
Merge into: lp:mir
Diff against target: 375 lines (+132/-5)
17 files modified
include/server/mir/default_configuration_options.h (+1/-0)
include/test/mir_test_doubles/mock_gl.h (+1/-0)
include/test/mir_test_doubles/mock_surface_renderer.h (+1/-0)
src/platform/graphics/android/output_builder.cpp (+2/-0)
src/server/compositor/default_configuration.cpp (+56/-2)
src/server/compositor/gl_renderer.cpp (+9/-1)
src/server/compositor/gl_renderer.h (+3/-0)
src/server/compositor/renderer.h (+5/-0)
src/server/default_configuration_options.cpp (+4/-0)
src/server/options/program_option.cpp (+1/-0)
tests/acceptance-tests/test_server_shutdown.cpp (+4/-0)
tests/integration-tests/test_session.cpp (+5/-0)
tests/integration-tests/test_surface_first_frame_sync.cpp (+5/-1)
tests/mir_test_doubles/mock_gl.cpp (+7/-1)
tests/mir_test_framework/stubbed_server_configuration.cpp (+4/-0)
tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp (+5/-0)
tests/unit-tests/compositor/test_gl_renderer.cpp (+19/-0)
To merge this branch: bzr merge lp:~andreas-pokorny/mir/configure-clear-color
Reviewer Review Type Date Requested Status
Daniel van Vugt Needs Fixing
Kevin DuBois (community) Needs Fixing
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+201456@code.launchpad.net

Commit message

Clear color Configuration option for GLRenderer

The configuration option is exposed through the default server configuration provided by mir_server. All demos that include a servers can be configured to use a different color with:
   --clear-color 0xDD4814FF

Description of the change

mir::compositor::Renderer is extended to allow setting the clear color and adds a --clear-color option to the demos.

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

PASSED: Continuous integration, rev:1331
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/663/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/638
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/634
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/242
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/389
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/389/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/392
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/392/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/242
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/242/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/261
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/3056

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

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

nit:
+ EXPECT_CALL(mock_gl, glClearColor(0.8f, 0.2f, 0.4f, 1.0f));
could use float matchers:
+ EXPECT_CALL(mock_gl, glClearColor(FloatEq(0.8f), FloatEq(0.2f), FloatEq(0.4f), FloatEq(1.0f)));

suggestion:
I'd prefer mg::GLRenderer not to acquire a set_clear_color(), and then begin() to actually perform the clear. (The semantics on this interface have become awkward, but its better not to compound the problem)

We could have the color come in as a parameter in the constructor of GLRenderer, since its set on the CLI and doesn't change.
Alternatively, we could have begin() be renamed to clear(Color) if we expect this to change every frame. This approach has less drawing-state stored in GLRenderer, and more kept in mir::compositor/mir::scene, where the surface stack state is

 has gotten very statefSeeing as our model keeps state in mir::compositor/mir::scene,

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

Some immediate design issues I'd like to see fixed:

(1) "clearing" is not a user-visible concept. But the screen "background" is. So I suggest renaming "clear-color" to "background", "bg" or similar.

(2) Asking the user to prefix a hex number with "0x" is possibly an unnecessary information leak of how you're implementing the parameter parsing. So I think "0x" should not be required from the user perspective. And you could replace the istringstream stuff with a simple:
    if (sscanf(value.c_str(), "%lx", &color) == 1)
or
    if (sscanf(value.c_str(), "%2x%2x%2x%2x", &r, %g, &b, %a) == 4)

(3) "float clamp(unsigned long color)" does not need to exist. The input parameters are already mathematically guaranteed [0,255]. So you could remove the function and replace:
    clamp(...)
with
    (...) / 255.0f

(4) The name "ClearColorSettingRendererFactory" feels awkward. Perhaps "CustomRendererFactory" is better.

(5) Your colour components have known names and purposes, so please don't make them anonymous:
    typedef std::tuple<float, float, float, float> Color;
should be:
    struct Color {float r, g, b, a;}
The resulting code will be easier to read and generate less binary (no templates).

review: Needs Fixing
Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

@Keven: agree, the interface begin/end/set_clear_color looks awkward - looking into - first attempt to improve was sublcassing GLRenderer and then stumbled over the GLRendererFactory having a mutex to serialize gl calls of the constructor.

(1) sure
(2) it already does both - just thought 0x looks more familiar
(3) improving - found a better name and use for clamp ..
(5) binary is equal or even smaller with tuple (only gcc knows why) but I agree to the readability wins

putting it back to WIP. Want to have a second thought on dealing with the GLRendererFactory topic.

Unmerged revisions

1331. By Andreas Pokorny

Clear color Configuration option for GLRenderer

The configuration option is exposed through the default server configuration provided by mir_server. All demos that include a servers can be configured to use a different color with
   --clear-color 0xDD4814FF

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/server/mir/default_configuration_options.h'
2--- include/server/mir/default_configuration_options.h 2014-01-13 04:48:36 +0000
3+++ include/server/mir/default_configuration_options.h 2014-01-13 16:47:16 +0000
4@@ -43,6 +43,7 @@
5 static char const* const frontend_threads;
6 static char const* const name_opt;
7 static char const* const offscreen_opt;
8+ static char const* const clear_color;
9
10 static char const* const glog;
11 static char const* const glog_stderrthreshold;
12
13=== modified file 'include/test/mir_test_doubles/mock_gl.h'
14--- include/test/mir_test_doubles/mock_gl.h 2014-01-13 04:48:36 +0000
15+++ include/test/mir_test_doubles/mock_gl.h 2014-01-13 16:47:16 +0000
16@@ -47,6 +47,7 @@
17 void(GLenum, GLsizeiptr, const GLvoid *, GLenum));
18 MOCK_METHOD1(glCheckFramebufferStatus, GLenum(GLenum));
19 MOCK_METHOD1(glClear, void(GLbitfield));
20+ MOCK_METHOD4(glClearColor, void (GLclampf, GLclampf, GLclampf, GLclampf));
21 MOCK_METHOD1(glCompileShader, void(GLuint));
22 MOCK_METHOD0(glCreateProgram, GLuint());
23 MOCK_METHOD1(glCreateShader, GLuint(GLenum));
24
25=== modified file 'include/test/mir_test_doubles/mock_surface_renderer.h'
26--- include/test/mir_test_doubles/mock_surface_renderer.h 2014-01-13 04:48:36 +0000
27+++ include/test/mir_test_doubles/mock_surface_renderer.h 2014-01-13 16:47:16 +0000
28@@ -35,6 +35,7 @@
29 MOCK_CONST_METHOD2(render, void(compositor::CompositingCriteria const&, graphics::Buffer&));
30 MOCK_CONST_METHOD0(end, void());
31 MOCK_METHOD0(suspend, void());
32+ MOCK_METHOD1(set_clear_color, void(Color const&));
33
34 ~MockSurfaceRenderer() noexcept {}
35 };
36
37=== modified file 'src/platform/graphics/android/output_builder.cpp'
38--- src/platform/graphics/android/output_builder.cpp 2014-01-10 03:59:43 +0000
39+++ src/platform/graphics/android/output_builder.cpp 2014-01-13 16:47:16 +0000
40@@ -26,6 +26,8 @@
41 #include "mir/graphics/display_report.h"
42 #include "mir/graphics/egl_resources.h"
43
44+#include <hardware/hwcomposer_defs.h>
45+
46 namespace mg = mir::graphics;
47 namespace mga = mir::graphics::android;
48 namespace geom = mir::geometry;
49
50=== modified file 'src/server/compositor/default_configuration.cpp'
51--- src/server/compositor/default_configuration.cpp 2014-01-13 04:48:36 +0000
52+++ src/server/compositor/default_configuration.cpp 2014-01-13 16:47:16 +0000
53@@ -19,12 +19,46 @@
54 #include "mir/default_server_configuration.h"
55 #include "buffer_stream_factory.h"
56 #include "default_display_buffer_compositor_factory.h"
57+#include "gl_renderer_factory.h"
58 #include "multi_threaded_compositor.h"
59-#include "gl_renderer_factory.h"
60+#include "renderer.h"
61+
62+#include <boost/throw_exception.hpp>
63+#include <stdexcept>
64+#include <sstream>
65
66 namespace mc = mir::compositor;
67 namespace ms = mir::scene;
68
69+namespace
70+{
71+
72+float clamp(unsigned long color)
73+{
74+ if (color > 255) color = 255;
75+ return float(color)/255.0f;
76+}
77+
78+mc::Renderer::Color extract_color(std::string const& value)
79+{
80+ unsigned long color;
81+ std::istringstream in(value);
82+ in >> std::hex >> color;
83+
84+ if (in.fail() || !(in >> std::ws).eof())
85+ BOOST_THROW_EXCEPTION(std::runtime_error("Clear color should be a hexadecmial number"));
86+
87+ return mc::Renderer::Color
88+ {
89+ clamp((color >> 24) & 0xFF),
90+ clamp((color >> 16) & 0xFF),
91+ clamp((color >> 8) & 0xFF),
92+ clamp(color & 0xFF)
93+ };
94+}
95+
96+}
97+
98 std::shared_ptr<ms::BufferStreamFactory>
99 mir::DefaultServerConfiguration::the_buffer_stream_factory()
100 {
101@@ -61,9 +95,29 @@
102
103 std::shared_ptr<mc::RendererFactory> mir::DefaultServerConfiguration::the_renderer_factory()
104 {
105+ struct ClearColorSettingRendererFactory : mc::GLRendererFactory
106+ {
107+ ClearColorSettingRendererFactory(mc::Renderer::Color const& color) : color(color) {}
108+
109+ std::unique_ptr<mc::Renderer> create_renderer_for(geometry::Rectangle const& rect) override
110+ {
111+ std::unique_ptr<mc::Renderer> renderer{GLRendererFactory::create_renderer_for(rect)};
112+ renderer->set_clear_color(color);
113+ return renderer;
114+ }
115+
116+ mc::Renderer::Color color;
117+ };
118+
119 return renderer_factory(
120- []()
121+ [this]() -> std::shared_ptr<mc::RendererFactory>
122 {
123+ auto const options = the_options();
124+ if (options->is_set(clear_color))
125+ {
126+ mc::Renderer::Color color = extract_color(options->get(clear_color, "0x000000FF"));
127+ return std::make_shared<ClearColorSettingRendererFactory>(color);
128+ }
129 return std::make_shared<mc::GLRendererFactory>();
130 });
131 }
132
133=== modified file 'src/server/compositor/gl_renderer.cpp'
134--- src/server/compositor/gl_renderer.cpp 2014-01-13 04:48:36 +0000
135+++ src/server/compositor/gl_renderer.cpp 2014-01-13 16:47:16 +0000
136@@ -125,7 +125,8 @@
137 texcoord_attr_loc(0),
138 transform_uniform_loc(0),
139 alpha_uniform_loc(0),
140- vertex_attribs_vbo(0)
141+ vertex_attribs_vbo(0),
142+ clear_color{0.0f,0.0f,0.0f,1.0f}
143 {
144 GLint param = 0;
145
146@@ -282,6 +283,8 @@
147
148 void mc::GLRenderer::begin() const
149 {
150+ glClearColor(std::get<0>(clear_color), std::get<1>(clear_color), std::get<2>(clear_color),
151+ std::get<3>(clear_color));
152 glClear(GL_COLOR_BUFFER_BIT);
153 }
154
155@@ -309,3 +312,8 @@
156 {
157 skipped = true;
158 }
159+
160+void mc::GLRenderer::set_clear_color(Color const& color)
161+{
162+ clear_color = color;
163+}
164
165=== modified file 'src/server/compositor/gl_renderer.h'
166--- src/server/compositor/gl_renderer.h 2014-01-13 04:48:36 +0000
167+++ src/server/compositor/gl_renderer.h 2014-01-13 16:47:16 +0000
168@@ -44,6 +44,8 @@
169 // This is called _without_ a GL context:
170 void suspend() override;
171
172+ void set_clear_color(Color const& color) override;
173+
174 private:
175 GLuint vertex_shader;
176 GLuint fragment_shader;
177@@ -53,6 +55,7 @@
178 GLuint transform_uniform_loc;
179 GLuint alpha_uniform_loc;
180 GLuint vertex_attribs_vbo;
181+ Color clear_color;
182
183 typedef CompositingCriteria const* SurfaceID;
184 struct Texture
185
186=== modified file 'src/server/compositor/renderer.h'
187--- src/server/compositor/renderer.h 2014-01-13 04:48:36 +0000
188+++ src/server/compositor/renderer.h 2014-01-13 16:47:16 +0000
189@@ -19,6 +19,8 @@
190 #ifndef MIR_COMPOSITOR_RENDERER_H_
191 #define MIR_COMPOSITOR_RENDERER_H_
192
193+#include <tuple>
194+
195 namespace mir
196 {
197 namespace graphics
198@@ -40,6 +42,9 @@
199
200 virtual void suspend() = 0; // called when begin/render/end skipped
201
202+ typedef std::tuple<float, float, float, float> Color;
203+ virtual void set_clear_color(Color const& color) = 0;
204+
205 protected:
206 Renderer() = default;
207 Renderer(const Renderer&) = delete;
208
209=== modified file 'src/server/default_configuration_options.cpp'
210--- src/server/default_configuration_options.cpp 2014-01-13 04:48:36 +0000
211+++ src/server/default_configuration_options.cpp 2014-01-13 16:47:16 +0000
212@@ -81,6 +81,7 @@
213 char const* const mir::ConfigurationOptions::frontend_threads = "ipc-thread-pool";
214 char const* const mir::ConfigurationOptions::name_opt = "name";
215 char const* const mir::ConfigurationOptions::offscreen_opt = "offscreen";
216+char const* const mir::ConfigurationOptions::clear_color = "clear-color";
217
218 char const* const mir::ConfigurationOptions::glog = "glog";
219 char const* const mir::ConfigurationOptions::glog_stderrthreshold = "glog-stderrthreshold";
220@@ -160,6 +161,9 @@
221 "When nested, the name Mir uses when registering with the host.")
222 (offscreen_opt,
223 "Render to offscreen buffers instead of the real outputs.")
224+ (clear_color, po::value<std::string>(),
225+ "Configure which color should be used for clearing the framebuffer."
226+ " [hexadecimal number 0xRRGGBBAA:default=0x000000FF")
227 ("vt", po::value<int>(),
228 "VT to run on or 0 to use current. [int:default=0]");
229 }
230
231=== modified file 'src/server/options/program_option.cpp'
232--- src/server/options/program_option.cpp 2013-09-27 21:01:24 +0000
233+++ src/server/options/program_option.cpp 2014-01-13 16:47:16 +0000
234@@ -139,3 +139,4 @@
235
236 return default_;
237 }
238+
239
240=== modified file 'tests/acceptance-tests/test_server_shutdown.cpp'
241--- tests/acceptance-tests/test_server_shutdown.cpp 2014-01-13 04:48:36 +0000
242+++ tests/acceptance-tests/test_server_shutdown.cpp 2014-01-13 16:47:16 +0000
243@@ -68,6 +68,10 @@
244 void suspend() override
245 {
246 }
247+
248+ void set_clear_color(Color const&) override
249+ {
250+ }
251 };
252
253 class NullRendererFactory : public mc::RendererFactory
254
255=== modified file 'tests/integration-tests/test_session.cpp'
256--- tests/integration-tests/test_session.cpp 2014-01-13 04:48:36 +0000
257+++ tests/integration-tests/test_session.cpp 2014-01-13 16:47:16 +0000
258@@ -90,6 +90,11 @@
259 void begin() const override
260 {
261 }
262+
263+ void set_clear_color(Color const&) override
264+ {
265+ }
266+
267 void render(mc::CompositingCriteria const&, mg::Buffer&) const override
268 {
269 }
270
271=== modified file 'tests/integration-tests/test_surface_first_frame_sync.cpp'
272--- tests/integration-tests/test_surface_first_frame_sync.cpp 2014-01-13 04:48:36 +0000
273+++ tests/integration-tests/test_surface_first_frame_sync.cpp 2014-01-13 16:47:16 +0000
274@@ -64,7 +64,7 @@
275 {
276 display_buffer_compositor_map[&display_buffer] = db_compositor_factory->create_compositor_for(display_buffer);
277 });
278-}
279+ }
280
281 void start()
282 {
283@@ -100,6 +100,10 @@
284 {
285 }
286
287+ void set_clear_color(Color const&) override
288+ {
289+ }
290+
291 void render(mc::CompositingCriteria const&, mg::Buffer&) const override
292 {
293 while (write(render_operations_fd, "a", 1) != 1) continue;
294
295=== modified file 'tests/mir_test_doubles/mock_gl.cpp'
296--- tests/mir_test_doubles/mock_gl.cpp 2014-01-13 04:48:36 +0000
297+++ tests/mir_test_doubles/mock_gl.cpp 2014-01-13 16:47:16 +0000
298@@ -76,12 +76,18 @@
299 global_mock_gl->glUseProgram (program);
300 }
301
302-void glClear (GLbitfield mask)
303+void glClear(GLbitfield mask)
304 {
305 CHECK_GLOBAL_VOID_MOCK();
306 global_mock_gl->glClear(mask);
307 }
308
309+void glClearColor(GLclampf red, GLclampf green, GLclampf blue, GLclampf alpha)
310+{
311+ CHECK_GLOBAL_VOID_MOCK();
312+ global_mock_gl->glClearColor(red, green, blue, alpha);
313+}
314+
315 void glEnable(GLenum func)
316 {
317 CHECK_GLOBAL_VOID_MOCK();
318
319=== modified file 'tests/mir_test_framework/stubbed_server_configuration.cpp'
320--- tests/mir_test_framework/stubbed_server_configuration.cpp 2014-01-13 04:48:36 +0000
321+++ tests/mir_test_framework/stubbed_server_configuration.cpp 2014-01-13 16:47:16 +0000
322@@ -216,6 +216,10 @@
323 void suspend() override
324 {
325 }
326+
327+ void set_clear_color(Color const&) override
328+ {
329+ }
330 };
331
332 class StubRendererFactory : public mc::RendererFactory
333
334=== modified file 'tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp'
335--- tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp 2014-01-13 04:48:36 +0000
336+++ tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp 2014-01-13 16:47:16 +0000
337@@ -128,6 +128,11 @@
338 renderer->suspend();
339 }
340
341+ void set_clear_color(Color const& color) override
342+ {
343+ renderer->set_clear_color(color);
344+ }
345+
346 mc::Renderer* const renderer;
347 };
348
349
350=== modified file 'tests/unit-tests/compositor/test_gl_renderer.cpp'
351--- tests/unit-tests/compositor/test_gl_renderer.cpp 2014-01-13 04:48:36 +0000
352+++ tests/unit-tests/compositor/test_gl_renderer.cpp 2014-01-13 16:47:16 +0000
353@@ -465,3 +465,22 @@
354 renderer->begin();
355 renderer->end();
356 }
357+
358+TEST_F(GLRenderer, clears_with_black_by_default)
359+{
360+ InSequence seq;
361+ EXPECT_CALL(mock_gl, glClearColor(0.0f, 0.0f, 0.0f, 1.0f));
362+ EXPECT_CALL(mock_gl, glClear(_));
363+
364+ renderer->begin();
365+}
366+
367+TEST_F(GLRenderer, clears_with_clear_color)
368+{
369+ InSequence seq;
370+ EXPECT_CALL(mock_gl, glClearColor(0.8f, 0.2f, 0.4f, 1.0f));
371+ EXPECT_CALL(mock_gl, glClear(_));
372+
373+ renderer->set_clear_color(mc::Renderer::Color{0.8f, 0.2f, 0.4f, 1.0f});
374+ renderer->begin();
375+}

Subscribers

People subscribed via source and target branches