Merge lp:~robertcarr/mir/fix-1249210 into lp:mir

[Robert Carr]

Ensure the session mediator releases acquired buffer resources before
attempting to acquire a new buffer on behalf of the client. This fixes
performance regression LP: #1249210.

Ensure the session mediator releases acquired buffer resources before attempting to acquire a new buffer on behalf of the client. This was dropped in 1196.

I'm having trouble understanding why it only triggers half FPS for bypass clients...

[PS Jenkins bot (ps-jenkins)]

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

The reason is in surfaces:Surface::advance_buffer()

review: Approve

[Daniel van Vugt (vanvugt)]

It works! But it would be extra-nice to regression-test this so we don't get caught by the same regressions again.

I guess it would only affect bypass clients because bypass is special; it needs to hold _two_ compositor buffers simultaneously. If we allow clients to hold two buffers simultaneously as well, then we have a need for _four_ buffers when there's only three in the bundle. Hence the client would be forced to wait each frame for the previous page flip to complete. That would explain it...

review: Approve

[Daniel van Vugt (vanvugt)]

P.S. Prior to the new SwitchingBundle algorithm, we probably would have just crashed immediately with an exception. Only the new algorithm supports multiple simultaneous client buffers.

So a simple way to regression test would be to enforce the requirement that only one client buffer exists at a time. And if not, throw a logic_error. Though I'm not totally in support of throwing, because we'd be losing functionality potentially useful for improving pipelining of client buffers (and hence performance) later.

=== modified file 'src/server/frontend/session_mediator.cpp'
--- src/server/frontend/session_mediator.cpp	2013-11-05 03:39:55 +0000
+++ src/server/frontend/session_mediator.cpp	2013-11-08 21:42:24 +0000
@@ -118,6 +118,8 @@
 {
     auto& tracker = client_buffer_tracker[surf_id];
     if (!tracker) tracker = std::make_shared<ClientBufferTracker>(client_buffer_cache_size);
+    
+    client_buffer_resource[surf_id].reset();
 
     auto client_buffer = surface.advance_client_buffer();
     auto id = client_buffer->id();