Merge lp:~vanvugt/mir/fix-buffers_ready_for_compositor into lp:mir
| Status: | Merged |
|---|---|
| Approved by: | Daniel van Vugt on 2015-02-03 |
| Approved revision: | 2140 |
| Merged at revision: | 2289 |
| Proposed branch: | lp:~vanvugt/mir/fix-buffers_ready_for_compositor |
| Merge into: | lp:mir |
| Diff against target: |
874 lines (+297/-63) 34 files modified
examples/render_overlays.cpp (+0/-5) include/platform/mir/graphics/renderable.h (+0/-1) include/server/mir/compositor/scene.h (+10/-0) include/server/mir/scene/surface.h (+1/-0) platform-ABI-sha1sums (+1/-1) server-ABI-sha1sums (+3/-3) src/include/server/mir/compositor/buffer_stream.h (+1/-1) src/server/compositor/buffer_bundle.h (+1/-1) src/server/compositor/buffer_queue.cpp (+10/-6) src/server/compositor/buffer_queue.h (+1/-1) src/server/compositor/buffer_stream_surfaces.cpp (+2/-2) src/server/compositor/buffer_stream_surfaces.h (+1/-1) src/server/compositor/multi_threaded_compositor.cpp (+12/-1) src/server/graphics/software_cursor.cpp (+0/-5) src/server/input/touchspot_controller.cpp (+0/-5) src/server/scene/basic_surface.cpp (+12/-4) src/server/scene/basic_surface.h (+1/-0) src/server/scene/surface_stack.cpp (+31/-2) src/server/scene/surface_stack.h (+2/-0) src/server/symbols.map (+1/-0) tests/include/mir_test_doubles/fake_renderable.h (+0/-5) tests/include/mir_test_doubles/mock_buffer_bundle.h (+1/-1) tests/include/mir_test_doubles/mock_buffer_stream.h (+3/-3) tests/include/mir_test_doubles/mock_renderable.h (+0/-3) tests/include/mir_test_doubles/mock_scene.h (+3/-0) tests/include/mir_test_doubles/stub_buffer_stream.h (+4/-1) tests/include/mir_test_doubles/stub_renderable.h (+0/-5) tests/include/mir_test_doubles/stub_scene.h (+5/-1) tests/include/mir_test_doubles/stub_scene_surface.h (+1/-0) tests/integration-tests/test_exchange_buffer.cpp (+1/-1) tests/integration-tests/test_surface_stack_with_compositor.cpp (+5/-4) tests/unit-tests/compositor/test_buffer_queue.cpp (+66/-0) tests/unit-tests/compositor/test_multi_threaded_compositor.cpp (+85/-0) tests/unit-tests/scene/test_surface_stack.cpp (+33/-0) |
| To merge this branch: | bzr merge lp:~vanvugt/mir/fix-buffers_ready_for_compositor |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| PS Jenkins bot | continuous-integration | Approve on 2015-02-03 | |
| Kevin DuBois (community) | Approve on 2015-02-02 | ||
| Alan Griffiths | Abstain on 2015-01-27 | ||
| Alberto Aguirre | 2015-01-21 | Approve on 2015-01-22 | |
|
Review via email:
|
|||
Commit Message
Fix dangerous underestimate of buffers_
an immediate problem when you start experimenting with double buffers,
and see occasional random freezes. (LP: #1395581)
It could also possibly affect the current triple buffering used for
multimonitor and bypass/overlays.
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:2134
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| Daniel van Vugt (vanvugt) wrote : | # |
Ugg. Same tests failing for all proposals :(
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:2135
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:2136
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Alan Griffiths (alan-griffiths) wrote : | # |
I need to think through what happens when the compositor drops renderables. But this seem promising.
| Kevin DuBois (kdub) wrote : | # |
#1
I don't like mg::Renderables
#2
This does circumvent the observer architecture by requiring feedback from the BufferQueues. I see how the arch is wrong in the case described, but before we were telling the compositors to composite so many times, but now we're telling them to composite so many times, and then asking if they really meant it. The BufferQueue doesn't have all the information about how many compositions are needed to drain because of things like position-
| Kevin DuBois (kdub) wrote : | # |
to elaborate a bit further #1 is the bigger concern, #2 is to point out in the review that mc::Scene now is somewhat strange in that we have two routes to observe the damage
//frames-pending method
virtual int frames_
//observer method
virtual void add_observer(
virtual void remove_
and that the observer method (public api) is still broken in that it can't drain the queue properly.
| Daniel van Vugt (vanvugt) wrote : | # |
#1
Yeah I knew about QtMir already. It will need fixing. Hopefully without reintroducing the old Renderable method but we'll see. It's a simple matter of code but needs the involvement of Unity8 guys more to test the changes.
#2
The observer architecture obviously doesn't solve all problems. Compositors can and will occasionally skip over some renderable and not consume its buffer. Compositors are free to make their own decisions, so we should support any outcome from that.
I don't think this is strange or inconsistent though. A compositor's default state is to be asleep. We _need_ the observer stuff to wake it up. And we _need_ a separate counter to cover the case of rendering N frames after N were scheduled might not be enough (due to occlusions whatever). Callback-based observers can not solve this problem, but semaphore-based observers could, so read on...
I did consider a more unified approach that might work. That is a singular counter (like a semaphore) stored in the scene where each surface adds to the counter when it has something new not consumed. And critically, only the surface/producer can then remove its count from the scene total when it's flushed. But that's an architectural rework, bigger than this proposal and possibly could end up more complicated with some new coupling too. So first a fix, and later maybe redesign.
| Kevin DuBois (kdub) wrote : | # |
#2: Fair enough, I think the unified approach is something to work towards, but in the meantime, we may as well use this approach to solve the problem. Keeping track of whether the stack is damaged seems a better concept than what's in trunk or the wakeup and keep-running-
#1: I guess I'd like to pause this branch to avoid breaking downstream in a way that we don't have a fix for yet. Its mostly just a timing issue, but I'd guess that the Belgium sprinters might want to integrate the tip of mir with the tip of the other components.
So anyways, approve from me, but would appreciate pausing the branch until we have a downstream update available.
| Daniel van Vugt (vanvugt) wrote : | # |
#1: I'm sitting next to Gerry and talking to him about it :)
| Kevin DuBois (kdub) wrote : | # |
As long as we can work towards a more unified approach, and there's a branch forthcoming downstream, lgtm.
| Daniel van Vugt (vanvugt) wrote : | # |
Fortunately we've branched 0.11 already so landing this for 0.12 puts us in no hurry to sync downstreams.
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Autolanding.
More details in the following jenkins job:
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
| Robert Carr (robertcarr) wrote : | # |
UNTaed until qtmir branch is available so we can release tomorrow.
| Daniel van Vugt (vanvugt) wrote : | # |
QtMir branch available:
https:/
| PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:2140
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://

FAILED: Continuous integration, rev:2133 jenkins. qa.ubuntu. com/job/ mir-ci/ 2773/ jenkins. qa.ubuntu. com/job/ mir-android- vivid-i386- build/1007/ console jenkins. qa.ubuntu. com/job/ mir-clang- vivid-amd64- build/1006 jenkins. qa.ubuntu. com/job/ mir-mediumtests -vivid- touch/968 jenkins. qa.ubuntu. com/job/ mir-vivid- amd64-ci/ 770 jenkins. qa.ubuntu. com/job/ mir-vivid- amd64-ci/ 770/artifact/ work/output/ *zip*/output. zip jenkins. qa.ubuntu. com/job/ mir-mediumtests -builder- vivid-armhf/ 968 jenkins. qa.ubuntu. com/job/ mir-mediumtests -builder- vivid-armhf/ 968/artifact/ work/output/ *zip*/output. zip jenkins. qa.ubuntu. com/job/ mir-mediumtests -runner- mako/4055 s-jenkins. ubuntu- ci:8080/ job/touch- flash-device/ 17351
http://
Executed test runs:
FAILURE: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild: s-jenkins. ubuntu- ci:8080/ job/mir- ci/2773/ rebuild
http://