[Cogl] [PATCH] Rework sdl integration api

[Robert Bragg]

On Wed, Apr 11, 2012 at 5:44 PM, Neil Roberts <neil at linux.intel.com> wrote:
> Robert Bragg wrote:
>
>> + * while (!data.quit)
>> + *   {
>> + *     cogl_sdl_idle (data.ctx);
>> + *     if (SDL_WaitEvent (&event))
>> + *       do
>> + *         {
>> + *           handle_event (&data, &event);
>> + *           cogl_sdl_handle_event (data.ctx, &event);
>> + *         }
>> + *       while (SDL_PollEvent (&event));
>> + *     else
>> + *       {
>> + *         g_critical ("Error waiting for SDL events");
>> + *         break;
>> + *       }
>> + *
>> + *      render (&data);
>> + *   }
>
> I'm not sure if this loop makes much sense. This will make the loop
> always block and wait for an event and then render after any event is
> received. Was this meant to be an example of an application that renders
> continuously? In that case the application effectively never goes idle,
> so it should never call cogl_sdl_idle, right?

Yeah I was thinking along the lines of an app rendering at full pelt
(maybe due to animations) but I suppose in that case such an
application would need to track when animations are running to avoid
calling SDL_WaitEvent during that time, so e.g. using something like:

if (data.animating || SDL_WaitEvent (&event))

but then we'd also need to only call cogl_sdl_idle() if !data.animating.

The listed loop is basically just based on the way cogl-sdl-hello
currently works but dispatching for every event. It assumes we are
idle when we know we've just handled all queued events got via
SDL_PollEvent. Technically though we don't know what events may have
been generated when rendering here so potentially there are more
events queued.

>
> For the cogl-sdl-hello example where it really does block and only does
> work in response to events, I think the loop should be more like this so
> that it only calls cogl_sdl_idle when it knows it will block:
>
>  while (!data.quit)
>    {
>      /* If there are currently no events then we are going to block
>       * so we should give Cogl a chance to do any idle work and then
>       * use SDL_WaitEvent to do the actual block */
>      if (!SDL_PollEvent (&event))
>        {
>          cogl_sdl_idle (ctx);
>          if (!SDL_WaitEvent (&event))
>            {
>              fprintf (stderr, "Error waiting for SDL events");
>              return 1;
>            }
>        }
>
>      handle_event (&data, &event);
>      cogl_sdl_handle_event (ctx, &event);
>    }

This fixes the problem of not checking data.quit after every handled
event which is good though I'm not sure that painting directly in
response to individual events is a good thing to recommend even though
that's what we currently do in cogl-sdl-hello. Processing all queued
events before redrawing could avoid DOSing the application with
redraws due to input events that it might never be able to catch up
with.

Taking another pass at this and trying to considering what you said, I
came up with this:

  data.redraw_queued = TRUE;
  while (!data.quit)
    {
      while (!data.quit)
        {
          int status = SDL_PollEvent (&event);
          if (!status && data.redraw_queued)
            break;
          else if (!status)
            {
              cogl_sdl_idle (ctx);
              if (!SDL_WaitEvent (&event))
                {
                  fprintf (stderr, "Error waiting for SDL events");
                  return 1;
                }
            }

          handle_event (&data, &event);
          cogl_sdl_handle_event (ctx, &event);
        }

      data.redraw_queued = redraw (&data);
    }

This example considers an application that may be handling animations
(so not purely event based), it also processes the events in batches
so input events can potentially be squashed and avoids being
overwhelmed by events triggering redraws that it can't keep up with.

I've modified cogl-sdl-hello to work like this to test it out and can
send out a separate patch.

>
>> +void
>> +cogl_sdl_handle_event (CoglContext *context, SDL_Event *event)
>> +{
> ...
>> +  if (G_UNLIKELY (renderer->sdl_event_type_set == FALSE))
>> +    g_critical ("cogl_sdl_renderer_set_event_type() ...")
>> +                "must be called during initialization");
>
> I think it would be better if this check was done in the implementation
> of CoglRenderer.connect instead because you also need have set the event
> type before that point anyway. Otherwise the screen would end up flooded
> with critical warnings because cogl_sdl_handle_event should be called
> very frequently.

Ah, yep that would be better. With a previous version of this patch
the event type was set after the context was created so this was the
most obvious point to put the check.

>
> Before, I think we agreed that if you are using the GSource and the
> GMainLoop the public API would let you be agnostic of the winsys. For
> that to work would the GSource somehow have to internally set the SDL
> event? I think it would be reasonable to steal any old SDL event number
> in that case because if you are using the GLib main loop then you
> probably aren't using any SDL-specific API.

Although using the GSource api is currently the only way to be
agnostic of the backend it doesn't necessarily work with all backends
(e.g. the egl android backend can't be used this way). Until recently
I was thinking the SDL backend was another case that we couldn't make
work with the GSource api, but since we were chatting earlier we did
figure that this should be doable.

Just to re-cap the what we talked about earlier on the mailing list:
The idea is that we would run a separate thread dedicated to running
an SDL event loop that would simply sit in a loop calling
SDL_WaitEvent() and for each event it would write to a pipe that could
be attached to a GSource that the mainloop thread can respond too.
Because in this setup cogl would be consuming all the SDL events we
can use whatever SDL user event type we like. If the application
needed to manually handle SDL events we could add some cogl_sdl_ api
for disabling automatic event retrieval by Cogl (similar to what we
have for x11) and also let the application specify the SDL user event
reserved for Cogl.

Out of curiosity, I wonder if you think this setup would work on the
Palm Pre which I recall you saying has some funky interactions between
SDL and glib?

I think maybe even if we want to do this we can keep this as a separate problem.

This does highlight though that Cogl doesn't currently have a way of
marking the SDL and EGL android backends as ones that should never be
probed for automatically, they should only be used if the user
explicitly requests them via api, but also this seems like it can be
addressed in a separate patch too.

regards,
- Robert

>
> Regards,
> - Neil