From 8655bc5d8de6a969e0ca83eff8e450f62d28fbee Mon Sep 17 00:00:00 2001 From: Daniel van Vugt Date: Thu, 24 May 2018 17:51:22 +0800 Subject: [PATCH] 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 +---- .../conform/actor-offscreen-limit-max-size.c | 119 ------------ .../tests/conform/actor-offscreen-redirect.c | 7 +- clutter/tests/conform/meson.build | 1 - 7 files changed, 146 insertions(+), 265 deletions(-) create mode 100644 clutter/clutter/clutter-actor-box-private.h delete mode 100644 clutter/tests/conform/actor-offscreen-limit-max-size.c 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 + +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); - - /* 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); - - /* Expand the viewport if the actor is partially off-stage, - * otherwise the actor will end up clipped to the stage viewport + /* 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... */ - 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); + clutter_actor_get_transform (priv->stage, &modelview); + cogl_set_modelview_matrix (&modelview); - 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); + /* 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]); - /* Set the viewport */ - cogl_set_viewport (-(priv->x_offset + xexpand), -(priv->y_offset + yexpand), - width + (2 * xexpand), height + (2 * yexpand)); + /* Set up the viewport so that it has the same size as the stage (avoid + * distortion), but translated to account for the FBO offset... + */ + 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-limit-max-size.c b/clutter/tests/conform/actor-offscreen-limit-max-size.c deleted file mode 100644 index 943c7d8a7..000000000 --- a/clutter/tests/conform/actor-offscreen-limit-max-size.c +++ /dev/null @@ -1,119 +0,0 @@ -#define CLUTTER_ENABLE_EXPERIMENTAL_API -#include - -#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) -) 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',