[PATCH] Add presentation_time and hit requests to wl_surface.

[Pekka Paalanen]

Hi Axel,

I agree with the clock comments, in that we need to know which clock to
use, and 32 bits may be too short. If we follow struct timespec,
uint32_t seconds + uint32_t nanoseconds should be fine. We don't have
64-bit integers in the protocol.

How about adding an event which tells the client, which clock the
compositor is using?

We cannot simply require CLOCK_MONOTONIC_RAW or CLOCK_MONOTONIC_COARSE,
because they are Linux-specific and introduced in 2.6.28 and 2.6.32,
respectively. The clock name should probably be platform-dependant, if
POSIX values are not enough, so e.g. weston could use
CLOCK_MONOTONIC_RAW, or whatever is the most appropriate, and on other
OSs use what's appropriate there.

More below...

On Sun,  3 Nov 2013 00:39:58 +0100
Axel Davy <axel.davy at ens.fr> wrote:

> These two new requests are designed to help video players
> to synchronize what is displayed on the screen and the audio,
> and to implement the X Present extension in XWayland.
> ---
>  protocol/wayland.xml | 70 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 67 insertions(+), 3 deletions(-)
> 
> diff --git a/protocol/wayland.xml b/protocol/wayland.xml
> index a1df007..553af61 100644
> --- a/protocol/wayland.xml
> +++ b/protocol/wayland.xml
> @@ -959,7 +959,7 @@
>      </event>
>    </interface>
>  
> -  <interface name="wl_surface" version="3">
> +  <interface name="wl_surface" version="4">
>      <description summary="an onscreen surface">
>        A surface is a rectangular area that is displayed on the screen.
>        It has a location, size and pixel contents.
> @@ -1057,8 +1057,9 @@
>  
>      <request name="frame">
>        <description summary="request repaint feedback">
> -	Request notification when the next frame is displayed. Useful
> -	for throttling redrawing operations, and driving animations.
> +	Request notification when the next frame is used for the first time
> +	by the compositor. Useful for throttling redrawing operations, and
> +	driving animations.

"For the first time"? Does that match what happens if a client sends
frame,commit without an attach, and the current wl_buffer is already on
screen?

TBF, this may have been a little vague to begin with.

>  	The frame request will take effect on the next wl_surface.commit.
>  	The notification will only be posted for one frame unless
>  	requested again.
> @@ -1235,6 +1236,69 @@
>      </request>
>     </interface>
>  
> +    <!-- Version 4 additions -->
> +
> +    <request name="presentation_time" since="4">
> +      <description summary="request the frame of next commit to hit the screen at a specific time">
> +	This request is used to indicate the compositor at which ust time the
> +	client wish the frame of next commit to hit the screen.
> +
> +	The request will take effect on the next wl_surface.commit.
> +	The ust time indicated is in Milliseconds.

Explain "ust"?

> +
> +	If the ust time requested has already happened, then the next commit
> +	will be processed as any other commit.

Could you be more specific in "as any other commit"?

What would happen in the following scenario:
- current time is T seconds
- client commits buffer D with ust = T+6
- client commits buffer C with ust = T+1
- client commits buffer B with ust = T-4
- client commits buffer A with ust = T-9

As buffer A is committed last with ust in the past, the commit will be
executed immediately, leaving buffer A on screen instead of B which
would be preferred based on timestamps.

This needs some more explanation as to what should actually happen. The
intent is probably clear, but following the wording produces a wrong
result, IMHO.

> +
> +	The client can do another commit request without cancelling
> +	a commit associated to a requested presentation time that has not
> +	already happened.

Is this saying, that one can queue frames in advance, and the queue can
hold multiple frames? In that case you should explicitly say, that
there is a queue involved.

How can a client cancel queued frames, or wipe the queue? A client may
have queued several frames' worth, when it gets an input event, which
requires it to cancel and perhaps redo already queued frames.

What state is queued, and what state is not? Is all
double-buffered state, that is latched in on commit, queued?

If so, how will the client be aware, which set of state is current at
any time, when e.g. input events come?

How does this interact with the sub-surface protocol, especially
- if this surface is a sub-surface in synchronized mode?
- if this surface is a parent surface with synchronized sub-surfaces?

> +
> +	The compositor can choose to ignore the indicated ust time, and for
> +	example, if the client has queued too much buffers, it can choose to
> +	treat some past commit with a future ust time, as commits with no
> +	ust time indicated.

It's good to specify what happens when there are too many buffers in a
queue, but what does that mean really? Is the compositor applying all
the queued state changes up to a point where the queue has a sane
length again? Shouldn't this be defined to take and process the commits
with the oldest ust first? Not in the order they actually were
committed?

Or, could we simply assume that any sane queue length will be much
smaller than some arbitrary implementation defined constant, and if that
number is ever reached, the client is misbehaving and deserves a
protocol error, disconnecting it? That would be a lot simpler, and
simply queueing a 1000 frames should not be enough to let a compositor
hit OOM - after all the client has had to have allocated the buffers in
the first place, so the client is susceptible to hit OOM much much
earlier; or the client is re-using buffers before they have been
released, so it is not a good citizen anyway.

> +	Calling a second time presentation_time on a wl_surface without doing
> +	a commit will replace the last ust time indicated.
> +      </description>
> +
> +      <arg name="ust" type="uint"/>
> +    </request>
> +
> +    <request name="hit" since="4">
> +      <description summary="request hit feedback">
> +	Request notification when the next frame hits a physical screen.
> +	This notification, which should happen after the frame notification,
> +	can be used to synchronize video and audio better. The time given
> +	will be ust time, in Milliseconds.

Shouldn't this be combined with presentation_time request? If a client
cares about the presentation time, it probably also wants to know how
it succeeded, right?

> +	If, for any reason, the compositor determines that the frame will
> +	never hit the screen, then the callback is called with 0 as argument,
> +	instead of the ust time. One possible reason is that the compositor
> +	choose to use a newer frame sent by the client.

0 as a special invalid timestamp... that is a bit sketchy, since I
don't think anything defines 0 as an invalid timestamp.

> +	The client is not supposed to throttle its drawing to this
> +	notification, but to the frame notification.
> +
> +	If the frame hits multiple physical screens, only the first time it
> +	hits a screen triggers the hit callback.

Maybe better to define it as "only one of them will trigger the
callback", so that compositors are free to trigger it when the output
they synchronize the surface to updates. For instance, if the surface
is mostly on one output, and bit on the other.

> +	The hit request will take effect on the next wl_surface.commit.
> +	The notification will only be posted for one frame unless
> +	requested again.
> +
> +	A client can request a hit callback even without an attach,
> +	damage, or any other state changes, since wl_surface.commit triggers a
> +	display update.

Triggering a display update is a compositor implementation detail, and
should not be mentioned in the protocol like this. It is perfectly
possible for the commit to not change anything visible on screen, and
therefore the compositor will not repaint.

If you want to allow requesting for the hit timestamp after the fact,
you should define it so, and compositors can reply with the original
timestamp when the buffer hit the screen the first time (on its main
output). It think that would be more explicit.

> +	The object returned by this request will be destroyed by the
> +	compositor after the callback is fired and as such the client must not
> +	attempt to use it after that point.
> +      </description>
> +
> +      <arg name="callback" type="new_id" interface="wl_callback"/>
> +    </request>
> +
>    <interface name="wl_seat" version="3">
>      <description summary="group of input devices">
>        A seat is a group of keyboards, pointer and touch devices. This

So, this proposal requires *all* compositors to implement presentation
timestamping and frame queues for wl_surfaces. And I really mean ALL
compositors, including any session switchers, system compositors,
embedded compositors, compositors nested in web-brosers for
each-tab-is-a-separate-process...

I think that is a bit much. Extending wl_surface like this does not
allow a compositor to not implement all this. This is also a bit risky
in case we ever need to change this interface.

Therefore I would really prefer the separate interface design, where
you would also have a natural place to put the what-clock-to-use event.


Thanks,
pq