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

[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

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

[PS Jenkins bot (ps-jenkins)]

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)

[Kevin DuBois (kdub)]

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

[Daniel van Vugt (vanvugt)]

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

[Andreas Pokorny (andreas-pokorny)]

@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 on 2014-01-13

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

=== modified file 'include/server/mir/default_configuration_options.h'
--- include/server/mir/default_configuration_options.h	2014-01-13 04:48:36 +0000
+++ include/server/mir/default_configuration_options.h	2014-01-13 16:47:16 +0000
@@ -43,6 +43,7 @@
     static char const* const frontend_threads;
     static char const* const name_opt;
     static char const* const offscreen_opt;
+    static char const* const clear_color;
 
     static char const* const glog;
     static char const* const glog_stderrthreshold;
 
=== modified file 'include/test/mir_test_doubles/mock_gl.h'
--- include/test/mir_test_doubles/mock_gl.h	2014-01-13 04:48:36 +0000
+++ include/test/mir_test_doubles/mock_gl.h	2014-01-13 16:47:16 +0000
@@ -47,6 +47,7 @@
                  void(GLenum, GLsizeiptr, const GLvoid *, GLenum));
     MOCK_METHOD1(glCheckFramebufferStatus, GLenum(GLenum));
     MOCK_METHOD1(glClear, void(GLbitfield));
+    MOCK_METHOD4(glClearColor, void (GLclampf, GLclampf, GLclampf, GLclampf));
     MOCK_METHOD1(glCompileShader, void(GLuint));
     MOCK_METHOD0(glCreateProgram, GLuint());
     MOCK_METHOD1(glCreateShader, GLuint(GLenum));
 
=== modified file 'include/test/mir_test_doubles/mock_surface_renderer.h'
--- include/test/mir_test_doubles/mock_surface_renderer.h	2014-01-13 04:48:36 +0000
+++ include/test/mir_test_doubles/mock_surface_renderer.h	2014-01-13 16:47:16 +0000
@@ -35,6 +35,7 @@
     MOCK_CONST_METHOD2(render, void(compositor::CompositingCriteria const&, graphics::Buffer&));
     MOCK_CONST_METHOD0(end, void());
     MOCK_METHOD0(suspend, void());
+    MOCK_METHOD1(set_clear_color, void(Color const&));
 
     ~MockSurfaceRenderer() noexcept {}
 };
 
=== modified file 'src/platform/graphics/android/output_builder.cpp'
--- src/platform/graphics/android/output_builder.cpp	2014-01-10 03:59:43 +0000
+++ src/platform/graphics/android/output_builder.cpp	2014-01-13 16:47:16 +0000
@@ -26,6 +26,8 @@
 #include "mir/graphics/display_report.h"
 #include "mir/graphics/egl_resources.h"
 
+#include <hardware/hwcomposer_defs.h>
+
 namespace mg = mir::graphics;
 namespace mga = mir::graphics::android;
 namespace geom = mir::geometry;
 
=== modified file 'src/server/compositor/default_configuration.cpp'
--- src/server/compositor/default_configuration.cpp	2014-01-13 04:48:36 +0000
+++ src/server/compositor/default_configuration.cpp	2014-01-13 16:47:16 +0000
@@ -19,12 +19,46 @@
 #include "mir/default_server_configuration.h"
 #include "buffer_stream_factory.h"
 #include "default_display_buffer_compositor_factory.h"
+#include "gl_renderer_factory.h"
 #include "multi_threaded_compositor.h"
-#include "gl_renderer_factory.h"
+#include "renderer.h"
+
+#include <boost/throw_exception.hpp>
+#include <stdexcept>
+#include <sstream>
 
 namespace mc = mir::compositor;
 namespace ms = mir::scene;
 
+namespace
+{
+
+float clamp(unsigned long color)
+{
+    if (color > 255) color = 255;
+    return float(color)/255.0f;
+}
+
+mc::Renderer::Color extract_color(std::string const& value)
+{
+    unsigned long color;
+    std::istringstream in(value);
+    in >> std::hex >> color;
+
+    if (in.fail() || !(in >> std::ws).eof())
+        BOOST_THROW_EXCEPTION(std::runtime_error("Clear color should be a hexadecmial number"));
+
+    return mc::Renderer::Color
+        {
+            clamp((color >> 24) & 0xFF),
+            clamp((color >> 16) & 0xFF),
+            clamp((color >> 8)  & 0xFF),
+            clamp(color         & 0xFF)
+        };
+}
+
+}
+
 std::shared_ptr<ms::BufferStreamFactory>
 mir::DefaultServerConfiguration::the_buffer_stream_factory()
 {
@@ -61,9 +95,29 @@
 
 std::shared_ptr<mc::RendererFactory> mir::DefaultServerConfiguration::the_renderer_factory()
 {
+    struct ClearColorSettingRendererFactory : mc::GLRendererFactory
+    {
+        ClearColorSettingRendererFactory(mc::Renderer::Color const& color) : color(color) {}
+
+        std::unique_ptr<mc::Renderer> create_renderer_for(geometry::Rectangle const& rect) override
+        {
+            std::unique_ptr<mc::Renderer> renderer{GLRendererFactory::create_renderer_for(rect)};
+            renderer->set_clear_color(color);
+            return renderer;
+        }
+
+        mc::Renderer::Color color;
+    };
+
     return renderer_factory(
-        []()
+        [this]() -> std::shared_ptr<mc::RendererFactory>
         {
+            auto const options = the_options();
+            if (options->is_set(clear_color))
+            {
+                mc::Renderer::Color color = extract_color(options->get(clear_color, "0x000000FF"));
+                return std::make_shared<ClearColorSettingRendererFactory>(color);
+            }
             return std::make_shared<mc::GLRendererFactory>();
         });
 }
 
=== modified file 'src/server/compositor/gl_renderer.cpp'
--- src/server/compositor/gl_renderer.cpp	2014-01-13 04:48:36 +0000
+++ src/server/compositor/gl_renderer.cpp	2014-01-13 16:47:16 +0000
@@ -125,7 +125,8 @@
     texcoord_attr_loc(0),
     transform_uniform_loc(0),
     alpha_uniform_loc(0),
-    vertex_attribs_vbo(0)
+    vertex_attribs_vbo(0),
+    clear_color{0.0f,0.0f,0.0f,1.0f}
 {
     GLint param = 0;
 
@@ -282,6 +283,8 @@
 
 void mc::GLRenderer::begin() const
 {
+    glClearColor(std::get<0>(clear_color), std::get<1>(clear_color), std::get<2>(clear_color),
+                 std::get<3>(clear_color));
     glClear(GL_COLOR_BUFFER_BIT);
 }
 
@@ -309,3 +312,8 @@
 {
     skipped = true;
 }
+
+void mc::GLRenderer::set_clear_color(Color const& color)
+{
+    clear_color = color;
+}
 
=== modified file 'src/server/compositor/gl_renderer.h'
--- src/server/compositor/gl_renderer.h	2014-01-13 04:48:36 +0000
+++ src/server/compositor/gl_renderer.h	2014-01-13 16:47:16 +0000
@@ -44,6 +44,8 @@
     // This is called _without_ a GL context:
     void suspend() override;
 
+    void set_clear_color(Color const& color) override;
+
 private:
     GLuint vertex_shader;
     GLuint fragment_shader;
@@ -53,6 +55,7 @@
     GLuint transform_uniform_loc;
     GLuint alpha_uniform_loc;
     GLuint vertex_attribs_vbo;
+    Color clear_color;
 
     typedef CompositingCriteria const* SurfaceID;
     struct Texture
 
=== modified file 'src/server/compositor/renderer.h'
--- src/server/compositor/renderer.h	2014-01-13 04:48:36 +0000
+++ src/server/compositor/renderer.h	2014-01-13 16:47:16 +0000
@@ -19,6 +19,8 @@
 #ifndef MIR_COMPOSITOR_RENDERER_H_
 #define MIR_COMPOSITOR_RENDERER_H_
 
+#include <tuple>
+
 namespace mir
 {
 namespace graphics
@@ -40,6 +42,9 @@
 
     virtual void suspend() = 0; // called when begin/render/end skipped
 
+    typedef std::tuple<float, float, float, float> Color;
+    virtual void set_clear_color(Color const& color) = 0;
+
 protected:
     Renderer() = default;
     Renderer(const Renderer&) = delete;
 
=== modified file 'src/server/default_configuration_options.cpp'
--- src/server/default_configuration_options.cpp	2014-01-13 04:48:36 +0000
+++ src/server/default_configuration_options.cpp	2014-01-13 16:47:16 +0000
@@ -81,6 +81,7 @@
 char const* const mir::ConfigurationOptions::frontend_threads            = "ipc-thread-pool";
 char const* const mir::ConfigurationOptions::name_opt                    = "name";
 char const* const mir::ConfigurationOptions::offscreen_opt               = "offscreen";
+char const* const mir::ConfigurationOptions::clear_color                 = "clear-color";
 
 char const* const mir::ConfigurationOptions::glog                 = "glog";
 char const* const mir::ConfigurationOptions::glog_stderrthreshold = "glog-stderrthreshold";
@@ -160,6 +161,9 @@
             "When nested, the name Mir uses when registering with the host.")
         (offscreen_opt,
             "Render to offscreen buffers instead of the real outputs.")
+        (clear_color, po::value<std::string>(),
+            "Configure which color should be used for clearing the framebuffer."
+            " [hexadecimal number 0xRRGGBBAA:default=0x000000FF")
         ("vt", po::value<int>(),
             "VT to run on or 0 to use current. [int:default=0]");
 }
 
=== modified file 'src/server/options/program_option.cpp'
--- src/server/options/program_option.cpp	2013-09-27 21:01:24 +0000
+++ src/server/options/program_option.cpp	2014-01-13 16:47:16 +0000
@@ -139,3 +139,4 @@
 
     return default_;
 }
+
 
=== modified file 'tests/acceptance-tests/test_server_shutdown.cpp'
--- tests/acceptance-tests/test_server_shutdown.cpp	2014-01-13 04:48:36 +0000
+++ tests/acceptance-tests/test_server_shutdown.cpp	2014-01-13 16:47:16 +0000
@@ -68,6 +68,10 @@
     void suspend() override
     {
     }
+
+    void set_clear_color(Color const&) override
+    {
+    }
 };
 
 class NullRendererFactory : public mc::RendererFactory
 
=== modified file 'tests/integration-tests/test_session.cpp'
--- tests/integration-tests/test_session.cpp	2014-01-13 04:48:36 +0000
+++ tests/integration-tests/test_session.cpp	2014-01-13 16:47:16 +0000
@@ -90,6 +90,11 @@
             void begin() const override
             {
             }
+
+            void set_clear_color(Color const&) override
+            {
+            }
+
             void render(mc::CompositingCriteria const&, mg::Buffer&) const override
             {
             }
 
=== modified file 'tests/integration-tests/test_surface_first_frame_sync.cpp'
--- tests/integration-tests/test_surface_first_frame_sync.cpp	2014-01-13 04:48:36 +0000
+++ tests/integration-tests/test_surface_first_frame_sync.cpp	2014-01-13 16:47:16 +0000
@@ -64,7 +64,7 @@
             {
                 display_buffer_compositor_map[&display_buffer] = db_compositor_factory->create_compositor_for(display_buffer);
             });
-}
+    }
 
     void start()
     {
@@ -100,6 +100,10 @@
     {
     }
 
+    void set_clear_color(Color const&) override
+    {
+    }
+
     void render(mc::CompositingCriteria const&, mg::Buffer&) const override
     {
         while (write(render_operations_fd, "a", 1) != 1) continue;
 
=== modified file 'tests/mir_test_doubles/mock_gl.cpp'
--- tests/mir_test_doubles/mock_gl.cpp	2014-01-13 04:48:36 +0000
+++ tests/mir_test_doubles/mock_gl.cpp	2014-01-13 16:47:16 +0000
@@ -76,12 +76,18 @@
     global_mock_gl->glUseProgram (program);
 }
 
-void glClear (GLbitfield mask)
+void glClear(GLbitfield mask)
 {
     CHECK_GLOBAL_VOID_MOCK();
     global_mock_gl->glClear(mask);
 }
 
+void glClearColor(GLclampf red, GLclampf green, GLclampf blue, GLclampf alpha)
+{
+    CHECK_GLOBAL_VOID_MOCK();
+    global_mock_gl->glClearColor(red, green, blue, alpha);
+}
+
 void glEnable(GLenum func)
 {
     CHECK_GLOBAL_VOID_MOCK();
 
=== modified file 'tests/mir_test_framework/stubbed_server_configuration.cpp'
--- tests/mir_test_framework/stubbed_server_configuration.cpp	2014-01-13 04:48:36 +0000
+++ tests/mir_test_framework/stubbed_server_configuration.cpp	2014-01-13 16:47:16 +0000
@@ -216,6 +216,10 @@
     void suspend() override
     {
     }
+
+    void set_clear_color(Color const&) override
+    {
+    }
 };
 
 class StubRendererFactory : public mc::RendererFactory
 
=== modified file 'tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp'
--- tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp	2014-01-13 04:48:36 +0000
+++ tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp	2014-01-13 16:47:16 +0000
@@ -128,6 +128,11 @@
         renderer->suspend();
     }
 
+    void set_clear_color(Color const& color) override
+    {
+        renderer->set_clear_color(color);
+    }
+
     mc::Renderer* const renderer;
 };
 
 
=== modified file 'tests/unit-tests/compositor/test_gl_renderer.cpp'
--- tests/unit-tests/compositor/test_gl_renderer.cpp	2014-01-13 04:48:36 +0000
+++ tests/unit-tests/compositor/test_gl_renderer.cpp	2014-01-13 16:47:16 +0000
@@ -465,3 +465,22 @@
     renderer->begin();
     renderer->end();
 }
+
+TEST_F(GLRenderer, clears_with_black_by_default)
+{
+    InSequence seq;
+    EXPECT_CALL(mock_gl, glClearColor(0.0f, 0.0f, 0.0f, 1.0f));
+    EXPECT_CALL(mock_gl, glClear(_));
+
+    renderer->begin();
+}
+
+TEST_F(GLRenderer, clears_with_clear_color)
+{
+    InSequence seq;
+    EXPECT_CALL(mock_gl, glClearColor(0.8f, 0.2f, 0.4f, 1.0f));
+    EXPECT_CALL(mock_gl, glClear(_));
+
+    renderer->set_clear_color(mc::Renderer::Color{0.8f, 0.2f, 0.4f, 1.0f});
+    renderer->begin();
+}