[Cogl] [PATCH] poll: Add general way to hook into mainloop without fd

[Neil Roberts]

Robert Bragg <robert at sixbynine.org> writes:

> -typedef CoglBool (*CoglPollCheckCallback) (void *user_data);
> +typedef CoglBool (*CoglPollCheckCallback) (void *user_data, int64_t
>                                                              *timeout);

Would it be simpler if instead of returning a CoglBool here it would
just directly return the timeout value? Then it could return 0 to mean
‘immediately ready’ as the equivalent of returning TRUE.

> @@ -66,14 +66,13 @@ cogl_poll_renderer_get_info (CoglRenderer *renderer,
>    for (l = renderer->poll_sources; l; l = l->next)
>      {
>        CoglPollSource *source = l->data;
> -      if (source->check && source->check (source->user_data))
> +      if (source->check && source->check (source->user_data, timeout))
>          {
>            *timeout = 0;

I think it would be better if it passed a new local variable to the
check function instead of passing the variable for the current minimum
timeout. Otherwise the callback is going to have to do something like
‘*timeout = MIN (the_time_we_want, *timeout)’, but also check if it's -1
and so on. It'd be nicer to do that outside of the callback in this loop
instead and also it would be more like what people who are used to
GMainLoop would expect.

> @@ -91,8 +90,21 @@ cogl_poll_renderer_dispatch (CoglRenderer *renderer,
>    for (l = renderer->poll_sources; l; l = l->next)
>      {
>        CoglPollSource *source = l->data;
> +      if (source->fd == -1)
> +        {
> +          source->dispatch (source->user_data);
> +          continue;
> +        }
> +    }

This doesn't look right, or at least it's quite different from how
GMainLoop works. Whenever cogl_poll_renderer_dispatch is called it's
going to dispatch all of the sources, regardless of whether the timeout
is actually hit. I notice that the second loop also has the same problem
because it doesn't check the revents field before dispatching.

In GMainLoop there is a separate ‘check’ vfunc which is called after the
poll is complete so the source can check if the timeout is hit. The
dispatch function will only be called if the check function returns TRUE
or the revents field for any of the source's fds are non-zero.

Maybe we could say that the dispatch function is ‘check_and_dispatch’ so
the callback is expected to check whether it should actually dispatch or
not. I'm not really sure what the point of separating the two steps
would be. In that case the check+dispatch function would need to know
the value for its revents field to know if the fd is ready.

I think we should change the names a bit or at least document it to make
it less confusing for people who are used to GMainLoop. The ‘check’
callback should probably be called ‘prepare’ and the dispatch can be
‘check_and_dispatch’ if we go with that idea.

Is there any reason why the sources with fd==-1 are checked in a
separate loop? Couldn't it just be in the same loop as the the one which
checks sources that have an fd?

Could we make the idle functions work in terms of
_cogl_poll_renderer_add_source? We could make add_idle just add a source
with a check function that always returns TRUE. That might simplify the
code a bit.

Regards,
- Neil
---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.