Mir

Merge lp:~vanvugt/mir/always-double-buffers into lp:mir

Proposed by Daniel van Vugt on 2015-02-20
Status: Merged
Approved by: Daniel van Vugt on 2015-02-23
Approved revision: 2331
Merged at revision: 2334
Proposed branch: lp:~vanvugt/mir/always-double-buffers
Merge into: lp:mir
Diff against target: 12 lines (+1/-2)
1 file modified
src/server/compositor/buffer_stream_factory.cpp (+1/-2)
To merge this branch: bzr merge lp:~vanvugt/mir/always-double-buffers
Reviewer Review Type Date Requested Status
Daniel van Vugt Abstain on 2015-02-23
Kevin DuBois (community) Approve on 2015-02-20
Robert Carr (community) Approve on 2015-02-20
Alan Griffiths 2015-02-20 Approve on 2015-02-20
PS Jenkins bot (community) continuous-integration Approve on 2015-02-20
Review via email: mp+250418@code.launchpad.net

Commit message

Always use double buffers instead of triple. (LP: #1240909)

This reduces queue lag by ~50% and resource usage by ~33%. It just
wasn't safe to do till we fixed all the BufferQueue-related bugs as of release 0.11.0.

Description of the change

Tested on desktop with bypass, frame dropping, multi-monitors. The improvement is easy to notice using a mouse.

Tested on phone (vivid mako). The improvement is quite a bit harder to notice. Not just because we've only removed a third of the rendering lag (was 6 now 4 frames), but it's probably much less than a third when you account for the fact that touch screens themselves usually have a significant latency.

All works fine now. Does it work for you?

To post a comment you must log in.
Daniel van Vugt (vanvugt) wrote :

This also significantly improves resize latency of course :)

Alan Griffiths (alan-griffiths) wrote :

I don't see any problems.

Two thoughts (not about this MP directly:

1. Maybe now is a good time to revisit the lttng reports on rendering?
2. We should try again to find a way to automate some smoke tests for visual issues

review: Approve
Robert Carr (robertcarr) wrote :

+1 Yay

review: Approve
Kevin DuBois (kdub) wrote :

> Two thoughts (not about this MP directly:
>
> 1. Maybe now is a good time to revisit the lttng reports on rendering?
> 2. We should try again to find a way to automate some smoke tests for visual
> issues

yes, and yes :)

Kevin DuBois (kdub) wrote :

okay changing the default by me.

Was it tested with the full stack on the n4, or just with the demo applications? (I'm curious to see if this plays nicely with Qt.) Also, bq/n10 have a bit different fencing then the n4, a sanity check there might be worthwhile

Kevin DuBois (kdub) wrote :

> Also, bq/n10
> have a bit different fencing then the n4, a sanity check there might be
> worthwhile

Did a sanity check on bq, lgtm!

review: Approve
Daniel van Vugt (vanvugt) wrote :

I tested full stack (backported to 0.11) and tested Ubuntu Touch proper on mako.

The bad news is I can now see stutters when crossing a screen boundary (multi-monitor with multiple display buffers historically needs triple or else the client starves). Also when entering and leaving bypass the transition is not seamless any more.

The worse news is I somehow made the server crash when testing this (invalid free) but can't reproduce it to debug.

Perhaps the ddouble branch still is the way forward. Either that or we need to examine our compositor/DB code to reduce buffer holding (stutters).

review: Needs Fixing
Daniel van Vugt (vanvugt) wrote :

Still vacillating lots :/

Part of me thinks while this works (which is a first since we started trying to enable it 10 months ago), we should land it. Not landing it is definitely slower, and risks us breaking double buffering support again without noticing.

Improvements like my other branch to dynamically assess client performance can come later.

review: Abstain
Daniel van Vugt (vanvugt) wrote :

OK, I've now tested this on arale/vivid with the full stack.

Works well. Only unity8-dash struggles to keep up with full frame rate. Sometimes 30 FPS, sometimes 60 FPS. Still, that's not bad and nothing new.

Daniel van Vugt (vanvugt) wrote :

Also the System Settings can't seem to scroll above 30 FPS. Still, not terrible and should be fixable.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/server/compositor/buffer_stream_factory.cpp'
2--- src/server/compositor/buffer_stream_factory.cpp 2015-01-21 07:34:50 +0000
3+++ src/server/compositor/buffer_stream_factory.cpp 2015-02-20 08:27:07 +0000
4@@ -47,7 +47,6 @@
5 std::shared_ptr<mc::BufferStream> mc::BufferStreamFactory::create_buffer_stream(
6 mg::BufferProperties const& buffer_properties)
7 {
8- // Note: Framedropping requires a minimum 3 buffers
9- auto switching_bundle = std::make_shared<mc::BufferQueue>(3, gralloc, buffer_properties, *policy_factory);
10+ auto switching_bundle = std::make_shared<mc::BufferQueue>(2, gralloc, buffer_properties, *policy_factory);
11 return std::make_shared<mc::BufferStreamSurfaces>(switching_bundle);
12 }

Subscribers

People subscribed via source and target branches