Merge lp:~afrantzis/mir/fix-1234563 into lp:mir

[Alexandros Frantzis]

examples: Restore GL state after initializing buffers, fixing crashes
observed in render_surfaces (LP: #1234563)

Due to the lazy allocation of buffers on the server, buffer
allocation and initialization may occur while the renderer
has already set up its GL state. Ensure that we don't
change the renderer's state when initializing buffers.

examples: Restore GL state after initializing buffers

Due to the lazy allocation of buffers on the server, buffer allocation and initialization may occur while the renderer has already set up its GL state. Ensure that we don't change the renderer's state when initializing buffers.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1162
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/254/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2494
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2379
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/251/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-armhf-ci/44
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-armhf-ci/44/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/254/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1162
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/256/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2496
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2381
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/253
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/253/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-armhf-ci/46
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-armhf-ci/46/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/256/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Could this fix bug 1211699 too?

[Daniel van Vugt (vanvugt)]

I feel the new GLState class is acting by side-effect in restoring state on destruction. The GL state is a very important and far-reaching set of globals, effectively. So rather than implicitly modify globals by RAII, it should be explicit. I think GLState should use methods: save(), restore()

If only OpenGL|ES didn't drop push/pop operations we could use with OpenGL :/

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

The copy semantics of GLState are probably inappropriate and unnecessary while the name doesn't really suggest the preserving role. Maybe "StoreGLState"?

Also, I think the use of forwarding constructors would be simpler the other way around. Vis:

GLState()
{
    glGetIntegerv(GL_CURRENT_PROGRAM, &program);
    glGetIntegerv(GL_TEXTURE_BINDING_2D, &texture);
    glGetIntegerv(GL_ARRAY_BUFFER_BINDING, &buffer);
    glGetIntegerv(GL_ACTIVE_TEXTURE, &active_texture_unit);
}

GLState(GLint attrib_loc) : GLState()
{
    this->attrib_loc = attrib_loc

    if (attrib_loc >= 0)
    {
        glGetVertexAttribiv(attrib_loc, GL_VERTEX_ATTRIB_ARRAY_ENABLED, &attrib_enabled);
        glGetVertexAttribiv(attrib_loc, GL_VERTEX_ATTRIB_ARRAY_SIZE, &attrib_size);
        glGetVertexAttribiv(attrib_loc, GL_VERTEX_ATTRIB_ARRAY_TYPE, &attrib_type);
        glGetVertexAttribiv(attrib_loc, GL_VERTEX_ATTRIB_ARRAY_NORMALIZED, &attrib_normalized);
        glGetVertexAttribiv(attrib_loc, GL_VERTEX_ATTRIB_ARRAY_STRIDE, &attrib_stride);
        glGetVertexAttribPointerv(attrib_loc, GL_VERTEX_ATTRIB_ARRAY_POINTER, &attrib_pointer);
    }
}

BTW I think that using scoped objects to save and restore state is entirely appropriate: embedding paired operations in construction and destruction is a normal C++ idiom.

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

> The copy semantics of GLState are probably inappropriate and unnecessary while the name doesn't
> really suggest the preserving role. Maybe "StoreGLState"?

The idea was to avoid stressing the store/restore implementation of the class, and provide another abstraction: when that object is in scope, it is the new target GL state for all state changes (which is just an illusion, of course). Perhaps "TargetGLState", "CurrentGLState", "TemporaryGLState", "ScopedGLState", "LocalGLState" ?

[Alan Griffiths (alan-griffiths)]

> > The copy semantics of GLState are probably inappropriate and unnecessary
> while the name doesn't
> > really suggest the preserving role. Maybe "StoreGLState"?
>
> The idea was to avoid stressing the store/restore implementation of the class,
> and provide another abstraction: when that object is in scope, it is the new
> target GL state for all state changes (which is just an illusion, of course).
> Perhaps "TargetGLState", "CurrentGLState", "TemporaryGLState",
> "ScopedGLState", "LocalGLState" ?

GLScope perhaps?

[Robert Carr (robertcarr)]

I think GLScope or GLStateScope is a telling name in this case.

Daniel: I wish we weren't having the same argument on RAII over and over. I think it confuses the argument and makes it more difficult for discussion to proceed:

A little word-swapping excercise ;)
===
"I feel the new GLState class is acting by side-effect in restoring state on destruction. The GL state is a very important and far-reaching set of globals, effectively. So rather than implicitly modify globals by RAII, it should be explicit. I think GLState should use methods: save(), restore()"
===

vs.

===
""I feel the new GLState class is acting putting burden on the user with save/restore method. The GL state is a very important and far-reaching set of globals, effectively. So rather than explicitly modify globals, it should be implicit via RAII. I think GLState should use c'tor/d'tor""
====

I don't think there is an 'argument' here, it's just an authoritative way to say "RAII should not be used". Of course, I respect your opinions on the matter, and more than that respect that you consistently express them. I think though that merge proposals are typically not the right place to express strong opinions on controversial topics. At the risk of violating the tenant I've just put forth, I'll say: I think it polarizes the conversation and detracts from the rigor/integrity of the discussion. I know if nothing else it makes it feel difficult for me to engage on merge proposals once they have been brought to such a level.

[Daniel van Vugt (vanvugt)]

I understand RAII and accept that people will use it in the context of the local object.

However RAII should not be used to modify global variables/state. That's a side effect, and side effects are widely recognised in software engineering as being a danger to maintainability.

[Daniel van Vugt (vanvugt)]

Hmm, OK I give in. This actually isn't much different or worse to other RAII use. And the GL state machine has the same dangers even without C++.

review: Abstain

[Daniel van Vugt (vanvugt)]

It occurs to me we don't really have time to argue over example code. Especially example code that is neither a client or server that people can learn from.

Verified the fix works. All good.

review: Approve

=== modified file 'examples/image_renderer.cpp'
--- examples/image_renderer.cpp	2013-03-21 03:32:59 +0000
+++ examples/image_renderer.cpp	2013-10-24 13:41:26 +0000
@@ -87,11 +87,68 @@
     BOOST_THROW_EXCEPTION(std::runtime_error(object_info_err));
 }
 
+class GLState
+{
+public:
+    GLState(GLint attrib_loc)
+        : attrib_loc{attrib_loc}
+    {
+        glGetIntegerv(GL_CURRENT_PROGRAM, &program);
+        glGetIntegerv(GL_TEXTURE_BINDING_2D, &texture);
+        glGetIntegerv(GL_ARRAY_BUFFER_BINDING, &buffer);
+        glGetIntegerv(GL_ACTIVE_TEXTURE, &active_texture_unit);
+        if (attrib_loc >= 0)
+        {
+            glGetVertexAttribiv(attrib_loc, GL_VERTEX_ATTRIB_ARRAY_ENABLED, &attrib_enabled);
+            glGetVertexAttribiv(attrib_loc, GL_VERTEX_ATTRIB_ARRAY_SIZE, &attrib_size);
+            glGetVertexAttribiv(attrib_loc, GL_VERTEX_ATTRIB_ARRAY_TYPE, &attrib_type);
+            glGetVertexAttribiv(attrib_loc, GL_VERTEX_ATTRIB_ARRAY_NORMALIZED, &attrib_normalized);
+            glGetVertexAttribiv(attrib_loc, GL_VERTEX_ATTRIB_ARRAY_STRIDE, &attrib_stride);
+            glGetVertexAttribPointerv(attrib_loc, GL_VERTEX_ATTRIB_ARRAY_POINTER, &attrib_pointer);
+        }
+    }
+
+    GLState() : GLState{invalid_attrib_loc} {}
+
+    ~GLState()
+    {
+        glUseProgram(program);
+        glBindTexture(GL_TEXTURE_2D, texture);
+        glBindBuffer(GL_ARRAY_BUFFER, buffer);
+        glActiveTexture(active_texture_unit);
+        if (attrib_loc >= 0)
+        {
+            glVertexAttribPointer(attrib_loc, attrib_size, attrib_type,
+                                  attrib_normalized, attrib_stride, attrib_pointer);
+            if (attrib_enabled)
+                glEnableVertexAttribArray(attrib_loc);
+            else
+                glDisableVertexAttribArray(attrib_loc);
+        }
+    }
+
+private:
+    static GLint const invalid_attrib_loc = -1;
+    GLint program = 0;
+    GLint texture = 0;
+    GLint buffer = 0;
+    GLint active_texture_unit = 0;
+    GLint attrib_loc = invalid_attrib_loc;
+    GLint attrib_enabled = 0;
+    GLint attrib_size = 0;
+    GLint attrib_type = 0;
+    GLint attrib_normalized = 0;
+    GLint attrib_stride = 0;
+    GLvoid* attrib_pointer = nullptr;
+};
+
 }
 
 mt::ImageRenderer::ImageRenderer(const uint8_t* pixel_data, mir::geometry::Size size,
                                  uint32_t bytes_per_pixel)
 {
+    GLState gl_state;
+
     resources.setup();
 
     /* Upload the texture */
@@ -173,7 +230,6 @@
     glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
 
     glUniform1i(tex_loc, 0);
-    glBindTexture(GL_TEXTURE_2D, 0);
 
     /* Create VBO */
     glGenBuffers(1, &vertex_attribs_vbo);
@@ -181,14 +237,13 @@
     glBindBuffer(GL_ARRAY_BUFFER, vertex_attribs_vbo);
     glBufferData(GL_ARRAY_BUFFER, sizeof(vertex_attribs),
                  glm::value_ptr(vertex_attribs[0]), GL_STATIC_DRAW);
-
-    glBindBuffer(GL_ARRAY_BUFFER, 0);
-    glUseProgram(0);
 }
 
 
 void mt::ImageRenderer::render()
 {
+    GLState gl_state(resources.position_attr_loc);
+
     glUseProgram(resources.program);
 
     glActiveTexture(GL_TEXTURE0);
@@ -202,5 +257,4 @@
     /* Draw */
     glEnableVertexAttribArray(resources.position_attr_loc);
     glDrawArrays(GL_TRIANGLE_STRIP, 0, 4);
-    glDisableVertexAttribArray(resources.position_attr_loc);
 }