[enhancement] Need a method of hiding surfaces until they are ready to draw themselves

[Michael Terry]

I've talked to several people on IRC about this (thanks Alan!), but I wanted a more persistent way to discuss this.

My desired goal is that the system compositor is able to hide session surfaces until they have content ready to display. [1]

One way to solve this would be to call hide() on the surfaces as they are created, and show() them when the first frame is posted. After experimenting with this, mir::run_mir seems to cause 4 calls to swap_buffers() due to DisplayServer instantiation (3 calls if I hide() the surface on creation). So it's hard to say when the *client* is posting buffers instead of Mir internals. And inspecting actual buffer data is hard, since that's kind of a GL implementation detail that the system compositor probably shouldn't be worrying about.

How can I implement this in the system compositor? Are the 4 calls to swap_buffers() a bug? Is there missing functionality that needs to be added? (a signal or overridable method that is emitted/called right before passing control to the client?) Or maybe I have the means now but don't know how to use it.

[1] As to *why* I want this: when using a split greeter, both the greeter session and the user session will be starting at the same time. If the user session wins the race, I do not want it shown on the screen before the greeter session is ready. So I want to hide them both until the greeter is ready to display (and in future show a little boot animation in the mean time).

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Daniel van Vugt (vanvugt)]

On one hand I think this makes a lot of sense. All existing toolkits have show/hide functionality. On the other hand, Mir's current approach makes sense and negates the need for show/hide. Mir simply hides a surface until you've explicitly showed it by calling swapbuffers.

The compositor calls should_be_rendered_in() which tells us (amongst other things) whether the surface has posted a frame. If your compositor is failing to check that, then it's probably the compositor that needs fixing.

See: src/server/compositor/* for multiple locations where should_be_rendereed_in() is called.

[Daniel van Vugt (vanvugt)]

Incidentally, should_be_rendered_in() also checks an internal "hidden" flag. That flag is already implemented on the server but there's no client API for it.

[Michael Terry (mterry)]

"Mir simply hides a surface until you've explicitly showed it by calling swapbuffers."

Right. I get that that's how Mir is *supposed* to work. But I'm saying that calling mir::run_mir() in the client calls swap_buffers() four times on the server before even giving control back to the client.

So that seems like a bug then, eh? It means that surfaces are shown before there is actual content to show.

[Daniel van Vugt (vanvugt)]

"you've explicitly showed it by calling swapbuffers."
means swap buffers on the _client_. That's completely unrelated to swap buffers on the server (which happens on startup yes).

Mir's default compositor won't (shouldn't) show a surface until there's a buffer completed by the client. And the mechanism for ensuring this is via should_be_rendered_in().

If you're using Mir's built-in compositor then it's probably a bug in Mir. Otherwise it's a bug in whatever compositor you're using. The compositor _must_ call should_be_rendered_in() to check if it should skip rendering of a new surface (which doesn't have any buffers yet).

[Chris Halse Rogers (raof)]

Yeah, that would be a bug :)

[Daniel van Vugt (vanvugt)]

Hmm, seems there might be a Mir bug at play --> bug 1281938

But that doesn't change the fact that all compositors need to call should_be_rendered_in() to cull the list of surfaces that gets rendered.

[Daniel van Vugt (vanvugt)]

mterry,

Yep, there's a bug in Mir. Not Mir exactly, but Mesa where it supports Mir. I assume you're only seeing problems on desktop/Mesa?... See bug 1281938.

[Michael Terry (mterry)]

I'm testing on mako. So not that bug. But maybe it's just normal startup buffers... Will look at be_rendered_in next

[Michael Terry (mterry)]

So trying to use should_be_rendered_in() seems impossible for a 3rd party (i.e. unity-system-compositor).

The CompositingCriteria class itself is exposed, but BasicSurface isn't, which is what would actually make it useful. As is, there's no way to get access to a Criteria instance for a surface.

[Daniel van Vugt (vanvugt)]

BasicSurface is an internal implementation class that shouldn't be exposed, and isn't. So that's correct.

If you look at Mir's src/server/compositor/* you will see all the compositor logic including several uses of should_be_rendered_in(), and none of it uses BasicSurface at all. Just higher-level interfaces.

I'm not sure if we're failing to expose some other interfaces required to get at CompositingCriteria...? That's all you need to test should_be_rendered_in(). Pass it a rectangle such as the current DisplayBuffer's view_area(). It's not a great interface I know, because its name doesn't even hint that it tests other things like buffer readiness and the hidden flag. But it does.

[Daniel van Vugt (vanvugt)]

Is this a duplicate of bug 1279422?

[Michael Terry (mterry)]

Daniel, I don't think this is a duplicate of bug 1279422. This is about the compositor being able to hide surfaces. That bug is about letting a session execute its internal code.

About BasicSurface, OK. I get that it's an internal implementation class. But do a grep for for CompositingCriteria in the include/ folder and you'll see there's no way to get to one. That needs to be exposed before should_be_rendered_in() can be useful. As it is, USC can't ask that question of a surface.

(Additionally, ideally it would be easy to pass should_be_rendered_in() a rectangle covering the surface. That only seems to be easy via BasicSurface's point/size combo. Which again, isn't exposed.)

[Daniel van Vugt (vanvugt)]

I did a grep, and the interface for getting to CompositingCriteria is public already, apparently:
include/server/mir/compositor/scene.h

That said, the whole team would like to abolish CompositingCriteria and merge it with a less abstract surface class. That way we can for example give the composting/rendering classes a single surface object instead of separate CompositingCriteria and Buffer parameters which originate from the same surface object anyway.

[Michael Terry (mterry)]

We talked about this a bit on IRC. The API in scene.h is not sufficient to match Criteria to Surfaces. And thus isn't of much real use. This is apparently a known deficiency. Just hasn't been address yet.

kdub is apparently working in this space. I'll talk to him to see .

[Daniel van Vugt (vanvugt)]

You can see the deficiency when GLRenderer needs a unique surface ID. The real surface ID is not accessible right now:
http://bazaar.launchpad.net/~mir-team/mir/development-branch/view/head:/src/server/compositor/gl_renderer.cpp#L250

[Michael Terry (mterry)]

Talked more on IRC about this. kdub is doing some reworking of how CompositingCriteria is exposed which might help.

But more importantly, Alan thinks the four buffer swaps are just a bug with nested compositing mode. If that were fixed, I think we'd be good...

[Alan Griffiths (alan-griffiths)]

<mterry> alan_g, it's not even *my* scenario. It's just a client calling mir::run_mir
<mterry> alan_g, something about constructing/running a DisplayServer causes 4 buffers to be swapped
<alan_g> mterry: so this client is a nested server?
<mterry> alan_g, yeah
<mterry> alan_g, (this is all in the context of USC which just deals with nested servers)
<alan_g> mterry: I strongly suspect a bug in the nested code then
<alan_g> With a second possibility that the android client code is the cause
<alan_g> *internal* client
<mterry> alan_g, would that be work-around-able?
<alf_> alan_g: mterry: Hmm, when the compositor starts up it triggers itself, each trigger being 3 compositings. In a nested server, swapping the buffer when compositing means sending the buffer to usc.
<alan_g> And the 4th could be something like the Mesa "advance_buffer" bug?
<alf_> alan_g: yeah
* anpok has quit (Ping timeout: 264 seconds)
<mterry> I do only get 3 if I hide() the surface first, FYI
<alan_g> mterry: that makes sense
<kdub> iirc, android shouldnt do that 'advance_buffer'
<kdub> rather, the same sort of bug as the Mesa "advance_buffer". it does request a buffer on eglCreateWindowSurface, but it should get the buffer associated with the surface creation
* jono_ has quit (Quit: Ex-Chat)
<kdub> (nexus 10 does something different)
<alan_g> kdub: ok, but I'll buy alf_ explanation - we need to something different when compositing in nested mode.
<alan_g> If no-one else grabs it I can have a go on Monday
* mterry hugs alan_g
* alan_g blushes
<mterry> alan_g, so the idea would be that USC could call hide(), watch for buffer swaps, and when it gets a non-null one, show() again?
* vila has quit (Quit: Leaving)
* anpok (~<email address hidden>) has joined #ubuntu-mir
<mterry> or maybe it wouldn't need to call hide if it weren't getting buffer swaps in the first place
<alan_g> mterry: yes, nested clients ought to behave like regular ones
<mterry> alan_g, sounds perfect
* alan_g decides to cut & paste the above discussion into the bug

[Daniel van Vugt (vanvugt)]

I'm a bit lost after the above comment. If there is a nesting-only bug I think it would be useful to describe that more concisely in a new bug report.

As for this enhancement request, it's still almost reasonable to just implement (comment #2). But we shouldn't do so while there appears to be bugs fuelling the motivation for the enhancement. Bugs should be fixed independently of the enhancement request.

[Michael Terry (mterry)]

Daniel, I think the idea was that we have a method of knowing when a surface is ready to be shown today (the compositor can hook into surfaces to know when they have swap_buffer called on them). So USC can just wait for that to happen with a non-null buffer to know that the surface is ready.

So the requested enhancement exists today, assuming the 4-buffer bug is fixed. That's the motivation to tie that flaw to this bug report.

Granted, maybe there could be a more elegant way to implement the enhancement? But I mostly care about it being possible, not necessarily elegant.

[Daniel van Vugt (vanvugt)]

Assuming the 4-buffer bug is fixed (which I think should be logged as a separate bug), then you can use should_be_rendered_in() or whatever comes along to replace it. Fortunately I am now in progress merging CompositingCriteria with Renderable which will be a richer, more useful and more logical interface.

[Michael Terry (mterry)]

Latest mir/devel works great for this, thanks to Alan's nested fixes. Will close.

[Daniel van Vugt (vanvugt)]

The enhancement was not implemented. It was resolved by bug fixes. Probably bug 1284739 whose fix is coming in Mir 0.1.7.