Merge into development-branch : non-blocking-swap-buffers : Code : Mir

Reviewer	Review Type	Date Requested	Status
Alberto Aguirre (community)		2014-04-08	Approve on 2014-04-10
PS Jenkins bot (community)	continuous-integration		Approve on 2014-04-10
Kevin DuBois (community)			Abstain on 2014-04-09
Review via email: mp+214755@code.staging.launchpad.net

Revision history for this message

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

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-04-08:

#

~~~
231+ static int const vsync_period_microsec{16666};
232+
233+ auto now = duration_cast<microseconds>(
234+ steady_clock::now().time_since_epoch());
235+ microseconds next_vsync{
236+ (now.count() / vsync_period_microsec + 1) * vsync_period_microsec};
237+ std::this_thread::sleep_until(steady_clock::time_point{next_vsync});
~~~

You can let chrono do the conversions instead

typedef std::chrono::duration<int, std::ratio<1, 60>> vsync_periods;

//truncate to vsync periods
auto now = std::chrono::duration_cast<vsync_periods>(std::chrono::steady_clock::now().time_since_epoch());
//convert back to a timepoint
auto now_tp = std::chrono::time_point<std::chrono::steady_clock, vsync_periods>{now};
//Next vsync time point
auto next_vsync = now_tp + vsync_periods(1);
std::this_thread::sleep_until(next_vsync);

For test at line 353 - doesn't it need a timeout mechanism in case it does block?

review: Needs Fixing

Revision history for this message

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

#

comments
216 + if (r->visible())
I intend on removing visible and the saved resources vector soon anyways.

needs fixing:
220 + wait_until_next_vsync();
I'd rather this be "wait_a_reasonable_amount()" or something like that

discussion:
given mc::BufferStream::allow_framedropping(), which is set by the client, it seems strange drop frames via BufferConsumingFunctor if we decide not to consume the buffer. Shouldn't problems with swapping and ordering be solved more in the BufferStream/SwitchingBundle?

another thought:
could we do this in one loop/thread? (and only generate the renderlist once?)

188 + run_compositing_loop([&] { return display_buffer_compositor->composite();});
could we do:

run_compositing_loop([&] {
return display_buffer_compositor->composite(list);
//consume hidden surfaces?
});

review: Needs Fixing

Revision history for this message

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

#

> 216 + if (r->visible())
> I intend on removing visible and the saved resources vector soon anyways.

Does that mean that scene->generate_renderable_list() will return only renderables that have buffers for rendering?

@saved_resources

I guess the plan is to keep buffers "alive" using some other mechanism. Will we be able to use that mechanism from outside the DisplayBufferCompositor, or do we need to continue use the saved resources vector in the BufferConsumingThread?

220 + wait_until_next_vsync();

wait_until_next_fake_vsync() ?

> given mc::BufferStream::allow_framedropping(), which is set by the client, it seems strange drop frames via
> BufferConsumingFunctor if we decide not to consume the buffer. Shouldn't problems with swapping and ordering be
> solved more in the BufferStream/SwitchingBundle?

I thought a bit about this (but not much really), but couldn't find a satisfactory way to do it in BufferStream/SwitchingBundle. The main issue is that this is not unconditional framedropping, but "framedropping" only if not rendered, which is only known at the compositor level. Even if we find a way to do it in BufferStream/SwitchingBundle, we need to take into account the complexity we are going to add to an already complex component. In any case, I am open to ideas :)

> another thought:
> could we do this in one loop/thread? (and only generate the renderlist once?)

We need to support the cases of multiple active monitors and of no active monitors (e.g. screen off on android). For multiple active monitors we could do as you propose if we have a way for the display_buffer_compositor to return all the surface buffers it didn't consume. Note, however, that in terms of consumed buffers it's almost the same as (or worse than) having the extra thread proposed in this MP. Imagine for example two compositing threads T1,T2 for two outputs and two sets of surfaces A (visible on first output) and B (visible on second output). If we consume hidden surfaces in the compositing threads then T1 will consume AUB and and T2 will consume BUA. With an extra thread T1 consumes A, T2 consumes B and the extra thread consumes AUB. So both ways consume A twice and B twice. For more than two outputs however, consuming in the compositing thread themselves leads to more consumptions than with the extra thread.

For no active monitors and hence no compositing threads, we need the extra consuming thread anyway, so I decided to use it unconditionally, since it provides the behavior we want and it's conceptually simple (essentially just another (fake) output from the POV of our buffer handling).

Granted that for only one output what you describe is a win: we only use one thread and consume the hidden buffers only once. However, it does add some complexity that I wanted to avoid at this point, since this is a matter of optimization not correctness. I am fine with optimizing this use case when things have settled.

> 216 + if (r->visible())
> I intend on removing visible and the saved resources vector soon anyways.

Does that mean that scene->generate_renderable_list() will return only renderables that have buffers for rendering?

@saved_resources

I guess the plan is to keep buffers "alive" using some other mechanism. Will we be able to use that mechanism from outside the DisplayBufferCompositor, or do we need to continue use the saved resources vector in the BufferConsumingThread?

220 + wait_until_next_vsync();

wait_until_next_fake_vsync() ?

> given mc::BufferStream::allow_framedropping(), which is set by the client, it seems strange drop frames via
> BufferConsumingFunctor if we decide not to consume the buffer. Shouldn't problems with swapping and ordering be
> solved more in the BufferStream/SwitchingBundle?

I thought a bit about this (but not much really), but couldn't find a satisfactory way to do it in BufferStream/SwitchingBundle. The main issue is that this is not unconditional framedropping, but "framedropping" only if not rendered, which is only known at the compositor level. Even if we find a way to do it in BufferStream/SwitchingBundle, we need to take into account the complexity we are going to add to an already complex component. In any case, I am open to ideas :)

> another thought:
> could we do this in one loop/thread? (and only generate the renderlist once?)

We need to support the cases of multiple active monitors and of no active monitors (e.g. screen off on android). For multiple active monitors we could do as you propose if we have a way for the display_buffer_compositor to return all the surface buffers it didn't consume. Note, however, that in terms of consumed buffers it's almost the same as (or worse than) having the extra thread proposed in this MP. Imagine for example two compositing threads T1,T2 for two outputs and two sets of surfaces A (visible on first output) and B (visible on second output). If we consume hidden surfaces in the compositing threads then T1 will consume AUB and and T2 will consume BUA. With an extra thread T1 consumes A, T2 consumes B and the extra thread consumes AUB. So both ways consume A twice and B twice. For more than two outputs however, consuming in the compositing thread themselves leads to more consumptions than with the extra thread.

For no active monitors and hence no compositing threads, we need the extra consuming thread anyway, so I decided to use it unconditionally, since it provides the behavior we want and it's conceptually simple (essentially just another (fake) output from the POV of our buffer handling).

Granted that for only one output what you describe is a win: we only use one thread and consume the hidden buffers only once. However, it does add some complexity that I wanted to avoid at this point, since this is a matter of optimization not correctness. I am fine with optimizing this use case when things have settled.

Revision history for this message

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

#

> You can let chrono do the conversions instead

Good idea. Fixed.

> For test at line 353 - doesn't it need a timeout mechanism in case it does block?

Fixed.

Revision history for this message

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

#

FAILED: Continuous integration, rev:1543
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1286/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1544
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1542
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1118/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1018/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1023/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1119/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-04-09:

#

Looks good.

review: Approve

Revision history for this message

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

#

Download full text (4.1 KiB)

> > 216 + if (r->visible())
> > I intend on removing visible and the saved resources vector soon anyways.
>
> Does that mean that scene->generate_renderable_list() will return only
> renderables that have buffers for rendering?

Right, the scene should be trimming out surfaces that are not visible().

>
> @saved_resources
>
> I guess the plan is to keep buffers "alive" using some other mechanism. Will
> we be able to use that mechanism from outside the DisplayBufferCompositor, or
> do we need to continue use the saved resources vector in the
> BufferConsumingThread?

We talked a bit about this during the standup with Daniel, I guess we'll have to see where it moves to. I was intending on moving it to the renderer or the RenderableList, although I quite liked moving it to the snapshots in the RenderableList on my first pass.

>
> 220 + wait_until_next_vsync();
>
> wait_until_next_fake_vsync() ?

This is fine, although I like
wait_until_next_client_unblock_event() a bit better. Either one.

>
> > given mc::BufferStream::allow_framedropping(), which is set by the client,
> it seems strange drop frames via
> > BufferConsumingFunctor if we decide not to consume the buffer. Shouldn't
> problems with swapping and ordering be
> > solved more in the BufferStream/SwitchingBundle?
>
> I thought a bit about this (but not much really), but couldn't find a
> satisfactory way to do it in BufferStream/SwitchingBundle. The main issue is
> that this is not unconditional framedropping, but "framedropping" only if not
> rendered, which is only known at the compositor level. Even if we find a way
> to do it in BufferStream/SwitchingBundle, we need to take into account the
> complexity we are going to add to an already complex component. In any case, I
> am open to ideas :)

Yeah, I don't have any ideas that don't involve complicating SwitchingBundle, so I'm happy to go with this solution... and hopefully follow up with an approach that doesn't involve a new thread once the dust has settled.

>
> > another thought:
> > could we do this in one loop/thread? (and only generate the renderlist
> once?)
>
> We need to support the cases of multiple active monitors and of no active
> monitors (e.g. screen off on android). For multiple active monitors we could
> do as you propose if we have a way for the display_buffer_compositor to return
> all the surface buffers it didn't consume. Note, however, that in terms of
> consumed buffers it's almost the same as (or worse than) having the extra
> thread proposed in this MP. Imagine for example two compositing threads T1,T2
> for two outputs and two sets of surfaces A (visible on first output) and B
> (visible on second output). If we consume hidden surfaces in the compositing
> threads then T1 will consume AUB and and T2 will consume BUA. With an extra
> thread T1 consumes A, T2 consumes B and the extra thread consumes AUB. So both
> ways consume A twice and B twice. For more than two outputs however, consuming
> in the compositing thread themselves leads to more consumptions than with the
> extra thread.
>
> For no active monitors and hence no compositing threads, we need the extra
> consuming thread anyway, so I d...

> > 216 + if (r->visible())
> > I intend on removing visible and the saved resources vector soon anyways.
> 
> Does that mean that scene->generate_renderable_list() will return only
> renderables that have buffers for rendering?

Right, the scene should be trimming out surfaces that are not visible().

> 
> @saved_resources
> 
> I guess the plan is to keep buffers "alive" using some other mechanism. Will
> we be able to use that mechanism from outside the DisplayBufferCompositor, or
> do we need to continue use the saved resources vector in the
> BufferConsumingThread?

We talked a bit about this during the standup with Daniel, I guess we'll have to see where it moves to. I was intending on moving it to the renderer or the RenderableList, although I quite liked moving it to the snapshots in the RenderableList on my first pass.

> 
> 220 + wait_until_next_vsync();
> 
> wait_until_next_fake_vsync() ?

This is fine, although I like
wait_until_next_client_unblock_event() a bit better. Either one.

> 
> > given mc::BufferStream::allow_framedropping(), which is set by the client,
> it seems strange drop frames via
> > BufferConsumingFunctor if we decide not to consume the buffer. Shouldn't
> problems with swapping and ordering be
> > solved more in the BufferStream/SwitchingBundle?
> 
> I thought a bit about this (but not much really), but couldn't find a
> satisfactory way to do it in BufferStream/SwitchingBundle. The main issue is
> that this is not unconditional framedropping, but "framedropping" only if not
> rendered, which is only known at the compositor level. Even if we find a way
> to do it in BufferStream/SwitchingBundle, we need to take into account the
> complexity we are going to add to an already complex component. In any case, I
> am open to ideas :)

Yeah, I don't have any ideas that don't involve complicating SwitchingBundle, so I'm happy to go with this solution... and hopefully follow up with an approach that doesn't involve a new thread once the dust has settled.

> 
> > another thought:
> > could we do this in one loop/thread? (and only generate the renderlist
> once?)
> 
> We need to support the cases of multiple active monitors and of no active
> monitors (e.g. screen off on android). For multiple active monitors we could
> do as you propose if we have a way for the display_buffer_compositor to return
> all the surface buffers it didn't consume. Note, however, that in terms of
> consumed buffers it's almost the same as (or worse than) having the extra
> thread proposed in this MP. Imagine for example two compositing threads T1,T2
> for two outputs and two sets of surfaces A (visible on first output) and B
> (visible on second output). If we consume hidden surfaces in the compositing
> threads then T1 will consume AUB and and T2 will consume BUA. With an extra
> thread T1 consumes A, T2 consumes B and the extra thread consumes AUB. So both
> ways consume A twice and B twice. For more than two outputs however, consuming
> in the compositing thread themselves leads to more consumptions than with the
> extra thread.
> 
> For no active monitors and hence no compositing threads, we need the extra
> consuming thread anyway, so I decided to use it unconditionally, since it
> provides the behavior we want and it's conceptually simple (essentially just
> another (fake) output from the POV of our buffer handling).
> 
> Granted that for only one output what you describe is a win: we only use one
> thread and consume the hidden buffers only once. However, it does add some
> complexity that I wanted to avoid at this point, since this is a matter of
> optimization not correctness. I am fine with optimizing this use case when
> things have settled.

ack, I'm okay with this approach, hopefully once the DisplayBufferCompositor is easily writable we can work around keeping that class easy to override, and figure out what to do with buffer consuming in the MultiThreadedCompositor case. I still smell that there are some logic-based approaches instead of timing/thread-based ones, but we can work through that a bit later.

I guess I'll abstain about the function naming, not a big enough objection to block the MP

review: Abstain

Revision history for this message

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

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-04-09:

#

434+ auto const surface = mir_connection_create_surface_sync(connection, &request_params);

Missing corresponding mir_surface_release_sync(surface);

review: Needs Fixing

Revision history for this message

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

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

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

#

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

review: Approve (continuous-integration)

Revision history for this message

Alberto Aguirre (albaguirre) on 2014-04-10:

#

review: Approve

Revision history for this message

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

#

A prototype of an alternative solution to provide non-blocking SwapBuffers involving only SwitchingBundle changes is:

https://code.launchpad.net/~raof/mir/1hz-rendering-always/

We can change the rate to be whatever we like, as long it's less than the smallest vsync rate of any of the attached displays (plus some safety margin).

!!!Note: the functionality is currently broken in r1545 of that branch

For now we will go with the current branch to unblock our stack with Qt 5.2, and we can discuss more about the alternative as it matures.

My 2 cents so I don't forget:

Pros of the alternative: completely transparent to compositor, only switching bundle needs to be changed, consumes only buffers that are not otherwise consumed

Cons of the alternative: not obviously correct, makes an already complex component more complex, can't stop all buffer consumption by just stopping the compositor, can't reach up to vsync rate

Mir

Merge lp://staging/~afrantzis/mir/non-blocking-swap-buffers into lp://staging/mir

Commit message

Description of the change

Preview Diff

Subscribers