clutter/stage: Don't pass QueueRedrawEntries to actors

We currently pass actors a reference to their associated
ClutterStageQueueRedrawEntry when queueing a redraw. This "splitting" of
the ownership of the entry has introduced quite a few bugs in the past
and is hard to follow.

So give up the "splitting" of the ownership and exclusively handle those
entries inside ClutterStage. To still allow removing the entry when an
actor gets unrealized introduce clutter_stage_dequeue_actor_redraw()
similar to what we already have for relayouts.

To be able to efficiently find entries when actors queue redraws, make
pending_queue_redraws a GHashTable, which fits quite nicely and also
allows removing the QueueRedrawEntries actor pointer in favour of the
key of the hashtable.

Since the struct is now private to ClutterStage, we can also rename it
to QueueRedrawEntry.

While at it, also sneak in the removal of the leading underscore from
clutter_stage_queue_actor_redraw().

Part-of: <https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1511>

3 files changed, 75 insertions, 152 deletions

diff --git a/clutter/clutter/clutter-actor.c b/clutter/clutter/clutter-actor.c
index 03e49a2d5..c061f722f 100644
--- a/clutter/clutter/clutter-actor.c
+++ b/clutter/clutter/clutter-actor.c
@@ -775,8 +775,6 @@ struct _ClutterActorPrivate
    */
   ClutterPaintVolume last_paint_volume;
 
-  ClutterStageQueueRedrawEntry *queue_redraw_entry;
-
   ClutterColor bg_color;
 
 #ifdef CLUTTER_ENABLE_DEBUG
@@ -2117,6 +2115,9 @@ unrealize_actor_after_children_cb (ClutterActor *self,
       priv->parent->flags & CLUTTER_ACTOR_NO_LAYOUT)
     clutter_stage_dequeue_actor_relayout (CLUTTER_STAGE (stage), self);
 
+  if (stage != NULL)
+    clutter_stage_dequeue_actor_redraw (CLUTTER_STAGE (stage), self);
+
   if (priv->unmapped_paint_branch_counter == 0)
     priv->allocation = (ClutterActorBox) CLUTTER_ACTOR_BOX_UNINITIALIZED;
 
@@ -4059,22 +4060,6 @@ _clutter_actor_stop_transitions (ClutterActor *self)
     }
 }
 
-static ClutterActorTraverseVisitFlags
-invalidate_queue_redraw_entry (ClutterActor *self,
-                               int           depth,
-                               gpointer      user_data)
-{
-  ClutterActorPrivate *priv = self->priv;
-
-  if (priv->queue_redraw_entry != NULL)
-    {
-      _clutter_stage_queue_redraw_entry_invalidate (priv->queue_redraw_entry);
-      priv->queue_redraw_entry = NULL;
-    }
-
-  return CLUTTER_ACTOR_TRAVERSE_VISIT_CONTINUE;
-}
-
 static inline void
 remove_child (ClutterActor *self,
               ClutterActor *child)
@@ -4107,10 +4092,9 @@ typedef enum
   REMOVE_CHILD_EMIT_PARENT_SET    = 1 << 1,
   REMOVE_CHILD_EMIT_ACTOR_REMOVED = 1 << 2,
   REMOVE_CHILD_CHECK_STATE        = 1 << 3,
-  REMOVE_CHILD_FLUSH_QUEUE        = 1 << 4,
-  REMOVE_CHILD_NOTIFY_FIRST_LAST  = 1 << 5,
-  REMOVE_CHILD_STOP_TRANSITIONS   = 1 << 6,
-  REMOVE_CHILD_CLEAR_STAGE_VIEWS  = 1 << 7,
+  REMOVE_CHILD_NOTIFY_FIRST_LAST  = 1 << 4,
+  REMOVE_CHILD_STOP_TRANSITIONS   = 1 << 5,
+  REMOVE_CHILD_CLEAR_STAGE_VIEWS  = 1 << 6,
 
   /* default flags for public API */
   REMOVE_CHILD_DEFAULT_FLAGS      = REMOVE_CHILD_STOP_TRANSITIONS |
@@ -4118,7 +4102,6 @@ typedef enum
                                     REMOVE_CHILD_EMIT_PARENT_SET |
                                     REMOVE_CHILD_EMIT_ACTOR_REMOVED |
                                     REMOVE_CHILD_CHECK_STATE |
-                                    REMOVE_CHILD_FLUSH_QUEUE |
                                     REMOVE_CHILD_NOTIFY_FIRST_LAST |
                                     REMOVE_CHILD_CLEAR_STAGE_VIEWS,
 } ClutterActorRemoveChildFlags;
@@ -4138,7 +4121,6 @@ clutter_actor_remove_child_internal (ClutterActor                 *self,
 {
   ClutterActor *old_first, *old_last;
   gboolean destroy_meta, emit_parent_set, emit_actor_removed, check_state;
-  gboolean flush_queue;
   gboolean notify_first_last;
   gboolean stop_transitions;
   gboolean clear_stage_views;
@@ -4155,7 +4137,6 @@ clutter_actor_remove_child_internal (ClutterActor                 *self,
   emit_parent_set = (flags & REMOVE_CHILD_EMIT_PARENT_SET) != 0;
   emit_actor_removed = (flags & REMOVE_CHILD_EMIT_ACTOR_REMOVED) != 0;
   check_state = (flags & REMOVE_CHILD_CHECK_STATE) != 0;
-  flush_queue = (flags & REMOVE_CHILD_FLUSH_QUEUE) != 0;
   notify_first_last = (flags & REMOVE_CHILD_NOTIFY_FIRST_LAST) != 0;
   stop_transitions = (flags & REMOVE_CHILD_STOP_TRANSITIONS) != 0;
   clear_stage_views = (flags & REMOVE_CHILD_CLEAR_STAGE_VIEWS) != 0;
@@ -4181,28 +4162,6 @@ clutter_actor_remove_child_internal (ClutterActor                 *self,
       clutter_actor_update_map_state (child, MAP_STATE_MAKE_UNREALIZED);
     }
 
-  if (flush_queue)
-    {
-      /* We take this opportunity to invalidate any queue redraw entry
-       * associated with the actor and descendants since we won't be able to
-       * determine the appropriate stage after this.
-       *
-       * we do this after we updated the mapped state because actors might
-       * end up queueing redraws inside their mapped/unmapped virtual
-       * functions, and if we invalidate the redraw entry we could end up
-       * with an inconsistent state and weird memory corruption. see
-       * bugs:
-       *
-       *   http://bugzilla.clutter-project.org/show_bug.cgi?id=2621
-       *   https://bugzilla.gnome.org/show_bug.cgi?id=652036
-       */
-      _clutter_actor_traverse (child,
-                               0,
-                               invalidate_queue_redraw_entry,
-                               NULL,
-                               NULL);
-    }
-
   old_first = self->priv->first_child;
   old_last = self->priv->last_child;
 
@@ -7902,20 +7861,6 @@ clutter_actor_destroy (ClutterActor *self)
 }
 
 void
-_clutter_actor_finish_queue_redraw (ClutterActor *self)
-{
-  ClutterActorPrivate *priv = self->priv;
-
-  /* Remove queue entry early in the process, otherwise a new
-     queue_redraw() during signal handling could put back this
-     object in the stage redraw list (but the entry is freed as
-     soon as we return from this function, causing a segfault
-     later)
-  */
-  priv->queue_redraw_entry = NULL;
-}
-
-void
 _clutter_actor_queue_redraw_full (ClutterActor             *self,
                                   const ClutterPaintVolume *volume,
                                   ClutterEffect            *effect)
@@ -7980,11 +7925,9 @@ _clutter_actor_queue_redraw_full (ClutterActor             *self,
   if (CLUTTER_ACTOR_IN_DESTRUCTION (stage))
     return;
 
-  self->priv->queue_redraw_entry =
-    _clutter_stage_queue_actor_redraw (CLUTTER_STAGE (stage),
-                                       priv->queue_redraw_entry,
-                                       self,
-                                       volume);
+  clutter_stage_queue_actor_redraw (CLUTTER_STAGE (stage),
+                                    self,
+                                    volume);
 
   /* If this is the first redraw queued then we can directly use the
      effect parameter */
diff --git a/clutter/clutter/clutter-stage-private.h b/clutter/clutter/clutter-stage-private.h
index 85f54e497..6bfdeec79 100644
--- a/clutter/clutter/clutter-stage-private.h
+++ b/clutter/clutter/clutter-stage-private.h
@@ -31,8 +31,6 @@
 
 G_BEGIN_DECLS
 
-typedef struct _ClutterStageQueueRedrawEntry ClutterStageQueueRedrawEntry;
-
 /* stage */
 ClutterStageWindow *_clutter_stage_get_default_window    (void);
 
@@ -89,11 +87,12 @@ ClutterActor *_clutter_stage_do_pick (ClutterStage    *stage,
 ClutterPaintVolume *_clutter_stage_paint_volume_stack_allocate (ClutterStage *stage);
 void                _clutter_stage_paint_volume_stack_free_all (ClutterStage *stage);
 
-ClutterStageQueueRedrawEntry *_clutter_stage_queue_actor_redraw            (ClutterStage                 *stage,
-                                                                            ClutterStageQueueRedrawEntry *entry,
-                                                                            ClutterActor                 *actor,
-                                                                            const ClutterPaintVolume     *clip);
-void                          _clutter_stage_queue_redraw_entry_invalidate (ClutterStageQueueRedrawEntry *entry);
+void clutter_stage_queue_actor_redraw (ClutterStage             *stage,
+                                       ClutterActor             *actor,
+                                       const ClutterPaintVolume *clip);
+
+void clutter_stage_dequeue_actor_redraw (ClutterStage *stage,
+                                         ClutterActor *actor);
 
 void            _clutter_stage_add_pointer_drag_actor    (ClutterStage       *stage,
                                                           ClutterInputDevice *device,
diff --git a/clutter/clutter/clutter-stage.c b/clutter/clutter/clutter-stage.c
index 6afc40ea5..d9191c0a2 100644
--- a/clutter/clutter/clutter-stage.c
+++ b/clutter/clutter/clutter-stage.c
@@ -79,12 +79,11 @@
 
 #define MAX_FRUSTA 64
 
-struct _ClutterStageQueueRedrawEntry
+typedef struct _QueueRedrawEntry
 {
-  ClutterActor *actor;
   gboolean has_clip;
   ClutterPaintVolume clip;
-};
+} QueueRedrawEntry;
 
 typedef struct _PickRecord
 {
@@ -118,7 +117,7 @@ struct _ClutterStagePrivate
   GArray *paint_volume_stack;
 
   GSList *pending_relayouts;
-  GList *pending_queue_redraws;
+  GHashTable *pending_queue_redraws;
 
   gint sync_delay;
 
@@ -173,7 +172,7 @@ static guint stage_signals[LAST_SIGNAL] = { 0, };
 
 static const ClutterColor default_stage_color = { 255, 255, 255, 255 };
 
-static void free_queue_redraw_entry (ClutterStageQueueRedrawEntry *entry);
+static void free_queue_redraw_entry (QueueRedrawEntry *entry);
 static void capture_view_into (ClutterStage          *stage,
                                gboolean               paint,
                                ClutterStageView      *view,
@@ -1309,9 +1308,7 @@ clutter_stage_dispose (GObject *object)
 
   clutter_actor_destroy_all_children (CLUTTER_ACTOR (object));
 
-  g_list_free_full (priv->pending_queue_redraws,
-                    (GDestroyNotify) free_queue_redraw_entry);
-  priv->pending_queue_redraws = NULL;
+  g_hash_table_remove_all (priv->pending_queue_redraws);
 
   g_slist_free_full (priv->pending_relayouts,
                      (GDestroyNotify) g_object_unref);
@@ -1649,6 +1646,11 @@ clutter_stage_init (ClutterStage *self)
 
   clutter_stage_set_viewport (self, geom.width, geom.height);
 
+  priv->pending_queue_redraws =
+    g_hash_table_new_full (NULL, NULL,
+                           g_object_unref,
+                           (GDestroyNotify) free_queue_redraw_entry);
+
   priv->paint_volume_stack =
     g_array_new (FALSE, FALSE, sizeof (ClutterPaintVolume));
 
@@ -2714,13 +2716,13 @@ _clutter_stage_paint_volume_stack_free_all (ClutterStage *stage)
  * paint volume so we can clip the redraw request even if the user
  * didn't explicitly do so.
  */
-ClutterStageQueueRedrawEntry *
-_clutter_stage_queue_actor_redraw (ClutterStage                 *stage,
-                                   ClutterStageQueueRedrawEntry *entry,
-                                   ClutterActor                 *actor,
-                                   const ClutterPaintVolume     *clip)
+void
+clutter_stage_queue_actor_redraw (ClutterStage             *stage,
+                                  ClutterActor             *actor,
+                                  const ClutterPaintVolume *clip)
 {
   ClutterStagePrivate *priv = stage->priv;
+  QueueRedrawEntry *entry = NULL;
 
   CLUTTER_NOTE (CLIPPING, "stage_queue_actor_redraw (actor=%s, clip=%p): ",
                 _clutter_actor_get_debug_name (actor), clip);
@@ -2745,6 +2747,8 @@ _clutter_stage_queue_actor_redraw (ClutterStage                 *stage,
       priv->pending_finish_queue_redraws = TRUE;
     }
 
+  entry = g_hash_table_lookup (priv->pending_queue_redraws, actor);
+
   if (entry)
     {
       /* Ignore all requests to queue a redraw for an actor if a full
@@ -2754,7 +2758,7 @@ _clutter_stage_queue_actor_redraw (ClutterStage                 *stage,
           CLUTTER_NOTE (CLIPPING, "Bail from stage_queue_actor_redraw (%s): "
                         "Unclipped redraw of actor already queued",
                         _clutter_actor_get_debug_name (actor));
-          return entry;
+          return;
         }
 
       /* If queuing a clipped redraw and a clipped redraw has
@@ -2767,12 +2771,10 @@ _clutter_stage_queue_actor_redraw (ClutterStage                 *stage,
           clutter_paint_volume_free (&entry->clip);
           entry->has_clip = FALSE;
         }
-      return entry;
     }
   else
     {
-      entry = g_slice_new (ClutterStageQueueRedrawEntry);
-      entry->actor = g_object_ref (actor);
+      entry = g_slice_new (QueueRedrawEntry);
 
       if (clip)
         {
@@ -2783,40 +2785,24 @@ _clutter_stage_queue_actor_redraw (ClutterStage                 *stage,
       else
         entry->has_clip = FALSE;
 
-      stage->priv->pending_queue_redraws =
-        g_list_prepend (stage->priv->pending_queue_redraws, entry);
-
-      return entry;
+      g_hash_table_insert (priv->pending_queue_redraws,
+                           g_object_ref (actor), entry);
     }
 }
 
 static void
-free_queue_redraw_entry (ClutterStageQueueRedrawEntry *entry)
+free_queue_redraw_entry (QueueRedrawEntry *entry)
 {
-  if (entry->actor)
-    g_object_unref (entry->actor);
   if (entry->has_clip)
     clutter_paint_volume_free (&entry->clip);
-  g_slice_free (ClutterStageQueueRedrawEntry, entry);
+  g_slice_free (QueueRedrawEntry, entry);
 }
 
 void
-_clutter_stage_queue_redraw_entry_invalidate (ClutterStageQueueRedrawEntry *entry)
+clutter_stage_dequeue_actor_redraw (ClutterStage *self,
+                                    ClutterActor *actor)
 {
-  if (entry == NULL)
-    return;
-
-  if (entry->actor != NULL)
-    {
-      g_object_unref (entry->actor);
-      entry->actor = NULL;
-    }
-
-  if (entry->has_clip)
-    {
-      clutter_paint_volume_free (&entry->clip);
-      entry->has_clip = FALSE;
-    }
+  g_hash_table_remove (self->priv->pending_queue_redraws, actor);
 }
 
 static void
@@ -2882,7 +2868,8 @@ void
 clutter_stage_maybe_finish_queue_redraws (ClutterStage *stage)
 {
   ClutterStagePrivate *priv = stage->priv;
-  GList *l;
+  GHashTableIter iter;
+  gpointer key, value;
 
   COGL_TRACE_BEGIN_SCOPED (ClutterStageFinishQueueRedraws, "FinishQueueRedraws");
 
@@ -2891,50 +2878,44 @@ clutter_stage_maybe_finish_queue_redraws (ClutterStage *stage)
 
   priv->pending_finish_queue_redraws = FALSE;
 
-  for (l = priv->pending_queue_redraws; l; l = l->next)
+  g_hash_table_iter_init (&iter, priv->pending_queue_redraws);
+  while (g_hash_table_iter_next (&iter, &key, &value))
     {
-      ClutterStageQueueRedrawEntry *entry = l->data;
+      ClutterActor *redraw_actor = key;
+      QueueRedrawEntry *entry = value;
+      ClutterPaintVolume old_actor_pv, new_actor_pv;
 
-      /* NB: Entries may be invalidated if the actor gets destroyed */
-      if (G_LIKELY (entry->actor != NULL))
-        {
-          ClutterPaintVolume old_actor_pv, new_actor_pv;
-
-          _clutter_paint_volume_init_static (&old_actor_pv, NULL);
-          _clutter_paint_volume_init_static (&new_actor_pv, NULL);
+      _clutter_paint_volume_init_static (&old_actor_pv, NULL);
+      _clutter_paint_volume_init_static (&new_actor_pv, NULL);
 
-          if (entry->has_clip)
-            {
-              add_to_stage_clip (stage, &entry->clip);
-            }
-          else if (clutter_actor_get_redraw_clip (entry->actor,
-                                                  &old_actor_pv,
-                                                  &new_actor_pv))
-            {
-              /* Add both the old paint volume of the actor (which is
-               * currently visible on the screen) and the new paint volume
-               * (which will be visible on the screen after this redraw)
-               * to the redraw clip.
-               * The former we do to ensure the old texture on the screen
-               * will be fully painted over in case the actor was moved.
-               */
-              add_to_stage_clip (stage, &old_actor_pv);
-              add_to_stage_clip (stage, &new_actor_pv);
-            }
-          else
-            {
-              /* If there's no clip we can use, we have to trigger an
-               * unclipped full stage redraw.
-               */
-              add_to_stage_clip (stage, NULL);
-            }
+      if (entry->has_clip)
+        {
+          add_to_stage_clip (stage, &entry->clip);
+        }
+      else if (clutter_actor_get_redraw_clip (redraw_actor,
+                                              &old_actor_pv,
+                                              &new_actor_pv))
+        {
+          /* Add both the old paint volume of the actor (which is
+           * currently visible on the screen) and the new paint volume
+           * (which will be visible on the screen after this redraw)
+           * to the redraw clip.
+           * The former we do to ensure the old texture on the screen
+           * will be fully painted over in case the actor was moved.
+           */
+          add_to_stage_clip (stage, &old_actor_pv);
+          add_to_stage_clip (stage, &new_actor_pv);
+        }
+      else
+        {
+          /* If there's no clip we can use, we have to trigger an
+           * unclipped full stage redraw.
+           */
+          add_to_stage_clip (stage, NULL);
         }
 
-      free_queue_redraw_entry (entry);
+      g_hash_table_iter_remove (&iter);
     }
-
-  g_list_free (priv->pending_queue_redraws);
-  priv->pending_queue_redraws = NULL;
 }
 
 /**