Mir

Merge lp://qastaging/~afrantzis/mir/consume-only-not-rendered-buffers into lp://qastaging/mir

consume-only-not-rendered-buffers
Merge into development-branch

Proposed by Alexandros Frantzis on 2014-04-22

Status:

Rejected

Rejected by:

Alexandros Frantzis on 2014-04-23

Proposed branch:

lp://qastaging/~afrantzis/mir/consume-only-not-rendered-buffers

Merge into:

lp://qastaging/mir

Diff against target:

365 lines (+82/-31)

11 files modified

examples/render_overlays.cpp (+11/-1)
include/platform/mir/graphics/renderable.h (+2/-0)
include/test/mir_test_doubles/fake_renderable.h (+10/-1)
include/test/mir_test_doubles/mock_renderable.h (+1/-0)
include/test/mir_test_doubles/stub_renderable.h (+13/-0)
src/server/compositor/multi_threaded_compositor.cpp (+23/-27)
src/server/scene/basic_surface.cpp (+8/-0)
src/server/scene/basic_surface.h (+3/-0)
src/server/scene/surface_stack.cpp (+3/-0)
tests/unit-tests/compositor/test_multi_threaded_compositor.cpp (+2/-2)
tests/unit-tests/graphics/android/test_hwc_device.cpp (+6/-0)

To merge this branch:

bzr merge lp://qastaging/~afrantzis/mir/consume-only-not-rendered-buffers

Related bugs:

Bug #1308843: [regression] [nonblockswap] Client judders, skipping frames periodically	High	Fix Released
Bug #1308844: [regression] [nonblockswap] BufferConsumingFunctor holds buffers too long, potentially hurting client render throughput	Medium	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2014-04-23
Daniel van Vugt			Needs Fixing on 2014-04-23
Kevin DuBois (community)			Needs Fixing on 2014-04-22
Alan Griffiths		2014-04-22	Needs Information on 2014-04-22
Review via email: mp+216725@code.qastaging.launchpad.net

Commit message

compositor: Only consume buffers that have not been rendered recently to unblock client eglSwapBuffers

Also resolves regressions - LP: #1308843 and LP: #1308844.

Description of the change

compositor: Only consume buffers that have not been rendered recently to unblock client eglSwapBuffers

Mix the best aspects of our various approaches to provide non-blocking swap buffers.

This MP uses the buffer consuming thread at the MultiThreadedCompositor level that's already present in mir/devel, but changes it so that it only consumes buffers that are not otherwise consumed by the real compositor, like https://code.launchpad.net/~raof/mir/1hz-rendering-always/+merge/216246 does. This avoids "beating" effects that are inherent in our handling of multiple compositors (i.e. multiple monitors).

Like 1hz-rendering-always, the selective buffer rendering is based on knowning when buffers are not used for some amount of time, so our consumption rate needs to be smaller than the least real compositor consumption rate (i.e. vsync). I have used 10Hz in this MP but we can change it to whatever value we want; we can even set it dynamically depending on the vsync rates of the connected screens.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-04-22:

I've nothing specifically against this MP but we seem to have a flurry of MPs trying to balance incompatible requirements. Vis:

1. The compositor can always get a buffer for a surface
2. A client can always swap buffers without blocking
3. We don't drop frames
4. There's a finite limit to how many buffers we allocate

As it is logically impossible to do all of these things some requirement has to be relaxed. Which is it?

review: Needs Information

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-04-22:

>> As it is logically impossible to do all of these things some requirement has to be relaxed. Which is it?

The way I have been modeling it in my head is when the screen is off or a client is occluded we may "drop frames"

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-04-22:

+ virtual std::chrono::steady_clock::time_point time_last_buffer_acquired() const = 0;

this makes Renderable leak the buffer stream abstraction even worse than it has before (I expound on this in my mp review: https://code.launchpad.net/~kdub/mir/produce-renderlist-for-particular-compositor/+merge/215052/comments/514979)

review: Needs Fixing

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-04-23:

> + virtual std::chrono::steady_clock::time_point time_last_buffer_acquired()
> const = 0;
>
> this makes Renderable leak the buffer stream abstraction even worse than it
> has before (I expound on this in my mp review:
> https://code.launchpad.net/~kdub/mir/produce-renderlist-for-particular-
> compositor/+merge/215052/comments/514979)

Note that the "buffer_acquired" in time_last_buffer_acquired() doesn't refer to the underlying buffer stream, but to Renderable::buffer(). That is, it returns the time point when someone last called Renderable::buffer(), regardless of the underlying implementation.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-04-23:

I think this is a slightly awkward way to express what I proposed (without using time measurement):
https://code.launchpad.net/~vanvugt/mir/judder/+merge/216694

I'd prefer all future work avoided the unreliability of time measurement. Because with some careful planning you can avoid it.

review: Needs Fixing

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-04-23:

> I think this is a slightly awkward way to express what I proposed (without
> using time measurement):
> https://code.launchpad.net/~vanvugt/mir/judder/+merge/216694
>
> I'd prefer all future work avoided the unreliability of time measurement.
> Because with some careful planning you can avoid it.

The two proposal are not equivalent. As noted in https://code.launchpad.net/~vanvugt/mir/judder/+merge/216694, the judder branch "reintroduces eglSwapBuffers() blocking for buffers that are not rendered by the real compositors (i.e. for surfaces that are either occluded or off screen). That is, it only keeps eglSwapBuffers() running when all monitors are off."

This proposal keeps eglSwapBuffers() running in all cases (monitors off, surface occluded, surface off-screen).

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-04-23:

Yes, that's what the unocclude branch solves.

I'm not rejecting this one, but I think it needs to be better (avoid time measurement), particularly since I've already proved doing so is possible.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-23:

FAILED: Continuous integration, rev:1568
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1370/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1666/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1664
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1238/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1102
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1102/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1107/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1240/console

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

review: Needs Fixing (continuous-integration)

lp://qastaging/~afrantzis/mir/consume-only-not-rendered-buffers updated on 2014-04-23

1569. By Alexandros Frantzis on 2014-04-23: tests: Fix android build

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-04-23:

The team has decided to go with an approach that handles non blocking inside the SwitchingBundle (for after 0.1.9).
That approach will also solve the "judder" issue, so since the "judder" issue is not critical (or even noticeable in many cases) we will probably ignore it for 0.1.9. Rejecting this MP.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-23:

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

review: Approve (continuous-integration)

Unmerged revisions

1569. By Alexandros Frantzis on 2014-04-23: tests: Fix android build
1568. By Alexandros Frantzis on 2014-04-22: compositor: Only consume buffers that have not been rendered recently to unblock client eglSwapBuffers