Merge lp:~kdub/mir/fix-1378326 into lp:mir

[Kevin DuBois]

android: have the fallback renderer clear the FB_TARGET layer to transparent black pixels (instead of opaque black pixels). This is necessary for the operation of some drivers, which choose to put the gl-rendered layer on top, and 'underlay' the hardware-optimized layers.

lp: #1378326

android: have the fallback renderer clear the FB_TARGET layer to transparent black pixels (instead of opaque black pixels). This is necessary for the operation of some drivers, which choose to put the gl-rendered layer on top, and 'underlay' the hardware-optimized layers.

lp: #1378326

note: this seems to be what surfaceflinger does as well.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1970
http://jenkins.qa.ubuntu.com/job/mir-ci/1771/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/2069
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/2076
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/2006
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/109
        deb: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/109/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/943
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/943/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3015
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/14446

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

Weird. I'm not sure where to look for unexpected side-effects but I can imagine this breaking unstated assumptions somewhere.

review: Approve

[Cemil Azizoglu (cemil-azizoglu)]

I don't think all display hw would require this. Shouldn't we be doing this based on the following hint from hwc?

    /*
     * HWC sets HWC_HINT_CLEAR_FB to tell SurfaceFlinger that it should clear the
     * framebuffer with transparent pixels where this layer would be.
     * SurfaceFlinger will only honor this flag when the layer has no blending
     *
     */
    HWC_HINT_CLEAR_FB = 0x00000002

review: Needs Information

[Daniel van Vugt (vanvugt)]

Looks reasonable, but Cemil makes a good point.

review: Abstain

[Kevin DuBois (kdub)]

We could clear just the region behind the squares that will be tagged overlays (and have that flag set).
After looking at surfaceflinger [1] though, I decided to go with this approach. SF clears the framebuffer to transparent black because clearing the whole region to black is more resource friendly than arranging to clear only the union of rectangles that have been marked overlays.

[1]
https://android.googlesource.com/platform/frameworks/native/+/master/services/surfaceflinger/SurfaceFlinger.cpp (line 1562)

[Alexandros Frantzis (afrantzis)]

OK.

review: Approve

[Cemil Azizoglu (cemil-azizoglu)]

I think it's okay, but there is one minor concern. Some GPUs are severely bandwidth-bound. Clearing the whole region would result in a lot of pixels being pushed out. So even though clearing the whole buffer is convenient, it may cause performance issues.

OTOH, usually these bandwidth-bound GPUs have (much smaller) "tile status" buffers. When a full surface is cleared, the tile status buffers get cleared to reflect that, and the buffer is not really written to with millions of pixels. But if you selectively clear partial rectangles, then the tile status buffers are not used and actual clear operation takes place.

So paradoxically, on these architectures at least, clearing the whole buffer would be much faster.

Anyways, if there are issues in the future, this should be easy to change.

review: Approve

[Kevin DuBois (kdub)]

So, in this mixed mode composition, we have a portion of the screen we have to render via gl, and a portion that we have to clear. In our use cases, we still push out all the pixels on the display, but just save in terms of the number of pixels that have been rendered vs the number that have been fast-cleared. So yes, there's still room for optimization, but it seems more of a subset of trying to do partial screen updates (which we don't try to do at the moment)

=== modified file 'src/platform/graphics/android/hwc_fallback_gl_renderer.cpp'
--- src/platform/graphics/android/hwc_fallback_gl_renderer.cpp	2014-09-30 06:43:36 +0000
+++ src/platform/graphics/android/hwc_fallback_gl_renderer.cpp	2014-10-09 17:21:32 +0000
@@ -103,7 +103,14 @@
 {
     glUseProgram(*program);
 
-    glClearColor(0.0, 0.0, 0.0, 1.0);
+    /* NOTE: some HWC implementations rely on the framebuffer target layer
+     * being cleared to transparent black. eg, in mixed-mode composition,
+     * krillin actually arranges the fb_target in the topmost level of its
+     * display controller and relies on blending to make the overlays appear
+     * /under/ the gl layer. (lp: #1378326)
+     */
+    glClearColor(0.0, 0.0, 0.0, 0.0);
+    glColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE);
     glClear(GL_COLOR_BUFFER_BIT);
     glBlendFunc(GL_ONE, GL_ONE_MINUS_SRC_ALPHA);
     glEnableVertexAttribArray(position_attr);

=== modified file 'tests/unit-tests/graphics/android/test_hwc_fallback_gl_renderer.cpp'
--- tests/unit-tests/graphics/android/test_hwc_fallback_gl_renderer.cpp	2014-09-11 05:51:44 +0000
+++ tests/unit-tests/graphics/android/test_hwc_fallback_gl_renderer.cpp	2014-10-09 17:21:32 +0000
@@ -291,7 +291,8 @@
 
     InSequence seq;
     EXPECT_CALL(mock_gl, glUseProgram(_));
-    EXPECT_CALL(mock_gl, glClearColor(FloatEq(0.0), FloatEq(0.0), FloatEq(0.0), FloatEq(1.0)));
+    EXPECT_CALL(mock_gl, glClearColor(FloatEq(0.0), FloatEq(0.0), FloatEq(0.0), FloatEq(0.0)));
+    EXPECT_CALL(mock_gl, glColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE));
     EXPECT_CALL(mock_gl, glClear(GL_COLOR_BUFFER_BIT));
     EXPECT_CALL(mock_gl, glEnableVertexAttribArray(position_attr_loc));
     EXPECT_CALL(mock_gl, glEnableVertexAttribArray(texcoord_attr_loc));