[clutter] actor: make offscreen_redirect prop take flags + default off

[Robert Bragg <rbragg src gnome org>]

commit 0aacbd47b7f8e44f7688365ee1a76149e6bbd565
Author: Robert Bragg <robert linux intel com>
Date:   Tue Jul 12 23:03:41 2011 +0100

    actor: make offscreen_redirect prop take flags + default off
    
    Because we have had several reports about significant performance
    regressions since we enabled offscreen redirection by default for
    handling correct opacity we are now turning this feature off by default.
    
    We feel that clutter should prioritize performance over correctness in
    this case. Correct opacity is still possible if required but the
    overhead of the numerous offscreen allocations as well as the cost of
    many render target switches per-frame seems too high relative the
    improvement in quality for many cases.
    
    On reviewing the offscreen_redirect property so we have a way to
    disable redirection by default we realized that it makes more sense for
    it to take a set of flags instead of an enum so we can potentially
    extend the number of things that might result in offscreen redirection.
    
    We removed the ability to say REDIRECT_ALWAYS_FOR_OPACITY, since it
    seems that implies you don't trust the implementation of an actor's
    has_overlaps() vfunc which doesn't seem right.
    
    The default value if actor::redirect_offscreen is now 0 which
    effectively means don't ever redirect the actor offscreen.

 clutter/clutter-actor.c                 |   84 +++++++++++++++---------------
 clutter/clutter-actor.h                 |   10 +---
 tests/conform/test-offscreen-redirect.c |   34 ++++++++++--
 3 files changed, 73 insertions(+), 55 deletions(-)
---
diff --git a/clutter/clutter-actor.c b/clutter/clutter-actor.c
index ed76d93..90bf3d6 100644
--- a/clutter/clutter-actor.c
+++ b/clutter/clutter-actor.c
@@ -2730,20 +2730,16 @@ needs_flatten_effect (ClutterActor *self)
                   CLUTTER_DEBUG_DISABLE_OFFSCREEN_REDIRECT))
     return FALSE;
 
-  switch (priv->offscreen_redirect)
+  if (priv->offscreen_redirect & CLUTTER_OFFSCREEN_REDIRECT_ALWAYS)
+    return TRUE;
+  else if (priv->offscreen_redirect & CLUTTER_OFFSCREEN_REDIRECT_AUTOMATIC_FOR_OPACITY)
     {
-    case CLUTTER_OFFSCREEN_REDIRECT_AUTOMATIC_FOR_OPACITY:
-      if (!clutter_actor_has_overlaps (self))
-        return FALSE;
-      /* flow through */
-    case CLUTTER_OFFSCREEN_REDIRECT_ALWAYS_FOR_OPACITY:
-      return clutter_actor_get_paint_opacity (self) < 255;
-
-    case CLUTTER_OFFSCREEN_REDIRECT_ALWAYS:
-      return TRUE;
+      if (clutter_actor_get_paint_opacity (self) < 255 &&
+          clutter_actor_has_overlaps (self))
+        return TRUE;
     }
 
-  g_assert_not_reached ();
+  return FALSE;
 }
 
 static void
@@ -3823,11 +3819,9 @@ clutter_actor_real_get_paint_volume (ClutterActor       *self,
 static gboolean
 clutter_actor_real_has_overlaps (ClutterActor *self)
 {
-  /* By default we'll assume that all actors need an offscreen
-     redirect to get the correct opacity. This effectively favours
-     accuracy over efficiency. Actors such as ClutterTexture that
-     would never need an offscreen redirect can override this to
-     return FALSE. */
+  /* By default we'll assume that all actors need an offscreen redirect to get
+   * the correct opacity. Actors such as ClutterTexture that would never need
+   * an offscreen redirect can override this to return FALSE. */
   return TRUE;
 }
 
@@ -4225,18 +4219,21 @@ clutter_actor_class_init (ClutterActorClass *klass)
   /**
    * ClutterActor:offscreen-redirect:
    *
-   * Whether to flatten the actor into a single image. See
+   * Determines the conditions in which the actor will be redirected
+   * to an offscreen framebuffer while being painted. For example this
+   * can be used to cache an actor in a framebuffer or for improved
+   * handling of transparent actors. See
    * clutter_actor_set_offscreen_redirect() for details.
    *
    * Since: 1.8
    */
-  pspec = g_param_spec_enum ("offscreen-redirect",
-                             P_("Offscreen redirect"),
-                             P_("Whether to flatten the actor into a "
-                                "single image"),
-                             CLUTTER_TYPE_OFFSCREEN_REDIRECT,
-                             CLUTTER_OFFSCREEN_REDIRECT_AUTOMATIC_FOR_OPACITY,
-                             CLUTTER_PARAM_READWRITE);
+  pspec = g_param_spec_flags ("offscreen-redirect",
+                              P_("Offscreen redirect"),
+                              P_("Flags controlling when to flatten the "
+                                 "actor into a single image"),
+                              CLUTTER_TYPE_OFFSCREEN_REDIRECT,
+                              0,
+                              CLUTTER_PARAM_READWRITE);
   obj_props[PROP_OFFSCREEN_REDIRECT] = pspec;
   g_object_class_install_property (object_class,
                                    PROP_OFFSCREEN_REDIRECT,
@@ -5288,7 +5285,6 @@ clutter_actor_init (ClutterActor *self)
   priv->parent_actor = NULL;
   priv->has_clip = FALSE;
   priv->opacity = 0xff;
-  priv->offscreen_redirect = CLUTTER_OFFSCREEN_REDIRECT_AUTOMATIC_FOR_OPACITY;
   priv->id = _clutter_context_acquire_id (self);
   priv->pick_id = -1;
   priv->scale_x = 1.0;
@@ -7598,15 +7594,16 @@ clutter_actor_get_opacity (ClutterActor *self)
 /**
  * clutter_actor_set_offscreen_redirect:
  * @self: A #ClutterActor
- * @redirect: New offscreen redirect value for the actor.
+ * @redirect: New offscreen redirect flags for the actor.
  *
- * Sets whether to redirect the actor into an offscreen image. The
- * offscreen image is used to flatten the actor into a single image
- * while painting for two main reasons. Firstly, when the actor is
- * painted a second time without any of its contents changing it can
- * simply repaint the cached image without descending further down the
- * actor hierarchy. Secondly, it will make the opacity look correct
- * even if there are overlapping primitives in the actor.
+ * Defines the circumstances where the actor should be redirected into
+ * an offscreen image. The offscreen image is used to flatten the
+ * actor into a single image while painting for two main reasons.
+ * Firstly, when the actor is painted a second time without any of its
+ * contents changing it can simply repaint the cached image without
+ * descending further down the actor hierarchy. Secondly, it will make
+ * the opacity look correct even if there are overlapping primitives
+ * in the actor.
  *
  * Caching the actor could in some cases be a performance win and in
  * some cases be a performance lose so it is important to determine
@@ -7643,13 +7640,15 @@ clutter_actor_get_opacity (ClutterActor *self)
  *   <graphic fileref="offscreen-redirect.png" format="PNG"/>
  * </figure>
  *
- * The default behaviour is
- * %CLUTTER_OFFSCREEN_REDIRECT_AUTOMATIC_FOR_OPACITY. This will end up
- * redirecting actors whenever they are semi-transparent unless their
- * has_overlaps() virtual returns %FALSE. This should mean that
- * generally all actors will be rendered with the correct opacity and
- * certain actors that don't need the offscreen redirect (such as
- * #ClutterTexture) will paint directly for efficiency.
+ * The default value for this property is 0, so we effectively will
+ * never redirect an actor offscreen by default. This means that there
+ * are times that transparent actors may look glassy as described
+ * above. The reason this is the default is because there is a
+ * performance trade off between quality and performance here. In many
+ * cases the default form of glassy opacity looks good enough, but if
+ * it's not you will need to set the
+ * %CLUTTER_OFFSCREEN_REDIRECT_AUTOMATIC_FOR_OPACITY flag to enable
+ * redirection for opacity.
  *
  * Custom actors that don't contain any overlapping primitives are
  * recommended to override the has_overlaps() virtual to return %FALSE
@@ -12408,8 +12407,9 @@ clutter_actor_get_paint_box (ClutterActor    *self,
  * Asks the actor's implementation whether it may contain overlapping
  * primitives.
  *
- * Clutter uses this to determine whether the painting should be redirected
- * to an offscreen buffer to correctly implement the opacity property.
+ * For example; Clutter may use this to determine whether the painting
+ * should be redirected to an offscreen buffer to correctly implement
+ * the opacity property.
  *
  * Custom actors can override the default response by implementing the
  * #ClutterActor <function>has_overlaps</function> virtual function. See
diff --git a/clutter/clutter-actor.h b/clutter/clutter-actor.h
index bdeeda5..25f5623 100644
--- a/clutter/clutter-actor.h
+++ b/clutter/clutter-actor.h
@@ -121,21 +121,17 @@ typedef enum
  * @CLUTTER_OFFSCREEN_REDIRECT_AUTOMATIC_FOR_OPACITY: Only redirect
  *   the actor if it is semi-transparent and its has_overlaps()
  *   virtual returns %TRUE. This is the default.
- * @CLUTTER_OFFSCREEN_REDIRECT_ALWAYS_FOR_OPACITY: Always redirect the
- *   actor if it is semi-transparent regardless of the return value of
- *   its has_overlaps() virtual.
  * @CLUTTER_OFFSCREEN_REDIRECT_ALWAYS: Always redirect the actor to an
  *   offscreen buffer even if it is fully opaque.
  *
- * Possible values to pass to clutter_actor_set_offscreen_redirect().
+ * Possible flags to pass to clutter_actor_set_offscreen_redirect().
  *
  * Since: 1.8
  */
 typedef enum
 {
-  CLUTTER_OFFSCREEN_REDIRECT_AUTOMATIC_FOR_OPACITY,
-  CLUTTER_OFFSCREEN_REDIRECT_ALWAYS_FOR_OPACITY,
-  CLUTTER_OFFSCREEN_REDIRECT_ALWAYS
+  CLUTTER_OFFSCREEN_REDIRECT_AUTOMATIC_FOR_OPACITY = 1<<0,
+  CLUTTER_OFFSCREEN_REDIRECT_ALWAYS = 1<<1
 } ClutterOffscreenRedirect;
 
 /**
diff --git a/tests/conform/test-offscreen-redirect.c b/tests/conform/test-offscreen-redirect.c
index 3e25c98..0952fa3 100644
--- a/tests/conform/test-offscreen-redirect.c
+++ b/tests/conform/test-offscreen-redirect.c
@@ -32,6 +32,8 @@ GType foo_actor_get_type (void) G_GNUC_CONST;
 
 G_DEFINE_TYPE (FooActor, foo_actor, CLUTTER_TYPE_ACTOR);
 
+static gboolean group_has_overlaps;
+
 static void
 foo_actor_paint (ClutterActor *actor)
 {
@@ -100,7 +102,7 @@ G_DEFINE_TYPE (FooGroup, foo_group, CLUTTER_TYPE_GROUP);
 static gboolean
 foo_group_has_overlaps (ClutterActor *actor)
 {
-  return FALSE;
+  return group_has_overlaps;
 }
 
 static void
@@ -177,6 +179,8 @@ timeout_cb (gpointer user_data)
 {
   Data *data = user_data;
 
+  group_has_overlaps = FALSE;
+
   /* By default the actor shouldn't be redirected so the redraw should
      cause the actor to be painted */
   verify_results (data,
@@ -191,12 +195,30 @@ timeout_cb (gpointer user_data)
                   1,
                   127);
 
-  /* Enable offscreen for opacity so it should now paint through the
-     fbo. The first paint will still cause the actor to draw because
-     it needs to fill the cache first. It should be painted with full
-     opacity */
+  /* With automatic redirect for opacity it shouldn't redirect if
+   * has_overlaps returns FALSE; */
   clutter_actor_set_offscreen_redirect
-    (data->container, CLUTTER_OFFSCREEN_REDIRECT_ALWAYS_FOR_OPACITY);
+    (data->container, CLUTTER_OFFSCREEN_REDIRECT_AUTOMATIC_FOR_OPACITY);
+  verify_results (data,
+                  255, 127, 127,
+                  1,
+                  127);
+
+  /* We do a double check here to verify that the actor wasn't cached
+   * during the last check. If it was cached then this check wouldn't
+   * result in any foo-actor re-paint. */
+  verify_results (data,
+                  255, 127, 127,
+                  1,
+                  127);
+
+  /* With automatic redirect for opacity it should redirect if
+   * has_overlaps returns TRUE.
+   * The first paint will still cause the actor to draw because
+   * it needs to fill the cache first. It should be painted with full
+   * opacity */
+  group_has_overlaps = TRUE;
+
   verify_results (data,
                   255, 127, 127,
                   1,