Merge lp:~robertcarr/mir/alpha-for-nested-servers into lp:mir

[Robert Carr]

Enable alpha channel for nested server surfaces.

Enable alpha channel for nested server surfaces. This is required by the split-out greeter.

[Michael Terry (mterry)]

Looks sensible to me, and I saw the positive results of your branch. But still, I feel like someone more familiar with Mir internals should do the actual blessing of this branch.

review: Approve

[Kevin DuBois (kdub)]

yes, overall, its sensible to me too.. no test though? :)

in deciding "find_transparent_surface_format", what about geom::PixelFormat::bgr_888?

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1177
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/287/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/45
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/44
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/16
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/16/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/16
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/16/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/287/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

1. You forgot mir_pixel_format_bgr_888. See also bug 1236254.
19 + if (*f != mir_pixel_format_xbgr_8888 &&
20 + *f != mir_pixel_format_xrgb_8888)

2. I assuming the nesting and/or alpha blending is disabled after login? Otherwise the performance overhead would be quite high.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Re #2: I'm not talking about nesting latency concerns. Just the performance overhead of blending.

During regular composition we rely on most surfaces to not be alpha, so they are candidates for bypass and occlusion culling. We also optimize the GL renderer for non-blendable surfaces, because it makes a performance difference.

So while blending is cool and great to use during various effects, I just want to make sure we're not blending still after login. Because that would be bad.

[Alexandros Frantzis (afrantzis)]

> Enable alpha channel for nested server surfaces. This is required by the split-out greeter.

What exactly does the greeter need to do?
Is per-surface opacity not enough for our use case?

review: Needs Information

[Michael Terry (mterry)]

> What exactly does the greeter need to do?
> Is per-surface opacity not enough for our use case?

The greeter wants to be able to show parts of the session behind it. (i.e. you swipe greeter away from the right, you see your session waiting for you) It's likely the greeter will be implemented as a mirserver. If it was just a mirclient, per-surface opacity would be enough. But each mirserver draws its back as black, so you can't stack mirservers with partial opacity.

[Michael Terry (mterry)]

> I assuming the nesting and/or alpha blending is disabled after login? Otherwise the performance overhead would be quite high.

The unity8 shell session will not take advantage of this background opacity. It will always just fill the screen with its dash. I assume if the background is filled with a surface, there isn't a performance overhead?

The greeter won't use any clever blending or blurring effects for the first go, just have some 0-opacity parts of the screen. Later on, we want to add some blurring, but that would of course go away after you log in.

[Kevin DuBois (kdub)]

Daniel makes a good point about the performance impact...

> The unity8 shell session will not take advantage of this background opacity.
> It will always just fill the screen with its dash. I assume if the background
> is filled with a surface, there isn't a performance overhead?

the final frame is the same, more work is done with alpha blending turned on

with blending off,
read src, write to dst

with blending on
read src, read dst, alpha blend operation, write to dst

[Daniel van Vugt (vanvugt)]

I'm told the Unity8 shell is implemented using alpha right now. Partly because Mir didn't have surface resizing implemented (yet). So I guess as soon as we can, we should make sure Unity8 uses non-alpha pixel formats (where possible). XMir already does.

As for the whole nested display... I think we need to default to opaque surface formats for performance, not:
38 + egl_pixel_format{find_transparent_surface_format(*connection)},
Although obviously to suit greeter needs, that should be configurable as transparent. Which is fine if it only occurs while the greeter is visible.

I'm basing this on assumptions about how nesting actually works, so need some guidance as to whether my concerns are realistic at all... So if the greeter's on top of a session, is it composited with the existing compositing logic or something new?

review: Needs Information

[Daniel van Vugt (vanvugt)]

P.S. If you only require a constant level of alpha transparency across the whole surface, then you can still use RGBX/XRGB etc. And then set the surface alpha property. It's only necessary to use an alpha pixel format if you intend to define shape and varying levels of transparency across the surface.

[Daniel van Vugt (vanvugt)]

Needs information but also still needs fixing (mir_pixel_format_bgr_888).

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

I can imagine that we might sometimes want alpha on a nested "FB", but always seems wrong.

Can I paraphrase what I think is being said:

The host will have surfaces for nested sessions for example unity8 and greeter.

There's no value (and cost) to alpha for the unity8 surface.

The idea is that the greeter surface can overlay the unity8 surface using transparency. Is this alpha varied across the surface? (For shapes? Or gradations?)

If we need variable opacity across the surface then the greeter needs to be able to request a surface with alpha. If not, then perhaps there are alternative options.

review: Needs Information

[Michael Terry (mterry)]

> The idea is that the greeter surface can overlay the unity8 surface using transparency. Is this alpha varied across the surface? (For shapes? Or gradations?)

For now, no variation. Eventually, we'd like to add some sort of blurring effect, performance willing.

[Daniel van Vugt (vanvugt)]

> For now, no variation. Eventually, we'd like to add some sort of blurring
> effect, performance willing.

In that case it sounds like the proposal should be rejected. And instead we need to expose something::Surface::set_alpha() so the greeter can vary its transparency.

[Michael Terry (mterry)]

> In that case it sounds like the proposal should be rejected. And instead we need to expose something::Surface::set_alpha() so the greeter can vary its transparency.

Well, and color, eh? Why not just have the background always be full transparency, and the greeter can add a layer that is semi-transparent if it wants?

[Daniel van Vugt (vanvugt)]

Always transparent is unacceptable. See the performance discussion above.

Also, needs fixing; mir_pixel_format_bgr_888. But ignoring that, I think this is overall the wrong approach. We need to at least propose a solution that doesn't bind us to being stuck with only one code path which impacts performance too much.

If you just need a consistent level of transparency (and later, blur), then please reject this proposal and replace it with something that sets the surface alpha value instead.

If you need varying transparency then please rewrite this proposal so that it does not default to blending always-on.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> Always transparent is unacceptable. See the performance discussion above.
>
> Also, needs fixing; mir_pixel_format_bgr_888. But ignoring that, I think this
> is overall the wrong approach. We need to at least propose a solution that
> doesn't bind us to being stuck with only one code path which impacts
> performance too much.
>
> If you just need a consistent level of transparency (and later, blur), then
> please reject this proposal and replace it with something that sets the
> surface alpha value instead.
>
> If you need varying transparency then please rewrite this proposal so that it
> does not default to blending always-on.

+1

review: Needs Fixing

[Robert Carr (robertcarr)]

I wonder if surface opacity is really enough for the greeter. How does the panel fit in. Michael?

Perhaps we should expose it is as a server configuration (build time) option so the greeter can use alpha but Unity will not have to.

[Alan Griffiths (alan-griffiths)]

> Perhaps we should expose it is as a server configuration (build time) option
> so the greeter can use alpha but Unity will not have to.

Why not make it a run time configuration option?

[Daniel van Vugt (vanvugt)]

Can anyone point to an image or mockup from design to show what the intended greater appearance is?

[Robert Carr (robertcarr)]

Having trouble describing the option well

--nested-use-transparent-framebuffer
--nested-pixel-formats="argb888,abgr888"

etc...

[Robert Carr (robertcarr)]

Seems like this doesn't matter right now

Unmerged revisions

1177. By Robert Carr on 2013-10-29

Fix

1176. By Robert Carr on 2013-10-29

Yeah. Alpha and stuff

=== modified file 'src/server/graphics/nested/nested_display.cpp'
--- src/server/graphics/nested/nested_display.cpp	2013-10-25 09:40:26 +0000
+++ src/server/graphics/nested/nested_display.cpp	2013-10-29 23:42:16 +0000
@@ -36,7 +36,7 @@
 namespace
 {
 
-MirPixelFormat find_opaque_surface_format(MirConnection* connection)
+MirPixelFormat find_transparent_surface_format(MirConnection* connection)
 {
     static unsigned const max_formats = 32;
     MirPixelFormat formats[max_formats];
@@ -48,8 +48,8 @@
     // Find an opaque surface format
     for (auto f = formats; f != formats+valid_formats; ++f)
     {
-        if (*f == mir_pixel_format_xbgr_8888 ||
-            *f == mir_pixel_format_xrgb_8888)
+        if (*f != mir_pixel_format_xbgr_8888 &&
+            *f != mir_pixel_format_xrgb_8888)
         {
             return *f;
         }
@@ -66,7 +66,7 @@
     EGL_RED_SIZE, 8,
     EGL_GREEN_SIZE, 8,
     EGL_BLUE_SIZE, 8,
-    EGL_ALPHA_SIZE, 0,
+    EGL_ALPHA_SIZE, 8,
     EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT,
     EGL_NONE
 };
@@ -153,7 +153,7 @@
     event_handler{event_handler},
     display_report{display_report},
     egl_display{*connection},
-    egl_pixel_format{find_opaque_surface_format(*connection)},
+    egl_pixel_format{find_transparent_surface_format(*connection)},
     outputs{}
 {
     egl_display.initialize();