Merge lp:~vanvugt/mir/fix-1301210 into lp:mir

[Daniel van Vugt]

Ensure the alpha component (if any) of the frame buffer is never changed
during compositing. Otherwise you could end up with translucent pixels which
produce incorrect looking screenshots/screencasts (LP: #1301210)

If anyone can figure out how/why our simple blending functions are producing pixels with alpha less than one, please let me know.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1577
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1409/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1718
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1716
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1288
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1141
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1141/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1146
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1146/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1291
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1291/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1189
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/6435

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

review: Approve (continuous-integration)

[Alberto Aguirre (albaguirre)]

LGTM

review: Approve

[Alan Griffiths (alan-griffiths)]

Code looks harmless. But it is a mystery to me too.

I also think we have a desperate need for automated tests covering this sort of behavior to avoid undetected regressions.

review: Approve

[Kevin DuBois (kdub)]

> Code looks harmless. But it is a mystery to me too.
>
> I also think we have a desperate need for automated tests covering this sort
> of behavior to avoid undetected regressions.

The 'render some simple golden images' in test project?

code looks okay to me

review: Approve

[Robert Carr (robertcarr)]

review: Approve

=== modified file 'examples/demo-shell/demo_renderer.cpp'
--- examples/demo-shell/demo_renderer.cpp	2014-04-24 08:42:12 +0000
+++ examples/demo-shell/demo_renderer.cpp	2014-04-26 08:06:09 +0000
@@ -156,6 +156,10 @@
 {
     glClearColor(0.2f, 0.2f, 0.2f, 1.0f);
     glClear(GL_COLOR_BUFFER_BIT);
+
+    // Ensure we don't change the framebuffer's alpha components (if any)
+    // as that would ruin the appearance of screengrabs. (LP: #1301210)
+    glColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_FALSE);
 }
 
 void DemoRenderer::tessellate(std::vector<Primitive>& primitives,

=== modified file 'include/test/mir_test_doubles/mock_gl.h'
--- include/test/mir_test_doubles/mock_gl.h	2014-03-06 09:08:31 +0000
+++ include/test/mir_test_doubles/mock_gl.h	2014-04-26 08:06:09 +0000
@@ -47,6 +47,7 @@
                  void(GLenum, GLsizeiptr, const GLvoid *, GLenum));
     MOCK_METHOD1(glCheckFramebufferStatus, GLenum(GLenum));
     MOCK_METHOD1(glClear, void(GLbitfield));
+    MOCK_METHOD4(glColorMask, void(GLboolean, GLboolean, GLboolean, GLboolean));
     MOCK_METHOD1(glCompileShader, void(GLuint));
     MOCK_METHOD0(glCreateProgram, GLuint());
     MOCK_METHOD1(glCreateShader, GLuint(GLenum));

=== modified file 'src/server/compositor/gl_renderer.cpp'
--- src/server/compositor/gl_renderer.cpp	2014-04-25 12:35:50 +0000
+++ src/server/compositor/gl_renderer.cpp	2014-04-26 08:06:09 +0000
@@ -284,6 +284,10 @@
 void mc::GLRenderer::begin() const
 {
     glClear(GL_COLOR_BUFFER_BIT);
+
+    // Ensure we don't change the framebuffer's alpha components (if any)
+    // as that would ruin the appearance of screengrabs. (LP: #1301210)
+    glColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_FALSE);
 }
 
 void mc::GLRenderer::end() const

=== modified file 'tests/mir_test_doubles/mock_gl.cpp'
--- tests/mir_test_doubles/mock_gl.cpp	2014-03-06 09:08:31 +0000
+++ tests/mir_test_doubles/mock_gl.cpp	2014-04-26 08:06:09 +0000
@@ -84,6 +84,12 @@
     global_mock_gl->glClear(mask);
 }
 
+void glColorMask(GLboolean r, GLboolean g, GLboolean b, GLboolean a)
+{
+    CHECK_GLOBAL_VOID_MOCK();
+    global_mock_gl->glColorMask(r, g, b, a);
+}
+
 void glEnable(GLenum func)
 {
     CHECK_GLOBAL_VOID_MOCK();

=== modified file 'tests/unit-tests/compositor/test_gl_renderer.cpp'
--- tests/unit-tests/compositor/test_gl_renderer.cpp	2014-04-24 08:42:12 +0000
+++ tests/unit-tests/compositor/test_gl_renderer.cpp	2014-04-26 08:06:09 +0000
@@ -163,6 +163,7 @@
     InSequence seq;
 
     EXPECT_CALL(mock_gl, glClear(_));
+    EXPECT_CALL(mock_gl, glColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_FALSE));
     EXPECT_CALL(mock_gl, glUseProgram(stub_program));
     EXPECT_CALL(renderable, shaped())
         .WillOnce(Return(true));