Merge lp:~vanvugt/mir/unocclude into lp:mir

[Daniel van Vugt]

Delete the occlusion detection logic. It was never used where it was needed
(on mobile platforms) since platform-api never got fixed. It's also
fundamentally incompatible with the new goals of never blocking clients.
Then again, what we lose in performance we gain in simplicity.

[Daniel van Vugt (vanvugt)]

I thought I needed this as a prerequisite to another fix I have in progress but now am not sure. Still, it's worth discussing.

[Kevin DuBois (kdub)]

my thoughts:

pros:
* The occlusion detection made it difficult to unify the compositor's behavior around consuming buffers, which in turn made it unclear what the SwitchingBundle's responsibilities were. Consuming every buffer on a compositor pass makes it easier to get the switching logic right, and is at least a consistent requirement for a compositor writer.
* It does seem at odds to try to not consume a buffer, and never block a client.

cons:
* occlusion detection is valuable in reducing workload for the compositor (texture uploads)
* an obscured, visible client that can submit frames fast enough can keep the compositor recompositing the same pixels because of the 'uncomposited_buffers' logic.
* loosing the logic puts us in a worse position if we're ever told to send 'occluded' messages to the client. (although this is a weak point due to YAGNI)

I am interested in what never got fixed in platform-api that made this code unused though.
An alternative compromise between the pros and cons could be to consume the buffer from all surfaces (unifying the behavior of the compositors/BufferBundle), and not calling render() on the occluded surfaces. This would also mean fixing whatever is broken in platform-api?

I think that the benefits of doing this now outweigh the cost, especially as we refine the SurfaceStack/Compositor relationship. I guess I'll 'approve', but am still interested in other's thoughts, so lets not top-approve until a bit more discussion happens.

review: Approve

[Alberto Aguirre (albaguirre)]

" loosing the logic puts us in a worse position if we're ever told to send 'occluded' messages to the client."

We already have that need. It should go hand-in-hand with the non-blocking swap behavior IMHO.

Clients need to know when their surfaces are no longer visible so they can stop their rendering loop - this is either from having a surface that is no longer visible in any display or from true occlusion.
In the Unity8 case this is currently being handled by the app lifecycle since we don't currently produce those events.

Anpok has a branch in papi that would benefit from the occlusion logic:
https://code.launchpad.net/~andreas-pokorny/platform-api/opaque_by_default/+merge/205259
We should still push through to get that branch merged.

review: Disapprove

[Kevin DuBois (kdub)]

> " loosing the logic puts us in a worse position if we're ever told to send
> 'occluded' messages to the client."
>
> We already have that need. It should go hand-in-hand with the non-blocking
> swap behavior IMHO.
>
> Clients need to know when their surfaces are no longer visible so they can
> stop their rendering loop - this is either from having a surface that is no
> longer visible in any display or from true occlusion.
> In the Unity8 case this is currently being handled by the app lifecycle since
> we don't currently produce those events.

Yeah, but where the occlusion code is now is not in a good position to notify clients. Compositors should not have the job of notifying clients of their decisions, the clients should get notifications based on things going on with their associated ms::BasicSurface.

This pushes me to think we have to shift occlusion detection to SurfaceStack, as it seems the common place this could be done. The compositors sure have to have a role in submitting what sort of occlusion they should do, but I don't want the compositors slowed down by having to send IPC events.

Now of course, this is tricky, because we could have multiple compositors, but I think can be done as the SurfaceStack gets a bit smarter about what compositors are viewing the SurfaceStack.

[Andreas Pokorny (andreas-pokorny)]

> This pushes me to think we have to shift occlusion detection to SurfaceStack,
> as it seems the common place this could be done. The compositors sure have to
> have a role in submitting what sort of occlusion they should do, but I don't
> want the compositors slowed down by having to send IPC events.
>
> Now of course, this is tricky, because we could have multiple compositors, but
> I think can be done as the SurfaceStack gets a bit smarter about what
> compositors are viewing the SurfaceStack.

+1
scene just needs to gain knowledge about the existing outputs. Their position within
the scene - the camera parameters. Those information could also be used to limit compositor
thread wakeups. Additionally that information is necessary to properly dispatch user input
within 3D shells.

We can solve the issue with decoration increasing the size of surfaces in a different way.
Treat decorations and window shadows as separate scene elements with a separate position
and size, that the shell adds to the scene..
Or maybe simpler: Extend the scenes occlusion logic with a filter-filter that filters out
false positives.
  as a graphical effect that extend the size of surfaces, which

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

I think you're planning for the wrong approach. The renderer should never feed back directly to the client as it's just a /view/. The renderer shall never communicate with clients in any way. Instead we should indirectly observe that a client's become occluded. This is easy using BufferConsumingFunctor which observes physical frame completion - if and when lp:~vanvugt/mir/judder lands.

Occlusion detection is designed to block clients that are not visible. That's it's purpose. If we're serious about making things non-blocking then it makes no sense to keep it as it only gets in the way (as mentioned here: https://code.launchpad.net/~vanvugt/mir/judder/+merge/216694)

[Andreas Pokorny (andreas-pokorny)]

> I think you're planning for the wrong approach. The renderer should never feed
> back directly to the client as it's just a /view/. The renderer shall never
> communicate with clients in any way. Instead we should indirectly observe that
> a client's become occluded. This is easy using BufferConsumingFunctor which
> observes physical frame completion - if and when lp:~vanvugt/mir/judder lands.

Hmm where did somebody indicate that the renderer should feed back to the clients?

> Occlusion detection is designed to block clients that are not visible. That's
> it's purpose. If we're serious about making things non-blocking then it makes
> no sense to keep it as it only gets in the way (as mentioned here:
> https://code.launchpad.net/~vanvugt/mir/judder/+merge/216694)

It also helps avoid unnecessary overdraw.

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1568
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1372/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1667
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1663
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1240/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1104
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1104/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1109
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1109/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1242
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1242/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1146/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/6252

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

review: Needs Fixing (continuous-integration)

[Kevin DuBois (kdub)]

> > I think you're planning for the wrong approach. The renderer should never
> feed
> > back directly to the client as it's just a /view/. The renderer shall never
> > communicate with clients in any way. Instead we should indirectly observe
> that
> > a client's become occluded. This is easy using BufferConsumingFunctor which
> > observes physical frame completion - if and when lp:~vanvugt/mir/judder
> lands.
>
> Hmm where did somebody indicate that the renderer should feed back to the
> clients?

"Compositors should not have the job of notifying clients of their decisions, the clients should get notifications based on things going on with their associated ms::BasicSurface."

Yes, it seems like the thing we agree upon is that nothing in the compositor system (GLRenderer, DefaultDisplayBufferCompositor, MultiThreadedCompositor) should do notifications of occlusion.

The big objection seems to be the future needs of other parts of the system noted by Alberto and Andreas and the loss of the logic by its removal.

[Daniel van Vugt (vanvugt)]

Hey I don't mind occlusion logic staying, but it is contradictory to non-blocking efforts. Choose one or the other.

I built the occlusion detection logic to block occluded clients. Its purpose is to block occluded clients from rendering, per bug 1227739. And it does that well.

If you choose both to have occlusion detection and non-blocking then you also require extra work (in pending proposals) to make them play together. Because the occlusion detection is only doing its job in blocking client rendering.

[Robert Carr (robertcarr)]

>> > Occlusion detection is designed to block clients that are not visible. That's
>> > it's purpose. If we're serious about making things non-blocking then it makes
>> > no sense to keep it as it only gets in the way (as mentioned here:
>> > https://code.launchpad.net/~vanvugt/mir/judder/+merge/216694)

Occlusion detection can also be used to prevent overdraw. Clearly whether buffers are returned to the client is a different question than whether buffers are part of the composition pass.

This isn't so different than for example an XRender client under Compiz...client rendering isnt blocked by compositor consumption at all (at least not in the serial queue frame consumption style we have)! Still Compiz will choose to not paint occluded windows. We improve this model by enforcing synchronization while clients are painted on screen, and are free to break this lockstep synchronization guarantee when clients are not rendered.

>> Hey I don't mind occlusion logic staying, but it is contradictory to non-blocking efforts. Choose one or
>> the other.

Not if you allow framedropping for unrendered surfaces.

review: Disapprove

[Daniel van Vugt (vanvugt)]

Heh. I'm not bothered either way. In fact, rather amused that other people are defending my code against deletion from myself.

[Kevin DuBois (kdub)]

I'm still for having the occlusion algorithm as something that's set by the compositing system, but if notifications are to go out about occluded surfaces, that should be something sent from the Scene system, not the compositor. Anyways, given the review comments, it seems that this will be explored differently, so abstain.

review: Abstain

[Alberto Aguirre (albaguirre)]

> I'm still for having the occlusion algorithm as something that's set by the
> compositing system, but if notifications are to go out about occluded
> surfaces, that should be something sent from the Scene system, not the
> compositor. Anyways, given the review comments, it seems that this will be
> explored differently, so abstain.

And that's ok, but until we actually have that alternative -whatever it is - it doesn't make sense to remove functionality until then.

[lp:~vanvugt/mir/unocclude]

1569. By Daniel van Vugt on 2014-04-24

Merge latest development-branch

1570. By Daniel van Vugt on 2014-04-28

Merge latest development-branch

1571. By Daniel van Vugt on 2014-04-29

Merge latest development-branch

1572. By Daniel van Vugt on 2014-04-29

Merge latest development-branch

1573. By Daniel van Vugt on 2014-04-30

Merge latest development-branch

[Daniel van Vugt (vanvugt)]

I'm not entirely sure now how much benefit occlusion detection could provide in the non-blocking world. You lose the benefit of limiting client CPU/GPU usage because you have to force unblocking of clients under the new model.

There's only one benefit remaining. That is to reduce the number of renderables that get thrown at the renderer. But OpenGL has always, by design, optimized out offscreen textures and geometry anyway. So that just gives you one obscure uncommon benefit: A small renderable hidden behind a larger one does not get its texture uploaded on that frame. However this benefit only applies to software surfaces where there is an actual "upload", and is irrelevant if the foreground is fullscreen because that's already optimized by bypass. The benefit of occlusion detection in the non-blocking world seems negligible.

[Daniel van Vugt (vanvugt)]

Also, when you throw decorations into the mix the current occlusion detection is wrong and buggy (like bug 1299977). With decorations you need even more complicated occlusion detection such that you only test if the client area of the foreground window fully occludes the decorated window frame of the background window.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1573
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1444/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1765
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1763
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1335
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1176
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1176/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1181
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1181/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1338
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1338/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1231
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/6548

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

review: Approve (continuous-integration)

[Alberto Aguirre (albaguirre)]

 A small renderable hidden behind a larger one
> does not get its texture uploaded on that frame. However this benefit only
> applies to software surfaces where there is an actual "upload", and is
> irrelevant if the foreground is fullscreen because that's already optimized by
> bypass. The benefit of occlusion detection in the non-blocking world seems
> negligible.

A "small" renderable could be simply one pixel less in width and height than the one that occludes it, which could be 2559x1599 in a nexus 10 for example.
It used to be that qualcomm chipsets were all direct rendering implementations, so it WOULD read that texture since we are drawing back to front. Now it seems it can switch between direct and deferred rendering so maybe it's smart enough not to do that anymore.
The Nexus 10 Mali does tile-based deferred rendering (as does the PowerVR stuff) so I suspect it's not a huge impact there either.

You could also get the additional hit of programming the GPU MMU - some architectures will lazily do that (i.e. they will not "upload" program the mmu until a draw command references it).

But we are both speculating - I would like to see some evidence that the occlusion optimization is negligible.

[lp:~vanvugt/mir/unocclude]

1574. By Daniel van Vugt on 2014-05-01

Merge latest development-branch and fix conflicts

[Daniel van Vugt (vanvugt)]

I wrote the texture cache logic in GLRenderer, so I know it behaves the same on any platform and that's not speculation.

The remaining point is whether uploading unused vertices is negligible for the number of surfaces on a desktop. I would argue after a while working on Compiz I got a good idea that yes, it is negligible. So that's a strongly educated guess rather than speculation.

[Daniel van Vugt (vanvugt)]

Hmm, actually the texture cache isn't relevant because it would be invalidated on frames that are important here. Yes I'm speculating then because the performance hit is now GPU-specific.

Another speculation is that performing occlusion tests at all is fast enough to justify the effort. That's an assumption made in Mir right now. But I know from previous work it doesn't always hold true.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1574
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1461/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1787
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1785
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1354/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1193
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1193/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1198
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1198/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1357
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1357/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1249/console
    FAILURE: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/6609/console

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

review: Needs Fixing (continuous-integration)

[lp:~vanvugt/mir/unocclude]

1575. By Daniel van Vugt on 2014-05-02

Merge latest development-branch

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1575
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1489/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/8
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/9
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/9
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/6/console
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/6/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/6
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/6/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/15
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/15/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1282
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/6682

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

review: Needs Fixing (continuous-integration)

[lp:~vanvugt/mir/unocclude]

1576. By Daniel van Vugt on 2014-05-05

Merge latest development-branch

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1576
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1511/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/39
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/40
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/40
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/30
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/30/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/28
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/28/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/46
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/46/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1310
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/6762

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

review: Approve (continuous-integration)

[lp:~vanvugt/mir/unocclude]

1577. By Daniel van Vugt on 2014-05-06

Merge latest development-branch and fix conflicts

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1577
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1528/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/62
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/63
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/63
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/47
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/47/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/45
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/45/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/80
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/80/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1332
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/6803

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

review: Approve (continuous-integration)

Unmerged revisions

1577. By Daniel van Vugt on 2014-05-06

Merge latest development-branch and fix conflicts

1576. By Daniel van Vugt on 2014-05-05

Merge latest development-branch

1575. By Daniel van Vugt on 2014-05-02

Merge latest development-branch

1574. By Daniel van Vugt on 2014-05-01

Merge latest development-branch and fix conflicts

1573. By Daniel van Vugt on 2014-04-30

Merge latest development-branch

1572. By Daniel van Vugt on 2014-04-29

Merge latest development-branch

1571. By Daniel van Vugt on 2014-04-29

Merge latest development-branch

1570. By Daniel van Vugt on 2014-04-28

Merge latest development-branch

1569. By Daniel van Vugt on 2014-04-24

Merge latest development-branch

1568. By Daniel van Vugt on 2014-04-22

First attempt at removing occlusion detection

=== modified file 'src/server/compositor/CMakeLists.txt'
--- src/server/compositor/CMakeLists.txt	2014-04-27 21:11:15 +0000
+++ src/server/compositor/CMakeLists.txt	2014-05-06 03:00:37 +0000
@@ -10,7 +10,6 @@
   gl_renderer_factory.cpp
   multi_threaded_compositor.cpp
   bypass.cpp
-  occlusion.cpp
   default_configuration.cpp
   screencast_display_buffer.cpp
   compositing_screencast.cpp

=== modified file 'src/server/compositor/default_display_buffer_compositor.cpp'
--- src/server/compositor/default_display_buffer_compositor.cpp	2014-05-05 05:43:02 +0000
+++ src/server/compositor/default_display_buffer_compositor.cpp	2014-05-06 03:00:37 +0000
@@ -26,7 +26,6 @@
 #include "mir/graphics/buffer.h"
 #include "mir/compositor/buffer_stream.h"
 #include "bypass.h"
-#include "occlusion.h"
 #include <mutex>
 #include <cstdlib>
 #include <algorithm>
@@ -55,7 +54,6 @@
 
     auto const& view_area = display_buffer.view_area();
     auto renderable_list = scene->renderable_list_for(this);
-    mc::filter_occlusions_from(renderable_list, view_area);
 
     //TODO: the DisplayBufferCompositor should not have to figure out if it has to force
     //      a subsequent compositon. The MultiThreadedCompositor should be smart enough to 

=== removed file 'src/server/compositor/occlusion.cpp'
--- src/server/compositor/occlusion.cpp	2014-04-17 01:34:35 +0000
+++ src/server/compositor/occlusion.cpp	1970-01-01 00:00:00 +0000
@@ -1,78 +0,0 @@
-/*
- * Copyright © 2013 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: Daniel van Vugt <[e-mail]>
- */
-
-#include "mir/geometry/rectangle.h"
-#include "mir/graphics/renderable.h"
-#include "occlusion.h"
-
-using namespace mir::geometry;
-using namespace mir::graphics;
-
-namespace
-{
-bool renderable_is_occluded(
-    Renderable const& renderable, 
-    Rectangle const& area,
-    std::vector<Rectangle>& coverage)
-{
-    static const glm::mat4 identity;
-    if (renderable.transformation() != identity)
-        return false;  // Weirdly transformed. Assume never occluded.
-
-    //TODO: remove this check, why are we getting a non visible renderable 
-    //      in the list of surfaces?
-    // This will check the surface is not hidden and has been posted.
-    if (!renderable.visible())
-        return true;  //invisible; definitely occluded.
-
-    // Not weirdly transformed but also not on this monitor? Don't care...
-    if (!area.overlaps(renderable.screen_position()))
-        return true;  // Not on the display; definitely occluded.
-
-    bool occluded = false;
-    Rectangle const& window = renderable.screen_position();
-    for (const auto &r : coverage)
-    {
-        if (r.contains(window))
-        {
-            occluded = true;
-            break;
-        }
-    }
-
-    if (!occluded && renderable.alpha() == 1.0f && !renderable.shaped())
-        coverage.push_back(window);
-
-    return occluded;
-}
-}
-
-void mir::compositor::filter_occlusions_from(
-    RenderableList& list,
-    Rectangle const& area)
-{
-    std::vector<Rectangle> coverage;
-    auto it = list.rbegin();
-    while (it != list.rend())
-    {
-        if (renderable_is_occluded(**it, area, coverage))
-            list.erase(std::prev(it.base()));
-        else
-            it++;
-    }
-}

=== removed file 'src/server/compositor/occlusion.h'
--- src/server/compositor/occlusion.h	2014-03-26 16:59:32 +0000
+++ src/server/compositor/occlusion.h	1970-01-01 00:00:00 +0000
@@ -1,37 +0,0 @@
-/*
- * Copyright © 2013 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: Daniel van Vugt <[e-mail]>
- */
-
-#ifndef MIR_COMPOSITOR_OCCLUSION_H_
-#define MIR_COMPOSITOR_OCCLUSION_H_
-
-#include "mir/graphics/renderable.h"
-#include "mir/compositor/scene.h"
-#include <vector>
-#include <set>
-
-namespace mir
-{
-namespace compositor
-{
-
-void filter_occlusions_from(graphics::RenderableList& list, geometry::Rectangle const& area);
-
-} // namespace compositor
-} // namespace mir
-
-#endif // MIR_COMPOSITOR_OCCLUSION_H_

=== modified file 'tests/integration-tests/test_surface_first_frame_sync.cpp'
--- tests/integration-tests/test_surface_first_frame_sync.cpp	2014-05-05 05:43:02 +0000
+++ tests/integration-tests/test_surface_first_frame_sync.cpp	2014-05-06 03:00:37 +0000
@@ -105,8 +105,8 @@
     {
         for (auto const& r : renderables)
         {
-            (void)r;
-            while (write(render_operations_fd, "a", 1) != 1) continue;
+            if (r->visible())
+                while (write(render_operations_fd, "a", 1) != 1) continue;
         }
     }
 

=== modified file 'tests/unit-tests/compositor/CMakeLists.txt'
--- tests/unit-tests/compositor/CMakeLists.txt	2014-04-21 15:56:41 +0000
+++ tests/unit-tests/compositor/CMakeLists.txt	2014-05-06 03:00:37 +0000
@@ -6,7 +6,6 @@
   ${CMAKE_CURRENT_SOURCE_DIR}/test_buffer_queue.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_gl_renderer.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_bypass.cpp
-  ${CMAKE_CURRENT_SOURCE_DIR}/test_occlusion.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_screencast_display_buffer.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_compositing_screencast.cpp
 )

=== modified file 'tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp'
--- tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp	2014-05-05 05:43:02 +0000
+++ tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp	2014-05-06 03:00:37 +0000
@@ -128,66 +128,6 @@
     compositor.composite();
 }
 
-TEST_F(DefaultDisplayBufferCompositor, skips_scene_that_should_not_be_rendered)
-{
-    using namespace testing;
-    
-    mtd::StubDisplayBuffer display_buffer{geom::Rectangle{{0,0},{14,14}}};
-    auto mock_renderable1 = std::make_shared<NiceMock<mtd::MockRenderable>>();
-    auto mock_renderable2 = std::make_shared<NiceMock<mtd::MockRenderable>>();
-    auto mock_renderable3 = std::make_shared<NiceMock<mtd::MockRenderable>>();
-
-    glm::mat4 simple;
-    EXPECT_CALL(*mock_renderable1, transformation())
-        .WillOnce(Return(simple));
-    EXPECT_CALL(*mock_renderable2, transformation())
-        .WillOnce(Return(simple));
-    EXPECT_CALL(*mock_renderable3, transformation())
-        .WillOnce(Return(simple));
-
-    EXPECT_CALL(*mock_renderable1, visible())
-        .WillRepeatedly(Return(true));
-    EXPECT_CALL(*mock_renderable2, visible())
-        .WillRepeatedly(Return(false));
-    EXPECT_CALL(*mock_renderable3, visible())
-        .WillRepeatedly(Return(true));
-
-    EXPECT_CALL(*mock_renderable1, alpha())
-        .WillOnce(Return(1.0f));
-    EXPECT_CALL(*mock_renderable3, alpha())
-        .WillOnce(Return(1.0f));
-
-    EXPECT_CALL(*mock_renderable1, shaped())
-        .WillOnce(Return(false));
-    EXPECT_CALL(*mock_renderable3, shaped())
-        .WillOnce(Return(false));
-
-    EXPECT_CALL(*mock_renderable1, screen_position())
-        .WillRepeatedly(Return(geom::Rectangle{{1,2}, {3,4}}));
-    EXPECT_CALL(*mock_renderable2, screen_position())
-        .WillRepeatedly(Return(geom::Rectangle{{1,2}, {3,4}}));
-    EXPECT_CALL(*mock_renderable3, screen_position())
-        .WillRepeatedly(Return(geom::Rectangle{{5,6}, {7,8}}));
-
-    mg::RenderableList list{
-        mock_renderable1,
-        mock_renderable2,
-        mock_renderable3
-    };
-    FakeScene scene(list);
-
-    mg::RenderableList const visible{mock_renderable1, mock_renderable3};
-    EXPECT_CALL(mock_renderer, render(visible))
-        .Times(1);
-
-    mc::DefaultDisplayBufferCompositor compositor(
-        display_buffer,
-        mt::fake_shared(scene),
-        mt::fake_shared(mock_renderer),
-        mr::null_compositor_report());
-    compositor.composite();
-}
-
 TEST_F(DefaultDisplayBufferCompositor, bypass_skips_composition)
 {
     using namespace testing;
@@ -304,8 +244,6 @@
     };
     FakeScene scene(list);
 
-    mg::RenderableList const visible{fullscreen};
-
     EXPECT_CALL(display_buffer, view_area())
         .WillRepeatedly(Return(screen));
     EXPECT_CALL(display_buffer, make_current())
@@ -316,7 +254,7 @@
         .Times(1);
     EXPECT_CALL(display_buffer, can_bypass())
         .WillRepeatedly(Return(false));
-    EXPECT_CALL(mock_renderer, render(visible))
+    EXPECT_CALL(mock_renderer, render(list))
         .Times(1);
 
     mc::DefaultDisplayBufferCompositor compositor(
@@ -348,8 +286,7 @@
     };
     FakeScene scene(list);
 
-    mg::RenderableList const visible{fullscreen};
-    EXPECT_CALL(mock_renderer, render(visible))
+    EXPECT_CALL(mock_renderer, render(list))
         .Times(1);
 
     auto nonbypassable = std::make_shared<mtd::MockBuffer>();
@@ -437,7 +374,7 @@
     fullscreen->set_buffer({});  // Avoid GMock complaining about false leaks
 }
 
-TEST_F(DefaultDisplayBufferCompositor, occluded_surfaces_are_not_rendered)
+TEST_F(DefaultDisplayBufferCompositor, occluded_surfaces_are_rendered_too)
 {
     using namespace testing;
     EXPECT_CALL(display_buffer, view_area())
@@ -462,12 +399,11 @@
     });
     FakeScene scene(list);
 
-    mg::RenderableList const visible{window0, window3};
 
     Sequence seq;
     EXPECT_CALL(display_buffer, make_current())
         .InSequence(seq);
-    EXPECT_CALL(mock_renderer, render(visible))
+    EXPECT_CALL(mock_renderer, render(list))
         .InSequence(seq);
     EXPECT_CALL(display_buffer, post_update())
         .InSequence(seq);

=== removed file 'tests/unit-tests/compositor/test_occlusion.cpp'
--- tests/unit-tests/compositor/test_occlusion.cpp	2014-04-02 03:10:06 +0000
+++ tests/unit-tests/compositor/test_occlusion.cpp	1970-01-01 00:00:00 +0000
@@ -1,202 +0,0 @@
-/*
- * Copyright © 2013 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: Daniel van Vugt <[e-mail]>
- */
-
-#include "mir/geometry/rectangle.h"
-#include "src/server/compositor/occlusion.h"
-#include "mir_test_doubles/fake_renderable.h"
-
-#include <gtest/gtest.h>
-#include <memory>
-
-using namespace testing;
-using namespace mir::geometry;
-using namespace mir::compositor;
-namespace mg = mir::graphics;
-namespace mtd = mir::test::doubles;
-
-struct OcclusionFilterTest : public Test
-{
-    OcclusionFilterTest()
-    {
-        monitor_rect.top_left = {0, 0};
-        monitor_rect.size = {1920, 1200};
-    }
-
-    Rectangle monitor_rect;
-};
-
-TEST_F(OcclusionFilterTest, single_window_not_occluded)
-{
-    auto window = std::make_shared<mtd::FakeRenderable>(12, 34, 56, 78);
-    mg::RenderableList list{window};
- 
-    filter_occlusions_from(list, monitor_rect);
-    ASSERT_EQ(1u, list.size());
-    EXPECT_EQ(window, list.front());
-}
-
-TEST_F(OcclusionFilterTest, partially_offscreen_still_visible)
-{ // Regression test for LP: #1301115
-    auto left =   std::make_shared<mtd::FakeRenderable>(-10,   10, 100, 100);
-    auto right =  std::make_shared<mtd::FakeRenderable>(1900,  10, 100, 100);
-    auto top =    std::make_shared<mtd::FakeRenderable>(500,   -1, 100, 100);
-    auto bottom = std::make_shared<mtd::FakeRenderable>(200, 1000, 100, 1000);
-    mg::RenderableList list{left, right, top, bottom};
- 
-    filter_occlusions_from(list, monitor_rect);
-    EXPECT_EQ(4u, list.size());
-}
-
-TEST_F(OcclusionFilterTest, smaller_window_occluded)
-{
-    auto top = std::make_shared<mtd::FakeRenderable>(10, 10, 10, 10);
-    auto bottom = std::make_shared<mtd::FakeRenderable>(12, 12, 5, 5);
-    mg::RenderableList list{bottom, top};
-
-    filter_occlusions_from(list, monitor_rect);
-
-    ASSERT_EQ(1u, list.size());
-    EXPECT_EQ(top, list.front());
-}
-
-TEST_F(OcclusionFilterTest, translucent_window_occludes_nothing)
-{
-    auto top = std::make_shared<mtd::FakeRenderable>(10, 10, 10, 10, 0.5f);
-    auto bottom = std::make_shared<mtd::FakeRenderable>(12, 12, 5, 5, 1.0f);
-    mg::RenderableList list{bottom, top};
-
-    filter_occlusions_from(list, monitor_rect);
-
-    ASSERT_EQ(2u, list.size());
-    EXPECT_EQ(bottom, list.front());
-    EXPECT_EQ(top, list.back());
-}
-
-TEST_F(OcclusionFilterTest, hidden_window_is_self_occluded)
-{
-    auto window = std::make_shared<mtd::FakeRenderable>(10, 10, 10, 10, 1.0f, true, false);
-    mg::RenderableList list{window};
-
-    filter_occlusions_from(list, monitor_rect);
-
-    EXPECT_EQ(0u, list.size());
-}
-
-TEST_F(OcclusionFilterTest, hidden_window_occludes_nothing)
-{
-    auto top = std::make_shared<mtd::FakeRenderable>(10, 10, 10, 10, 1.0f, true, false);
-    auto bottom = std::make_shared<mtd::FakeRenderable>(12, 12, 5, 5);
-    mg::RenderableList list{bottom, top};
-
-    filter_occlusions_from(list, monitor_rect);
-
-    ASSERT_EQ(1u, list.size());
-    EXPECT_EQ(bottom, list.front());
-}
-
-TEST_F(OcclusionFilterTest, shaped_window_occludes_nothing)
-{
-    auto top = std::make_shared<mtd::FakeRenderable>(10, 10, 10, 10, 1.0f, false, true);
-    auto bottom = std::make_shared<mtd::FakeRenderable>(12, 12, 5, 5);
-    mg::RenderableList list{bottom, top};
-
-    filter_occlusions_from(list, monitor_rect);
-
-    ASSERT_EQ(2u, list.size());
-    EXPECT_EQ(bottom, list.front());
-    EXPECT_EQ(top, list.back());
-}
-
-TEST_F(OcclusionFilterTest, identical_window_occluded)
-{
-    auto top = std::make_shared<mtd::FakeRenderable>(10, 10, 10, 10);
-    auto bottom = std::make_shared<mtd::FakeRenderable>(10, 10, 10, 10);
-    mg::RenderableList list{bottom, top};
-
-    filter_occlusions_from(list, monitor_rect);
-
-    ASSERT_EQ(1u, list.size());
-    EXPECT_EQ(top, list.front());
-}
-
-TEST_F(OcclusionFilterTest, larger_window_never_occluded)
-{
-    auto top = std::make_shared<mtd::FakeRenderable>(10, 10, 10, 10);
-    auto bottom = std::make_shared<mtd::FakeRenderable>(9, 9, 12, 12);
-    mg::RenderableList list{bottom, top};
-
-    filter_occlusions_from(list, monitor_rect);
-
-    ASSERT_EQ(2u, list.size());
-    EXPECT_EQ(bottom, list.front());
-    EXPECT_EQ(top, list.back());
-}
-
-TEST_F(OcclusionFilterTest, cascaded_windows_never_occluded)
-{
-    mg::RenderableList list;
-    unsigned int const num_windows{10u};
-    for (auto x = 0u; x < num_windows; x++)
-        list.push_back(std::make_shared<mtd::FakeRenderable>(x, x, 200, 100));
-
-    filter_occlusions_from(list, monitor_rect);
-    EXPECT_EQ(num_windows, list.size());
-}
-
-TEST_F(OcclusionFilterTest, some_occluded_and_some_not)
-{
-    auto window0 = std::make_shared<mtd::FakeRenderable>(10, 20, 400, 300);
-    auto window1 = std::make_shared<mtd::FakeRenderable>(10, 20, 5, 5);
-    auto window2 = std::make_shared<mtd::FakeRenderable>(100, 100, 20, 20);
-    auto window3 = std::make_shared<mtd::FakeRenderable>(200, 200, 50, 50);
-    auto window4 = std::make_shared<mtd::FakeRenderable>(500, 600, 34, 56);
-    auto window5 = std::make_shared<mtd::FakeRenderable>(200, 200, 1000, 1000);
-    mg::RenderableList list{
-        window5, //not occluded
-        window4, //not occluded
-        window3, //occluded
-        window2, //occluded
-        window1, //occluded
-        window0  //not occluded
-    };
-
-    filter_occlusions_from(list, monitor_rect);
-
-    auto expected_size = 3u;
-    ASSERT_EQ(expected_size, list.size());
-    auto it = list.begin();
-    for(auto count = 0u; count < expected_size; count++)
-    {
-        switch (count)
-        {
-            case 0u:
-                EXPECT_EQ(window5, *it);
-                break;
-            case 1u:
-                EXPECT_EQ(window4, *it);
-                break;
-            case 2u:
-                EXPECT_EQ(window0, *it);
-                break;
-            default:
-                FAIL();
-                break;
-        }
-        it++;
-    }
-}