Merge lp:~kdub/mir/gl-program-creation-to-common-place into lp:mir

[Kevin DuBois]

graphics: move gl program creation code to a common place. Ensure gl program creation is serialized in the GLRenderer factory (as opposed to the constructor of the GLRenderer). Address some exception safety issues in making gl programs/shaders.

graphics: move gl program creation code to a common place. Ensure gl program creation is serialized in the GLRenderer factory (as opposed to the constructor of the GLRenderer). Address some exception safety issues in making gl programs/shaders.

I intend to have a non-overridable gl program to handle the android surface fallback. The GLRenderer and the android fallback renderer are different enough that two programs are warranted (one can do arbitrary transforms, can inject different behaviors like drop shadows, etc the other just should be the bare minimum shader, maybe with some helpful debug features)

I put this common code in include/server/mir/compositor as GLRenderer is exposed publicly, and that class needs the definitions. I hope to avoid having libmirplatformgraphics.so link to OpenGL. The exception safety issue was that we would leak GL resources if we threw at lines [285,297,310]. These are fatal errors anyways as the code currently stands.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1543
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1266/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1519
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1517
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1093
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/998
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/998/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1003
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1003/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1094
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1094/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1012
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/5714

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1545
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1268/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1521
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1519
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1095
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1000
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1000/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1005
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1005/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1096
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1096/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1014
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/5720

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

review: Approve (continuous-integration)

[Alberto Aguirre (albaguirre)]

~~~
+mc::GLShader::operator GLuint() const
184+{
185+ return shader;
186+}

215+mc::GLProgram::operator GLuint() const
216+{
217+ return program;
218+}
~~~

Sneaky :)

review: Approve

[Alexandros Frantzis (afrantzis)]

380 + /*
381 + * We need to serialize renderer creation because some GL calls used
382 + * during renderer construction that create unique resource ids
383 + * (e.g. glCreateProgram) are not thread-safe when the threads are
384 + * have the same or shared EGL contexts.
385 + */
386 + std::mutex mutex;

By moving the mutex to the renderer factory, we protect renderer creation from races. However, since we wan to use the GLProgram/Shader classes independently, what's the plan for dealing with races there (and between the independent cases and renderer creation cases)? Perhaps we should move the mutex inside the the GL* classes?

review: Needs Information

[Robert Carr (robertcarr)]

I like this. I was thinking about this in the Zoom branch when daniel references attempting to do it in the renderer and encountering issues...clearly the renderer could be split in to multiple useful useful primitives and this is one of them.

Not my most expert area of code so not sure if this is a great suggestion but...

Maybe test_gl_renderer should now be testing interactions with GLProgram and test_gl_program should come in to existence and test against the mock_gl. On one hand its frustrating because you need an interface whereas its hard to imagine production alternative implementations of GLProgram...(though actually now that I think about it, could perhaps come in to play for Desktop GL v. GLES). On the other hand, I think it could make test_gl_renderer more clear. You know this code and tests better so I will let you decide!

review: Needs Information

[Kevin DuBois (kdub)]

> By moving the mutex to the renderer factory, we protect renderer creation from
> races. However, since we wan to use the GLProgram/Shader classes
> independently, what's the plan for dealing with races there (and between the
> independent cases and renderer creation cases)? Perhaps we should move the
> mutex inside the the GL* classes?

Still have to poke around different options to sort out which one feels the best. My instinct says there's some resources (RAII) that we have to tease out of the implementation, so different gl programs can use them cooperatively.

[Kevin DuBois (kdub)]

> I like this. I was thinking about this in the Zoom branch when daniel
> references attempting to do it in the renderer and encountering
> issues...clearly the renderer could be split in to multiple useful useful
> primitives and this is one of them.
>
> Not my most expert area of code so not sure if this is a great suggestion
> but...
>
> Maybe test_gl_renderer should now be testing interactions with GLProgram and
> test_gl_program should come in to existence and test against the mock_gl. On
> one hand its frustrating because you need an interface whereas its hard to
> imagine production alternative implementations of GLProgram...(though actually
> now that I think about it, could perhaps come in to play for Desktop GL v.
> GLES). On the other hand, I think it could make test_gl_renderer more clear.
> You know this code and tests better so I will let you decide!

The GLProgram/GLshader code is still under test in GLRenderer.*, although the test is one of the oldest and could use an overhaul. It'll probably be necessary to do that at some point, but I judged it wasn't necessary right now.

[Robert Carr (robertcarr)]

>> The GLProgram/GLshader code is still under test in GLRenderer.*, although the test is one of the oldest and
>> could use an overhaul. It'll probably be necessary to do that at some point, but I judged it wasn't
>> necessary right now.

Sounds good!

review: Approve

[Alexandros Frantzis (afrantzis)]

OK.

review: Approve

=== added file 'include/server/mir/compositor/gl_program.h'
--- include/server/mir/compositor/gl_program.h	1970-01-01 00:00:00 +0000
+++ include/server/mir/compositor/gl_program.h	2014-04-07 23:06:24 +0000
@@ -0,0 +1,65 @@
+/*
+ * Copyright © 2013-2014 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Kevin DuBois <[e-mail]>
+ */
+
+#ifndef MIR_COMPOSITOR_GL_PROGRAM_H_
+#define MIR_COMPOSITOR_GL_PROGRAM_H_
+
+#include <GLES2/gl2.h>
+
+namespace mir
+{
+namespace compositor
+{
+
+class GLShader
+{
+public:
+    GLShader(GLchar const* shader_src, GLuint type);
+    ~GLShader();
+    operator GLuint() const;
+
+private:
+    GLShader(GLShader const&) = delete;
+    GLShader& operator=(GLShader const&) = delete;
+
+    GLuint const shader;
+};
+
+class GLProgram
+{
+public:
+    GLProgram(
+        GLchar const* vertex_shader_src,
+        GLchar const* fragment_shader_src);
+    ~GLProgram();
+
+    operator GLuint() const;
+
+private:
+    GLProgram(GLProgram const&) = delete;
+    GLProgram& operator=(GLProgram const&) = delete;
+
+    GLShader const vertex_shader; 
+    GLShader const fragment_shader;
+    GLuint const program;
+};
+
+}
+}
+
+#endif /* MIR_COMPOSITOR_GL_PROGRAM_H_ */

=== modified file 'include/server/mir/compositor/gl_renderer.h'
--- include/server/mir/compositor/gl_renderer.h	2014-04-03 07:16:36 +0000
+++ include/server/mir/compositor/gl_renderer.h	2014-04-07 23:06:24 +0000
@@ -20,6 +20,7 @@
 #define MIR_COMPOSITOR_GL_RENDERER_H_
 
 #include <mir/compositor/renderer.h>
+#include <mir/compositor/gl_program.h>
 #include <mir/geometry/rectangle.h>
 #include <mir/graphics/buffer_id.h>
 #include <mir/graphics/renderable.h>
@@ -97,9 +98,7 @@
                                 graphics::Buffer& buffer) const;
 
 private:
-    GLuint vertex_shader;
-    GLuint fragment_shader;
-    GLuint program;
+    GLProgram program;
     GLuint position_attr_loc;
     GLuint texcoord_attr_loc;
     GLuint centre_uniform_loc;

=== modified file 'src/server/compositor/CMakeLists.txt'
--- src/server/compositor/CMakeLists.txt	2014-04-02 22:23:14 +0000
+++ src/server/compositor/CMakeLists.txt	2014-04-07 23:06:24 +0000
@@ -6,6 +6,7 @@
   temporary_buffers.cpp
   buffer_stream_factory.cpp
   buffer_stream_surfaces.cpp
+  gl_program.cpp
   gl_renderer.cpp
   gl_renderer_factory.cpp
   multi_threaded_compositor.cpp

=== added file 'src/server/compositor/gl_program.cpp'
--- src/server/compositor/gl_program.cpp	1970-01-01 00:00:00 +0000
+++ src/server/compositor/gl_program.cpp	2014-04-07 23:06:24 +0000
@@ -0,0 +1,109 @@
+/*
+ * Copyright © 2013-2014 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Kevin DuBois <[e-mail]>
+ */
+
+#include "mir/compositor/gl_program.h"
+#include <boost/throw_exception.hpp>
+#include <stdexcept>
+
+namespace mc = mir::compositor;
+
+namespace
+{
+typedef void(*MirGLGetObjectInfoLog)(GLuint, GLsizei, GLsizei *, GLchar *);
+typedef void(*MirGLGetObjectiv)(GLuint, GLenum, GLint *);
+
+void GetObjectLogAndThrow(MirGLGetObjectInfoLog getObjectInfoLog,
+                          MirGLGetObjectiv      getObjectiv,
+                          std::string const &   msg,
+                          GLuint                object)
+{
+    GLint object_log_length = 0;
+    (*getObjectiv)(object, GL_INFO_LOG_LENGTH, &object_log_length);
+
+    const GLuint object_log_buffer_length = object_log_length + 1;
+    std::string  object_info_log;
+
+    object_info_log.resize(object_log_buffer_length);
+    (*getObjectInfoLog)(object, object_log_length, NULL,
+                        const_cast<GLchar *>(object_info_log.data()));
+
+    std::string object_info_err(msg + "\n");
+    object_info_err += object_info_log;
+
+    BOOST_THROW_EXCEPTION(std::runtime_error(object_info_err));
+}
+}
+
+mc::GLShader::GLShader(GLchar const* shader_src, GLuint type)
+    : shader(glCreateShader(type))
+{
+    GLint param{0};
+    glShaderSource(shader, 1, &shader_src, 0);
+    glCompileShader(shader);
+    glGetShaderiv(shader, GL_COMPILE_STATUS, &param);
+    if (param == GL_FALSE)
+    {
+        glDeleteShader(shader);
+        GetObjectLogAndThrow(glGetShaderInfoLog,
+            glGetShaderiv,
+            "Failed to compile shader:",
+            shader);
+    }
+}
+
+mc::GLShader::~GLShader()
+{
+    glDeleteShader(shader);
+}
+
+mc::GLShader::operator GLuint() const
+{
+    return shader;
+}
+
+mc::GLProgram::GLProgram(
+    GLchar const* vertex_shader_src,
+    GLchar const* fragment_shader_src)
+  : vertex_shader(vertex_shader_src, GL_VERTEX_SHADER),
+    fragment_shader(fragment_shader_src, GL_FRAGMENT_SHADER),
+    program(glCreateProgram())
+{
+    glAttachShader(program, vertex_shader);
+    glAttachShader(program, fragment_shader);
+    glLinkProgram(program);
+    GLint param = 0;
+    glGetProgramiv(program, GL_LINK_STATUS, &param);
+    if (param == GL_FALSE)
+    {
+        glDeleteProgram(program);
+        GetObjectLogAndThrow(glGetProgramInfoLog,
+            glGetProgramiv,
+            "Failed to link program:",
+            program);
+    }
+}
+
+mc::GLProgram::~GLProgram()
+{
+    glDeleteProgram(program);
+}
+
+mc::GLProgram::operator GLuint() const
+{
+    return program;
+}

=== modified file 'src/server/compositor/gl_renderer.cpp'
--- src/server/compositor/gl_renderer.cpp	2014-04-03 07:16:36 +0000
+++ src/server/compositor/gl_renderer.cpp	2014-04-07 23:06:24 +0000
@@ -27,7 +27,6 @@
 #include <boost/throw_exception.hpp>
 #include <stdexcept>
 #include <cmath>
-#include <mutex>
 
 namespace mg = mir::graphics;
 namespace mc = mir::compositor;
@@ -64,93 +63,17 @@
     "   gl_FragColor = vec4(frag.xyz, frag.a * alpha);\n"
     "}\n"
 };
-
-typedef void(*MirGLGetObjectInfoLog)(GLuint, GLsizei, GLsizei *, GLchar *);
-typedef void(*MirGLGetObjectiv)(GLuint, GLenum, GLint *);
-
-void GetObjectLogAndThrow(MirGLGetObjectInfoLog getObjectInfoLog,
-                          MirGLGetObjectiv      getObjectiv,
-                          std::string const &   msg,
-                          GLuint                object)
-{
-    GLint object_log_length = 0;
-    (*getObjectiv)(object, GL_INFO_LOG_LENGTH, &object_log_length);
-
-    const GLuint object_log_buffer_length = object_log_length + 1;
-    std::string  object_info_log;
-
-    object_info_log.resize(object_log_buffer_length);
-    (*getObjectInfoLog)(object, object_log_length, NULL,
-                        const_cast<GLchar *>(object_info_log.data()));
-
-    std::string object_info_err(msg + "\n");
-    object_info_err += object_info_log;
-
-    BOOST_THROW_EXCEPTION(std::runtime_error(object_info_err));
-}
-
-}
-
-mc::GLRenderer::GLRenderer(geom::Rectangle const& display_area) :
-    vertex_shader(0),
-    fragment_shader(0),
-    program(0),
-    position_attr_loc(0),
-    texcoord_attr_loc(0),
-    centre_uniform_loc(0),
-    transform_uniform_loc(0),
-    alpha_uniform_loc(0),
-    rotation(NAN) // ensure the first set_rotation succeeds
-{
-    /*
-     * We need to serialize renderer creation because some GL calls used
-     * during renderer construction that create unique resource ids
-     * (e.g. glCreateProgram) are not thread-safe when the threads are
-     * have the same or shared EGL contexts.
-     */
-    static std::mutex mutex;
-    std::lock_guard<std::mutex> lock(mutex);
-
-    GLint param = 0;
-
-    /* Create shaders and program */
-    vertex_shader = glCreateShader(GL_VERTEX_SHADER);
-    glShaderSource(vertex_shader, 1, &vertex_shader_src, 0);
-    glCompileShader(vertex_shader);
-    glGetShaderiv(vertex_shader, GL_COMPILE_STATUS, &param);
-    if (param == GL_FALSE)
-    {
-        GetObjectLogAndThrow(glGetShaderInfoLog,
-            glGetShaderiv,
-            "Failed to compile vertex shader:",
-            vertex_shader);
-    }
-
-    fragment_shader = glCreateShader(GL_FRAGMENT_SHADER);
-    glShaderSource(fragment_shader, 1, &fragment_shader_src, 0);
-    glCompileShader(fragment_shader);
-    glGetShaderiv(fragment_shader, GL_COMPILE_STATUS, &param);
-    if (param == GL_FALSE)
-    {
-        GetObjectLogAndThrow(glGetShaderInfoLog,
-            glGetShaderiv,
-            "Failed to compile fragment shader:",
-            fragment_shader);
-    }
-
-    program = glCreateProgram();
-    glAttachShader(program, vertex_shader);
-    glAttachShader(program, fragment_shader);
-    glLinkProgram(program);
-    glGetProgramiv(program, GL_LINK_STATUS, &param);
-    if (param == GL_FALSE)
-    {
-        GetObjectLogAndThrow(glGetProgramInfoLog,
-            glGetProgramiv,
-            "Failed to link program:",
-            program);
-    }
-
+}
+
+mc::GLRenderer::GLRenderer(geom::Rectangle const& display_area)
+    : program(vertex_shader_src, fragment_shader_src),
+      position_attr_loc(0),
+      texcoord_attr_loc(0),
+      centre_uniform_loc(0),
+      transform_uniform_loc(0),
+      alpha_uniform_loc(0),
+      rotation(NAN) // ensure the first set_rotation succeeds
+{
     glUseProgram(program);
 
     /* Set up program variables */
@@ -173,12 +96,6 @@
 
 mc::GLRenderer::~GLRenderer() noexcept
 {
-    if (vertex_shader)
-        glDeleteShader(vertex_shader);
-    if (fragment_shader)
-        glDeleteShader(fragment_shader);
-    if (program)
-        glDeleteProgram(program);
     for (auto& t : textures)
         glDeleteTextures(1, &t.second.id);
 }

=== modified file 'src/server/compositor/gl_renderer_factory.cpp'
--- src/server/compositor/gl_renderer_factory.cpp	2014-03-06 06:05:17 +0000
+++ src/server/compositor/gl_renderer_factory.cpp	2014-04-07 23:06:24 +0000
@@ -26,6 +26,8 @@
 std::unique_ptr<mc::Renderer>
 mc::GLRendererFactory::create_renderer_for(geom::Rectangle const& rect)
 {
+    std::lock_guard<std::mutex> lock(mutex);
+
     auto raw = new GLRenderer(rect);
     return std::unique_ptr<mc::Renderer>(raw);
 }

=== modified file 'src/server/compositor/gl_renderer_factory.h'
--- src/server/compositor/gl_renderer_factory.h	2014-03-06 06:05:17 +0000
+++ src/server/compositor/gl_renderer_factory.h	2014-04-07 23:06:24 +0000
@@ -20,6 +20,7 @@
 #define MIR_COMPOSITOR_GL_RENDERER_FACTORY_H_
 
 #include "mir/compositor/renderer_factory.h"
+#include <mutex>
 
 namespace mir
 {
@@ -30,6 +31,15 @@
 {
 public:
     std::unique_ptr<Renderer> create_renderer_for(geometry::Rectangle const& rect);
+
+private:
+    /*
+     * We need to serialize renderer creation because some GL calls used
+     * during renderer construction that create unique resource ids
+     * (e.g. glCreateProgram) are not thread-safe when the threads are
+     * have the same or shared EGL contexts.
+     */
+    std::mutex mutex;
 };
 
 }

=== modified file 'tests/unit-tests/compositor/test_gl_renderer.cpp'
--- tests/unit-tests/compositor/test_gl_renderer.cpp	2014-03-28 20:12:32 +0000
+++ tests/unit-tests/compositor/test_gl_renderer.cpp	2014-04-07 23:06:24 +0000
@@ -272,20 +272,20 @@
         EXPECT_CALL(mock_gl, glGetUniformLocation(stub_program, _))
             .WillOnce(Return(screen_to_gl_coords_uniform_location));
 
-        mc::GLRendererFactory gl_renderer_factory;
         display_area = {{1, 2}, {3, 4}};
-        renderer = gl_renderer_factory.create_renderer_for(display_area);
 
+        EXPECT_CALL(mock_gl, glDeleteProgram(stub_program));
+        EXPECT_CALL(mock_gl, glDeleteShader(stub_f_shader));
         EXPECT_CALL(mock_gl, glDeleteShader(stub_v_shader));
-        EXPECT_CALL(mock_gl, glDeleteShader(stub_f_shader));
-        EXPECT_CALL(mock_gl, glDeleteProgram(stub_program));
     }
 
-    mtd::MockGL         mock_gl;
+    testing::NiceMock<mtd::MockGL> mock_gl;
+    testing::NiceMock<mtd::MockBuffer> mock_buffer;
     mir::geometry::Rectangle display_area;
     std::unique_ptr<mc::Renderer> renderer;
     glm::mat4           trans;
-    mtd::MockRenderable renderable;
+    testing::NiceMock<mtd::MockRenderable> renderable;
+    mc::GLRendererFactory gl_renderer_factory;
 };
 
 }
@@ -294,7 +294,7 @@
 {
     using namespace std::placeholders;
 
-    mtd::MockBuffer mock_buffer;
+    auto renderer = gl_renderer_factory.create_renderer_for(display_area);
 
     InSequence seq;
 
@@ -357,7 +357,7 @@
 
 TEST_F(GLRenderer, disables_blending_for_rgbx_surfaces)
 {
-    mtd::MockBuffer mock_buffer;
+    auto renderer = gl_renderer_factory.create_renderer_for(display_area);
 
     InSequence seq;
     EXPECT_CALL(renderable, shaped())
@@ -392,7 +392,7 @@
 
 TEST_F(GLRenderer, caches_and_uploads_texture_only_on_buffer_changes)
 {
-    mtd::MockBuffer mock_buffer;
+    auto renderer = gl_renderer_factory.create_renderer_for(display_area);
 
     EXPECT_CALL(mock_buffer, size())
         .WillRepeatedly(Return(mir::geometry::Size{123, 456}));