Merge lp:~albaguirre/mir/make-screencast-capture-opaque into lp:mir

[Alberto Aguirre]

Make screencast capture opaque.

This is an alternative fix to LP: #1301210 which also addresses LP: #1317260

Make screencast capture opaque.

This is an alternative fix to LP: #1301210 which also addresses LP: #1317260

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1613
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1568/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/112
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/113
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/113
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/87
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/87/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/85
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/85/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/249
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/249/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1377
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/6974

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

I suggest this perhaps should not be fixed. If we have any nested servers utilizing the alpha channel that's an unacceptable performance hit on anything other than high-end hardware. Fixing this bug just supports people writing very bad graphics code.

[Daniel van Vugt (vanvugt)]

Oh, I remember now. We agreed nested servers would sometimes need to be blended for fancy shaped transitions. But we also agreed they must not use the alpha channel during regular usage. So a fix will be required at some stage. But at the same time, it does suggest we have slow-rendering nested Mir servers around that need improving.

[Daniel van Vugt (vanvugt)]

(1) There's a performance issue here; glGet functions require a round-trip stalling the pipeline:
57 + glGetFloatv(GL_COLOR_CLEAR_VALUE, old_clear_color);
58 + glGetBooleanv(GL_COLOR_WRITEMASK, old_color_mask);
[http://www.opengl.org/wiki/Common_Mistakes#glGetFloatv_glGetBooleanv_glGetDoublev_glGetIntegerv]
But maybe it's OK in this case since it's immediately before glFinish() which is a definitive round-trip sync anyway.

(2) glClear() in the post_update() function looks like it's almost certainly in the wrong place. It can only work if you assume the compositor recycles the framebuffer, which is a dangerous assumption(?)
74 + make_opaque();
75 glFinish();

(3) Calling glClear() twice per frame is a major bottleneck [1] which should be avoided. How about something like:
void mc::GLRenderer::begin() const
{
    glColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE);
    glClear(GL_COLOR_BUFFER_BIT);
    glColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_FALSE);
}
...?

[1] https://bugs.launchpad.net/mir/+bug/1170243

review: Needs Fixing

[Alberto Aguirre (albaguirre)]

(1) Ack - but I didn't want to just change state and not restore it though.
(2) Ack - I just don't know of any better place that only appliest to screencasting though.

(3) That's what I had on the previous MP (https://code.launchpad.net/~albaguirre/mir/render-opaque-background/+merge/218712)
    Except you have to set the clear color since the default has alpha set to 0.0

However, should be messing with the alpha at all in the normal rendering path? Or should we just impose that the compositor will enforce opaqueness once it's done? I think it *NEEDS DISCUSSION* - but I'm leaning towards opaqueness.

In any case the purpose of this MP was to *FOR NOW* limit the opaqueness decision to screencasting only.

[Alberto Aguirre (albaguirre)]

OK, after some more thought I do think the previous MP (https://code.launchpad.net/~albaguirre/mir/render-opaque-background/+merge/218712) is a better solution.

Currently mir does not support transparent framebuffers properly anyway (per-pixel alpha in resulting framebuffer is not correct).

Unmerged revisions

1613. By Alberto Aguirre on 2014-05-08

Make screencast capture opaque.

This is an alternative fix to LP: #1301210 which also addresses LP: #1317260

1612. By Alberto Aguirre on 2014-05-08

Revert r1581.

=== modified file 'examples/demo-shell/demo_renderer.cpp'
--- examples/demo-shell/demo_renderer.cpp	2014-05-05 03:36:45 +0000
+++ examples/demo-shell/demo_renderer.cpp	2014-05-08 22:14:30 +0000
@@ -156,10 +156,6 @@
 {
     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-05-05 03:36:45 +0000
+++ include/test/mir_test_doubles/mock_gl.h	2014-05-08 22:14:30 +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_METHOD4(glColorMask, void(GLboolean, GLboolean, GLboolean, GLboolean));
     MOCK_METHOD1(glCompileShader, void(GLuint));
     MOCK_METHOD0(glCreateProgram, GLuint());

=== modified file 'src/server/compositor/gl_renderer.cpp'
--- src/server/compositor/gl_renderer.cpp	2014-05-06 03:33:05 +0000
+++ src/server/compositor/gl_renderer.cpp	2014-05-08 22:14:30 +0000
@@ -290,10 +290,6 @@
 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 'src/server/compositor/screencast_display_buffer.cpp'
--- src/server/compositor/screencast_display_buffer.cpp	2014-03-26 05:48:59 +0000
+++ src/server/compositor/screencast_display_buffer.cpp	2014-05-08 22:14:30 +0000
@@ -24,6 +24,23 @@
 namespace mc = mir::compositor;
 namespace mg = mir::graphics;
 namespace geom = mir::geometry;
+namespace
+{
+void make_opaque()
+{
+    // Ensure screen captures are opaque (LP: #1301210)
+    GLfloat old_clear_color[4] {0.0f, 0.0f, 0.0f, 0.0f};
+    GLboolean old_color_mask[4] {GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE};
+    glGetFloatv(GL_COLOR_CLEAR_VALUE, old_clear_color);
+    glGetBooleanv(GL_COLOR_WRITEMASK, old_color_mask);
+
+    glClearColor(0.0f, 0.0f, 0.0f, 1.0f);
+    glColorMask(GL_FALSE, GL_FALSE, GL_FALSE, GL_TRUE);
+    glClear(GL_COLOR_BUFFER_BIT);
+    glClearColor(old_clear_color[0], old_clear_color[1], old_clear_color[2], old_clear_color[3]);
+    glColorMask(old_color_mask[0], old_color_mask[1], old_color_mask[2], old_color_mask[3]);
+}
+}
 
 mc::ScreencastDisplayBuffer::ScreencastDisplayBuffer(
     geom::Rectangle const& rect,
@@ -97,6 +114,7 @@
 
 void mc::ScreencastDisplayBuffer::post_update()
 {
+    make_opaque();
     glFinish();
 }
 

=== modified file 'tests/mir_test_doubles/mock_gl.cpp'
--- tests/mir_test_doubles/mock_gl.cpp	2014-05-05 03:36:45 +0000
+++ tests/mir_test_doubles/mock_gl.cpp	2014-05-08 22:14:30 +0000
@@ -90,6 +90,12 @@
     global_mock_gl->glColorMask(r, g, b, a);
 }
 
+void glClearColor(GLclampf r, GLclampf g, GLclampf b, GLclampf a)
+{
+    CHECK_GLOBAL_VOID_MOCK();
+    global_mock_gl->glClearColor(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-05-06 03:33:05 +0000
+++ tests/unit-tests/compositor/test_gl_renderer.cpp	2014-05-08 22:14:30 +0000
@@ -167,7 +167,6 @@
     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));

=== modified file 'tests/unit-tests/compositor/test_screencast_display_buffer.cpp'
--- tests/unit-tests/compositor/test_screencast_display_buffer.cpp	2014-03-05 07:02:29 +0000
+++ tests/unit-tests/compositor/test_screencast_display_buffer.cpp	2014-05-08 22:14:30 +0000
@@ -184,3 +184,24 @@
 
     db.render_and_post_update(renderables, std::ref(mock_render_functor));
 }
+
+TEST_F(ScreencastDisplayBufferTest, makes_capture_opaque)
+{
+    using namespace testing;
+
+    geom::Rectangle const rect{{100,100}, {800,600}};
+    mtd::StubBuffer stub_buffer;
+
+    mc::ScreencastDisplayBuffer db{rect, stub_buffer};
+
+    Mock::VerifyAndClearExpectations(&mock_gl);
+
+    InSequence s;
+    EXPECT_CALL(mock_gl, glClearColor(_, _, _, 1.0f));
+    EXPECT_CALL(mock_gl, glColorMask(GL_FALSE, GL_FALSE, GL_FALSE, GL_TRUE));
+    EXPECT_CALL(mock_gl, glClear(GL_COLOR_BUFFER_BIT));
+    EXPECT_CALL(mock_gl, glClearColor(_, _, _, _));
+    EXPECT_CALL(mock_gl, glColorMask(_, _, _, _));
+
+    db.post_update();
+}