[Cogl] [PATCH] Add cogl_onscreen_add_swap_complete_callback() api

[Owen Taylor]

On Thu, 2013-01-24 at 21:28 +0000, Robert Bragg wrote:
> On Thu, Jan 24, 2013 at 7:11 PM, Owen Taylor <otaylor at redhat.com> wrote:
> > At this point, I lose track of what's going on in
> >
> >  wip/rib/frame-synchronization
> >
> > This patch does a bunch of different things.
> >
> >  * Reworks the callback structure - I think with the later patches
> >    on top in your branch the final API is fine,  but code is removed
> >    from the GLX code that I believe is necessary to dispatch the right
> >    event at the right time, and I don't see it added back later.
> >
> >  * Removes a public frame history indexed by frame counter.
> >    If you dislike it, I don't need it, could be added back later.
> >
> >  * Removes begin_frame() and the idea that there is a current frame
> >    while drawing and any ability to access the FrameInfo until you
> >    get the first event. I disagree with these changes. It means that
> >    any history that an application keeps is going to have a strange
> >    structure where frames that have been drawn don't appear in the
> >    history until later.
> >
> >  * Removes the frame time. Don't really care - I think it's a useful
> >    concept, but if you want COGL really to be minimal and imply nothing
> >    about the structure of the application, I can see where it could
> >    be considered an extraneous
> >
> >  * Replaces the refresh interval in the FrameInfo with the Output - with
> >    your later changes to add the refresh interval back in addition,
> >    this is OK.
> >
> > My plan of attack here is to rebase your patches that I agree
> > with before this patch, and then rework this patch to go directly to
> > your final API and not pull in the changes that I disagree with, then we
> > can discuss those more separately :-)
> 
> hmm, I guess this is the problem of having a long Christmas holiday
> break since starting our discussions.
> 
> In particular your concern about not being able to access the
> FrameInfo mid-frame was something you raised before and I tried to
> re-assure you that this approach does not preclude us accessing the
> FrameInfo mid-frame. It is only a technical detail with the current
> implementation that means the FrameInfo is only allocated at the point
> of swap_buffers() and it's done that way because in practice we don't
> currently need the FrameInfo earlier. Conceptually though the
> frame-info can be considered to exist for the full duration of the
> frame while commands are being submitted etc and it should be trivial
> to change the internals so that this is the case. I don't think any
> _begin_frame() api is required to address this concern. If it would
> help I can write a patch to clarify what I mean, but basically I think
> it just means allocating the first CoglFrameInfo when the framebuffer
> itself is allocated, and then all subsequent FrameInfos are allocated
> in the _swap_buffers() but instead of allocating the FrameInfo for the
> current frame (as is done with my current patch) it would allocate the
> info for the next frame instead.

I know that begin_frame() isn't necessary, since I had it that way in an
earlier version of my patch. The main reason I introduced it was that 
I didn't want Cogl and GTK+ to have different semantics for what the
value of the frame counter was "between frames". I also introduced it
as a convenient and natural way of handling the frame time (see below).

If you are willing to consider a cogl_onscreen_get_current_frame_info(),
then that could mostly replace what I have in other aspects and is good
enough.

> regarding the frame time, this is something we went back and forth
> over a few times, and to confuse matters I also changed my mind over
> this, and then back again, at one point. The rationale for dropping
> this wasn't really about minimalism, but in the end we concluded that
> CoglOnscreen is not the right place conceptually to be trying to track
> that frame timestamp, and even in practice it doesn't look like it
> would be a good place to store the data.
> 
> I think you had previously suggested that associating the stamp with
> the FrameInfo made it easy to share between the different
> toolkit/application components that need access that frame timestamp,
> but since since there can be numerous framebuffers involved in
> rendering a frame, including multiple onscreen framebuffers
> potentially, it wouldn't seem like a good/convenient way to share the
> data in clutter for example. In clutter it seems like the stage would
> be a more appropriate place to share that timestamp if sharing is
> required. Related to this, I think that at some point it would be good
> for Clutter to introduce the idea of a PaintContext and if it did
> that, that may be an even more appropriate place to share the frame
> time.

The point wasn't to get the frame time associated with the CoglOnscreen,
it was to get it stored in the FrameInfo, which is convenient if you are
using the FrameInfo as a repository for the common timing information
you need about the frame. My argument was that frame time is, if not a
universal idea, a very general idea. There was no requirement in the API
to provide it if it didn't make sense for a particular user.

But I don't need this for what I'm doing now, and the absence can be
worked around for future usage.

> I guess I was kind of hoping we were already pretty close to having
> something that we could land (mostly just a case of squashing commits
> with some relatively minor cleanups/fixes) but either way I guess it
> shouldn't be too much trouble if you do as you say and squash the bits
> you are happy with (at least that way I guess you don't end up with
> attribution for stuff you disagree with :-)) and then we can hopefully
> resolve the remaining issues from there.
>
> Depending on how quickly you get started on squashing work, hopefully
> you will see my latest wip/rib/frame-synchronization which is based on
> the cogl-1.14 branch including the change to emitting _COMPLETE events
> instead of _PRESENTED events.

I don't think there's necessarily a big delta between your branch and
what I completely or mostly agree with, I just found it fairly difficult
to work with the branch with its current history. I suppose a different
approach would have just been to review the diff between my branch and
your branch as one gigantic patch, but I ended up disassembling your
branch into squashed logical chunks instead.

I'll push a new version of my branch and follow up with:

 A) Annotated changelog
 B) Annotated residual differences

- Owen