Merge lp:~kdub/mir/prep-for-1348330 into lp:mir

[Kevin DuBois]

Simplify the demo shell occlusion detection.

Simplify the demo shell occlusion detection.

This averts, but does not fix lp: #1299977
It is preparation for fixing lp: #1348330, as the demo shell must avert calling mg::DisplayBuffer::post_renderables_if_optimizable in more cases than the default shell does.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1690
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2099/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/869
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/875/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/868
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/620
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/620/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/618
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/618/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2767
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2767/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2012
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/9497

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

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

I started on a proper solution for bug 1299977 back in May. That involves separating the concepts of window size and client size (as you'll find all existing windowing systems do). Once that's clear, we just switch the occlusion test to use the extents (including frame) of a window instead of its client region.

So the right (and elegant) solution is coming. I'm concerned about any change that appears to fix bug 1299977 without implementing this. It's suspicious but I haven't done a review yet...

If the only goal is to resolve bug 1299977 then I suspect this is probably the wrong answer.

[Kevin DuBois (kdub)]

I think the disagreement goes a little bit deeper because we haven't come to a consensus about the concepts that we're incorporating into the scene. I argue that we shouldn't have the concept of window size and client size in the scene because we have 1 user that handles this in a custom way (U8) and one user that doesn't need this at all (USC).

Once we start getting the concept of window size vs decoration side, we will have this awkward situation where the android hwc code will just have to start rejecting renderables with decorations.

So really, this solution puts the demo shell on the trajectory of reimplementing the scene in the same way that the users have chosen to reimplement the scene (that is, reimplementing parts of the compositing system at a higher level than the Renderer).

I want to protect the core (buffer swapping, turning the display on, setting up the GL context) from the desktop-style additional needs (animations, window decorations, 'bling', cursor images, etc etc).

My idea/"unconcensussed plan" for this is to have the SceneElement be expandable so that a compositor writer can pack it with data that the core mir will preserve, but this data will be opaque to the "core mir". So for instance, the compositor can 'tag' each SceneElement with a bunch of information about its window decorations, which will be specific to the compositor implementation. On the next pass of the shell's render the data would be preserved (or perhaps updated in some universal way, like timing info).

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1691
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2113/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/886
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/892
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/885
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/634
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/634/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/632
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/632/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2831
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2831/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2029
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/9562

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

review: Approve (continuous-integration)

[Cemil Azizoglu (cemil-azizoglu)]

I'm interested in seeing Daniel's solution as well. Since Kevin is blocked by this for hwc optimizations, I'll go ahead with the review.

Minor things:

9 +add_library(mirdraw STATIC graphics_utils.cpp demo-shell/occlusion.cpp)

mirdraw should be a lib that others (besides demo-shell) can use and hence its source should not be part of a particular example program. So occlusion.cpp should move outside demo-shell/.

294 -
295 +namespace mc = mir::compositor;

preserve the blank line?

review: Approve

[Alan Griffiths (alan-griffiths)]

I'm not entirely sure how to validate this but the code looks OK and it runs. We can fix problems later if they arise.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-autolanding/885/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/950/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/956/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/948/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-autolanding/269/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-autolanding/269/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3147/console

review: Needs Fixing (continuous-integration)

[Alexandros Frantzis (afrantzis)]

OK for now.

> SceneElement be expandable so that a compositor writer can pack it with data that the core mir will > preserve, but this data will be opaque to the "core mir"

Interesting idea.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1692
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2192/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/997
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1003/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/992/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/713/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/711/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3362/console

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1693
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2193/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1000
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1006
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/995
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/715
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/715/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/716
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/716/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3374
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3374/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2123
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/10091

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

I don't like to be the spoilsport, but will block this for two reasons:

(1) As mentioned in the description this proposal branches (duplicates) functionality on the assumption we will re-unify it later. I think that's a bad assumption and unlikely to happen.

(2) A generic solution is possible. One that works for all (most?) shells. I think I'd prefer to continue with that (started in May) instead of landing solutions that are shell-specific.

review: Disapprove

[Daniel van Vugt (vanvugt)]

P.S. If you want to enable overlay optimizations by default, then you should probably be focusing more on how the Ubuntu touch Qt shell needs it.

In the least you can overlay full-screen surfaces (on the host server when nesting) right now using the bypass detection algorithm. Then worry about enabling hardware overlays in the nested server later. So we can get most of the performance benefits of overlays turned on right now without worrying at all about anything to to with demo shell.

[Kevin DuBois (kdub)]

Its more that the default occlusion algorithm and the demo occlusion algorithm are doing different things, with the demo occlusion algorithm having to factor in the decorations on the top,bottom & right. There is some overlap because both are rectangle-based occlusion filtering but this is an attempt to satisfy the primary complaints about lp:~kdub/mir/fix-1299977. Namely, we don't want to use a private algorithm in the demos, yet, we don't want to expose the algorithm publicly because its not universal.

[Kevin DuBois (kdub)]

> (2) A generic solution is possible. One that works for all (most?) shells. I
> think I'd prefer to continue with that (started in May) instead of landing
> solutions that are shell-specific.

If we try to come up with a generic solution that addresses all shells, we will end up with some shells that don't use some of the features. We also end up with some shells trying to jam features into the "one algorithm" that other shells don't want, and we limit those shells if they don't get their special feature landed. (eg, say someone wants an obscure fragment shader that affects occlusion detection). Given the flexibility and extensibility that we're aiming for, I'd rather see compositor writers rewriting things that are specific to that shell's needs, then aiming for one set of behaviors that covers every shell we encounter.

Like, every shell writer can be expected to write GL code, and every shell writer would have the responsibility to do occlusions and visibility detection, based on the unique things that the shell wants to appear on the screen. This is the role of the compositor object, and would let us pop in new compositor objects that have differing behavior (LSP) without changing the correctness of the mir system. (and also without burdening core mir with the responsibility to make the most universal drawing code to assist the shell writers)

As we determine things that are truly universal to all shells we should pull them into core mir. For instance mg::DisplayBuffer is needed by every shell so its proper to have that interface there. Other things like an animation framework might also fit in with core mir. Occlusion seems to me that there are as many variations as there are possible shells, so it seems less likely that it belongs in core mir.

[Kevin DuBois (kdub)]

> (2) A generic solution is possible. One that works for all (most?) shells. I
> think I'd prefer to continue with that (started in May) instead of landing
> solutions that are shell-specific.

If we try to come up with a generic solution that addresses all shells, we will end up with some shells that don't use some of the features. We also end up with some shells trying to jam features into the "one algorithm" that other shells don't want, and we limit those shells if they don't get their special feature landed. (eg, say someone wants an obscure fragment shader that affects occlusion detection). Given the flexibility and extensibility that we're aiming for, I'd rather see compositor writers rewriting things that are specific to that shell's needs, then aiming for one set of behaviors that covers every shell we encounter.

Like, every shell writer can be expected to write GL code, and every shell writer would have the responsibility to do occlusions and visibility detection, based on the unique things that the shell wants to appear on the screen. This is the role of the compositor object, and would let us pop in new compositor objects that have differing behavior (LSP) without changing the correctness of the mir system. (and also without burdening core mir with the responsibility to make the most universal drawing code to assist the shell writers)

As we determine things that are truly universal to all shells we should pull them into core mir. For instance mg::DisplayBuffer is needed by every shell so its proper to have that interface there. Other things like an animation framework might also fit in with core mir. Occlusion seems to me that there are as many variations as there are possible shells, so it seems less likely that it belongs in core mir.

[Kevin DuBois (kdub)]

I don't know why Launchpad posted twice, but same content in each posting.

[Daniel van Vugt (vanvugt)]

Yeah, if you do assume in future every shell should provide its own compositor then this partially makes sense. However I don't think that's a path we want to go down, and have explicitly decided not to. We do want to unify and provide common compositing ability where possible. A shell shouldn't be forced to write a new compositor. If we did it then I'd like to see it done absolutely, and the default compositor removed at the same time (and thus get team agreement on the plan).

Other things like occlusion testing between windows that are kind-of rectangular and have a little bit of shape to them are common problems. We shouldn't re-invent occlusion testing every time a new shell comes along... On the other hand, if we defaulted to no occlusion testing at all and make it an optional feature that some shells can implement then I'm happy with that -- It would not be a required feature to build a shell and bug 1299977 would also be solved.

Back to the primary problem, where occlusion testing needs to deal with decorations, I think there are three good options:
  1. No occlusion testing by default. Put it all in your shell if you want it but make it optional for shells.
  2. Continue with the work I started in May and provide extended region information allowing window-based shells to detect occlusions accurately. This is relatively simple in theory, but blocked by other RTM-related work taking precedent. It also has to be finished to implement correct window placement anyway.
  3. Continue with the plan that everything is a node of the scene graph. Then occlusion testing is a generic graph traversal algorithm that can support any primitives. Not just decorations.

[Daniel van Vugt (vanvugt)]

I particularly do not want this branch to land because it adds complexity that would be hard to wind back. And we would unwind it as at least options (2) and (3) above are going to be implemented (already begun).

[Cemil Azizoglu (cemil-azizoglu)]

Daniel, will you be providing your version soon? If it'll take some time, perhaps we should let kdub proceed and when you get to it, you can put up an MP the way you like it. I just feel that progress should continue. But it's all software and can be changed later.

Or perhaps there is some way that this MP can be fixed in a minimally acceptible (to you) manner. If so, please comment and let Kevin take a stab at it.

[Kevin DuBois (kdub)]

> Yeah, if you do assume in future every shell should provide its own compositor
> then this partially makes sense. However I don't think that's a path we want
> to go down, and have explicitly decided not to. We do want to unify and
> provide common compositing ability where possible. A shell shouldn't be forced
> to write a new compositor. If we did it then I'd like to see it done
> absolutely, and the default compositor removed at the same time (and thus get
> team agreement on the plan).

In the present we have 3 shells/clients between USC, demo, and unity8, that have all chosen to have different compositor implementations. We've already gone down the path, but haven't been able to shepherd all the implementations into using the interfaces that we intended. :)
This hopefully gets the demo/usc using the mc::Compositor interface like we intended, and we'll have to work more closely to get qtcomp/unity8 not to replace the MultiThreadedCompositor system.

> Other things like occlusion testing between windows that are kind-of
> rectangular and have a little bit of shape to them are common problems. We
> shouldn't re-invent occlusion testing every time a new shell comes along... On
> the other hand, if we defaulted to no occlusion testing at all and make it an
> optional feature that some shells can implement then I'm happy with that -- It
> would not be a required feature to build a shell and bug 1299977 would also be
> solved.

I agree that the easier we make things for the shell writers, but I think its more important not to force the shell writers into extending implementations. So, to put it another way, the best way to make things nice for the shell writers is to have a system of objects that we can plug into the core. Code reuse is good of course, but my first attempt was thwarted because we decided that code reuse doesn't trump exposing an additional 'shell writers api/abi'. The hard problem is how to get those public server interfaces right, and this seems to favor making it a bit easier by rallying around the interfaces that we want the users to implement, and by not introducing a mini-toolkit (or a big swiss army object) that aims to 'make opengl easy' :)
In this case, the judgement call comes at the expense of having occlusion look somewhat the same, but doing different things to factor in the shell's decorations.

[Kevin DuBois (kdub)]

> Yeah, if you do assume in future every shell should provide its own compositor
> then this partially makes sense. However I don't think that's a path we want
> to go down, and have explicitly decided not to. We do want to unify and
> provide common compositing ability where possible. A shell shouldn't be forced
> to write a new compositor. If we did it then I'd like to see it done
> absolutely, and the default compositor removed at the same time (and thus get
> team agreement on the plan).

In the present we have 3 shells/clients between USC, demo, and unity8, that have all chosen to have different compositor implementations. We've already gone down the path, but haven't been able to shepherd all the implementations into using the interfaces that we intended. :)
This hopefully gets the demo/usc using the mc::Compositor interface like we intended, and we'll have to work more closely to get qtcomp/unity8 not to replace the MultiThreadedCompositor system.

> Other things like occlusion testing between windows that are kind-of
> rectangular and have a little bit of shape to them are common problems. We
> shouldn't re-invent occlusion testing every time a new shell comes along... On
> the other hand, if we defaulted to no occlusion testing at all and make it an
> optional feature that some shells can implement then I'm happy with that -- It
> would not be a required feature to build a shell and bug 1299977 would also be
> solved.

I agree that the easier we make things for the shell writers, but I think its more important not to force the shell writers into extending implementations. So, to put it another way, the best way to make things nice for the shell writers is to have a system of objects that we can plug into the core. Code reuse is good of course, but my first attempt was thwarted because we decided that code reuse doesn't trump exposing an additional 'shell writers api/abi'. The hard problem is how to get those public server interfaces right, and this seems to favor making it a bit easier by rallying around the interfaces that we want the users to implement, and by not introducing a mini-toolkit (or a big swiss army object) that aims to 'make opengl easy' :)
In this case, the judgement call comes at the expense of having occlusion look somewhat the same, but doing different things to factor in the shell's decorations.

[Kevin DuBois (kdub)]

strange launchpad keeps double-posting me...

[Daniel van Vugt (vanvugt)]

Yes it does appear people are writing their own compositors despite us recommending against it. And I too want to resolve the blurry seams that have formed between compositor classes and renderers.

OK I can make suggestions for moving forward:

(1) Remove all references to bug 1299977. Because what's proposed here is only for demo shell, and not a generic fix for bug 1299977. We can do enough work to unblock overlay enablement without waiting for a full fix of bug 1299977 to land.

(2) Make demo-shell's flagging of occlusions simple and dumb, so that the word "decoration" is never mentioned. Just allow overlays for the surface that's "on top" (last on the render list). A generic solution which accounts for decorations more carefully will come later. You don't need to wait for that to unblock.

We can argue about the existence of DefaultDisplayByfferCompositor another day.

It's a little bit silly that demo shell should block unity8 progress so let's get over this hump with minimal code change (which also makes it more likely to get backported to stable series 0.5 for RTM).

review: Needs Fixing

[Kevin DuBois (kdub)]

Cool, sounds good to me. Working on fixing.

[Kevin DuBois (kdub)]

okay, so the new approach of doing a much more simple filtering of the renderable list is significantly different than improving the algorithm, in that it doesn't fix 1299977, it just averts it.

So I'll move this MP to rejected. I'll push the improvements to lp:~kdub/mir/fix-1348330 and submit a new merge proposal (which is mostly the same as this one, but it helps to document the problem better, and remove the references to the 1299977 bug)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1698
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~kdub/mir/prep-for-1348330/+merge/228188/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2230/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1075
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1081
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1065/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/752/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/753
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/753/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3703
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3703/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2182/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/10379

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1698
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~kdub/mir/prep-for-1348330/+merge/228188/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2243/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1100
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1106
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1091
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/765
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/765/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/766
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/766/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/26
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/26/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2211
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/10465

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

review: Needs Fixing (continuous-integration)

[Kevin DuBois (kdub)]

The failure was just me forgetting to set the commit message, no real reason to retrigger^^.

[Alexandros Frantzis (afrantzis)]

Hmm, the mir demo shell is a helpful tool to try out changes in our default DB compositor (the demo server is too simple to act as such a tool). With this change we would lose this tool. Would it make sense to only enable the custom compositor with --enable-overlays, or perhaps have another option to force the default compositor (--default-compositor)?

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1698
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2255/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1119
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1125
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1110/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/777
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/777/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/778
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/778/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/45
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/45/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2233/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/10570

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

review: Needs Fixing (continuous-integration)

[Alberto Aguirre (albaguirre)]

> Hmm, the mir demo shell is a helpful tool to try out changes in our default DB
> compositor (the demo server is too simple to act as such a tool). With this
> change we would lose this tool. Would it make sense to only enable the custom
> compositor with --enable-overlays, or perhaps have another option to force the
> default compositor (--default-compositor)?

+1 for --default-compositor or perhaps --disable-decorations

[Daniel van Vugt (vanvugt)]

I'm still a bit nervous about the code duplication between:
  void me::DemoCompositor::composite()
and
  mc::DefaultDisplayBufferCompositor::composite()

They're similar but different. I had changes planned for mc::DefaultDisplayBufferCompositor::composite() soon and if we forget to duplicate such changes in DemoCompositor then we can easily regress (since it's not tested) without being aware.

[Daniel van Vugt (vanvugt)]

Actually, I'd like to avoid any contentious changes to demo shell like giving it redundant compositor code right now. That's bad for maintenance and another strike against "break it now and hope we have time to fix it later".

+1 instead for simply making overlays a command line option (which knowingly break the decorations, that's fine).

review: Needs Fixing

[Kevin DuBois (kdub)]

@ --disable-decorations

"./mir_demo_server_shell --disable-decorations" would paint the scene fine and use the hwc code.

However, ./mir_demo_server_shell would still use the hwc code, but wouldn't paint the scene properly. It just averts the problem when a switch is set, it doesn't solve it in a generic way that gives an example to shell writers as to how to write a proper shell implementation.

@ --default-compositor
My concept is that ./mir_demo_server_basic is the implementation that uses the defaults in the simplest way. ./mir_demo_server_shell is the example that overrides the default to show our users how to write an alternative shell. I agree that testing the default compositor is very useful. However, I'd say that we should improve the ./mir_demo_server_basic (and the defaults in the mir code) so that the basic server is usable for testing. This would probably involve pulling some of the useful stuff from ./mir_demo_server_shell into the core code.

[Kevin DuBois (kdub)]

@code duplication
This is walking the sometimes-fine line between duplication and the principle of substitutability. Given that a core objective of mir is to have a substitutable shell, its important that we get this right.

So obviously, I've concluded that its time to err in favor of substitutability.
My thinking goes like this:
default-compositor:
* paints squares
* alpha blends
* clears to black
* uses our optimizations
demo-compositor:
* paints squares
* paints shadows
* paints titlebar
* alpha blends
* clears to grey
* attempts to use the optimizations (has a platform-specific problem atm)

In the interest of /not/ duplicating the shared tasks, I want to have the top-level object (the "compositor") be different, while reusing objects that perform these various sub-tasks (DB and GLrenderer). This MP aims to make that top level object different so it can command the sub-objects to do different things. Admittedly the object structure of the GLRenderer makes it non-obvious that different things are actually being commanded. When render() is called, it does markedly different things in the mc::GLRenderer vs the me::GLRenderer because various private functions that are called by render() have been overridden by the example renderer.

The reason to override the mc::Compositor interface as opposed as the renderer is because the difference in behavior involves the DisplayBuffers. The demo compositor needs to command the DisplayBuffer differently than the default compositor. We've 'substituted' at just one level too low; this made sense when it was done, but now we have to shift a bit to override at a level that allows the DisplayBuffer object to be ordered to do different things.

[Alexandros Frantzis (afrantzis)]

> However, I'd say that we should improve the ./mir_demo_server_basic (and the defaults in the mir
> code) so that the basic server is usable for testing. This would probably involve pulling some of
> the useful stuff from ./mir_demo_server_shell into the core code.

I would go with that (I haven't used ./mir_demo_server_basic in ages), with the caveat that I think we should be careful about what we pull into "mir core" code. I'd rather we shared code by using common infrastructure inside examples/ only, but of course it depends on the details.

Basically what I want in terms of testing is what mir_demo_server_shell currently provides, sans the decorations. Perhaps that's too much for a server to be called basic. We can always introduce a new server if want to keep the 'basic' one intact. As mentioned, I haven't found 'basic' useful for testing, and I am not sure if it really offers much in terms of being an example, since all the complexity (or lack of it) lies in the ServerConfiguration object => just don't override anything for a really basic server.

[Alexandros Frantzis (afrantzis)]

I guess bottom line, I am OK with this change, as long as we plan to have a replacement for the testing opportunities we are losing.

review: Approve

[Kevin DuBois (kdub)]

glmark2-es2-mir: egl.c:243: eglCreateWindowSurface: Assertion `((struct ANativeWindowBuffer *) win)->common.magic == (((unsigned)('_')<<24)|((unsigned)('w')<<16)|((unsigned)('n')<<8)|(unsigned)('d'))' failed.

makes me think that somehow the android/mesa platforms got mixed up when this was ran... retriggering CI

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1698
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2293/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1173
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1179
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1161
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/815
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/815/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/816
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/816/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/96
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/96/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2284
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/10879

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Keep in mind mir_demo_server_basic was going to be deleted. It's superseded by mir_demo_server_minimal, which should stay absolutely minimal. I think it's too confusing maintaining mir_demo_server_basic that is neither a working shell, nor minimal.

OK, I'm not comfortable with this but don't need to block kdub's work for the sake of a couple dozen lines of duplication.

review: Abstain

[Alan Griffiths (alan-griffiths)]

> Keep in mind mir_demo_server_basic was going to be deleted. It's superseded by
> mir_demo_server_minimal, which should stay absolutely minimal. I think it's
> too confusing maintaining mir_demo_server_basic that is neither a working
> shell, nor minimal.

Wow! I knew about mir_demo_server_basic and mir_demo_server_shell but I see we also have:

bin/mir_demo_server_minimal
bin/mir_demo_server_translucent

Four example servers are probably too many. (Not that this MP affects that)

[Alan Griffiths (alan-griffiths)]

I think we need to review the example servers we provide and what functionality we need to demonstrate and/or exercise.

But this MP doesn't make anything worse.

review: Approve

[Cemil Azizoglu (cemil-azizoglu)]

LGTM. I'm glad we moved this one along. Nice!

review: Approve

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'examples/demo-shell/CMakeLists.txt'
--- examples/demo-shell/CMakeLists.txt	2014-03-06 06:05:17 +0000
+++ examples/demo-shell/CMakeLists.txt	2014-07-24 19:26:33 +0000
@@ -1,5 +1,6 @@
 add_executable(mir_demo_server_shell
   demo_shell.cpp
+  demo_compositor.cpp
   demo_renderer.cpp
   fullscreen_placement_strategy.cpp
   window_manager.cpp

=== added file 'examples/demo-shell/demo_compositor.cpp'
--- examples/demo-shell/demo_compositor.cpp	1970-01-01 00:00:00 +0000
+++ examples/demo-shell/demo_compositor.cpp	2014-07-24 19:26:33 +0000
@@ -0,0 +1,96 @@
+/*
+ * Copyright © 2014 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Kevin DuBois <[e-mail]>
+ */
+
+#include "mir/graphics/display_buffer.h"
+#include "mir/compositor/compositor_report.h"
+#include "mir/compositor/scene.h"
+#include "mir/compositor/scene_element.h"
+#include "mir/compositor/destination_alpha.h"
+#include "demo_compositor.h"
+
+namespace me = mir::examples;
+namespace mg = mir::graphics;
+namespace mc = mir::compositor;
+namespace geom = mir::geometry;
+
+namespace
+{
+mc::DestinationAlpha destination_alpha(mg::DisplayBuffer const& db)
+{
+    return db.uses_alpha() ? mc::DestinationAlpha::generate_from_source : mc::DestinationAlpha::opaque;
+}
+}
+
+me::DemoCompositor::DemoCompositor(
+    mg::DisplayBuffer& display_buffer,
+    std::shared_ptr<mc::Scene> const& scene,
+    mg::GLProgramFactory const& factory,
+    std::shared_ptr<mc::CompositorReport> const& report) :
+    display_buffer(display_buffer),
+    scene(scene),
+    report(report),
+    renderer(
+        factory,
+        display_buffer.view_area(),
+        destination_alpha(display_buffer))
+{
+}
+
+mg::RenderableList me::DemoCompositor::generate_renderables()
+{
+    //a simple filtering out of renderables that shouldn't be drawn
+    //the elements should be notified if they are rendered or not
+    mg::RenderableList renderable_list;
+    auto elements = scene->scene_elements_for(this);
+    for(auto const& it : elements)
+    {
+        auto const& renderable = it->renderable(); 
+        if (renderable->visible())
+        {
+            renderable_list.push_back(renderable);
+            it->rendered_in(this);
+        }
+        else
+        {
+            it->occluded_in(this);
+        }
+    }
+    return renderable_list;
+}
+
+void me::DemoCompositor::composite()
+{
+    report->began_frame(this);
+    auto const& renderable_list = generate_renderables();
+    if (display_buffer.post_renderables_if_optimizable(renderable_list))
+    {
+        renderer.suspend();
+        report->finished_frame(true, this);
+    }
+    else
+    {
+        display_buffer.make_current();
+
+        renderer.set_rotation(display_buffer.orientation());
+        renderer.begin();
+        renderer.render(renderable_list);
+        display_buffer.post_update();
+        renderer.end();
+        report->finished_frame(false, this);
+    }
+}

=== added file 'examples/demo-shell/demo_compositor.h'
--- examples/demo-shell/demo_compositor.h	1970-01-01 00:00:00 +0000
+++ examples/demo-shell/demo_compositor.h	2014-07-24 19:26:33 +0000
@@ -0,0 +1,63 @@
+/*
+ * Copyright © 2014 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Kevin DuBois <[e-mail]>
+ */
+
+#ifndef MIR_EXAMPLES_DEMO_COMPOSITOR_H_
+#define MIR_EXAMPLES_DEMO_COMPOSITOR_H_
+
+#include "mir/compositor/display_buffer_compositor.h"
+#include "mir/compositor/scene.h"
+#include "mir/graphics/renderable.h"
+#include "demo_renderer.h"
+
+namespace mir
+{
+namespace compositor
+{
+class Scene;
+class CompositorReport;
+}
+namespace graphics
+{
+class DisplayBuffer;
+}
+namespace examples
+{
+
+class DemoCompositor : public compositor::DisplayBufferCompositor
+{
+public:
+    DemoCompositor(
+        graphics::DisplayBuffer& display_buffer,
+        std::shared_ptr<compositor::Scene> const& scene,
+        graphics::GLProgramFactory const& factory,
+        std::shared_ptr<compositor::CompositorReport> const& report);
+
+    void composite() override;
+
+private:
+    graphics::RenderableList generate_renderables();
+    graphics::DisplayBuffer& display_buffer;
+    std::shared_ptr<compositor::Scene> const scene;
+    std::shared_ptr<compositor::CompositorReport> const report;
+    DemoRenderer renderer;
+};
+
+} // namespace examples
+} // namespace mir
+
+#endif // MIR_EXAMPLES_DEMO_COMPOSITOR_H_

=== modified file 'examples/demo-shell/demo_shell.cpp'
--- examples/demo-shell/demo_shell.cpp	2014-07-21 03:35:31 +0000
+++ examples/demo-shell/demo_shell.cpp	2014-07-24 19:26:33 +0000
@@ -18,7 +18,7 @@
 
 /// \example demo_shell.cpp A simple mir shell
 
-#include "demo_renderer.h"
+#include "demo_compositor.h"
 #include "window_manager.h"
 #include "fullscreen_placement_strategy.h"
 #include "../server_configuration.h"
@@ -28,6 +28,8 @@
 #include "mir/report_exception.h"
 #include "mir/graphics/display.h"
 #include "mir/input/composite_event_filter.h"
+#include "mir/compositor/display_buffer_compositor_factory.h"
+#include "mir/compositor/destination_alpha.h"
 #include "mir/compositor/renderer_factory.h"
 #include "mir/shell/host_lifecycle_event_listener.h"
 
@@ -39,6 +41,7 @@
 namespace mf = mir::frontend;
 namespace mi = mir::input;
 namespace mo = mir::options;
+namespace mc = mir::compositor;
 namespace msh = mir::shell;
 
 namespace mir
@@ -46,22 +49,30 @@
 namespace examples
 {
 
-class DemoRendererFactory : public compositor::RendererFactory
+class DisplayBufferCompositorFactory : public mc::DisplayBufferCompositorFactory
 {
 public:
-    DemoRendererFactory(std::shared_ptr<graphics::GLProgramFactory> const& gl_program_factory) :
-        gl_program_factory(gl_program_factory)
-    {
-    }
-
-    std::unique_ptr<compositor::Renderer> create_renderer_for(
-        geometry::Rectangle const& rect,
-        mir::compositor::DestinationAlpha dest_alpha) override
-    {
-        return std::unique_ptr<compositor::Renderer>(new DemoRenderer(*gl_program_factory, rect, dest_alpha));
-    }
+    DisplayBufferCompositorFactory(
+        std::shared_ptr<mc::Scene> const& scene,
+        std::shared_ptr<mg::GLProgramFactory> const& gl_program_factory,
+        std::shared_ptr<mc::CompositorReport> const& report) :
+        scene(scene),
+        gl_program_factory(gl_program_factory),
+        report(report)
+    {
+    }
+
+    std::unique_ptr<mc::DisplayBufferCompositor> create_compositor_for(
+        mg::DisplayBuffer& display_buffer) override
+    {
+        return std::unique_ptr<mc::DisplayBufferCompositor>(
+            new me::DemoCompositor{display_buffer, scene, *gl_program_factory, report});
+    }
+
 private:
-    std::shared_ptr<graphics::GLProgramFactory> const gl_program_factory;
+    std::shared_ptr<mc::Scene> const scene;
+    std::shared_ptr<mg::GLProgramFactory> const gl_program_factory;
+    std::shared_ptr<mc::CompositorReport> const report;
 };
 
 class DemoServerConfiguration : public mir::examples::ServerConfiguration
@@ -84,6 +95,19 @@
     {
     }
 
+
+    std::shared_ptr<compositor::DisplayBufferCompositorFactory> the_display_buffer_compositor_factory()
+    {
+        return display_buffer_compositor_factory(
+            [this]()
+            {
+                return std::make_shared<me::DisplayBufferCompositorFactory>(
+                    the_scene(),
+                    the_gl_program_factory(),
+                    the_compositor_report());
+            });
+    }
+
     std::shared_ptr<ms::PlacementStrategy> the_placement_strategy() override
     {
         return shell_placement_strategy(
@@ -105,11 +129,6 @@
         return composite_filter;
     }
 
-    std::shared_ptr<compositor::RendererFactory> the_renderer_factory() override
-    {
-        return std::make_shared<DemoRendererFactory>(the_gl_program_factory());
-    }
-
     class NestedLifecycleEventListener : public msh::HostLifecycleEventListener
     {
     public: