[Cogl] [PATCH 1/2] journal: Always keep a pointer back to the framebuffer

Robert Bragg robert at sixbynine.org
Fri Mar 16 11:29:15 PDT 2012


This looks good to me and sets things up nicely for the patch I'm looking
at to add cogl_framebuffer_draw_rectangle/textured_rectangle... functions
to replace
cogl_rectangle() etc for the 2.0 api.

thanks,
- Robert

On Fri, Mar 16, 2012 at 5:43 PM, Neil Roberts <neil at linux.intel.com> wrote:

> Previously when adding a quad to the journal it would assume the
> journal belongs to the framebuffer at the top of the framebuffer stack
> and store a reference to that. We eventually want to get rid of the
> framebuffer stack so we should avoid using it here. The journal now
> takes a pointer back to the framebuffer in its constructor and it
> always retains the pointer. As was done previously, the journal still
> does not take a reference on the framebuffer unless it is non-empty so
> it does not create a permanent circular reference.
> ---
>  cogl/cogl-attribute.c       |    2 +-
>  cogl/cogl-framebuffer.c     |    4 +-
>  cogl/cogl-journal-private.h |    5 +--
>  cogl/cogl-journal.c         |   70
> +++++++++++++++++++-----------------------
>  4 files changed, 37 insertions(+), 44 deletions(-)
>
> diff --git a/cogl/cogl-attribute.c b/cogl/cogl-attribute.c
> index e9f6cef..3f1ae71 100644
> --- a/cogl/cogl-attribute.c
> +++ b/cogl/cogl-attribute.c
> @@ -562,7 +562,7 @@ _cogl_flush_attributes_state (CoglFramebuffer
> *framebuffer,
>   CoglContext *ctx = framebuffer->context;
>
>   if (!(flags & COGL_DRAW_SKIP_JOURNAL_FLUSH))
> -    _cogl_journal_flush (framebuffer->journal, framebuffer);
> +    _cogl_journal_flush (framebuffer->journal);
>
>   layers_state.unit = 0;
>   layers_state.options.flags = 0;
> diff --git a/cogl/cogl-framebuffer.c b/cogl/cogl-framebuffer.c
> index 799f406..ab7f0aa 100644
> --- a/cogl/cogl-framebuffer.c
> +++ b/cogl/cogl-framebuffer.c
> @@ -189,7 +189,7 @@ _cogl_framebuffer_init (CoglFramebuffer *framebuffer,
>   /* Initialise the clip stack */
>   _cogl_clip_state_init (&framebuffer->clip_state);
>
> -  framebuffer->journal = _cogl_journal_new ();
> +  framebuffer->journal = _cogl_journal_new (framebuffer);
>
>   /* Ensure we know the framebuffer->clear_color* members can't be
>    * referenced for our fast-path read-pixel optimization (see
> @@ -633,7 +633,7 @@ _cogl_framebuffer_remove_all_dependencies
> (CoglFramebuffer *framebuffer)
>  void
>  _cogl_framebuffer_flush_journal (CoglFramebuffer *framebuffer)
>  {
> -  _cogl_journal_flush (framebuffer->journal, framebuffer);
> +  _cogl_journal_flush (framebuffer->journal);
>  }
>
>  void
> diff --git a/cogl/cogl-journal-private.h b/cogl/cogl-journal-private.h
> index 6e9764a..90363cd 100644
> --- a/cogl/cogl-journal-private.h
> +++ b/cogl/cogl-journal-private.h
> @@ -77,7 +77,7 @@ typedef struct _CoglJournalEntry
>  } CoglJournalEntry;
>
>  CoglJournal *
> -_cogl_journal_new (void);
> +_cogl_journal_new (CoglFramebuffer *framebuffer);
>
>  void
>  _cogl_journal_log_quad (CoglJournal  *journal,
> @@ -89,8 +89,7 @@ _cogl_journal_log_quad (CoglJournal  *journal,
>                         unsigned int  tex_coords_len);
>
>  void
> -_cogl_journal_flush (CoglJournal *journal,
> -                     CoglFramebuffer *framebuffer);
> +_cogl_journal_flush (CoglJournal *journal);
>
>  void
>  _cogl_journal_discard (CoglJournal *journal);
> diff --git a/cogl/cogl-journal.c b/cogl/cogl-journal.c
> index b8191c0..59a9fd8 100644
> --- a/cogl/cogl-journal.c
> +++ b/cogl/cogl-journal.c
> @@ -96,8 +96,6 @@ typedef struct _CoglJournalFlushState
>  {
>   CoglJournal         *journal;
>
> -  CoglFramebuffer     *framebuffer;
> -
>   CoglAttributeBuffer *attribute_buffer;
>   GArray              *attributes;
>   int                  current_attribute;
> @@ -143,10 +141,18 @@ _cogl_journal_free (CoglJournal *journal)
>  }
>
>  CoglJournal *
> -_cogl_journal_new (void)
> +_cogl_journal_new (CoglFramebuffer *framebuffer)
>  {
>   CoglJournal *journal = g_slice_new0 (CoglJournal);
>
> +  /* The journal keeps a pointer back to the framebuffer because there
> +     is effectively a 1:1 mapping between journals and framebuffers.
> +     However, to avoid a circular reference the journal doesn't take a
> +     reference unless it is non-empty. The framebuffer has a special
> +     unref implementation to ensure that the journal is flushed when
> +     the journal is the only thing keeping it alive */
> +  journal->framebuffer = framebuffer;
> +
>   journal->entries = g_array_new (FALSE, FALSE, sizeof (CoglJournalEntry));
>   journal->vertices = g_array_new (FALSE, FALSE, sizeof (float));
>
> @@ -270,6 +276,7 @@ _cogl_journal_flush_modelview_and_entries
> (CoglJournalEntry *batch_start,
>                                            void             *data)
>  {
>   CoglJournalFlushState *state = data;
> +  CoglFramebuffer *framebuffer = state->journal->framebuffer;
>   CoglAttribute **attributes;
>   CoglDrawFlags draw_flags = (COGL_DRAW_SKIP_JOURNAL_FLUSH |
>                               COGL_DRAW_SKIP_PIPELINE_VALIDATION |
> @@ -305,7 +312,7 @@ _cogl_journal_flush_modelview_and_entries
> (CoglJournalEntry *batch_start,
>   if (ctx->driver == COGL_DRIVER_GL)
>     {
>       /* XXX: it's rather evil that we sneak in the GL_QUADS enum here...
> */
> -      _cogl_framebuffer_draw_attributes (state->framebuffer,
> +      _cogl_framebuffer_draw_attributes (framebuffer,
>                                          state->pipeline,
>                                          GL_QUADS,
>                                          state->current_vertex, batch_len
> * 4,
> @@ -320,7 +327,7 @@ _cogl_journal_flush_modelview_and_entries
> (CoglJournalEntry *batch_start,
>         {
>           CoglVerticesMode mode = COGL_VERTICES_MODE_TRIANGLES;
>           int first_vertex = state->current_vertex * 6 / 4;
> -          _cogl_framebuffer_draw_indexed_attributes (state->framebuffer,
> +          _cogl_framebuffer_draw_indexed_attributes (framebuffer,
>                                                      state->pipeline,
>                                                      mode,
>                                                      first_vertex,
> @@ -332,7 +339,7 @@ _cogl_journal_flush_modelview_and_entries
> (CoglJournalEntry *batch_start,
>         }
>       else
>         {
> -          _cogl_framebuffer_draw_attributes (state->framebuffer,
> +          _cogl_framebuffer_draw_attributes (framebuffer,
>                                              state->pipeline,
>
>  COGL_VERTICES_MODE_TRIANGLE_FAN,
>                                              state->current_vertex, 4,
> @@ -379,7 +386,7 @@ _cogl_journal_flush_modelview_and_entries
> (CoglJournalEntry *batch_start,
>
>       loop_attributes[0] = attributes[0]; /* we just want the position */
>       for (i = 0; i < batch_len; i++)
> -        _cogl_framebuffer_draw_attributes (state->framebuffer,
> +        _cogl_framebuffer_draw_attributes (framebuffer,
>                                            outline,
>                                            COGL_VERTICES_MODE_LINE_LOOP,
>                                            4 * i + state->current_vertex,
> 4,
> @@ -576,7 +583,7 @@ _cogl_journal_flush_vbo_offsets_and_entries
> (CoglJournalEntry *batch_start,
>                                              void             *data)
>  {
>   CoglJournalFlushState   *state = data;
> -  CoglContext             *ctx = state->framebuffer->context;
> +  CoglContext             *ctx = state->journal->framebuffer->context;
>   gsize                    stride;
>   int                      i;
>   CoglAttribute          **attribute_entry;
> @@ -711,7 +718,7 @@ _cogl_journal_flush_clip_stacks_and_entries
> (CoglJournalEntry *batch_start,
>   if (G_UNLIKELY (COGL_DEBUG_ENABLED (COGL_DEBUG_BATCHING)))
>     g_print ("BATCHING:  clip stack batch len = %d\n", batch_len);
>
> -  _cogl_clip_stack_flush (batch_start->clip_stack, state->framebuffer);
> +  _cogl_clip_stack_flush (batch_start->clip_stack,
> state->journal->framebuffer);
>
>   /* XXX: Because we are manually flushing clip state here we need to
>    * make sure that the clip state gets updated the next time we flush
> @@ -1254,6 +1261,9 @@ _cogl_journal_discard (CoglJournal *journal)
>  {
>   int i;
>
> +  if (journal->entries->len <= 0)
> +    return;
> +
>   for (i = 0; i < journal->entries->len; i++)
>     {
>       CoglJournalEntry *entry =
> @@ -1269,11 +1279,7 @@ _cogl_journal_discard (CoglJournal *journal)
>
>   /* The journal only holds a reference to the framebuffer while the
>      journal is not empty */
> -  if (journal->framebuffer)
> -    {
> -      cogl_object_unref (journal->framebuffer);
> -      journal->framebuffer = NULL;
> -    }
> +  cogl_object_unref (journal->framebuffer);
>  }
>
>  /* Note: A return value of FALSE doesn't mean 'no' it means
> @@ -1349,8 +1355,7 @@ _cogl_journal_all_entries_within_bounds (CoglJournal
> *journal,
>  * is undefined.
>  */
>  void
> -_cogl_journal_flush (CoglJournal *journal,
> -                     CoglFramebuffer *framebuffer)
> +_cogl_journal_flush (CoglJournal *journal)
>  {
>   CoglJournalFlushState state;
>   int                   i;
> @@ -1366,31 +1371,25 @@ _cogl_journal_flush (CoglJournal *journal,
>   if (journal->entries->len == 0)
>     return;
>
> -  /* Something has gone wrong if we're using a different framebuffer
> -     to flush than the one that was current when the entries were
> -     added */
> -  _COGL_RETURN_IF_FAIL (framebuffer == journal->framebuffer);
> -
>   /* The entries in this journal may depend on images in other
>    * framebuffers which may require that we flush the journals
>    * associated with those framebuffers before we can flush
>    * this journal... */
> -  _cogl_framebuffer_flush_dependency_journals (framebuffer);
> +  _cogl_framebuffer_flush_dependency_journals (journal->framebuffer);
>
>   /* Note: we start the timer after flushing dependency journals so
>    * that the timer isn't started recursively. */
>   COGL_TIMER_START (_cogl_uprof_context, flush_timer);
>
> -  state.framebuffer = framebuffer;
> -  cogl_push_framebuffer (framebuffer);
> +  cogl_push_framebuffer (journal->framebuffer);
>
>   if (G_UNLIKELY (COGL_DEBUG_ENABLED (COGL_DEBUG_BATCHING)))
>     g_print ("BATCHING: journal len = %d\n", journal->entries->len);
>
>   /* NB: the journal deals with flushing the modelview stack and clip
>      state manually */
> -  _cogl_framebuffer_flush_state (framebuffer,
> -                                 framebuffer,
> +  _cogl_framebuffer_flush_state (journal->framebuffer,
> +                                 journal->framebuffer,
>                                  COGL_FRAMEBUFFER_STATE_ALL &
>                                  ~(COGL_FRAMEBUFFER_STATE_MODELVIEW |
>                                    COGL_FRAMEBUFFER_STATE_CLIP));
> @@ -1399,9 +1398,11 @@ _cogl_journal_flush (CoglJournal *journal,
>
>   state.attributes = ctx->journal_flush_attributes_array;
>
> -  modelview_stack = _cogl_framebuffer_get_modelview_stack (framebuffer);
> +  modelview_stack =
> +    _cogl_framebuffer_get_modelview_stack (journal->framebuffer);
>   state.modelview_stack = modelview_stack;
> -  state.projection_stack = _cogl_framebuffer_get_projection_stack
> (framebuffer);
> +  state.projection_stack =
> +    _cogl_framebuffer_get_projection_stack (journal->framebuffer);
>
>   if (G_UNLIKELY ((COGL_DEBUG_ENABLED (COGL_DEBUG_DISABLE_SOFTWARE_CLIP))
> == 0))
>     {
> @@ -1516,11 +1517,9 @@ _cogl_journal_log_quad (CoglJournal  *journal,
>
>   /* If the framebuffer was previously empty then we'll take a
>      reference to the current framebuffer. This reference will be
> -     removed when the journal is flushed. FIXME: This should probably
> -     be being passed a pointer to the framebuffer from further up so
> -     that we don't have to rely on the global framebuffer stack */
> +     removed when the journal is flushed */
>   if (journal->vertices->len == 0)
> -    journal->framebuffer = cogl_object_ref (cogl_get_draw_framebuffer ());
> +    cogl_object_ref (journal->framebuffer);
>
>   /* The vertex data is logged into a separate array. The data needs
>      to be copied into a vertex array before it's given to GL so we
> @@ -1611,13 +1610,8 @@ _cogl_journal_log_quad (CoglJournal  *journal,
>                                          add_framebuffer_deps_cb,
>                                          journal->framebuffer);
>
> -  /* XXX: It doesn't feel very nice that in this case we just assume
> -   * that the journal is associated with the current framebuffer. I
> -   * think a journal->framebuffer reference would seem nicer here but
> -   * the reason we don't have that currently is that it would
> -   * introduce a circular reference. */
>   if (G_UNLIKELY (COGL_DEBUG_ENABLED (COGL_DEBUG_DISABLE_BATCHING)))
> -    _cogl_framebuffer_flush_journal (journal->framebuffer);
> +    _cogl_journal_flush (journal);
>
>   COGL_TIMER_STOP (_cogl_uprof_context, log_timer);
>  }
> --
> 1.7.3.16.g9464b
>
> _______________________________________________
> Cogl mailing list
> Cogl at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/cogl
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/cogl/attachments/20120316/b149907d/attachment.htm>


More information about the Cogl mailing list