clutter/actor: Refactor redraw clip to be centered around ClutterActor

So far our logic for queueing redraws goes like this: Actor notices that it
needs to redraw -> actor tells stage that it needs to redraw via
clutter_stage_queue_actor_redraw() -> stage collects more and more redraws
into a QueueRedrawList before the actual stage update happens -> when
that happens, the stage collects the actual redraw clips from the actors via
clutter_actor_get_redraw_clip().

The logic behind this QueueRedrawList was that by storing a list of
redraw entries on the stage, way we can avoid traversing the whole actor
tree one more time to build the redraw clip before the stage update.

These days we have clutter_actor_finish_layout() though, which is basically
exactly that, a whole actor tree traversal that happens before every stage
update.

Since we have that now, we might as well get rid of the whole dance back and
forth between ClutterStage and ClutterActor, and simply merge the logic to
queue redraws into the finish-layout step.

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

clutter/clutter/clutter-actor-private.h

@@ -266,10 +266,6 @@ void clutter_actor_queue_immediate_relayout (ClutterActor *self);
 
 gboolean clutter_actor_is_painting_unmapped (ClutterActor *self);
 
-gboolean clutter_actor_get_redraw_clip (ClutterActor       *self,
-                                        ClutterPaintVolume *dst_old_pv,
-                                        ClutterPaintVolume *dst_new_pv);
-
 void clutter_actor_attach_grab (ClutterActor *actor,
                                 ClutterGrab  *grab);
 void clutter_actor_detach_grab (ClutterActor *actor,

clutter/clutter/clutter-actor.c

@@ -805,6 +805,8 @@ struct _ClutterActorPrivate
   unsigned int n_pointers;
   unsigned int implicitly_grabbed_count;
 
+  GSList *next_redraw_clips;
+
   /* bitfields: KEEP AT THE END */
 
   /* fixed position and sizes */
@@ -847,6 +849,7 @@ struct _ClutterActorPrivate
   guint had_effects_on_last_paint_volume_update : 1;
   guint needs_update_stage_views    : 1;
   guint clear_stage_views_needs_stage_views_changed : 1;
+  guint needs_redraw : 1;
 };
 
 enum
@@ -2146,9 +2149,6 @@ 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;
 
@@ -5505,6 +5505,7 @@ clutter_actor_dispose (GObject *object)
     }
 
   g_clear_pointer (&priv->stage_views, g_list_free);
+  g_clear_slist (&priv->next_redraw_clips, (GDestroyNotify) clutter_paint_volume_free);
 
   G_OBJECT_CLASS (clutter_actor_parent_class)->dispose (object);
 }
@@ -7652,29 +7653,6 @@ _clutter_actor_queue_redraw_full (ClutterActor             *self,
   ClutterActorPrivate *priv = self->priv;
   ClutterActor *stage;
 
-  /* Here's an outline of the actor queue redraw mechanism:
-   *
-   * The process starts in clutter_actor_queue_redraw() which is a
-   * wrapper for this function. Additionally, an effect can queue a
-   * redraw by wrapping this function in clutter_effect_queue_repaint().
-   *
-   * This functions queues an entry in a list associated with the
-   * stage which is a list of actors that queued a redraw while
-   * updating the timelines, performing layouting and processing other
-   * mainloop sources before the next paint starts.
-   *
-   * When all updates are complete and we come to paint the stage then
-   * we iterate this list and build the redraw clip of the stage by
-   * either using the clip that was supplied to
-   * _clutter_actor_queue_redraw_full() or by asking the actor for its
-   * redraw clip using clutter_actor_get_redraw_clip().
-   *
-   * Doing this later during the stage update instead of now is an
-   * important optimization, because later it's more likely we will be
-   * able to determine the paint volume of an actor (its allocation
-   * should be up to date).
-   */
-
   /* ignore queueing a redraw for actors being destroyed */
   if (CLUTTER_ACTOR_IN_DESTRUCTION (self))
     return;
@@ -7709,9 +7687,33 @@ _clutter_actor_queue_redraw_full (ClutterActor             *self,
   if (CLUTTER_ACTOR_IN_DESTRUCTION (stage))
     return;
 
-  clutter_stage_queue_actor_redraw (CLUTTER_STAGE (stage),
+  if (priv->needs_redraw && !priv->next_redraw_clips)
-                                    self,
+    {
-                                    volume);
+      /* priv->needs_redraw is TRUE while priv->next_redraw_clips is NULL, this
+       * means an unclipped redraw is already queued, no need to do anything.
+       */
+    }
+  else
+    {
+      if (!priv->needs_redraw)
+        {
+          priv->needs_redraw = TRUE;
+
+          clutter_stage_schedule_update (CLUTTER_STAGE (stage));
+        }
+
+      if (volume)
+        {
+          ClutterPaintVolume *clip_pv = _clutter_paint_volume_new (self);
+
+          _clutter_paint_volume_set_from_volume (clip_pv, volume);
+          priv->next_redraw_clips = g_slist_prepend (priv->next_redraw_clips, clip_pv);
+        }
+      else
+        {
+          g_clear_slist (&priv->next_redraw_clips, (GDestroyNotify) clutter_paint_volume_free);
+        }
+    }
 
   /* If this is the first redraw queued then we can directly use the
      effect parameter */
@@ -15367,6 +15369,48 @@ clutter_actor_get_resource_scale (ClutterActor *self)
   return ceilf (clutter_actor_get_real_resource_scale (self));
 }
 
+static void
+add_actor_to_redraw_clip (ClutterActor       *self,
+                          gboolean            actor_moved,
+                          ClutterPaintVolume *old_visible_paint_volume)
+{
+  ClutterActorPrivate *priv = self->priv;
+  ClutterStage *stage = CLUTTER_STAGE (_clutter_actor_get_stage_internal (self));
+
+  if (priv->next_redraw_clips)
+    {
+      GSList *l;
+
+      for (l = priv->next_redraw_clips; l; l = l->next)
+        clutter_stage_add_to_redraw_clip (stage, l->data);
+
+      g_clear_slist (&priv->next_redraw_clips, (GDestroyNotify) clutter_paint_volume_free);
+    }
+  else if (actor_moved)
+    {
+      /* For a clipped redraw to work we need both the old paint volume and the new
+       * one, if any is missing we'll need to do an unclipped redraw.
+       */
+      if (old_visible_paint_volume == NULL || !priv->visible_paint_volume_valid)
+        goto full_stage_redraw;
+
+      clutter_stage_add_to_redraw_clip (stage, old_visible_paint_volume);
+      clutter_stage_add_to_redraw_clip (stage, &priv->visible_paint_volume);
+    }
+  else
+    {
+      if (!priv->visible_paint_volume_valid)
+        goto full_stage_redraw;
+
+      clutter_stage_add_to_redraw_clip (stage, &priv->visible_paint_volume);
+    }
+
+  return;
+
+full_stage_redraw:
+  clutter_stage_add_to_redraw_clip (stage, NULL);
+}
+
 static gboolean
 sorted_lists_equal (GList *list_a,
                     GList *list_b)
@@ -15462,6 +15506,9 @@ clutter_actor_finish_layout (ClutterActor *self,
 {
   ClutterActorPrivate *priv = self->priv;
   ClutterActor *child;
+  gboolean actor_moved = FALSE;
+  gboolean old_visible_paint_volume_valid = FALSE;
+  ClutterPaintVolume old_visible_paint_volume;
 
   if ((!CLUTTER_ACTOR_IS_MAPPED (self) &&
        !clutter_actor_has_mapped_clones (self)) ||
@@ -15472,6 +15519,10 @@ clutter_actor_finish_layout (ClutterActor *self,
     {
       ensure_paint_volume (self);
 
+      actor_moved = TRUE;
+      old_visible_paint_volume = priv->visible_paint_volume;
+      old_visible_paint_volume_valid = priv->visible_paint_volume_valid;
+
       if (priv->has_paint_volume)
         {
           _clutter_paint_volume_copy_static (&priv->paint_volume,
@@ -15492,6 +15543,14 @@ clutter_actor_finish_layout (ClutterActor *self,
       priv->needs_update_stage_views = FALSE;
     }
 
+  if (priv->needs_redraw)
+    {
+      add_actor_to_redraw_clip (self,
+                                actor_moved,
+                                old_visible_paint_volume_valid ? &old_visible_paint_volume : NULL);
+      priv->needs_redraw = FALSE;
+    }
+
   for (child = priv->first_child; child; child = child->priv->next_sibling)
     clutter_actor_finish_layout (child, use_max_scale);
 }
@@ -19054,27 +19113,6 @@ clutter_actor_invalidate_paint_volume (ClutterActor *self)
   queue_update_paint_volume (self);
 }
 
-gboolean
-clutter_actor_get_redraw_clip (ClutterActor       *self,
-                               ClutterPaintVolume *dst_old_pv,
-                               ClutterPaintVolume *dst_new_pv)
-{
-  ClutterActorPrivate *priv = self->priv;
-
-  ensure_paint_volume (self);
-
-  /* For a clipped redraw to work we need both the old paint volume and the new
-   * one, if any is missing we'll need to do an unclipped redraw.
-   */
-  if (!priv->visible_paint_volume_valid || !priv->has_paint_volume)
-    return FALSE;
-
-  _clutter_paint_volume_set_from_volume (dst_old_pv, &priv->visible_paint_volume);
-  _clutter_paint_volume_set_from_volume (dst_new_pv, &priv->paint_volume);
-
-  return TRUE;
-}
-
 void
 clutter_actor_attach_grab (ClutterActor *self,
                            ClutterGrab  *grab)

clutter/clutter/clutter-stage-private.h

@@ -82,7 +82,6 @@ CLUTTER_EXPORT
 void                _clutter_stage_maybe_setup_viewport  (ClutterStage          *stage,
                                                           ClutterStageView      *view);
 void                clutter_stage_maybe_relayout         (ClutterActor          *stage);
-void                clutter_stage_maybe_finish_queue_redraws (ClutterStage      *stage);
 GSList *            clutter_stage_find_updated_devices   (ClutterStage          *stage,
                                                           ClutterStageView      *view);
 void                clutter_stage_update_devices         (ClutterStage          *stage,
@@ -101,13 +100,6 @@ gboolean _clutter_stage_has_full_redraw_queued            (ClutterStage *stage);
 ClutterPaintVolume *_clutter_stage_paint_volume_stack_allocate (ClutterStage *stage);
 void                _clutter_stage_paint_volume_stack_free_all (ClutterStage *stage);
 
-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,
                                                           ClutterActor       *actor);
@@ -184,6 +176,9 @@ void clutter_stage_notify_action_implicit_grab (ClutterStage         *self,
                                                 ClutterInputDevice   *device,
                                                 ClutterEventSequence *sequence);
 
+void clutter_stage_add_to_redraw_clip (ClutterStage       *self,
+                                       ClutterPaintVolume *clip);
+
 G_END_DECLS
 
 #endif /* __CLUTTER_STAGE_PRIVATE_H__ */

clutter/clutter/clutter-stage-view.c

@@ -1250,7 +1250,6 @@ handle_frame_clock_frame (ClutterFrameClock *frame_clock,
   clutter_stage_emit_before_update (stage, view, frame);
 
   clutter_stage_maybe_relayout (CLUTTER_ACTOR (stage));
-  clutter_stage_maybe_finish_queue_redraws (stage);
 
   clutter_stage_finish_layout (stage);
 

clutter/clutter/clutter-stage.c

@@ -75,11 +75,6 @@
 
 #define MAX_FRUSTA 64
 
-typedef struct _QueueRedrawEntry
-{
-  GSList *clips;
-} QueueRedrawEntry;
-
 typedef struct _PickRecord
 {
   graphene_point_t vertex[4];
@@ -139,12 +134,9 @@ struct _ClutterStagePrivate
   GArray *paint_volume_stack;
 
   GSList *pending_relayouts;
-  GHashTable *pending_queue_redraws;
 
   int update_freeze_count;
 
-  gboolean pending_finish_queue_redraws;
-
   GHashTable *pointer_devices;
   GHashTable *touch_sequences;
 
@@ -193,7 +185,6 @@ static guint stage_signals[LAST_SIGNAL] = { 0, };
 
 static const ClutterColor default_stage_color = { 255, 255, 255, 255 };
 
-static void free_queue_redraw_entry (QueueRedrawEntry *entry);
 static void free_pointer_device_entry (PointerDeviceEntry *entry);
 static void free_event_receiver (EventReceiver *receiver);
 static void clutter_stage_update_view_perspective (ClutterStage *stage);
@@ -937,7 +928,6 @@ clutter_stage_finish_layout (ClutterStage *stage)
 
       priv->actor_needs_immediate_relayout = FALSE;
       clutter_stage_maybe_relayout (actor);
-      clutter_stage_maybe_finish_queue_redraws (stage);
     }
 
   g_warn_if_fail (!priv->actor_needs_immediate_relayout);
@@ -1229,8 +1219,6 @@ clutter_stage_dispose (GObject *object)
 
   clutter_actor_destroy_all_children (CLUTTER_ACTOR (object));
 
-  g_hash_table_remove_all (priv->pending_queue_redraws);
-
   g_slist_free_full (priv->pending_relayouts,
                      (GDestroyNotify) g_object_unref);
   priv->pending_relayouts = NULL;
@@ -1634,11 +1622,6 @@ clutter_stage_init (ClutterStage *self)
   clutter_stage_set_key_focus (self, NULL);
   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));
 }
@@ -2421,7 +2404,7 @@ gboolean
 clutter_stage_is_redraw_queued_on_view (ClutterStage     *stage,
                                         ClutterStageView *view)
 {
-  clutter_stage_maybe_finish_queue_redraws (stage);
+  clutter_stage_finish_layout (stage);
 
   return clutter_stage_view_has_redraw_clip (view);
 }
@@ -2516,90 +2499,9 @@ _clutter_stage_paint_volume_stack_free_all (ClutterStage *stage)
   g_array_set_size (paint_volume_stack, 0);
 }
 
-/* When an actor queues a redraw we add it to a list on the stage that
- * gets processed once all updates to the stage have been finished.
- *
- * This deferred approach to processing queue_redraw requests means
- * that we can avoid redundant transformations of clip volumes if
- * something later triggers a full stage redraw anyway. It also means
- * we can be more sure that all the referenced actors will have valid
- * allocations improving the chance that we can determine the actors
- * paint volume so we can clip the redraw request even if the user
- * didn't explicitly do so.
- */
 void
-clutter_stage_queue_actor_redraw (ClutterStage             *stage,
+clutter_stage_add_to_redraw_clip (ClutterStage       *stage,
-                                  ClutterActor             *actor,
+                                  ClutterPaintVolume *redraw_clip)
-                                  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);
-
-  if (!priv->pending_finish_queue_redraws)
-    {
-      GList *l;
-
-      for (l = clutter_stage_peek_stage_views (stage); l; l = l->next)
-        {
-          ClutterStageView *view = l->data;
-
-          clutter_stage_view_schedule_update (view);
-        }
-
-      priv->pending_finish_queue_redraws = TRUE;
-    }
-
-  entry = g_hash_table_lookup (priv->pending_queue_redraws, actor);
-
-  if (!entry)
-    {
-      entry = g_new0 (QueueRedrawEntry, 1);
-      g_hash_table_insert (priv->pending_queue_redraws,
-                           g_object_ref (actor), entry);
-    }
-  else if (!entry->clips)
-    {
-      CLUTTER_NOTE (CLIPPING, "Bail from stage_queue_actor_redraw (%s): "
-                    "Unclipped redraw of actor already queued",
-                    _clutter_actor_get_debug_name (actor));
-      return;
-    }
-
-  /* If queuing a clipped redraw then append the latest
-   * clip to the clip list */
-  if (clip)
-    {
-      ClutterPaintVolume *clip_pv = _clutter_paint_volume_new (actor);
-
-      _clutter_paint_volume_set_from_volume (clip_pv, clip);
-      entry->clips = g_slist_prepend (entry->clips, clip_pv);
-    }
-  else
-    {
-      g_clear_slist (&entry->clips, (GDestroyNotify) clutter_paint_volume_free);
-    }
-}
-
-static void
-free_queue_redraw_entry (QueueRedrawEntry *entry)
-{
-  g_clear_slist (&entry->clips, (GDestroyNotify) clutter_paint_volume_free);
-  g_free (entry);
-}
-
-void
-clutter_stage_dequeue_actor_redraw (ClutterStage *self,
-                                    ClutterActor *actor)
-{
-  g_hash_table_remove (self->priv->pending_queue_redraws, actor);
-}
-
-static void
-add_to_stage_clip (ClutterStage       *stage,
-                   ClutterPaintVolume *redraw_clip)
 {
   ClutterStageWindow *stage_window;
   ClutterActorBox bounding_box;
@@ -2652,77 +2554,6 @@ add_to_stage_clip (ClutterStage       *stage,
   clutter_stage_add_redraw_clip (stage, &stage_clip);
 }
 
-void
-clutter_stage_maybe_finish_queue_redraws (ClutterStage *stage)
-{
-  ClutterStagePrivate *priv = stage->priv;
-  GHashTableIter iter;
-  gpointer key, value;
-
-  COGL_TRACE_BEGIN_SCOPED (ClutterStageFinishQueueRedraws, "FinishQueueRedraws");
-
-  if (!priv->pending_finish_queue_redraws)
-    return;
-
-  priv->pending_finish_queue_redraws = FALSE;
-
-  g_hash_table_iter_init (&iter, priv->pending_queue_redraws);
-  while (g_hash_table_iter_next (&iter, &key, &value))
-    {
-      ClutterActor *redraw_actor = key;
-      QueueRedrawEntry *entry = value;
-
-      g_hash_table_iter_steal (&iter);
-
-      if (clutter_actor_is_mapped (redraw_actor))
-        {
-          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);
-
-          if (entry->clips)
-            {
-              GSList *l;
-
-              for (l = entry->clips; l; l = l->next)
-                add_to_stage_clip (stage, l->data);
-            }
-          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);
-            }
-        }
-
-      g_object_unref (redraw_actor);
-      free_queue_redraw_entry (entry);
-
-      /* get_paint_volume() vfuncs might queue redraws and can cause our
-       * iterator to now be invalidated. So start over. This isn't wasting
-       * any time since we already stole (removed) the elements previously
-       * visited.
-       */
-      g_hash_table_iter_init (&iter, priv->pending_queue_redraws);
-    }
-}
-
 void
 _clutter_stage_add_pointer_drag_actor (ClutterStage       *stage,
                                        ClutterInputDevice *device,