[Cogl] [PATCH] ensure _SYNC and _COMPLETE events are always dispatched
Robert Bragg
robert at sixbynine.org
Tue Jan 29 11:00:41 PST 2013
On Tue, Jan 29, 2013 at 6:04 PM, Owen Taylor <otaylor at redhat.com> wrote:
> In the commit message you say:
>
>> [...] Previously applications would check
>> for the _SWAP_BUFFERS_EVENT feature to see if they could use
>> cogl_onscreen_add_swap_buffers_callback() as a means to throttle their
>> paint cycle but they would also have to have a fallback that simply used
>> and idle callback for painting. This should simplify the boilerplate
>> required for using Cogl.
>
> But an app shouldn't just draw as fast as possible as soon as they get
> the sync message - Clutter looks at COGL_WINSYS_FEATURE_SWAP_THROTTLE
> and if it's absent will always wait until 1/60th of a second after the
> start of the last frame before starting a new frame. Does the
> "boilerplate" need to include logic like this? should Cogl throttle if
> COGL_WINSYS_FEATURE_SWAP_THROTTLE is missing? is it pathological if
> COGL_WINSYS_FEATURE_SWAP_THROTTLE is missing?
hmm, that winsys feature is currently not handled consistently by Cogl
(e.g. none of the EGL platforms set this feature even though it would
probably be supported for all of them). Possibly we should fix that in
Cogl as a bug, but really these COGL_WINSYS_ features aren't intended
to be public and its only because we are still maintaining various
cogl_clutter_ apis back from the days when cogl was part of clutter
that clutter is able to look at that feature.
I think it could be fine these days for applications to just assume
all cogl window systems support throttling. If there really is a
corner case we need to emulate throttling for then we could move
clutter's fallback throttling down into cogl instead.
>
> I was initially worried that sending the SYNC message always would be
> problematical since an app would care whether SYNC messages are
> throttled or not, but it seems that at least for Clutter it doesn't
> matter - the behavior it wants is that if the swap is throttled, it will
> start drawing immediately unless it is waiting for a SYNC message, so
> an immediate SYNC message will have the same effect.
I think applications should be able to assume that the _SYNC message
are always throttled unless cogl_onscreen_set_throttled (onscreen,
FALSE) was called.
- regards
Robert
>
> - Owen
>
>> This patch removes COGL_FEATURE_ID_FRAME_SYNC feature since we now
>> guarantee that this event is always dispatched.
>>
>> TODO: squash this back into _add_frame_callback patch
>> ---
>> cogl/cogl-context-private.h | 3 +
>> cogl/cogl-context.h | 3 -
>> cogl/cogl-onscreen-private.h | 16 ++++++
>> cogl/cogl-onscreen.c | 76 +++++++++++++++++++++++++
>> cogl/cogl-onscreen.h | 38 ++++++-------
>> cogl/cogl-poll.c | 10 ++++
>> cogl/cogl-sdl.c | 2 +-
>> cogl/cogl-types.h | 3 +
>> cogl/winsys/cogl-winsys-egl-kms.c | 6 +-
>> cogl/winsys/cogl-winsys-glx-feature-functions.h | 3 +-
>> cogl/winsys/cogl-winsys-glx.c | 35 ++++++++----
>> 11 files changed, 156 insertions(+), 39 deletions(-)
>>
>> diff --git a/cogl/cogl-context-private.h b/cogl/cogl-context-private.h
>> index ae8e3dc..fc528e7 100644
>> --- a/cogl/cogl-context-private.h
>> +++ b/cogl/cogl-context-private.h
>> @@ -49,6 +49,7 @@
>> #include "cogl-gpu-info-private.h"
>> #include "cogl-gl-header.h"
>> #include "cogl-framebuffer-private.h"
>> +#include "cogl-onscreen-private.h"
>>
>> typedef struct
>> {
>> @@ -175,6 +176,8 @@ struct _CoglContext
>> GHashTable *swap_callback_closures;
>> int next_swap_callback_id;
>>
>> + CoglOnscreenEventList onscreen_events_queue;
>> +
>> CoglGLES2Context *current_gles2_context;
>> GQueue gles2_context_stack;
>>
>> diff --git a/cogl/cogl-context.h b/cogl/cogl-context.h
>> index 2b4eaf2..1b6b878 100644
>> --- a/cogl/cogl-context.h
>> +++ b/cogl/cogl-context.h
>> @@ -204,8 +204,6 @@ cogl_is_context (void *object);
>> * suported.
>> * @COGL_FEATURE_ID_DEPTH_TEXTURE: Whether #CoglFramebuffer support rendering
>> * the depth buffer to a texture.
>> - * @COGL_FEATURE_ID_FRAME_SYNC: Whether %COGL_FRAME_EVENT_SYNC events
>> - * will be sent to registered #CoglFrameCallback functions.
>> * @COGL_FEATURE_ID_PRESENTATION_TIME: Whether frame presentation
>> * time stamps will be recorded in #CoglFrameInfo objects.
>> *
>> @@ -237,7 +235,6 @@ typedef enum _CoglFeatureID
>> COGL_FEATURE_ID_SWAP_BUFFERS_EVENT,
>> COGL_FEATURE_ID_GLES2_CONTEXT,
>> COGL_FEATURE_ID_DEPTH_TEXTURE,
>> - COGL_FEATURE_ID_FRAME_SYNC,
>> COGL_FEATURE_ID_PRESENTATION_TIME,
>>
>> /*< private >*/
>> diff --git a/cogl/cogl-onscreen-private.h b/cogl/cogl-onscreen-private.h
>> index 7751d80..89efb0e 100644
>> --- a/cogl/cogl-onscreen-private.h
>> +++ b/cogl/cogl-onscreen-private.h
>> @@ -59,6 +59,19 @@ struct _CoglResizeNotifyEntry
>> unsigned int id;
>> };
>>
>> +typedef struct _CoglOnscreenEvent CoglOnscreenEvent;
>> +
>> +COGL_TAILQ_HEAD (CoglOnscreenEventList, CoglOnscreenEvent);
>> +
>> +struct _CoglOnscreenEvent
>> +{
>> + COGL_TAILQ_ENTRY (CoglOnscreenEvent) list_node;
>> +
>> + CoglOnscreen *onscreen;
>> + CoglFrameInfo *info;
>> + CoglFrameEvent type;
>> +};
>> +
>> struct _CoglOnscreen
>> {
>> CoglFramebuffer _parent;
>> @@ -102,4 +115,7 @@ _cogl_onscreen_notify_complete (CoglOnscreen *onscreen, CoglFrameInfo *info);
>> void
>> _cogl_onscreen_notify_resize (CoglOnscreen *onscreen);
>>
>> +void
>> +_cogl_dispatch_onscreen_events (CoglContext *context);
>> +
>> #endif /* __COGL_ONSCREEN_PRIVATE_H */
>> diff --git a/cogl/cogl-onscreen.c b/cogl/cogl-onscreen.c
>> index ba5efbb..3887a1a 100644
>> --- a/cogl/cogl-onscreen.c
>> +++ b/cogl/cogl-onscreen.c
>> @@ -119,6 +119,22 @@ _cogl_onscreen_free (CoglOnscreen *onscreen)
>> g_free (onscreen);
>> }
>>
>> +static void
>> +_cogl_onscreen_queue_event (CoglOnscreen *onscreen,
>> + CoglFrameEvent type,
>> + CoglFrameInfo *info)
>> +{
>> + CoglContext *ctx = COGL_FRAMEBUFFER (onscreen)->context;
>> +
>> + CoglOnscreenEvent *event = g_slice_new (CoglOnscreenEvent);
>> +
>> + event->onscreen = cogl_object_ref (onscreen);
>> + event->info = cogl_object_ref (info);
>> + event->type = type;
>> +
>> + COGL_TAILQ_INSERT_TAIL (&ctx->onscreen_events_queue, event, list_node);
>> +}
>> +
>> void
>> cogl_onscreen_swap_buffers (CoglOnscreen *onscreen)
>> {
>> @@ -140,6 +156,21 @@ cogl_onscreen_swap_buffers (CoglOnscreen *onscreen)
>> COGL_BUFFER_BIT_COLOR |
>> COGL_BUFFER_BIT_DEPTH |
>> COGL_BUFFER_BIT_STENCIL);
>> +
>> + if (!_cogl_winsys_has_feature (COGL_WINSYS_FEATURE_SYNC_AND_COMPLETE_EVENT))
>> + {
>> + CoglFrameInfo *info;
>> +
>> + g_warn_if_fail (onscreen->pending_frame_infos.length == 1);
>> +
>> + info = g_queue_pop_tail (&onscreen->pending_frame_infos);
>> +
>> + _cogl_onscreen_queue_event (onscreen, COGL_FRAME_EVENT_SYNC, info);
>> + _cogl_onscreen_queue_event (onscreen, COGL_FRAME_EVENT_COMPLETE, info);
>> +
>> + cogl_object_unref (info);
>> + }
>> +
>> onscreen->frame_counter++;
>> }
>>
>> @@ -174,6 +205,21 @@ cogl_onscreen_swap_region (CoglOnscreen *onscreen,
>> COGL_BUFFER_BIT_COLOR |
>> COGL_BUFFER_BIT_DEPTH |
>> COGL_BUFFER_BIT_STENCIL);
>> +
>> + if (!_cogl_winsys_has_feature (COGL_WINSYS_FEATURE_SYNC_AND_COMPLETE_EVENT))
>> + {
>> + CoglFrameInfo *info;
>> +
>> + g_warn_if_fail (onscreen->pending_frame_infos.length == 1);
>> +
>> + info = g_queue_pop_tail (&onscreen->pending_frame_infos);
>> +
>> + _cogl_onscreen_queue_event (onscreen, COGL_FRAME_EVENT_SYNC, info);
>> + _cogl_onscreen_queue_event (onscreen, COGL_FRAME_EVENT_COMPLETE, info);
>> +
>> + cogl_object_unref (info);
>> + }
>> +
>> onscreen->frame_counter++;
>> }
>>
>> @@ -422,6 +468,36 @@ cogl_onscreen_hide (CoglOnscreen *onscreen)
>> winsys->onscreen_set_visibility (onscreen, FALSE);
>> }
>> }
>> +void
>> +_cogl_dispatch_onscreen_events (CoglContext *context)
>> +{
>> + CoglOnscreenEvent *event, *tmp;
>> +
>> + COGL_TAILQ_FOREACH_SAFE (event,
>> + &context->onscreen_events_queue,
>> + list_node,
>> + tmp)
>> + {
>> + CoglOnscreen *onscreen = event->onscreen;
>> + CoglFrameInfo *info = event->info;
>> +
>> + switch (event->type)
>> + {
>> + case COGL_FRAME_EVENT_SYNC:
>> + _cogl_onscreen_notify_frame_sync (onscreen, info);
>> + break;
>> + case COGL_FRAME_EVENT_COMPLETE:
>> + _cogl_onscreen_notify_complete (onscreen, info);
>> + break;
>> + }
>> +
>> + cogl_object_unref (onscreen);
>> + cogl_object_unref (info);
>> +
>> + COGL_TAILQ_REMOVE (&context->onscreen_events_queue, event, list_node);
>> + g_slice_free (CoglOnscreenEvent, event);
>> + }
>> +}
>>
>> void
>> _cogl_onscreen_notify_frame_sync (CoglOnscreen *onscreen, CoglFrameInfo *info)
>> diff --git a/cogl/cogl-onscreen.h b/cogl/cogl-onscreen.h
>> index d079cd7..0a59eb8 100644
>> --- a/cogl/cogl-onscreen.h
>> +++ b/cogl/cogl-onscreen.h
>> @@ -409,9 +409,7 @@ cogl_onscreen_swap_region (CoglOnscreen *onscreen,
>> * CoglFrameEvent:
>> * @COGL_FRAME_EVENT_SYNC: Notifies that the system compositor has
>> * acknowledged a frame and is ready for a
>> - * new frame to be created. Only delivered
>> - * if the %COGL_FEATURE_ID_FRAME_SYNC
>> - * feature is supported.
>> + * new frame to be created.
>> * @COGL_FRAME_EVENT_COMPLETE: Notifies that a frame has ended. This
>> * is a good time for applications to
>> * collect statistics about the frame
>> @@ -493,30 +491,26 @@ typedef struct _CoglFrameClosure CoglFrameClosure;
>> * Installs a @callback function that will be called for significant
>> * events relating to the given @onscreen framebuffer.
>> *
>> - * If the %COGL_FEATURE_ID_FRAME_SYNC feature is supported
>> - * then @callback will be used to notify when the system compositor is
>> + * The @callback will be used to notify when the system compositor is
>> * ready for this application to render a new frame. In this case
>> * %COGL_FRAME_EVENT_SYNC will be passed as the event argument to the
>> * given @callback in addition to the #CoglFrameInfo corresponding to
>> * the frame beeing acknowledged by the compositor.
>> *
>> - * If the %COGL_FEATURE_ID_PRESENTATION_EVENTS is available then
>> - * @callback will be called to notify when the frame has become
>> - * visible to the user. In this case %COGL_FRAME_EVENT_PRESENTED will
>> - * be passed as the event argument to the given @callback in addition
>> - * to the #CoglFrameInfo corresponding to the newly presented frame.
>> - *
>> - * We recommend throttling your application according to
>> - * %COGL_FRAME_EVENT_SYNC events whenever the
>> - * %COGL_FEATURE_ID_FRAME_SYNC feature is available. This allows your
>> - * application to avoid being blocked by the driver which will block
>> - * your applications mainloop.
>> - *
>> - * <note>Applications should not make assumptions about the relative
>> - * ordering of different types of frame events. A
>> - * %COGL_FRAME_EVENT_PRESENTED event can be received before a
>> - * %COGL_FRAME_EVENT_SYNC if the compositor is agressively throttling
>> - * your application.</note>
>> + * The @callback will also be called to notify when the frame has
>> + * ended. In this case %COGL_FRAME_EVENT_COMPLETE will be passed as
>> + * the event argument to the given @callback in addition to the
>> + * #CoglFrameInfo corresponding to the newly presented frame. The
>> + * meaning of "ended" here simply means that no more timing
>> + * information will be collected within the corresponding
>> + * #CoglFrameInfo and so this is a good opportunity to analyse the
>> + * given info. It does not necessarily mean that the GPU has finished
>> + * rendering the corresponding frame.
>> + *
>> + * We highly recommend throttling your application according to
>> + * %COGL_FRAME_EVENT_SYNC events so that your application can avoid
>> + * wasting resources, drawing more frames than your system compositor
>> + * can display.
>> *
>> * Return value: a #CoglFrameClosure pointer that can be used to
>> * remove the callback and associated @user_data later.
>> diff --git a/cogl/cogl-poll.c b/cogl/cogl-poll.c
>> index 86be2cf..c6d19fb 100644
>> --- a/cogl/cogl-poll.c
>> +++ b/cogl/cogl-poll.c
>> @@ -44,6 +44,13 @@ cogl_poll_get_info (CoglContext *context,
>> _COGL_RETURN_IF_FAIL (n_poll_fds != NULL);
>> _COGL_RETURN_IF_FAIL (timeout != NULL);
>>
>> + if (!COGL_TAILQ_EMPTY (&context->onscreen_events_queue))
>> + {
>> + *n_poll_fds = 0;
>> + *timeout = 0;
>> + return;
>> + }
>> +
>> winsys = _cogl_context_get_winsys (context);
>>
>> if (winsys->poll_get_info)
>> @@ -70,6 +77,9 @@ cogl_poll_dispatch (CoglContext *context,
>>
>> _COGL_RETURN_IF_FAIL (cogl_is_context (context));
>>
>> + if (!COGL_TAILQ_EMPTY (&context->onscreen_events_queue))
>> + _cogl_dispatch_onscreen_events (context);
>> +
>> winsys = _cogl_context_get_winsys (context);
>>
>> if (winsys->poll_dispatch)
>> diff --git a/cogl/cogl-sdl.c b/cogl/cogl-sdl.c
>> index 03afeb7..b49890f 100644
>> --- a/cogl/cogl-sdl.c
>> +++ b/cogl/cogl-sdl.c
>> @@ -83,5 +83,5 @@ cogl_sdl_handle_event (CoglContext *context, SDL_Event *event)
>> void
>> cogl_sdl_idle (CoglContext *context)
>> {
>> - /* NOP since Cogl doesn't currently need to do anything when idle */
>> + _cogl_dispatch_onscreen_events (context);
>> }
>> diff --git a/cogl/cogl-types.h b/cogl/cogl-types.h
>> index 01ad4c9..e4d8aae 100644
>> --- a/cogl/cogl-types.h
>> +++ b/cogl/cogl-types.h
>> @@ -658,6 +658,9 @@ typedef enum _CoglWinsysFeature
>> /* Avaiable if the age of the back buffer can be queried */
>> COGL_WINSYS_FEATURE_BUFFER_AGE,
>>
>> + /* Avaiable if the winsys directly handles _SYNC and _COMPLETE events */
>> + COGL_WINSYS_FEATURE_SYNC_AND_COMPLETE_EVENT,
>> +
>> COGL_WINSYS_FEATURE_N_FEATURES
>> } CoglWinsysFeature;
>>
>> diff --git a/cogl/winsys/cogl-winsys-egl-kms.c b/cogl/winsys/cogl-winsys-egl-kms.c
>> index a984afb..db2b496 100644
>> --- a/cogl/winsys/cogl-winsys-egl-kms.c
>> +++ b/cogl/winsys/cogl-winsys-egl-kms.c
>> @@ -753,12 +753,14 @@ _cogl_winsys_egl_context_init (CoglContext *context,
>> CoglError **error)
>> {
>> COGL_FLAGS_SET (context->features,
>> - COGL_FEATURE_ID_FRAME_SYNC, TRUE);
>> - COGL_FLAGS_SET (context->features,
>> COGL_FEATURE_ID_SWAP_BUFFERS_EVENT, TRUE);
>> + /* TODO: remove this deprecated feature */
>> COGL_FLAGS_SET (context->winsys_features,
>> COGL_WINSYS_FEATURE_SWAP_BUFFERS_EVENT,
>> TRUE);
>> + COGL_FLAGS_SET (context->winsys_features,
>> + COGL_WINSYS_FEATURE_SYNC_AND_COMPLETE_EVENT,
>> + TRUE);
>>
>> return TRUE;
>> }
>> diff --git a/cogl/winsys/cogl-winsys-glx-feature-functions.h b/cogl/winsys/cogl-winsys-glx-feature-functions.h
>> index d0d2de3..156b58f 100644
>> --- a/cogl/winsys/cogl-winsys-glx-feature-functions.h
>> +++ b/cogl/winsys/cogl-winsys-glx-feature-functions.h
>> @@ -169,7 +169,8 @@ COGL_WINSYS_FEATURE_BEGIN (255, 255,
>> swap_event,
>> "INTEL\0",
>> "swap_event\0",
>> - COGL_WINSYS_FEATURE_SWAP_BUFFERS_EVENT)
>> + COGL_WINSYS_FEATURE_SWAP_BUFFERS_EVENT |
>> + COGL_WINSYS_FEATURE_SYNC_AND_COMPLETE_EVENT)
>> COGL_WINSYS_FEATURE_END ()
>>
>> COGL_WINSYS_FEATURE_BEGIN (255, 255,
>> diff --git a/cogl/winsys/cogl-winsys-glx.c b/cogl/winsys/cogl-winsys-glx.c
>> index 2dbe0d7..0893af7 100644
>> --- a/cogl/winsys/cogl-winsys-glx.c
>> +++ b/cogl/winsys/cogl-winsys-glx.c
>> @@ -301,6 +301,17 @@ _cogl_winsys_get_clock_time (CoglContext *context)
>> }
>>
>> static void
>> +set_sync_pending (CoglOnscreen *onscreen)
>> +{
>> + CoglOnscreenGLX *glx_onscreen = onscreen->winsys;
>> + CoglContext *context = COGL_FRAMEBUFFER (onscreen)->context;
>> + CoglGLXDisplay *glx_display = context->display->winsys;
>> +
>> + glx_display->pending_sync_notify = TRUE;
>> + glx_onscreen->pending_sync_notify = TRUE;
>> +}
>> +
>> +static void
>> set_complete_pending (CoglOnscreen *onscreen)
>> {
>> CoglOnscreenGLX *glx_onscreen = onscreen->winsys;
>> @@ -315,8 +326,6 @@ static void
>> notify_swap_buffers (CoglContext *context, GLXBufferSwapComplete *swap_event)
>> {
>> CoglOnscreen *onscreen = find_onscreen_for_xid (context, (uint32_t)swap_event->drawable);
>> - CoglDisplay *display = context->display;
>> - CoglGLXDisplay *glx_display = display->winsys;
>> CoglOnscreenGLX *glx_onscreen;
>>
>> if (!onscreen)
>> @@ -326,8 +335,7 @@ notify_swap_buffers (CoglContext *context, GLXBufferSwapComplete *swap_event)
>> /* We only want to notify that the swap is complete when the
>> application calls cogl_context_dispatch so instead of immediately
>> notifying we'll set a flag to remember to notify later */
>> - glx_display->pending_sync_notify = TRUE;
>> - glx_onscreen->pending_sync_notify = TRUE;
>> + set_sync_pending (onscreen);
>>
>> if (swap_event->ust != 0)
>> {
>> @@ -716,11 +724,9 @@ update_winsys_features (CoglContext *context, CoglError **error)
>> COGL_FLAGS_SET (context->winsys_features,
>> COGL_WINSYS_FEATURE_SWAP_REGION_THROTTLE, TRUE);
>>
>> - if (_cogl_winsys_has_feature (COGL_WINSYS_FEATURE_SWAP_BUFFERS_EVENT))
>> + if (_cogl_winsys_has_feature (COGL_WINSYS_FEATURE_SYNC_AND_COMPLETE_EVENT))
>> {
>> - COGL_FLAGS_SET (context->features,
>> - COGL_FEATURE_ID_FRAME_SYNC,
>> - TRUE);
>> + /* TODO: remove this deprecated feature */
>> COGL_FLAGS_SET (context->features,
>> COGL_FEATURE_ID_SWAP_BUFFERS_EVENT,
>> TRUE);
>> @@ -1266,7 +1272,7 @@ _cogl_winsys_onscreen_init (CoglOnscreen *onscreen,
>> }
>>
>> #ifdef GLX_INTEL_swap_event
>> - if (_cogl_winsys_has_feature (COGL_WINSYS_FEATURE_SWAP_BUFFERS_EVENT))
>> + if (_cogl_winsys_has_feature (COGL_WINSYS_FEATURE_SYNC_AND_COMPLETE_EVENT))
>> {
>> GLXDrawable drawable =
>> glx_onscreen->glxwin ? glx_onscreen->glxwin : xlib_onscreen->xwin;
>> @@ -1749,7 +1755,16 @@ _cogl_winsys_onscreen_swap_region (CoglOnscreen *onscreen,
>> set_frame_info_output (onscreen, output);
>> }
>>
>> - set_complete_pending (onscreen);
>> + /* XXX: we don't get SwapComplete events based on how we implement
>> + * the _swap_region() API but if cogl-onscreen.c knows we are
>> + * handling _SYNC and _COMPLETE events in the winsys then we need to
>> + * send fake events in this case.
>> + */
>> + if (_cogl_winsys_has_feature (COGL_WINSYS_FEATURE_SYNC_AND_COMPLETE_EVENT))
>> + {
>> + set_sync_pending (onscreen);
>> + set_complete_pending (onscreen);
>> + }
>> }
>>
>> static void
>
>
More information about the Cogl
mailing list