[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