[PATCH 1/8] protocol: add presentation extension v4

[Pekka Paalanen]

On Mon, 15 Sep 2014 23:12:24 -0700
Bryce Harrington <bryce at osg.samsung.com> wrote:

> Mostly just some minor grammar improvement suggestions...

Appreciated. :-)

> On Mon, Sep 15, 2014 at 04:16:40PM -0400, Louis-Francis Ratté-Boulianne wrote:
> <snip>
> > diff --git a/Makefile.am b/Makefile.am
> > diff --git a/protocol/presentation_timing.xml b/protocol/presentation_timing.xml
> > new file mode 100644
> > index 0000000..8aadff0
> > --- /dev/null
> > +++ b/protocol/presentation_timing.xml
> > @@ -0,0 +1,247 @@
> > +<?xml version="1.0" encoding="UTF-8"?>
> > +<protocol name="presentation_timing">
> > +<!-- wrap:70 -->
> > +
> > +
> > +    <request name="feedback">
> > +      <description summary="request presentation feedback information">
> > +        With this request, presentation feedback will be provided for
> > +        the current content submission of the given surface. A new
> > +        presentation_feedback object is created, and that object will
> > +        deliver the information once. The object is tied to this
> > +        content submission only. Multiple presentation_feedback
> > +        objects may be created for the same submission, and they will
> > +        all return the same information.
> 
> The language here is a bit clunky.  Here's my try:
> 
> "In response to this request, a new presentation_feedback object is
> created for the given surface's current content submission.  The object
> contains a static snapshot of the information.  Each
> request against the same submission will create a new
> presentation_feedback object, but they will all have the same
> information."
> 
> Not sure that's any better!  But maybe gives some wording ideas.

Yeah, not sure...

How about:

"Request presentation feedback for the current content submission on
the given surface. This creates a new presentation_feedback object,
which will deliver the feedback information once. If multiple
presentation_feedback objects are created for the same submission, they
will all deliver the same information."

> Also, it sounds like the client will need to free this object itself?
> Either way, it might be worth stating explicitly here.

Clients will always need to free all objects themselves, that's part of
the Wayland C bindings. No need to add it here, as it applies to
everything.

Also the destructor protocol existing is a clue, that getting an event
won't destroy the protocol object.

That's actually something we could change. Remove the destructor
protocol, and specify that 'presented' and 'discarded' events destroy
the protocol object. Presentation_feedback being a pure feedback
interface, I cannot imagine ever adding any requests to it. The only use
case for destructor protocol would be to cancel the feedback, which
probably is not common, and would still be possible anyway.

> > +        For details on what information is returned, see
> > +        presentation_feedback interface.
> > +      </description>
> > +
> > +      <arg name="surface" type="object" interface="wl_surface"
> > +           summary="target surface"/>
> > +      <arg name="callback" type="new_id" interface="presentation_feedback"
> > +           summary="new feedback object"/>
> > +    </request>
> > +
> > +    <event name="clock_id">
> > +      <description summary="clock ID for timestamps">
> > +        This event tells the client, in which clock domain the
> 
> Comma is incorrect here.
> 
> "...tells the client which clock domain..."
> 
> > +        compositor interprets the timestamps used by the presentation
> > +        extension. This clock is called the presentation clock.
> > +
> > +        The compositor sends this event when the client binds to the
> > +        presentation interface. The presentation clock does not change
> > +        during the lifetime of the client connection.
> > +
> > +        The clock identifier is platform dependent. Clients must be
> > +        able to query the current clock value directly, not by asking
> > +        the compositor.
> > +
> > +        On Linux/glibc, the identifer value is one of the clockid_t
> > +        values accepted by clock_gettime(). clock_gettime() is defined
> > +        by POSIX.1-2001.
> > +
> > +        Compositors should prefer a clock, which does not jump and is
> 
> Comma also incorrect here.
> 
> "Compositors should prefer a clock that does not jump and..."
> 
> > +        not slewed e.g. by NTP. The absolute value of the clock is
> > +        irrelevant. Precision of one millisecond or better is
> > +        recommended.
> > +
> > +        Timestamps in this clock domain are expressed as tv_sec_hi,
> > +        tv_sec_lo, tv_nsec triples, each component being an unsigned
> > +        32-bit value. Whole seconds are in tv_sec which is a 64-bit
> > +        value combined from tv_sec_hi and tv_sec_lo, and the
> > +        additional fractional part in tv_nsec as nanoseconds. Hence,
> > +        for valid timestamps tv_nsec must be in [0, 999999999].
> > +
> > +        Note, that clock_id applies only to the presentation clock,
> 
> No comma needed after Note.  You can either do "Note that clock_id..."
> or "Note: clock_id ..."
> 
> > +        and implies nothing about e.g. the timestamps used in the
> > +        Wayland core protocol input events.
> > +      </description>
> > +
> > +      <arg name="clk_id" type="uint" summary="platform clock identifier"/>
> > +    </event>
> > +
> > +  </interface>
> > +
> > +  <interface name="presentation_feedback" version="1">
> > +    <description summary="presentation time feedback event">
> > +      A presentation_feedback object returns the feedback information
> > +      about a wl_surface content update becoming visible to the user.
> 
> "A presentation_feedback object returns an indication that a wl_surface
> content update has become visible to the user."
> 
> Otherwise the sentence sounds a bit like, "An apple tastes like an
> apple."  ;-)

Except it may not have become visible to the user: 'discarded'.

Presentation_feedback is the type of an object that delivers feedback
about presentation, so I see no repetition there as the name is
arbitrary and its meaning needs to be defined.

s/return/deliver/ would be better I think, if we use "deliver" above.

How about: (see further below...)

> > +      One object corresponds to one content update submission
> > +      (wl_surface.commit). There are two possible outcomes: the
> > +      content update may be presented to the user, in which case the
> > +      presentation timestamp is delivered. Otherwise, the content
> > +      update is discarded, and the user never had a chance to see it
> > +      before it was superseded or the surface was destroyed.
> 
> "There are two possible outcomes:  The content update is presented to
> the user, and a presentation timestamp delivered; or, the user did not
> see the content update because it was superseded or its surface
> destroyed, and the content update is discarded."

How about:

"A presentation_feedback object delivers the result of the
associated wl_surface content update: whether it was presented to
the user or discarded. Discarding may be due to the update being
superseded by another update or the surface being destroyed."

Maybe that says it all?

> > +      Once a presentation_feedback object has delivered an event, it
> > +      becomes inert, and should be destroyed by the client.
> 
> The second comma is incorrect; the first one is ok but I think it can be
> omitted as well.

Also, "an event" is too vague. The events are exactly 'presented' and
'discarded'. Event 'sync_output' does not do this.

> 
> > +    </description>
> > +
> > +    <request name="destroy" type="destructor">
> > +      <description summary="destroy presentation feedback object">
> > +        The object is destroyed. If a feedback event had not been
> > +        delivered yet, it is cancelled.
> 
> Comma can be omitted.
> 
> > +      </description>
> > +    </request>
> > +
> > +    <event name="sync_output">
> > +      <description summary="presentation synchronized to this output">
> > +        As presentation can be synchronized to only one output at a
> > +        time, this event tells which output it was. This event is only
> > +        sent prior to the presented event.
> > +
> > +        As clients may bind to the same global wl_output multiple
> > +        times, this event is sent for each bound instance that matches
> > +        the synchronized output. If a client has not bound to the
> > +        right wl_output global at all, this event is not sent.
> > +      </description>
> > +
> > +      <arg name="output" type="object" interface="wl_output"
> > +           summary="presentation output"/>
> > +    </event>
> > +
> > +    <enum name="kind">
> > +      <description summary="bitmask of flags in presented event">
> > +      </description>
> > +
> > +      <entry name="none" value="0"/>
> > +    </enum>
> > +
> > +    <event name="presented">
> > +      <description summary="the content update was displayed">
> > +        The associated content update was displayed to the user at the
> > +        indicated time (tv_sec_hi/lo, tv_nsec). For the interpretation of
> > +        the timestamp, see presentation.clock_id event.
> > +
> > +        The timestamp corresponds to the time when the content update
> > +        turned into light the first time on the surface's main output.
> > +        Compositors may approximate this from the framebuffer flip
> > +        completion events from the system, and the latency of the
> > +        physical display path if known.
> > +
> > +        This event is preceeded by all related sync_output events
> > +        telling which output's refresh cycle the feedback corresponds
> > +        to, i.e. the main output for the surface. Compositors are
> > +        recommended to choose to the output containing the largest
> 
> "to choose to" sounds wrong...

Yup, should be "to choose the output".

> > +        part of the wl_surface, or keeping the output they previously
> > +        chose. Having a stable presentation output association helps
> > +        clients to predict future output refreshes (vblank).
> 
> "...helps clients predict..."
> 
> > +        Argument 'refresh' gives the compositor's prediction of how
> > +        many nanoseconds after tv_sec, tv_nsec the very next output
> > +        refresh may occur. This is to further aid clients in
> > +        predicting future refreshes, i.e., estimating the timestamps
> > +        targeting the next few vblanks. If such prediction cannot
> > +        usefully be done, the argument is zero.
> > +
> > +        The 64-bit value combined from seq_hi and seq_lo is the value
> > +        of the output's vertical retrace counter when the content
> > +        update was first scanned out to the display. This value must
> > +        be compatible with the definition of MSC in
> > +        GLX_OML_sync_control specification. Note, that if the display
> > +        path has a non-zero latency, the time instant specified by
> > +        this counter may differ from the timestamp's.
> > +
> > +        If the output does not have a constant refresh rate, explicit
> > +        video mode switches excluded, then the refresh argument must
> > +        be zero.
> > +
> > +        If the output does not have a concept of vertical retrace or a
> > +        refresh cycle, or the output device is self-refreshing without
> > +        a way to query the refresh count, then the arguments seq_hi
> > +        and seq_lo must be zero.
> > +      </description>
> > +
> > +      <arg name="tv_sec_hi" type="uint"
> > +           summary="high 32 bits of the seconds part of the presentation timestamp"/>
> > +      <arg name="tv_sec_lo" type="uint"
> > +           summary="low 32 bits of the seconds part of the presentation timestamp"/>
> > +      <arg name="tv_nsec" type="uint"
> > +           summary="nanoseconds part of the presentation timestamp"/>
> > +      <arg name="refresh" type="uint" summary="nanoseconds till next refresh"/>
> > +      <arg name="seq_hi" type="uint"
> > +           summary="high 32 bits of refresh counter"/>
> > +      <arg name="seq_lo" type="uint"
> > +           summary="low 32 bits of refresh counter"/>
> > +      <arg name="flags" type="uint" summary="combination of 'kind' values"/>
> > +    </event>
> > +
> > +    <event name="discarded">
> > +      <description summary="the content update was not displayed">
> > +        The content update was never displayed to the user.
> > +      </description>
> > +    </event>
> > +  </interface>
> 
> Bryce

Agreed with every comment I didn't explicitly reply to above. :-)


Thanks,
pq