Mir

Merge lp://qastaging/~alan-griffiths/mir/fix-1535894-alt into lp://qastaging/mir

fix-1535894-alt
Merge into development-branch

Proposed by Alan Griffiths on 2016-02-05

Status:	Rejected
Rejected by:	Alan Griffiths on 2016-02-11
Proposed branch:	lp://qastaging/~alan-griffiths/mir/fix-1535894-alt
Merge into:	lp://qastaging/mir
Diff against target:	984 lines (+181/-263) 26 files modified examples/render_surfaces.cpp (+3/-2) examples/server_example_adorning_compositor.cpp (+12/-1) examples/server_example_adorning_compositor.h (+1/-1) include/server/mir/compositor/display_buffer_compositor.h (+1/-1) include/server/mir/compositor/scene.h (+0/-10) include/server/mir/compositor/scene_element.h (+1/-0) include/test/mir/test/doubles/null_display_buffer_compositor_factory.h (+2/-1) playground/demo-shell/demo_compositor.cpp (+13/-1) playground/demo-shell/demo_compositor.h (+1/-1) src/server/compositor/default_display_buffer_compositor.cpp (+43/-46) src/server/compositor/default_display_buffer_compositor.h (+1/-1) src/server/compositor/multi_threaded_compositor.cpp (+3/-11) src/server/scene/rendering_tracker.cpp (+9/-0) src/server/scene/rendering_tracker.h (+1/-0) src/server/scene/surface_stack.cpp (+10/-24) src/server/scene/surface_stack.h (+0/-1) tests/acceptance-tests/test_buffer_stream_arrangement.cpp (+2/-2) tests/acceptance-tests/test_client_scaling.cpp (+2/-1) tests/acceptance-tests/test_render_override.cpp (+3/-1) tests/include/mir/test/doubles/stub_scene.h (+0/-4) tests/include/mir/test/doubles/stub_scene_element.h (+5/-0) tests/integration-tests/test_server_shutdown.cpp (+1/-1) tests/unit-tests/compositor/test_compositing_screencast.cpp (+4/-3) tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp (+44/-10) tests/unit-tests/compositor/test_multi_threaded_compositor.cpp (+19/-22) tests/unit-tests/scene/test_surface_stack.cpp (+0/-118)
To merge this branch:	bzr merge lp://qastaging/~alan-griffiths/mir/fix-1535894-alt
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2016-02-11
Mir CI Bot	continuous-integration		Approve on 2016-02-11
Daniel van Vugt		2016-02-05	Needs Fixing on 2016-02-11
Review via email: mp+285223@code.qastaging.launchpad.net

Commit message

compositor, scene: Use the DisplayBufferCompositor filtering to decide which surface have buffers that may still need compositing.

Description of the change

compositor, scene: Use the DisplayBufferCompositor filtering to decide which surface have buffers that may still need compositing.

This doesn't include logic that allows -c 3274 to be reverted, but if this approach is favoured, then I'll rework that to put the DisplayBufferCompositor(s) in control.

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-02-08:

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/253/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-02-08:

Errors look unrelated:

15:52:50 Setting up libuuid1:amd64 (2.27.1-1ubuntu4) ...
15:52:50 dpkg: error processing package libuuid1:amd64 (--configure):
15:52:50 subprocess installed post-installation script returned error exit status 1
15:52:50 Processing triggers for libc-bin (2.21-0ubuntu5) ...
15:52:51 Errors were encountered while processing:
15:52:51 libuuid1:amd64
15:52:51 W: No sandbox user '_apt' on the system, can not drop privileges
15:52:51 E: Sub-process /usr/bin/dpkg returned an error code (1)
15:52:51 apt-get failed.
15:52:51 Package installation failed

15:52:55 dpkg: error processing package libuuid1:i386 (--configure):
15:52:55 subprocess installed post-installation script returned error exit status 1
15:52:55 Processing triggers for libc-bin (2.21-0ubuntu5) ...
15:52:55 Errors were encountered while processing:
15:52:55 libuuid1:i386
15:52:55 W: No sandbox user '_apt' on the system, can not drop privileges
15:52:55 E: Sub-process /usr/bin/dpkg returned an error code (1)
15:52:55 apt-get failed.
15:52:55 Package installation failed

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-02-08:

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/254/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-02-08:

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/255/rebuild

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2016-02-08:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/6231/rebuild

review: Approve (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-02-10:

1. Needs info: I thought the goal here was to skip rendering on idle displays. That code doesn't seem to be present yet, so maybe it comes later? It would be: composite() returns "I had nothing new to render" so then post() never gets called. You either need to composite and post, or do neither. But never composite without post (wasted of GPU), and never post without composite (uninitialized pixels appear on screen).

Although that raises an important question: Is the bug safe to fix at all under the new display groups architecture? How do we safely tell Android to only post one of the displays in the group but not the other? I'm wondering if it's even possible to fix this under the display group approach... because you must either have one composite and one post, or zero of all.

2. Needs fixing: Please add the FIXME comment back in. It's an important one we shouldn't forget.

I'm also nervous that we're rewriting so much code that covers years of regression fixes. But that's not quite a blocking issue.

review: Needs Fixing

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-02-10:

3. This looks like important code that will cause regressions if removed:
- int result = scene_changed ? 1 : 0;

That tells us to redraw the screen if a window has been dragged, but no windows contents changed. Obviously was missing a test.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-02-10:

OK, assuming Android has some magic workaround built-in to cope with posting a display group where only half of that group has been rendered, what we actually want instead of this branch is:

bool composited = false
for each compositor:
composited |= compositor->composite();
if (composited)
group.post();

That also means you can keep the frame counting logic where it is in the scene. No need to duplicate it into each compositor implementation.

But if Android can't handle half the monitors being rendered and then being told to post the whole group, then we actually can't safely do any of this at all...

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-02-10:

> 1. Needs info: I thought the goal here was to skip rendering on idle displays.
> That code doesn't seem to be present yet, so maybe it comes later? It would
> be: composite() returns "I had nothing new to render" so then post() never
> gets called. You either need to composite and post, or do neither. But never
> composite without post (wasted of GPU), and never post without composite
> (uninitialized pixels appear on screen).

Nope.

The point of *this* MP is that the DisplayBufferCompositor knows which surfaces it has rendered and interrogates them to see if they have further buffers pending.

With the logic in scene we were counting buffers belonging to surfaces that had not contributed to rendering. (And would not be consumed by the consequent render pass.)

In the scenario from the linked bug the nested server would repeatedly schedule a recomposite of whichever display on which nothing had been posted.

> 2. Needs fixing: Please add the FIXME comment back in. It's an important one
> we shouldn't forget.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-02-10:

> 3. This looks like important code that will cause regressions if removed:
> - int result = scene_changed ? 1 : 0;
>
> That tells us to redraw the screen if a window has been dragged, but no
> windows contents changed. Obviously was missing a test.

Obviously, this code duplicated the effect of CompositingFunctor::schedule_compositing(int num) and was redundant.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-02-11:

Oh cool. That all sounds like good news then. I'll need to re-evaluate later.

review: Abstain

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-02-11:

(1) Thanks for explaining. So I took some time today adding and analysing some debug logging to try and catch Mir in the act of repeatedly scheduling frames on some display but never consuming them. Unfortunately I can't yet reproduce such a busy wait but that could be just that I'm using desktop, or it could just be bad luck. So I'm assuming there's still a theoretical problem needing fixing.

But now I'm curious: You've clarified you're just fixing a busy wait. Or as I would say it "made Mir sleep properly". That's very good for power management, but only a small piece of the performance story. Even if this branch is working perfectly, you're still re-posting all displays on every frame (including the idle one) while any one of them is animating. So this is really a power management fix, but how is it an improvement to performance? You haven't changed the rendering load, and haven't reduced the number of posts either.

(2) Needs fixing: FIXME comment is still missing.

(4) I'm not yet confident this branch is working. As stated in (1) I can't yet put together a manual test case on desktop to reproduce any issue, and the diff below shows no new regression tests for anything getting fixed. So I think you need to try and write a regression test for the issue.

(5) In theory there's a much simpler solution you should consider. That is just make every call to BasicSurface::generate_renderables acquire its compositor buffer immediately, instead of the lazy acquisition it does in SurfaceSnapshot::buffer(). In theory that's a tiny change, it would solve the same problem, and is easier to test.

review: Needs Fixing

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-02-11:

(1) "You haven't changed the rendering load, and haven't reduced the number of posts either" [while any display is changing anyway]

(5) Here's possible alternative solution:
https://code.launchpad.net/~mir-team/mir/compositing-always-finishes/+merge/285684

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-02-11:

> but only a small piece of the performance story. Even if this branch is
> working perfectly, you're still re-posting all displays on every frame
> (including the idle one) while any one of them is animating.

Remember, this is in the nested server which has a CompositingFunctor for each output.

The earlier change (that you have valid concerns about) ensures that the CompositingFunctor doesn't run for changes that do not apply to its display buffers compositor.

> So this is really
> a power management fix, but how is it an improvement to performance? You
> haven't changed the rendering load, and haven't reduced the number of posts
> either.

The problem this addresses occurs whenever the "idle" CompositingFunctor runs. First it composites the surfaces visible to its DBC and then checks the scene for unconsumed frames. Updates on the other screen cause it to increment frames_scheduled and composite unnecessarily.

By only checking for unconsumed frames in the surfaces that have been rendered we avoid that extra pass.

> (2) Needs fixing: FIXME comment is still missing.
>
> (4) I'm not yet confident this branch is working. As stated in (1) I can't yet
> put together a manual test case on desktop to reproduce any issue, and the
> diff below shows no new regression tests for anything getting fixed. So I
> think you need to try and write a regression test for the issue.

Ack.

lp://qastaging/~alan-griffiths/mir/fix-1535894-alt updated on 2016-02-11

3301. By Alan Griffiths on 2016-02-11: merge lp:mir
3302. By Alan Griffiths on 2016-02-11: Reinstate FIXME comment
3303. By Alan Griffiths on 2016-02-11: Remove some unused headers

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-02-11:

nm. -c 3301 seems to have addressed the problem I was seeing. (Not sure how, so I must have misinterpreted the system behaviour - we need better reporting out of the compositor.)

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-02-11:

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/285/rebuild

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2016-02-11:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/6271/rebuild

review: Approve (continuous-integration)

Unmerged revisions

3303. By Alan Griffiths on 2016-02-11: Remove some unused headers
3302. By Alan Griffiths on 2016-02-11: Reinstate FIXME comment
3301. By Alan Griffiths on 2016-02-11: merge lp:mir
3300. By Alan Griffiths on 2016-02-08: Cleaner code in examples
3299. By Alan Griffiths on 2016-02-08: Cleaner code (that also passes StaleFrames.* tests)
3298. By Alan Griffiths on 2016-02-05: Add corresponding DefaultDisplayBufferCompositor tests
3297. By Alan Griffiths on 2016-02-05: Remove unnecessary Scene::frames_pending()
3296. By Alan Griffiths on 2016-02-05: Fixup broken test
3295. By Alan Griffiths on 2016-02-05: First pass at pending buffer count