Mir

Merge lp://qastaging/~afrantzis/mir/client-surface-buffering-mode into lp://qastaging/mir

client-surface-buffering-mode
Merge into development-branch

Proposed by Alexandros Frantzis on 2015-04-16

Status:

Work in progress

Proposed branch:

lp://qastaging/~afrantzis/mir/client-surface-buffering-mode

Merge into:

lp://qastaging/mir

Diff against target:

499 lines (+233/-12)

18 files modified

include/client/mir_toolkit/mir_surface.h (+8/-0)
include/common/mir_toolkit/common.h (+7/-0)
include/server/mir/scene/surface_creation_parameters.h (+1/-0)
src/client/mir_surface.cpp (+2/-0)
src/client/mir_surface.h (+1/-0)
src/client/mir_surface_api.cpp (+6/-0)
src/client/symbols.map (+1/-0)
src/protobuf/mir_protobuf.proto (+4/-0)
src/server/frontend/session_mediator.cpp (+1/-0)
src/server/scene/surface_allocator.cpp (+10/-2)
src/server/scene/surface_creation_parameters.cpp (+1/-0)
tests/acceptance-tests/CMakeLists.txt (+1/-0)
tests/acceptance-tests/test_client_surface_buffering_mode.cpp (+133/-0)
tests/include/mir_test_doubles/null_display_sync_group.h (+19/-3)
tests/include/mir_test_doubles/stub_display.h (+13/-6)
tests/include/mir_test_framework/stub_server_platform_factory.h (+3/-0)
tests/mir_test_framework/stub_server_platform_factory.cpp (+13/-0)
tests/mir_test_framework/stubbed_graphics_platform.cpp (+9/-1)

To merge this branch:

bzr merge lp://qastaging/~afrantzis/mir/client-surface-buffering-mode

Related bugs:

Bug #1194333: [enhancement] Add support for single buffered clients	Low	Triaged
Bug #1444857: [enhancement] Allow clients to specify buffering mode for their surfaces	Medium	New

Link a bug report

Reviewer	Review Type	Date Requested	Status
Robert Carr (community)			Approve on 2015-04-22
Chris Halse Rogers			Needs Information on 2015-04-21
PS Jenkins bot (community)	continuous-integration		Approve on 2015-04-17
Kevin DuBois (community)			Needs Information on 2015-04-16
Daniel van Vugt			Needs Fixing on 2015-04-16
Alan Griffiths		2015-04-16	Approve on 2015-04-16
Review via email: mp+256455@code.qastaging.launchpad.net

Commit message

client,server: Allow clients to specify the buffering mode for their surfaces

Description of the change

client,server: Allow clients to specify the buffering mode for their surfaces

This MP only allows setting the buffering mode at surface creation time. In the future it may be useful to also allow changing the buffering mode after creation time.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-04-16:

89+bool mir_surface_spec_set_buffering_mode(MirSurfaceSpec* spec, MirBufferingMode mode)
90+{
91+ spec->buffering_mode = mode;
92+ return true;
93+}

Possibly worth validating the buffering mode at this point and returning false if it is nonsense?

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-04-16:

std::chrono::milliseconds{0}

You can use literals now

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-04-16:

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-04-16:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2015-04-16:

7: [ FAILED ] ServerShutdown.normal_exit_removes_endpoint
7: [ FAILED ] ClientFocusNotification.a_surface_is_notified_of_receiving_focus
7: [ FAILED ] ClientFocusNotification.two_surfaces_are_notified_of_gaining_and_losing_focus
7: [ FAILED ] ClientCredsTestFixture.session_authorizer_receives_pid_of_connecting_clients
7: [ FAILED ] ClientCredsTestFixture.authorizer_may_prevent_connection_of_clients
7: [ FAILED ] ServerDisconnect.is_detected_by_client
7: [ FAILED ] ServerDisconnect.doesnt_stop_client_calling_API_functions
7: [ FAILED ] ServerDisconnect.causes_client_to_terminate_by_default
7: [ FAILED ] ServerStartup.creates_endpoint_on_filesystem
7: [ FAILED ] ServerStartup.after_server_sigkilled_can_start_new_instance
7: [ FAILED ] UnresponsiveClient.does_not_hang_server
7: [ FAILED ] CustomInputDispatcher.receives_input
7: [ FAILED ] CustomInputDispatcher.gets_started_and_stopped
7: [ FAILED ] CustomInputDispatcher.receives_focus_changes
7: [ FAILED ] FocusSelection.when_surface_created_shell_is_notified_of_session
7: [ FAILED ] FocusSelection.when_surface_created_input_focus_is_set

11: [ FAILED ] MultiplexingDispatchableTest.removal_is_threadsafe

Ehh?

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-04-16:

FAILED: Continuous integration, rev:2490
http://jenkins.qa.ubuntu.com/job/mir-ci/3541/
Executed test runs:
    None: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2079/console
    None: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2030/console
    None: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1538/console
    None: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2030/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-04-16:

(0) In general I don't think this is a good idea. When I finish dynamic switching:
https://code.launchpad.net/~vanvugt/mir/ddouble
only clients using the default auto setting will benefit. But we want all clients to benefit. So I suggest disapprove based on that.

(1) This enum doesn't need to exist. An int is clearer:
28 +typedef enum MirBufferingMode
29 +{
30 + mir_buffering_mode_default = 0,
31 + mir_buffering_mode_double = 2,
32 + mir_buffering_mode_triple = 3
33 +} MirBufferingMode;

review: Disapprove

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-04-16:

(0) Come to think of it, there still is a use case for this. In future if we support single buffering properly then we need this interface.

(1) Still needs to be an int.

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-04-16:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-04-16:

It might be more flexible to just let the clients acquire more than one buffer at once, and then the clients just submit the buffer that they last-generated to the server for it to use.

Advantages:
* Gets us a bit closer to server-or-client-allocated buffers (as opposed to our server allocated buffers nowadays)
* This diffuses the density of the logic in BufferQueue a bit, as the client shares in the responsibility of how the flip chain works.
* The server doesn't even have to know if the client is running in swapinterval 0 or not, it would just have a last-submitted buffer.
* Nested-passthrough can use the normal BufferQueue, instead of dancing around the limitation that it can only get one buffer at a time.
* nested becomes an normal platform, as it has a way to allocate a buffer.
* we don't have to hash out what "double" and "triple" means exactly in the client api.

review: Needs Information

lp://qastaging/~afrantzis/mir/client-surface-buffering-mode updated on 2015-04-17

2491. By Alexandros Frantzis on 2015-04-17: client: Make mir_surface_spec_set_buffering_mode symbol public

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-04-17:

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

review: Approve (continuous-integration)

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-04-21:

*Needs discussion*

It *would* be more flexible for the client to be able to borrow multiple buffers. The simplifications Kevin mentions look pretty enticing.

It'd also make Daniel happy; we can make es2gears produce higher numbers in swapinterval 0 mode :).

This seems like it'd need a bit of design, though; particularly - what happens on resize or other invalidation, does the client need to submit buffers in the same order as received from the server?

Also worth wondering how we can use this to make Vulkan WSI easier.

review: Needs Information

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2015-04-21:

> (0) In general I don't think this is a good idea. When I finish dynamic
> switching:
> https://code.launchpad.net/~vanvugt/mir/ddouble
> only clients using the default auto setting will benefit. But we want all
> clients to benefit. So I suggest disapprove based on that.

Any predictive algorithm needs the ability to be disabled. Especially by the client, as that the entity that has more knowledge about it's own rendering (And the tradeoffs it wants to make) that what you can guess at the server

Revision history for this message

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

Kevins approach is compelling.

This is a good gain for now though.

review: Approve

Revision history for this message

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

"Kevin's approach" sounds like what was described in bug 1253868. Maybe we need to reopen that...

Revision history for this message

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

It seems that we need to make some decisions at a higher level first (i.e. do allow clients to hold multiple buffers?) before deciding whether what's proposed in this MP is needed. The sprint seems a good time to discuss further.

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-04-23:

Yeah, I was expecting that we'd discuss this at the sprint.

Unmerged revisions

2491. By Alexandros Frantzis on 2015-04-17: client: Make mir_surface_spec_set_buffering_mode symbol public
2490. By Alexandros Frantzis on 2015-04-16: client,server: Allow clients to specify the buffering mode for their surface