[mutter] clutter: Fix offscreen-effect painting of clones

From: Marco Trevisan <marcotrevi src gnome org>
To: commits-list gnome org
Cc:
Subject: [mutter] clutter: Fix offscreen-effect painting of clones
Date: Thu, 24 Jan 2019 17:05:49 +0000 (UTC)
commit 8655bc5d8de6a969e0ca83eff8e450f62d28fbee
Author: Daniel van Vugt <daniel van vugt canonical com>
Date:   Thu May 24 17:51:22 2018 +0800

    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        |  12 ++
 clutter/clutter/clutter-actor-box.c                |  52 +++++++
 clutter/clutter/clutter-offscreen-effect.c         | 172 +++++++++------------
 clutter/clutter/clutter-paint-volume.c             |  48 +-----
 .../tests/conform/actor-offscreen-limit-max-size.c | 119 --------------
 clutter/tests/conform/actor-offscreen-redirect.c   |   7 +-
 clutter/tests/conform/meson.build                  |   1 -
 7 files changed, 146 insertions(+), 265 deletions(-)
---
diff --git a/clutter/clutter/clutter-actor-box-private.h b/clutter/clutter/clutter-actor-box-private.h
new file mode 100644
index 000000000..e7aeb8857
--- /dev/null
+++ b/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__ */
diff --git a/clutter/clutter/clutter-actor-box.c b/clutter/clutter/clutter-actor-box.c
index d6ad0d662..8be2f377e 100644
--- a/clutter/clutter/clutter-actor-box.c
+++ b/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,
diff --git a/clutter/clutter/clutter-offscreen-effect.c b/clutter/clutter/clutter-offscreen-effect.c
index 8a90be260..9dae899ed 100644
--- a/clutter/clutter/clutter-offscreen-effect.c
+++ b/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;
-  gfloat y_offset;
+  ClutterVertex position;
+
+  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;
-  gfloat xexpand, yexpand;
-  int texture_width, texture_height;
+  ClutterVertex local_offset = { 0.f, 0.f, 0.f };
+  gfloat old_viewport[4];
 
   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
-   * stage coordinates. This will give us the size for the framebuffer
-   * we need to redirect its rendering offscreen and its position will
-   * be used to setup an offset viewport */
-  if (clutter_actor_get_paint_box (priv->actor, &box))
+  /* Get the minimal bounding box for what we want to paint, relative to the
+   * parent of priv->actor. Note that we may actually be painting a clone of
+   * priv->actor so we need to be careful to avoid querying the transformation
+   * of priv->actor (like clutter_actor_get_paint_box would). Just stay in
+   * local coordinates for now...
+   */
+  volume = clutter_actor_get_paint_volume (priv->actor);
+  if (volume)
     {
-      clutter_actor_box_get_size (&box, &fbo_width, &fbo_height);
-      clutter_actor_box_get_origin (&box, &priv->x_offset, &priv->y_offset);
+      ClutterPaintVolume mutable_volume;
 
-      fbo_width = MIN (fbo_width, stage_width);
-      fbo_height = MIN (fbo_height, stage_height);
+      _clutter_paint_volume_copy_static (volume, &mutable_volume);
+      _clutter_paint_volume_get_bounding_box (&mutable_volume, &raw_box);
+      clutter_paint_volume_free (&mutable_volume);
     }
   else
     {
-      fbo_width = stage_width;
-      fbo_height = stage_height;
+      clutter_actor_get_allocation_box (priv->actor, &raw_box);
     }
 
-  if (fbo_width == stage_width)
-    priv->x_offset = 0.0f;
-  if (fbo_height == stage_height)
-    priv->y_offset = 0.0f;
+  box = raw_box;
+  _clutter_actor_box_enlarge_for_effects (&box);
+
+  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);
-  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);
+  cogl_get_modelview_matrix (&old_modelview);
 
   /* let's draw offscreen */
   cogl_push_framebuffer (priv->offscreen);
 
-  /* Copy the modelview that would have been used if rendering onscreen */
-  cogl_set_modelview_matrix (&priv->last_matrix_drawn);
+  /* We don't want the FBO contents to be transformed. That could waste memory
+   * (e.g. during zoom), or result in something that's not rectangular (clipped
+   * incorrectly). So drop the modelview matrix of the current paint chain.
+   * This is fine since paint_texture runs with the same modelview matrix,
+   * so it will come out correctly whenever that is used to put the FBO
+   * contents on screen...
+   */
+  clutter_actor_get_transform (priv->stage, &modelview);
+  cogl_set_modelview_matrix (&modelview);
 
-  /* Set up the viewport so that it has the same size as the stage,
-   * but offset it so that the actor of interest lands on our
-   * framebuffer. */
-  clutter_actor_get_size (priv->stage, &width, &height);
+  /* Save the original viewport for calculating priv->position */
+  _clutter_stage_get_viewport (CLUTTER_STAGE (priv->stage),
+                               &old_viewport[0],
+                               &old_viewport[1],
+                               &old_viewport[2],
+                               &old_viewport[3]);
 
-  /* Expand the viewport if the actor is partially off-stage,
-   * otherwise the actor will end up clipped to the stage viewport
+  /* Set up the viewport so that it has the same size as the stage (avoid
+   * distortion), but translated to account for the FBO offset...
    */
-  xexpand = 0.f;
-  if (priv->x_offset < 0.f)
-    xexpand = -priv->x_offset;
-  if (priv->x_offset + texture_width > width)
-    xexpand = MAX (xexpand, (priv->x_offset + texture_width) - width);
-
-  yexpand = 0.f;
-  if (priv->y_offset < 0.f)
-    yexpand = -priv->y_offset;
-  if (priv->y_offset + texture_height > height)
-    yexpand = MAX (yexpand, (priv->y_offset + texture_height) - height);
-
-  /* Set the viewport */
-  cogl_set_viewport (-(priv->x_offset + xexpand), -(priv->y_offset + yexpand),
-                     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
-   * matrix accordingly (as it's been initialised to work with the
-   * original viewport and not our expanded one).
+  /* Now save the global position of the effect (not just of the actor).
+   * It doesn't appear anyone actually uses this yet, but get_target_rect is
+   * documented as returning it. So we should...
    */
-  if (xexpand > 0.f || yexpand > 0.f)
-    {
-      gfloat new_width, new_height;
-
-      new_width = width + (2 * xexpand);
-      new_height = height + (2 * yexpand);
-
-      cogl_matrix_scale (&projection,
-                         width / new_width,
-                         height / new_height,
-                         1);
-    }
+  _clutter_util_fully_transform_vertices (&old_modelview,
+                                          &projection,
+                                          old_viewport,
+                                          &local_offset,
+                                          &priv->position,
+                                          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
-   * we can drawn the result of our offscreen render as a textured
-   * quad... */
-
-  cogl_matrix_init_identity (&modelview);
-  _clutter_actor_apply_modelview_transform (priv->stage, &modelview);
-  cogl_matrix_translate (&modelview, priv->x_offset, priv->y_offset, 0.0f);
+  /* The current modelview matrix is *almost* perfect already. It's only
+   * missing a correction for the expanded FBO and offset rendering within...
+   */
+  cogl_get_modelview_matrix (&modelview);
+  cogl_matrix_translate (&modelview,
+                         priv->fbo_offset_x,
+                         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 for the same matrix and the
-     actor hasn't been redrawn then we can just use the cached image
-     in the fbo */
-  if (priv->offscreen == NULL ||
-      (flags & CLUTTER_EFFECT_PAINT_ACTOR_DIRTY) ||
-      !cogl_matrix_equal (&matrix, &priv->last_matrix_drawn))
+  /* 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 (priv->offscreen == NULL || (flags & CLUTTER_EFFECT_PAINT_ACTOR_DIRTY))
     {
       /* 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->y_offset,
+                     priv->position.x,
+                     priv->position.y,
                      cogl_texture_get_width (priv->texture),
                      cogl_texture_get_height (priv->texture));
 
diff --git a/clutter/clutter/clutter-paint-volume.c b/clutter/clutter/clutter-paint-volume.c
index c0deaebf3..6adf626a4 100644
--- a/clutter/clutter/clutter-paint-volume.c
+++ b/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
-   * 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_actor_box_enlarge_for_effects (box);
 
   clutter_paint_volume_free (&projected_pv);
 }
diff --git a/clutter/tests/conform/actor-offscreen-redirect.c 
b/clutter/tests/conform/actor-offscreen-redirect.c
index 63d5e6cfd..9dbb7d2c5 100644
--- a/clutter/tests/conform/actor-offscreen-redirect.c
+++ b/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
-     redraw */
+  /* Modifying the transformation on the parent should not cause a 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);
diff --git a/clutter/tests/conform/meson.build b/clutter/tests/conform/meson.build
index 4c60354b2..acb73d488 100644
--- a/clutter/tests/conform/meson.build
+++ b/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',
[Date Prev][Date Next] [Thread Prev][Thread Next] [Thread Index] [Date Index] [Author Index]