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

[Daniel van Vugt]

Avoid spuriously blending RGBX as RGBA (LP: #1423462)

Not only was it sub-optimal in performance but more importantly caused
unpredictable discolouration of RGBX clients (first observed by 3v1n0 in
mplayer-SDL but more recently also seen in Xmir -sw). Any clients that
filled the 'X' byte with 0xff were unaffected.

Manual test case:
Force a client to use an RGBX pixel format with alpha values below 0xff:
  $ mir_demo_client_multiwin -p2 # or -p4
Before the fix: Transparent windows.
After the fix: No transparency.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3057
http://jenkins.qa.ubuntu.com/job/mir-ci/5392/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4627
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3534
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4569
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/527/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1546
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1546/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/527
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/527/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4570
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4570/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7194
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/24539
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/528/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5392/rebuild

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Looks good.

Style nit:

+ if (renderable.shaped())

+ if (blend_func_dest == GL_ZERO)
+ glDisable(GL_BLEND);

If any clause of the if-elseif-else statement uses curly braces then all clauses (even single line ones) should use it for consistency.

review: Approve

[Andreas Pokorny (andreas-pokorny)]

> Looks good.
>
> Style nit:
>
> + if (renderable.shaped())
>
> + if (blend_func_dest == GL_ZERO)
> + glDisable(GL_BLEND);
>
> If any clause of the if-elseif-else statement uses curly braces then all
> clauses (even single line ones) should use it for consistency.

yes this looks a bit awkward...
feel free to cleanup, lgtm otherwise

review: Approve

[Daniel van Vugt (vanvugt)]

Hmm, if the framebuffer honours alpha then there's still going to be a minor problem with translucent RGBX windows passing through some uninitialized alpha data.

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3058
http://jenkins.qa.ubuntu.com/job/mir-ci/5393/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4629
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3536
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4571/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/529/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1547
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1547/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/529
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/529/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4572
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4572/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7196/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/24544
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/530/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5393/rebuild

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

review: Abstain

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3060
http://jenkins.qa.ubuntu.com/job/mir-ci/5394/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4630
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3537
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4572
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/530/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1548
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1548/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/530/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4573
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4573/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7197
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/24547
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/531/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5394/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

I see no obvious problems

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3061
http://jenkins.qa.ubuntu.com/job/mir-ci/5402/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4639
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3546
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4581
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/539/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1556
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1556/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/539
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/539/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4582
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4582/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7205
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/24563
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/540/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5402/rebuild

review: Approve (continuous-integration)

=== modified file 'playground/demo-shell/demo_renderer.cpp'
--- playground/demo-shell/demo_renderer.cpp	2015-10-06 02:30:52 +0000
+++ playground/demo-shell/demo_renderer.cpp	2015-10-26 13:25:32 +0000
@@ -311,10 +311,6 @@
     tl_shadow.vertices[1] = {{leftr, top,  0.0f}, {1.0f, 0.0f}};
     tl_shadow.vertices[2] = {{leftr, topr, 0.0f}, {1.0f, 1.0f}};
     tl_shadow.vertices[3] = {{left,  topr, 0.0f}, {0.0f, 1.0f}};
-
-    // Shadows always need blending...
-    glEnable(GL_BLEND);
-    glBlendFunc(GL_ONE, GL_ONE_MINUS_SRC_ALPHA);
 }
 
 void DemoRenderer::tessellate_frame(std::vector<gl::Primitive>& primitives,

=== modified file 'src/renderers/gl/renderer.cpp'
--- src/renderers/gl/renderer.cpp	2015-10-06 02:30:52 +0000
+++ src/renderers/gl/renderer.cpp	2015-10-26 13:25:32 +0000
@@ -209,16 +209,6 @@
 void mrg::Renderer::draw(mg::Renderable const& renderable,
                           Renderer::Program const& prog) const
 {
-    if (renderable.alpha() < 1.0f || renderable.shaped())
-    {
-        glEnable(GL_BLEND);
-        glBlendFunc(GL_ONE, GL_ONE_MINUS_SRC_ALPHA);
-    }
-    else
-    {
-        glDisable(GL_BLEND);
-    }
-
     glUseProgram(prog.id);
     if (prog.last_used_frameno != frameno)
     {   // Avoid reloading the screen-global uniforms on every renderable
@@ -253,12 +243,48 @@
 
     auto surface_tex = texture_cache->load(renderable);
 
+    typedef struct  // Represents parameters of glBlendFuncSeparate()
+    {
+        GLenum src_rgb, dst_rgb, src_alpha, dst_alpha;
+    } BlendSeparate;
+
+    BlendSeparate client_blend;
+
+    // These renderable method names could be better (see LP: #1236224)
+    if (renderable.shaped())  // Client is RGBA:
+    {
+        client_blend = {GL_ONE, GL_ONE_MINUS_SRC_ALPHA,
+                        GL_ONE, GL_ONE_MINUS_SRC_ALPHA};
+    }
+    else if (renderable.alpha() == 1.0f)  // RGBX and no window translucency:
+    {
+        client_blend = {GL_ONE,  GL_ZERO,
+                        GL_ZERO, GL_ONE};  // Avoid using src_alpha!
+    }
+    else
+    {   // Client is RGBX but we also have window translucency.
+        // The texture alpha channel is possibly uninitialized so we must be
+        // careful and avoid using SRC_ALPHA (LP: #1423462).
+        client_blend = {GL_ONE,  GL_ONE_MINUS_CONSTANT_ALPHA,
+                        GL_ZERO, GL_ONE};
+        glBlendColor(0.0f, 0.0f, 0.0f, renderable.alpha());
+    }
+
     for (auto const& p : primitives)
     {
+        BlendSeparate blend;
+
         if (p.tex_id == 0)   // The client surface texture
+        {
+            blend = client_blend;
             surface_tex->bind();
-        else   // Some other texture from the shell (e.g. decorations)
+        }
+        else   // Some other texture from the shell (e.g. decorations) which
+        {      // is always RGBA (valid SRC_ALPHA).
+            blend = {GL_ONE, GL_ONE_MINUS_SRC_ALPHA,
+                     GL_ONE, GL_ONE_MINUS_SRC_ALPHA};
             glBindTexture(GL_TEXTURE_2D, p.tex_id);
+        }
 
         glVertexAttribPointer(prog.position_attr, 3, GL_FLOAT,
                               GL_FALSE, sizeof(mgl::Vertex),
@@ -267,6 +293,17 @@
                               GL_FALSE, sizeof(mgl::Vertex),
                               &p.vertices[0].texcoord);
 
+        if (blend.dst_rgb == GL_ZERO)
+        {
+            glDisable(GL_BLEND);
+        }
+        else
+        {
+            glEnable(GL_BLEND);
+            glBlendFuncSeparate(blend.src_rgb,   blend.dst_rgb,
+                                blend.src_alpha, blend.dst_alpha);
+        }
+
         glDrawArrays(p.type, 0, p.nvertices);
     }
 

=== modified file 'tests/include/mir/test/doubles/mock_gl.h'
--- tests/include/mir/test/doubles/mock_gl.h	2015-07-16 07:03:19 +0000
+++ tests/include/mir/test/doubles/mock_gl.h	2015-10-26 13:25:32 +0000
@@ -44,6 +44,7 @@
     MOCK_METHOD2(glBindRenderbuffer, void(GLenum, GLuint));
     MOCK_METHOD2(glBindTexture, void(GLenum, GLuint));
     MOCK_METHOD2(glBlendFunc, void(GLenum, GLenum));
+    MOCK_METHOD4(glBlendFuncSeparate, void(GLenum, GLenum, GLenum, GLenum));
     MOCK_METHOD4(glBufferData,
                  void(GLenum, GLsizeiptr, const GLvoid *, GLenum));
     MOCK_METHOD1(glCheckFramebufferStatus, GLenum(GLenum));

=== modified file 'tests/mir_test_doubles/mock_gl.cpp'
--- tests/mir_test_doubles/mock_gl.cpp	2015-07-16 07:03:19 +0000
+++ tests/mir_test_doubles/mock_gl.cpp	2015-10-26 13:25:32 +0000
@@ -135,6 +135,13 @@
     global_mock_gl->glBlendFunc (src, dst);
 }
 
+void glBlendFuncSeparate(GLenum src_rgb, GLenum dst_rgb,
+                         GLenum src_alpha, GLenum dst_alpha)
+{
+    CHECK_GLOBAL_VOID_MOCK();
+    global_mock_gl->glBlendFuncSeparate(src_rgb, dst_rgb, src_alpha, dst_alpha);
+}
+
 void glActiveTexture(GLenum unit)
 {
     CHECK_GLOBAL_VOID_MOCK();

=== modified file 'tests/unit-tests/renderers/gl/test_gl_renderer.cpp'
--- tests/unit-tests/renderers/gl/test_gl_renderer.cpp	2015-10-06 02:30:52 +0000
+++ tests/unit-tests/renderers/gl/test_gl_renderer.cpp	2015-10-26 13:25:32 +0000
@@ -166,6 +166,53 @@
     renderer.render(renderable_list);
 }
 
+TEST_F(GLRenderer, enables_blending_for_rgba_surfaces)
+{
+    EXPECT_CALL(*renderable, shaped()).WillOnce(Return(true));
+    EXPECT_CALL(mock_gl, glDisable(GL_BLEND)).Times(0);
+    EXPECT_CALL(mock_gl, glEnable(GL_BLEND));
+
+    mrg::Renderer renderer(display_buffer);
+    renderer.render(renderable_list);
+}
+
+TEST_F(GLRenderer, enables_blending_for_rgbx_translucent_surfaces)
+{
+    EXPECT_CALL(*renderable, alpha()).WillRepeatedly(Return(0.5f));
+    EXPECT_CALL(*renderable, shaped()).WillOnce(Return(false));
+    EXPECT_CALL(mock_gl, glDisable(GL_BLEND)).Times(0);
+    EXPECT_CALL(mock_gl, glEnable(GL_BLEND));
+
+    mrg::Renderer renderer(display_buffer);
+    renderer.render(renderable_list);
+}
+
+TEST_F(GLRenderer, uses_premultiplied_src_alpha_for_rgba_surfaces)
+{
+    EXPECT_CALL(*renderable, shaped()).WillOnce(Return(true));
+    EXPECT_CALL(mock_gl, glDisable(GL_BLEND)).Times(0);
+    EXPECT_CALL(mock_gl, glEnable(GL_BLEND));
+    EXPECT_CALL(mock_gl, glBlendFuncSeparate(GL_ONE, GL_ONE_MINUS_SRC_ALPHA,
+                                             GL_ONE, GL_ONE_MINUS_SRC_ALPHA));
+
+    mrg::Renderer renderer(display_buffer);
+    renderer.render(renderable_list);
+}
+
+TEST_F(GLRenderer, avoids_src_alpha_for_rgbx_blending)  // LP: #1423462
+{
+    EXPECT_CALL(*renderable, alpha()).WillRepeatedly(Return(0.5f));
+    EXPECT_CALL(*renderable, shaped()).WillOnce(Return(false));
+    EXPECT_CALL(mock_gl, glDisable(GL_BLEND)).Times(0);
+    EXPECT_CALL(mock_gl, glEnable(GL_BLEND));
+    EXPECT_CALL(mock_gl,
+                glBlendFuncSeparate(GL_ONE, GL_ONE_MINUS_CONSTANT_ALPHA,
+                                    GL_ZERO, GL_ONE));
+
+    mrg::Renderer renderer(display_buffer);
+    renderer.render(renderable_list);
+}
+
 TEST_F(GLRenderer, binds_for_every_primitive_when_tessellate_is_overridden)
 {
     //'listening to the tests', it would be a bit easier to use a tessellator mock of some sort