[Cogl] [PATCH 3/3] Add CoglFrameTimings

[Robert Bragg]

On Wed, Nov 28, 2012 at 4:47 PM,  <otaylor at redhat.com> wrote:
> From: "Owen W. Taylor" <otaylor at fishsoup.net>
>
> Add a CoglFrameTimings object that tracks timing information
> for frames that are drawn. We track a frame counter and frame
> timing information for each CoglOnscreen. Frames timing information
> is deliminated by a new cogl_onscreen_begin_frame() and retrieved using
> cogl_onscreen_get_frame_timings().

We definitely want to be able to expose this timing information to
apps though I do have a few concerns about this current approach as it
stands...

I'd really like to see if we can avoid needing to expose a history of
frame timings that you lookup based on a frame number and that you
have to manually check the completeness of. Instead I'd prefer to only
deliver the information to the app via a callback when the data
arrives in a way that directly relates the information to the frame
that the information is for.

Ideally I'd like to see this data passed directly to the swap buffer
callbacks but since we can't change the api for cogl-1.14 we can
instead add a replacement cogl_onscreen_add_swap_complete_callback()
and define a CoglSwapCompleteNotify type that has an extra argument.
We can then make the old api into a shim over the new and deprecate
it.

I also have some concerns about the guarantees we can offer for our
reported timestamps given how variable drivers can be.

> +#ifndef __COGL_FRAME_TIMINGS_PRIVATE_H
> +#define __COGL_FRAME_TIMINGS_PRIVATE_H
> +
> +#include "cogl-frame-timings.h"
> +#include "cogl-object-private.h"
> +
> +struct _CoglFrameTimings
> +{
> +  CoglObject _parent;
> +
> +  int64_t frame_counter;
> +  int64_t frame_time;
> +  int64_t presentation_time;
> +  int64_t refresh_interval;
> +
> +  guint complete : 1;
> +};

I think instead of "CoglFrameTimings" this could instead be renamed to
something like "CoglSwapInfo" so that non-timing information about the
frame completion can also be queried this way. For example the output
that the framebuffer was presented on could be queried from here.

> +/**
> + * cogl_frame_timings_get_complete:
> + * @timings: a #CoglFrameTimings object
> + *
> + * Gets whether all information that will potentially be provided for
> + * the frame has been provided. Once a frame timings object is complete,
> + * no further changes will be made to it.
> + *
> + * Return value: whether the frame timings object is complete.
> + * Since: 2.0
> + * Stability: unstable
> + */
> +CoglBool cogl_frame_timings_get_complete (CoglFrameTimings *timings);

We can get rid of the complete member and
cogl_frame_timings_get_complete() if we know we only ever deliver the
info when it is complete.

> +/**
> + * cogl_frame_timings_get_presentation_time:
> + * @timings: a #CoglFrameTimings object
> + *
> + * Gets the presentation time for the frame. This is the time at which
> + * the frame became visible to the user.
> + *
> + * Return value: the presentation time for the frame, in
> + *  the timescale of g_get_monotonic_time().
> + * Since: 2.0
> + * Stability: unstable
> + */
> +int64_t cogl_frame_timings_get_presentation_time (CoglFrameTimings *timings);

It doesn't seem right to try and shoehorn unadjusted-system-time
values into the g_get_monotonic_time() timescale. On Linux
g_get_monotonic_time will use CLOCK_MONOTONIC and given the documented
semantics for UST I can see it making sense for a driver to use
CLOCK_MONOTONIC_RAW so we could end up mapping CLOCK_MONOTONIC_RAW to
CLOCK_MONOTONIC and that sounds a bit odd. On windows
g_get_monotonic_time uses GetTickCount64 which is known to have a very
low resolution, so it seems unlikely that a driver implementing
WGL_OML_sync_control would just use that. On OSX g_get_monotonic_time
will fallback to gettimeofday() it seems which is not even monotonic
and mapping a monotonic UST into the gettimeofday timescale sounds a
bit odd too.

I think perhaps, similar to OpenGL, we should just define an abstract
"system time" like UST with an unspecified starting point which we
shouldn't document in terms of any other api so that we maintain the
freedom to change how we acquire our system timestamps. All we should
really guarantee are the units. Given that Mesa currently implements
UST using gettimeofday() we should perhaps not even guarantee a
monotonic counter like the OpenGL spec does. At least this way its
clear that applications need to gracefully handle large time jumps and
time going backwards.

> +
> +/**
> + * cogl_frame_timings_get_refresh_interval:
> + * @timings: a #CoglFrameTimings object
> + *
> + * Gets the refresh interval for the output that the frame was on at the
> + * time the frame was presented. This is the number of microseconds between
> + * refreshes of the screen, and is equal to 1000000 / refresh_rate.
> + *
> + * Return value: the refresh interval, in microsecoonds.
> + *  .
> + * Since: 2.0
> + * Stability: unstable
> + */
> +int64_t cogl_frame_timings_get_refresh_interval (CoglFrameTimings *timings);

This is more a property of the output so it doesn't seem like the
right place to expose it. Instead I think it would be better to report
what CoglOutput the frame was presented on and then let an application
query the refresh rate of that output directly if they want.

> +void
> +cogl_onscreen_begin_frame (CoglOnscreen *onscreen,
> +                           gint64        frame_time)
> +{
> +  onscreen->frame_counter++;
> +  onscreen->current_frame_timings = (onscreen->current_frame_timings + 1) % COGL_ONSCREEN_MAX_FRAME_TIMINGS;
> +
> +  if (onscreen->n_frame_timings < COGL_ONSCREEN_MAX_FRAME_TIMINGS)
> +    onscreen->n_frame_timings++;
> +  else
> +    cogl_object_unref (onscreen->frame_timings[onscreen->current_frame_timings]);
> +
> +  onscreen->frame_timings[onscreen->current_frame_timings] = _cogl_frame_timings_new ();
> +  onscreen->frame_timings[onscreen->current_frame_timings]->frame_counter = onscreen->frame_counter;
> +  onscreen->frame_timings[onscreen->current_frame_timings]->frame_time = frame_time;
> +}

Assuming we leave it to applications to track the frame_time
themselves as discussed above I think we can remove this api since we
can increment onscreen->frame_counter in cogl_onscreen_swap_buffers
instead and the CoglSwapInfo state can be created at the last moment
before we notify the application of swap completion.

> +
> +static void
> +cogl_onscreen_before_swap (CoglOnscreen *onscreen)
> +{
> +  if (onscreen->swap_frame_counter == onscreen->frame_counter)
> +    cogl_onscreen_begin_frame (onscreen, 0);
> +
> +  onscreen->swap_frame_counter = onscreen->frame_counter;
> +}

I think maybe I'm missing something about why we need to distinguish
the swap-count from the frame-counter here. Hopefully the distinction
was only required as a convenience for doing lookups into the history
of frame timings and we don't need this distinction if we manage to
remove the need to track a history?

In general the idea of exposing a frame/swap counter for an onscreen
sounds like it could be a nice convenience for us to expose.

> +unsigned int
> +cogl_onscreen_add_frame_timings_callback (CoglOnscreen *onscreen,
> +                                          CoglFrameTimingsCallback callback,
> +                                          void *user_data)
> +{
> +  CoglFrameTimingsCallbackEntry *entry = g_slice_new (CoglFrameTimingsCallbackEntry);
> +  static int next_resize_callback_id = 0;
> +
> +  entry->callback = callback;
> +  entry->user_data = user_data;
> +  entry->id = next_resize_callback_id++;
> +
> +  COGL_TAILQ_INSERT_TAIL (&onscreen->frame_timings_callbacks, entry, list_node);
> +
> +  return entry->id;
> +}
> +
> +void
> +cogl_onscreen_remove_frame_timings_callback (CoglOnscreen *onscreen,
> +                                             unsigned int id)
> +{
> +  CoglFrameTimingsCallbackEntry *entry;
> +
> +  COGL_TAILQ_FOREACH (entry, &onscreen->frame_timings_callbacks, list_node)
> +    {
> +      if (entry->id == id)
> +        {
> +          COGL_TAILQ_REMOVE (&onscreen->frame_timings_callbacks, entry, list_node);
> +          g_slice_free (CoglFrameTimingsCallbackEntry, entry);
> +          break;
> +        }
> +    }
> +}

Instead of needing new apis like this, I think we can build on the
existing cogl_onscreen_add/remove_swap_buffers_callback() functions
and infrastructure. The existing functions can be renamed to
cogl_onscreen_add/remove_swap_complete_callback() and we can create a
CoglSwapCompleteNotify callback type that has a CoglSwapInfo argument.
We can then create shim implementations of the
cogl_onscreen_add/remove_swap_buffers_callback() functions for
compatibility.

> +static gint64
> +ust_to_monotonic_time (CoglRenderer *renderer,
> +                       GLXDrawable   drawable,
> +                       int64_t       ust)
> +{
> +  CoglGLXRenderer *glx_renderer =  renderer->winsys;
> +  CoglXlibRenderer *xlib_renderer = _cogl_xlib_renderer_get_data (renderer);
> +
> +  ensure_ust_type (renderer, drawable);
> +
> +  switch (glx_renderer->ust_type)
> +    {
> +    case COGL_GLX_UST_IS_UNKNOWN:
> +      g_assert_not_reached ();
> +      break;
> +    case COGL_GLX_UST_IS_GETTIMEOFDAY:
> +      {
> +        struct timeval tv;
> +        gint64 current_system_time;
> +        gint64 current_monotonic_time;
> +
> +        gettimeofday(&tv, NULL);
> +        current_system_time = (tv.tv_sec * G_GINT64_CONSTANT (1000000)) + tv.tv_usec;
> +        current_monotonic_time = g_get_monotonic_time ();
> +
> +        return ust + current_monotonic_time - current_system_time;
> +      }
> +    case COGL_GLX_UST_IS_MONOTONIC_TIME:
> +      return ust;
> +    case COGL_GLX_UST_IS_OTHER:
> +      {
> +        if (glx_renderer->pf_glXGetSyncValues)
> +          {
> +            gint64 current_monotonic_time;
> +            int64_t ust;
> +            int64_t msc;
> +            int64_t sbc;
> +
> +            glx_renderer->pf_glXGetSyncValues (xlib_renderer->xdpy, drawable,
> +                                               &ust, &msc, &sbc);
> +
> +            current_monotonic_time = g_get_monotonic_time ();
> +            return ust + current_monotonic_time - ust;

It looks like the int64_t ust declared here shadows the ust passed
into the function itself and I guess this should be something like:

return ust + current_monotonic_time - current_ust;

This seems pretty fragile though so as discussed above I think perhaps
we should aim for a design that doesn't require us to attempt this
mapping.

Ok, so in summary I currently think the next steps for this patch are:

- Rename CoglFrameTimings to CoglSwapInfo
- Rename cogl_onscreen_add/remove_swap_buffers_callback to
cogl_onscreen_add/remove_swap_complete_callback which takes a
CoglSwapCompleteNotify callback that has a CoglSwapInfo argument
- Provide a shim implementation of
cogl_onscreen_add/remove_swap_buffers_callback on the new api
- Drop the api that works in terms of having a history of frame timings
- Document that _get_presentation_time returns a "system time"
timestamp on a scale with an undefined starting point and that is not
guaranteed to be monotonic, and warn that applications are responsible
for gracefully handling jumps forward or backwards in time.
- Add something like cogl_swap_info_get_output() to query what output
a frame was presented on and remove the _get_refresh_interval() api.

I've made up a few patches to do the initial rename and also to make
some pedantic coding style fixes which I can forward and if you agree
with them they can simply be squashed into your patch before making
further revisions.

kind regards,
- Robert