framebuffer: Flush the journal on destruction

Instead of flushing the journal whenever the current framebuffer on a
context is changed it is now flushed whenever the framebuffer is about
to be destroyed instead. To do this it implements a custom unref
function which detects when there is going to be exactly one reference
on the framebuffer and then flushes its journal. The journal now
always has a reference on the framebuffer whenever it is non-empty.
That means the unref will only cause a flush if the only thing keeping
the framebuffer alive is the entries in the journal.

Reviewed-by: Robert Bragg <robert@linux.intel.com>

cogl/cogl-framebuffer-private.h

@@ -364,4 +364,7 @@ _cogl_framebuffer_save_clip_stack (CoglFramebuffer *framebuffer);
 void
 _cogl_framebuffer_restore_clip_stack (CoglFramebuffer *framebuffer);
 
+void
+_cogl_framebuffer_unref (CoglFramebuffer *framebuffer);
+
 #endif /* __COGL_FRAMEBUFFER_PRIVATE_H */

cogl/cogl-framebuffer.c

@@ -113,7 +113,9 @@ extern CoglObjectClass _cogl_onscreen_class;
 
 static void _cogl_offscreen_free (CoglOffscreen *offscreen);
 
-COGL_OBJECT_DEFINE (Offscreen, offscreen);
+COGL_OBJECT_DEFINE_WITH_CODE (Offscreen, offscreen,
+                              _cogl_offscreen_class.virt_unref =
+                              _cogl_framebuffer_unref);
 COGL_OBJECT_DEFINE_DEPRECATED_REF_COUNTING (offscreen);
 
 /* XXX:
@@ -1167,19 +1169,7 @@ _cogl_set_framebuffers (CoglFramebuffer *draw_buffer,
 
   if (current_draw_buffer != draw_buffer ||
       current_read_buffer != read_buffer)
-    {
+    _cogl_set_framebuffers_real (draw_buffer, read_buffer);
-      /* XXX: eventually we want to remove this implicit journal flush
-       * so we can log into the journal beyond framebuffer changes to
-       * support batching scenes that depend on the results of
-       * mid-scene renders to textures. Current will be NULL when the
-       * framebuffer stack is first created so we need to guard
-       * against that here */
-      if (current_draw_buffer)
-        _cogl_framebuffer_flush_journal (current_draw_buffer);
-      if (current_read_buffer)
-        _cogl_framebuffer_flush_journal (current_read_buffer);
-      _cogl_set_framebuffers_real (draw_buffer, read_buffer);
-    }
 }
 
 void
@@ -1292,19 +1282,10 @@ cogl_pop_framebuffer (void)
 
   if (to_pop->draw_buffer != to_restore->draw_buffer ||
       to_pop->read_buffer != to_restore->read_buffer)
-    {
+    notify_buffers_changed (to_pop->draw_buffer,
-      /* XXX: eventually we want to remove this implicit journal flush
+                            to_restore->draw_buffer,
-       * so we can log into the journal beyond framebuffer changes to
+                            to_pop->read_buffer,
-       * support batching scenes that depend on the results of
+                            to_restore->read_buffer);
-       * mid-scene renders to textures. */
-      _cogl_framebuffer_flush_journal (to_pop->draw_buffer);
-      _cogl_framebuffer_flush_journal (to_pop->read_buffer);
-
-      notify_buffers_changed (to_pop->draw_buffer,
-                              to_restore->draw_buffer,
-                              to_pop->read_buffer,
-                              to_restore->read_buffer);
-    }
 
   cogl_object_unref (to_pop->draw_buffer);
   cogl_object_unref (to_pop->read_buffer);
@@ -2407,3 +2388,31 @@ _cogl_framebuffer_restore_clip_stack (CoglFramebuffer *framebuffer)
     framebuffer->context->current_draw_buffer_changes |=
       COGL_FRAMEBUFFER_STATE_CLIP;
 }
+
+void
+_cogl_framebuffer_unref (CoglFramebuffer *framebuffer)
+{
+  /* The journal holds a reference to the framebuffer whenever it is
+     non-empty. Therefore if the journal is non-empty and we will have
+     exactly one reference then we know the journal is the only thing
+     keeping the framebuffer alive. In that case we want to flush the
+     journal and let the framebuffer die. It is fine at this point if
+     flushing the journal causes something else to take a reference to
+     it and it comes back to life */
+  if (framebuffer->journal->entries->len > 0)
+    {
+      unsigned int ref_count = ((CoglObject *) framebuffer)->ref_count;
+
+      /* There should be at least two references - the one we are
+         about to drop and the one held by the journal */
+      if (ref_count < 2)
+        g_warning ("Inconsistent ref count on a framebuffer with journal "
+                   "entries.");
+
+      if (ref_count == 2)
+        _cogl_framebuffer_flush_journal (framebuffer);
+    }
+
+  /* Chain-up */
+  _cogl_object_default_unref (framebuffer);
+}

cogl/cogl-journal-private.h

@@ -34,6 +34,14 @@ typedef struct _CoglJournal
 {
   CoglObject _parent;
 
+  /* A pointer the framebuffer that is using this journal. This is
+     only valid when the journal is not empty. It *does* take a
+     reference on the framebuffer. Although this creates a circular
+     reference, the framebuffer has special code to handle the case
+     where the journal is the only thing holding a reference and it
+     will cause the journal to flush */
+  CoglFramebuffer *framebuffer;
+
   GArray *entries;
   GArray *vertices;
   size_t needed_vbo_len;

cogl/cogl-journal.c

@@ -1261,6 +1261,14 @@ _cogl_journal_discard (CoglJournal *journal)
   g_array_set_size (journal->vertices, 0);
   journal->needed_vbo_len = 0;
   journal->fast_read_pixel_count = 0;
+
+  /* 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;
+    }
 }
 
 /* Note: A return value of FALSE doesn't mean 'no' it means
@@ -1353,6 +1361,11 @@ _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
@@ -1496,6 +1509,14 @@ _cogl_journal_log_quad (CoglJournal  *journal,
 
   COGL_TIMER_START (_cogl_uprof_context, log_timer);
 
+  /* 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 */
+  if (journal->vertices->len == 0)
+    journal->framebuffer = cogl_object_ref (cogl_get_draw_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
      only store two vertices per quad and expand it to four while
@@ -1573,7 +1594,7 @@ _cogl_journal_log_quad (CoglJournal  *journal,
 
   entry->pipeline = _cogl_pipeline_journal_ref (source);
 
-  clip_stack = _cogl_framebuffer_get_clip_stack (cogl_get_draw_framebuffer ());
+  clip_stack = _cogl_framebuffer_get_clip_stack (journal->framebuffer);
   entry->clip_stack = _cogl_clip_stack_ref (clip_stack);
 
   if (G_UNLIKELY (source != pipeline))
@@ -1583,7 +1604,7 @@ _cogl_journal_log_quad (CoglJournal  *journal,
 
   _cogl_pipeline_foreach_layer_internal (pipeline,
                                          add_framebuffer_deps_cb,
-                                         cogl_get_draw_framebuffer ());
+                                         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
@@ -1591,13 +1612,14 @@ _cogl_journal_log_quad (CoglJournal  *journal,
    * 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 (cogl_get_draw_framebuffer ());
+    _cogl_framebuffer_flush_journal (journal->framebuffer);
 
   COGL_TIMER_STOP (_cogl_uprof_context, log_timer);
 }
 
 static void
-entry_to_screen_polygon (const CoglJournalEntry *entry,
+entry_to_screen_polygon (CoglFramebuffer *framebuffer,
+                         const CoglJournalEntry *entry,
                          float *vertices,
                          float *poly)
 {
@@ -1642,7 +1664,7 @@ entry_to_screen_polygon (const CoglJournalEntry *entry,
                                 4 /* n_points */);
 
   projection_stack =
-    _cogl_framebuffer_get_projection_stack (cogl_get_draw_framebuffer ());
+    _cogl_framebuffer_get_projection_stack (framebuffer);
   _cogl_matrix_stack_get (projection_stack, &projection);
 
   cogl_matrix_project_points (&projection,
@@ -1654,7 +1676,7 @@ entry_to_screen_polygon (const CoglJournalEntry *entry,
                               poly, /* points_out */
                               4 /* n_points */);
 
-  cogl_framebuffer_get_viewport4fv (cogl_get_draw_framebuffer (), viewport);
+  cogl_framebuffer_get_viewport4fv (framebuffer, viewport);
 
 /* Scale from OpenGL normalized device coordinates (ranging from -1 to 1)
  * to Cogl window/framebuffer coordinates (ranging from 0 to buffer-size) with
@@ -1688,7 +1710,8 @@ entry_to_screen_polygon (const CoglJournalEntry *entry,
 }
 
 static gboolean
-try_checking_point_hits_entry_after_clipping (CoglJournalEntry *entry,
+try_checking_point_hits_entry_after_clipping (CoglFramebuffer *framebuffer,
+                                              CoglJournalEntry *entry,
                                               float *vertices,
                                               float x,
                                               float y,
@@ -1750,7 +1773,7 @@ try_checking_point_hits_entry_after_clipping (CoglJournalEntry *entry,
         return FALSE;
 
       software_clip_entry (entry, vertices, &clip_bounds);
-      entry_to_screen_polygon (entry, vertices, poly);
+      entry_to_screen_polygon (framebuffer, entry, vertices, poly);
 
       *hit = _cogl_util_point_in_screen_poly (x, y, poly, sizeof (float) * 4, 4);
       return TRUE;
@@ -1803,22 +1826,20 @@ _cogl_journal_try_read_pixel (CoglJournal *journal,
                                                 entry->array_offset);
       float *vertices = (float *)color + 1;
       float poly[16];
+      CoglFramebuffer *framebuffer = journal->framebuffer;
 
-      entry_to_screen_polygon (entry, vertices, poly);
+      entry_to_screen_polygon (framebuffer, entry, vertices, poly);
 
       if (!_cogl_util_point_in_screen_poly (x, y, poly, sizeof (float) * 4, 4))
         continue;
 
-      /* FIXME: the journal should have a back pointer to the
-       * associated framebuffer, because it should be possible to read
-       * a pixel from arbitrary framebuffers without needing to
-       * internally call _cogl_push/pop_framebuffer.
-       */
       if (entry->clip_stack)
         {
           gboolean hit;
 
-          if (!try_checking_point_hits_entry_after_clipping (entry, vertices,
+          if (!try_checking_point_hits_entry_after_clipping (framebuffer,
+                                                             entry,
+                                                             vertices,
                                                              x, y, &hit))
             return FALSE; /* hit couldn't be determined */
 

cogl/cogl-onscreen.c

@@ -34,7 +34,9 @@
 
 static void _cogl_onscreen_free (CoglOnscreen *onscreen);
 
-COGL_OBJECT_INTERNAL_DEFINE (Onscreen, onscreen);
+COGL_OBJECT_INTERNAL_DEFINE_WITH_CODE (Onscreen, onscreen,
+                                       _cogl_onscreen_class.virt_unref =
+                                       _cogl_framebuffer_unref);
 
 static void
 _cogl_onscreen_init_from_template (CoglOnscreen *onscreen,