clutter: Fix offscreen-effect painting of clones

`ClutterOffscreenEffect` had been getting the wrong bounding box in the
case of clones and descendents of clones, causing visibly incorrect
clipping. This was due to `clutter_actor_get_paint_box` only ever being
given the source actor during a paint (which is correct) and not the clone.
Even if we weren't painting a clone but an offscreened descendent of a
clone (like in gnome-shell's desktop zoom), we would get the wrong result.

Fortunately we don't need to know the actual clone/actor being painted so
don't need to call the problematic `clutter_actor_get_paint_box` at all.
The solution is to only keep untransformed rendering in the FBO and leave
the correct transformation for later. The correct clone/actor's
transformation is already set for us as the current cogl modelview matrix
by `clutter_actor_paint`.

Bonus optimization: This all means we don't need to keep `last_matrix_drawn`
or force a full repaint every time some part of the transformation changes.
Because the FBO contents are no longer affected by transformations. As it
should be. In other words, offscreen-effected actors can now move around
on screen without themselves being repainted.

Special thanks to Mai Lavelle for identifying the cause of the problem.

Fixes:
https://bugzilla.gnome.org/show_bug.cgi?id=789050,
https://bugzilla.gnome.org/show_bug.cgi?id=659523#c9,
https://gitlab.gnome.org/GNOME/mutter/issues/196,
https://gitlab.gnome.org/GNOME/mutter/issues/282,
https://gitlab.gnome.org/GNOME/gnome-shell/issues/387,
https://launchpad.net/bugs/1767648,
https://launchpad.net/bugs/1779615

clutter/clutter/clutter-actor-box-private.h

@@ -0,0 +1,12 @@
+#ifndef __CLUTTER_ACTOR_BOX_PRIVATE_H__
+#define __CLUTTER_ACTOR_BOX_PRIVATE_H__
+
+#include <clutter/clutter-types.h>
+
+G_BEGIN_DECLS
+
+void _clutter_actor_box_enlarge_for_effects (ClutterActorBox *box);
+
+G_END_DECLS
+
+#endif /* __CLUTTER_ACTOR_BOX_PRIVATE_H__ */

clutter/clutter/clutter-actor-box.c

@@ -5,6 +5,7 @@
 #include "clutter-types.h"
 #include "clutter-interval.h"
 #include "clutter-private.h"
+#include "clutter-actor-box-private.h"
 
 /**
  * clutter_actor_box_new:
@@ -542,6 +543,57 @@ clutter_actor_box_set_size (ClutterActorBox *box,
   box->y2 = box->y1 + height;
 }
 
+void
+_clutter_actor_box_enlarge_for_effects (ClutterActorBox *box)
+{
+  float width, height;
+
+  /* The aim here is that for a given rectangle defined with floating point
+   * coordinates we want to determine a stable quantized size in pixels
+   * that doesn't vary due to the original box's sub-pixel position.
+   *
+   * The reason this is important is because effects will use this
+   * API to determine the size of offscreen framebuffers and so for
+   * a fixed-size object that may be animated accross the screen we
+   * want to make sure that the stage paint-box has an equally stable
+   * size so that effects aren't made to continuously re-allocate
+   * a corresponding fbo.
+   *
+   * The other thing we consider is that the calculation of this box is
+   * subject to floating point precision issues that might be slightly
+   * different to the precision issues involved with actually painting the
+   * actor, which might result in painting slightly leaking outside the
+   * user's calculated paint-volume. For this we simply aim to pad out the
+   * paint-volume by at least half a pixel all the way around.
+   */
+  width = box->x2 - box->x1;
+  height = box->y2 - box->y1;
+  width = CLUTTER_NEARBYINT (width);
+  height = CLUTTER_NEARBYINT (height);
+  /* XXX: NB the width/height may now be up to 0.5px too small so we
+   * must also pad by 0.25px all around to account for this. In total we
+   * must padd by at least 0.75px around all sides. */
+
+  /* XXX: The furthest that we can overshoot the bottom right corner by
+   * here is 1.75px in total if you consider that the 0.75 padding could
+   * just cross an integer boundary and so ceil will effectively add 1.
+   */
+  box->x2 = ceilf (box->x2 + 0.75);
+  box->y2 = ceilf (box->y2 + 0.75);
+
+  /* Now we redefine the top-left relative to the bottom right based on the
+   * rounded width/height determined above + a constant so that the overall
+   * size of the box will be stable and not dependant on the box's
+   * position.
+   *
+   * Adding 3px to the width/height will ensure we cover the maximum of
+   * 1.75px padding on the bottom/right and still ensure we have > 0.75px
+   * padding on the top/left.
+   */
+  box->x1 = box->x2 - width - 3;
+  box->y1 = box->y2 - height - 3;
+}
+
 G_DEFINE_BOXED_TYPE_WITH_CODE (ClutterActorBox, clutter_actor_box,
                                clutter_actor_box_copy,
                                clutter_actor_box_free,

clutter/clutter/clutter-offscreen-effect.c

@@ -72,6 +72,8 @@
 #include "clutter-debug.h"
 #include "clutter-private.h"
 #include "clutter-stage-private.h"
+#include "clutter-paint-volume-private.h"
+#include "clutter-actor-box-private.h"
 
 struct _ClutterOffscreenEffectPrivate
 {
@@ -82,8 +84,10 @@ struct _ClutterOffscreenEffectPrivate
   ClutterActor *actor;
   ClutterActor *stage;
 
-  gfloat x_offset;
+  ClutterVertex position;
-  gfloat y_offset;
+
+  int fbo_offset_x;
+  int fbo_offset_y;
 
   /* This is the calculated size of the fbo before being passed
      through create_texture(). This needs to be tracked separately so
@@ -93,16 +97,6 @@ struct _ClutterOffscreenEffectPrivate
   int fbo_height;
 
   gint old_opacity_override;
-
-  /* The matrix that was current the last time the fbo was updated. We
-     need to keep track of this to detect when we can reuse the
-     contents of the fbo without redrawing the actor. We need the
-     actual matrix rather than just detecting queued redraws on the
-     actor because any change in the parent hierarchy (even just a
-     translation) could cause the actor to look completely different
-     and it won't cause a redraw to be queued on the parent's
-     children. */
-  CoglMatrix last_matrix_drawn;
 };
 
 G_DEFINE_ABSTRACT_TYPE_WITH_PRIVATE (ClutterOffscreenEffect,
@@ -222,15 +216,15 @@ clutter_offscreen_effect_pre_paint (ClutterEffect *effect)
 {
   ClutterOffscreenEffect *self = CLUTTER_OFFSCREEN_EFFECT (effect);
   ClutterOffscreenEffectPrivate *priv = self->priv;
-  ClutterActorBox box;
+  ClutterActorBox raw_box, box;
   ClutterActor *stage;
-  CoglMatrix projection;
+  CoglMatrix projection, old_modelview, modelview;
+  const ClutterPaintVolume *volume;
   CoglColor transparent;
   gfloat stage_width, stage_height;
   gfloat fbo_width = -1, fbo_height = -1;
-  gfloat width, height;
+  ClutterVertex local_offset = { 0.f, 0.f, 0.f };
-  gfloat xexpand, yexpand;
+  gfloat old_viewport[4];
-  int texture_width, texture_height;
 
   if (!clutter_actor_meta_get_enabled (CLUTTER_ACTOR_META (effect)))
     return FALSE;
@@ -241,92 +235,82 @@ clutter_offscreen_effect_pre_paint (ClutterEffect *effect)
   stage = _clutter_actor_get_stage_internal (priv->actor);
   clutter_actor_get_size (stage, &stage_width, &stage_height);
 
-  /* The paint box is the bounding box of the actor's paint volume in
+  /* Get the minimal bounding box for what we want to paint, relative to the
-   * stage coordinates. This will give us the size for the framebuffer
+   * parent of priv->actor. Note that we may actually be painting a clone of
-   * we need to redirect its rendering offscreen and its position will
+   * priv->actor so we need to be careful to avoid querying the transformation
-   * be used to setup an offset viewport */
+   * of priv->actor (like clutter_actor_get_paint_box would). Just stay in
-  if (clutter_actor_get_paint_box (priv->actor, &box))
+   * local coordinates for now...
+   */
+  volume = clutter_actor_get_paint_volume (priv->actor);
+  if (volume)
     {
-      clutter_actor_box_get_size (&box, &fbo_width, &fbo_height);
+      ClutterPaintVolume mutable_volume;
-      clutter_actor_box_get_origin (&box, &priv->x_offset, &priv->y_offset);
 
-      fbo_width = MIN (fbo_width, stage_width);
+      _clutter_paint_volume_copy_static (volume, &mutable_volume);
-      fbo_height = MIN (fbo_height, stage_height);
+      _clutter_paint_volume_get_bounding_box (&mutable_volume, &raw_box);
+      clutter_paint_volume_free (&mutable_volume);
     }
   else
     {
-      fbo_width = stage_width;
+      clutter_actor_get_allocation_box (priv->actor, &raw_box);
-      fbo_height = stage_height;
     }
 
-  if (fbo_width == stage_width)
+  box = raw_box;
-    priv->x_offset = 0.0f;
+  _clutter_actor_box_enlarge_for_effects (&box);
-  if (fbo_height == stage_height)
+
-    priv->y_offset = 0.0f;
+  priv->fbo_offset_x = box.x1 - raw_box.x1;
+  priv->fbo_offset_y = box.y1 - raw_box.y1;
+
+  clutter_actor_box_get_size (&box, &fbo_width, &fbo_height);
 
   /* First assert that the framebuffer is the right size... */
   if (!update_fbo (effect, fbo_width, fbo_height))
     return FALSE;
 
-  texture_width = cogl_texture_get_width (priv->texture);
+  cogl_get_modelview_matrix (&old_modelview);
-  texture_height = cogl_texture_get_height (priv->texture);
-
-  /* get the current modelview matrix so that we can copy it to the
-   * framebuffer. We also store the matrix that was last used when we
-   * updated the FBO so that we can detect when we don't need to
-   * update the FBO to paint a second time */
-  cogl_get_modelview_matrix (&priv->last_matrix_drawn);
 
   /* let's draw offscreen */
   cogl_push_framebuffer (priv->offscreen);
 
-  /* Copy the modelview that would have been used if rendering onscreen */
+  /* We don't want the FBO contents to be transformed. That could waste memory
-  cogl_set_modelview_matrix (&priv->last_matrix_drawn);
+   * (e.g. during zoom), or result in something that's not rectangular (clipped
-
+   * incorrectly). So drop the modelview matrix of the current paint chain.
-  /* Set up the viewport so that it has the same size as the stage,
+   * This is fine since paint_texture runs with the same modelview matrix,
-   * but offset it so that the actor of interest lands on our
+   * so it will come out correctly whenever that is used to put the FBO
-   * framebuffer. */
+   * contents on screen...
-  clutter_actor_get_size (priv->stage, &width, &height);
-
-  /* Expand the viewport if the actor is partially off-stage,
-   * otherwise the actor will end up clipped to the stage viewport
    */
-  xexpand = 0.f;
+  clutter_actor_get_transform (priv->stage, &modelview);
-  if (priv->x_offset < 0.f)
+  cogl_set_modelview_matrix (&modelview);
-    xexpand = -priv->x_offset;
-  if (priv->x_offset + texture_width > width)
-    xexpand = MAX (xexpand, (priv->x_offset + texture_width) - width);
 
-  yexpand = 0.f;
+  /* Save the original viewport for calculating priv->position */
-  if (priv->y_offset < 0.f)
+  _clutter_stage_get_viewport (CLUTTER_STAGE (priv->stage),
-    yexpand = -priv->y_offset;
+                               &old_viewport[0],
-  if (priv->y_offset + texture_height > height)
+                               &old_viewport[1],
-    yexpand = MAX (yexpand, (priv->y_offset + texture_height) - height);
+                               &old_viewport[2],
+                               &old_viewport[3]);
 
-  /* Set the viewport */
+  /* Set up the viewport so that it has the same size as the stage (avoid
-  cogl_set_viewport (-(priv->x_offset + xexpand), -(priv->y_offset + yexpand),
+   * distortion), but translated to account for the FBO offset...
-                     width + (2 * xexpand), height + (2 * yexpand));
+   */
+  cogl_set_viewport (-priv->fbo_offset_x,
+                     -priv->fbo_offset_y,
+                     stage_width,
+                     stage_height);
 
   /* Copy the stage's projection matrix across to the framebuffer */
   _clutter_stage_get_projection_matrix (CLUTTER_STAGE (priv->stage),
                                         &projection);
 
-  /* If we've expanded the viewport, make sure to scale the projection
+  /* Now save the global position of the effect (not just of the actor).
-   * matrix accordingly (as it's been initialised to work with the
+   * It doesn't appear anyone actually uses this yet, but get_target_rect is
-   * original viewport and not our expanded one).
+   * documented as returning it. So we should...
    */
-  if (xexpand > 0.f || yexpand > 0.f)
+  _clutter_util_fully_transform_vertices (&old_modelview,
-    {
+                                          &projection,
-      gfloat new_width, new_height;
+                                          old_viewport,
-
+                                          &local_offset,
-      new_width = width + (2 * xexpand);
+                                          &priv->position,
-      new_height = height + (2 * yexpand);
-
-      cogl_matrix_scale (&projection,
-                         width / new_width,
-                         height / new_height,
                                           1);
-    }
 
   cogl_set_projection_matrix (&projection);
 
@@ -385,13 +369,14 @@ clutter_offscreen_effect_paint_texture (ClutterOffscreenEffect *effect)
 
   cogl_push_matrix ();
 
-  /* Now reset the modelview to put us in stage coordinates so
+  /* The current modelview matrix is *almost* perfect already. It's only
-   * we can drawn the result of our offscreen render as a textured
+   * missing a correction for the expanded FBO and offset rendering within...
-   * quad... */
+   */
-
+  cogl_get_modelview_matrix (&modelview);
-  cogl_matrix_init_identity (&modelview);
+  cogl_matrix_translate (&modelview,
-  _clutter_actor_apply_modelview_transform (priv->stage, &modelview);
+                         priv->fbo_offset_x,
-  cogl_matrix_translate (&modelview, priv->x_offset, priv->y_offset, 0.0f);
+                         priv->fbo_offset_y,
+                         0.0f);
   cogl_set_modelview_matrix (&modelview);
 
   /* paint the target material; this is virtualized for
@@ -428,16 +413,11 @@ clutter_offscreen_effect_paint (ClutterEffect           *effect,
 {
   ClutterOffscreenEffect *self = CLUTTER_OFFSCREEN_EFFECT (effect);
   ClutterOffscreenEffectPrivate *priv = self->priv;
-  CoglMatrix matrix;
 
-  cogl_get_modelview_matrix (&matrix);
+  /* If we've already got a cached image and the actor hasn't been redrawn
-
+   * then we can just use the cached image in the FBO.
-  /* If we've already got a cached image for the same matrix and the
+   */
-     actor hasn't been redrawn then we can just use the cached image
+  if (priv->offscreen == NULL || (flags & CLUTTER_EFFECT_PAINT_ACTOR_DIRTY))
-     in the fbo */
-  if (priv->offscreen == NULL ||
-      (flags & CLUTTER_EFFECT_PAINT_ACTOR_DIRTY) ||
-      !cogl_matrix_equal (&matrix, &priv->last_matrix_drawn))
     {
       /* Chain up to the parent paint method which will call the pre and
          post paint functions to update the image */
@@ -663,8 +643,8 @@ clutter_offscreen_effect_get_target_rect (ClutterOffscreenEffect *effect,
     return FALSE;
 
   clutter_rect_init (rect,
-                     priv->x_offset,
+                     priv->position.x,
-                     priv->y_offset,
+                     priv->position.y,
                      cogl_texture_get_width (priv->texture),
                      cogl_texture_get_height (priv->texture));
 

clutter/clutter/clutter-paint-volume.c

@@ -35,6 +35,7 @@
 #include "clutter-paint-volume-private.h"
 #include "clutter-private.h"
 #include "clutter-stage-private.h"
+#include "clutter-actor-box-private.h"
 
 G_DEFINE_BOXED_TYPE (ClutterPaintVolume, clutter_paint_volume,
                      clutter_paint_volume_copy,
@@ -1138,8 +1139,6 @@ _clutter_paint_volume_get_stage_paint_box (ClutterPaintVolume *pv,
   CoglMatrix modelview;
   CoglMatrix projection;
   float viewport[4];
-  float width;
-  float height;
 
   _clutter_paint_volume_copy_static (pv, &projected_pv);
 
@@ -1179,50 +1178,7 @@ _clutter_paint_volume_get_stage_paint_box (ClutterPaintVolume *pv,
       return;
     }
 
-  /* The aim here is that for a given rectangle defined with floating point
+  _clutter_actor_box_enlarge_for_effects (box);
-   * coordinates we want to determine a stable quantized size in pixels
-   * that doesn't vary due to the original box's sub-pixel position.
-   *
-   * The reason this is important is because effects will use this
-   * API to determine the size of offscreen framebuffers and so for
-   * a fixed-size object that may be animated accross the screen we
-   * want to make sure that the stage paint-box has an equally stable
-   * size so that effects aren't made to continuously re-allocate
-   * a corresponding fbo.
-   *
-   * The other thing we consider is that the calculation of this box is
-   * subject to floating point precision issues that might be slightly
-   * different to the precision issues involved with actually painting the
-   * actor, which might result in painting slightly leaking outside the
-   * user's calculated paint-volume. For this we simply aim to pad out the
-   * paint-volume by at least half a pixel all the way around.
-   */
-  width = box->x2 - box->x1;
-  height = box->y2 - box->y1;
-  width = CLUTTER_NEARBYINT (width);
-  height = CLUTTER_NEARBYINT (height);
-  /* XXX: NB the width/height may now be up to 0.5px too small so we
-   * must also pad by 0.25px all around to account for this. In total we
-   * must padd by at least 0.75px around all sides. */
-
-  /* XXX: The furthest that we can overshoot the bottom right corner by
-   * here is 1.75px in total if you consider that the 0.75 padding could
-   * just cross an integer boundary and so ceil will effectively add 1.
-   */
-  box->x2 = ceilf (box->x2 + 0.75);
-  box->y2 = ceilf (box->y2 + 0.75);
-
-  /* Now we redefine the top-left relative to the bottom right based on the
-   * rounded width/height determined above + a constant so that the overall
-   * size of the box will be stable and not dependant on the box's
-   * position.
-   *
-   * Adding 3px to the width/height will ensure we cover the maximum of
-   * 1.75px padding on the bottom/right and still ensure we have > 0.75px
-   * padding on the top/left.
-   */
-  box->x1 = box->x2 - width - 3;
-  box->y1 = box->y2 - height - 3;
 
   clutter_paint_volume_free (&projected_pv);
 }

clutter/tests/conform/actor-offscreen-limit-max-size.c

@@ -1,119 +0,0 @@
-#define CLUTTER_ENABLE_EXPERIMENTAL_API
-#include <clutter/clutter.h>
-
-#define STAGE_WIDTH (300)
-#define STAGE_HEIGHT (300)
-
-typedef struct
-{
-  ClutterActor *stage;
-
-  ClutterActor *actor_group1;
-  ClutterEffect *blur_effect1;
-
-  ClutterActor *actor_group2;
-  ClutterEffect *blur_effect2;
-} Data;
-
-static void
-check_results (ClutterStage *stage, gpointer user_data)
-{
-  Data *data = user_data;
-  gfloat width, height;
-  ClutterRect rect;
-
-  clutter_offscreen_effect_get_target_rect (CLUTTER_OFFSCREEN_EFFECT (data->blur_effect1),
-                                            &rect);
-
-  width = clutter_rect_get_width (&rect);
-  height = clutter_rect_get_height (&rect);
-
-  if (g_test_verbose ())
-    g_print ("Checking effect1 size: %.2f x %.2f\n",
-             clutter_rect_get_width (&rect),
-             clutter_rect_get_height (&rect));
-
-  g_assert_cmpint (width, <, STAGE_WIDTH);
-  g_assert_cmpint (height, <, STAGE_HEIGHT);
-
-  clutter_offscreen_effect_get_target_rect (CLUTTER_OFFSCREEN_EFFECT (data->blur_effect2),
-                                            &rect);
-
-  width = clutter_rect_get_width (&rect);
-  height = clutter_rect_get_height (&rect);
-
-  if (g_test_verbose ())
-    g_print ("Checking effect2 size: %.2f x %.2f\n", width, height);
-
-  g_assert_cmpint (width, ==, STAGE_WIDTH);
-  g_assert_cmpint (height, ==, STAGE_HEIGHT);
-
-
-  clutter_main_quit ();
-}
-
-static ClutterActor *
-create_actor (gfloat x, gfloat y,
-              gfloat width, gfloat height,
-              const ClutterColor *color)
-{
-  return g_object_new (CLUTTER_TYPE_ACTOR,
-                       "x", x,
-                       "y", y,
-                       "width", width,
-                       "height", height,
-                       "background-color", color,
-                       NULL);
-}
-
-static void
-actor_offscreen_limit_max_size (void)
-{
-  Data data;
-
-  if (!cogl_features_available (COGL_FEATURE_OFFSCREEN))
-    return;
-
-  data.stage = clutter_test_get_stage ();
-  g_signal_connect (data.stage, "after-paint",
-                    G_CALLBACK (check_results), &data);
-  clutter_actor_set_size (data.stage, STAGE_WIDTH, STAGE_HEIGHT);
-
-  data.actor_group1 = clutter_actor_new ();
-  clutter_actor_add_child (data.stage, data.actor_group1);
-  data.blur_effect1 = clutter_blur_effect_new ();
-  clutter_actor_add_effect (data.actor_group1, data.blur_effect1);
-  clutter_actor_add_child (data.actor_group1,
-                           create_actor (10, 10,
-                                         100, 100,
-                                         CLUTTER_COLOR_Blue));
-  clutter_actor_add_child (data.actor_group1,
-                           create_actor (100, 100,
-                                         100, 100,
-                                         CLUTTER_COLOR_Gray));
-
-  data.actor_group2 = clutter_actor_new ();
-  clutter_actor_add_child (data.stage, data.actor_group2);
-  data.blur_effect2 = clutter_blur_effect_new ();
-  clutter_actor_add_effect (data.actor_group2, data.blur_effect2);
-  clutter_actor_add_child (data.actor_group2,
-                           create_actor (-10, -10,
-                                         100, 100,
-                                         CLUTTER_COLOR_Yellow));
-  clutter_actor_add_child (data.actor_group2,
-                           create_actor (250, 10,
-                                         100, 100,
-                                         CLUTTER_COLOR_ScarletRed));
-  clutter_actor_add_child (data.actor_group2,
-                           create_actor (10, 250,
-                                         100, 100,
-                                         CLUTTER_COLOR_Yellow));
-
-  clutter_actor_show (data.stage);
-
-  clutter_main ();
-}
-
-CLUTTER_TEST_SUITE (
-  CLUTTER_TEST_UNIT ("/actor/offscreen/limit-max-size", actor_offscreen_limit_max_size)
-)

clutter/tests/conform/actor-offscreen-redirect.c

@@ -189,10 +189,11 @@ verify_redraws (gpointer user_data)
   clutter_actor_queue_redraw (data->child);
   verify_redraw (data, 1);
 
-  /* Modifying the transformation on the parent should cause a
+  /* Modifying the transformation on the parent should not cause a redraw,
-     redraw */
+     since the FBO stores pre-transformed rendering that can be reused with
+     any transformation. */
   clutter_actor_set_anchor_point (data->parent_container, 0, 1);
-  verify_redraw (data, 1);
+  verify_redraw (data, 0);
 
   /* Redrawing an unrelated actor shouldn't cause a redraw */
   clutter_actor_set_position (data->unrelated_actor, 0, 1);

clutter/tests/conform/meson.build

@@ -19,7 +19,6 @@ clutter_conform_tests_actor_tests = [
   'actor-iter',
   'actor-layout',
   'actor-meta',
-  'actor-offscreen-limit-max-size',
   'actor-offscreen-redirect',
   'actor-paint-opacity',
   'actor-pick',