GNOME Bugzilla – Bug 699675
Offscreen effects allocating too much memory
Last modified: 2013-05-15 14:41:36 UTC
When using a ClutterOffscreenEffect, the size of the offscreen buffer allocated to perform the effect is currently computed using the paint volume and in the case the paint volume cannot be computed, the effect falls back to using the stage's size. Now if you zoom an actor enough so its paint volume is much bigger that the size of the screen, you can end up running out of memory. In some cases, we need to allocate an offscreen buffer larger than the stage's size. But in some cases, it's not really interesting, we just want to some something and anything outside the stage, doesn't even need to be painted. Attached is a patch to add a property to the ClutterOffscreenEffect class to let the developer choose whether he wants to use the paint volume or not.
Created attachment 243294 [details] [review] offscreen-effect: add use-paint-volume property
Created attachment 243296 [details] test scaling with blur effect Adding a test to show the crash, just keep scrolling up to zoom and at some point it will crash running out of memory, or unable to allocate an offscreen buffer big enough.
Created attachment 243297 [details] test scaling with blur effect
Created attachment 243298 [details] same test showing non crash after applied patch
Review of attachment 243294 [details] [review]: I'd actually would like to have API in Cogl to get the GL max texture size, and clamp the FBO size using that, without having an "unbreak my API" property. alternatively, we may want to always clamp the FBO to the size of the stage ± a factor, regardless of the fact that we're using the paint volume. it's not that I don't see the problem, though; it's just that I don't want a "don't crash my application" API that is still opt-in.
It would be nice to check the max texture size indeed, looking at this. The only other question I had was about clones. There might be cases where we need more than the stage's size. That's why I went with the paint volume opt-out option, when you know what's you're doing, you can limit the size to the stage's size.
Created attachment 244193 [details] [review] offscreen-effect: limit offscreen fbo size to the stage's size
Removed the property and always limit the size of the fbo to the stage's size.
Created attachment 244194 [details] [review] offscreen-effect: limit offscreen fbo size to the stage's size
Review of attachment 244193 [details] [review]: ::: clutter/clutter-offscreen-effect.c @@ +109,3 @@ }; +enum this chunk should go away. @@ +259,3 @@ + clutter_actor_get_size (stage, &stage_width, &stage_height); + + if (stage_width < fbo_width && could be replaced by: fbo_width = MIN (fbo_width, stage_width); fbo_height = MIN (fbo_height, stage_height); if (fbo_width == stage_width) priv->x_offset = 0.f; if (fbo_height == stage_height) priv->y_offset = 0.f; so we allow FBOs to be smaller than the stage size in either direction.
seems I reviewed too fast — but comment 10 still applies.
Created attachment 244223 [details] [review] offscreen-effect: limit offscreen fbo size to the stage's size
Review of attachment 244223 [details] [review]: looks okay to me; I assume you tested it. :-) it could be interesting to have a conformance test that uses a custom OffscreenEffect which checks the size of the target and verifies that the size is less than (or equal) to the size of the Stage.
Created attachment 244314 [details] [review] offscreen-effect: limit offscreen fbo size to the stage's size Thanks for forcing me writing a test, the last rework was indeed wrong... Adding a conform test.
Created attachment 244315 [details] [review] conform: add offscreen effects fbo size check
Review of attachment 244315 [details] [review]: looks generally okay. ::: tests/conform/actor-offscreen-limit-max-size.c @@ +25,3 @@ + clutter_offscreen_effect_get_target_size (CLUTTER_OFFSCREEN_EFFECT (data->blur_effect1), + &width, &height); + would be nice to have a debug message, like: if (g_test_verbose ()) g_print ("Checking effect1 size: %.2f x %.2f\n", width, height); @@ +31,3 @@ + clutter_offscreen_effect_get_target_size (CLUTTER_OFFSCREEN_EFFECT (data->blur_effect2), + &width, &height); + same as above. @@ +46,3 @@ + const ClutterColor *color) +{ + return (ClutterActor *) g_object_new (CLUTTER_TYPE_ACTOR, no need for the cast: g_object_new() returns a gpointer. @@ +66,3 @@ + clutter_actor_set_size (data.stage, STAGE_WIDTH, STAGE_HEIGHT); + + data.actor_group1 = clutter_group_new (); please, don't use deprecated API. ClutterGroup can be effectively replaced by ClutterActor. @@ +80,3 @@ + + + data.actor_group2 = clutter_group_new (); same as above. @@ +101,3 @@ + /* Start the test after a short delay to allow the stage to + render its initial frames without affecting the results */ + g_timeout_add_full (G_PRIORITY_LOW, 250, timeout_cb, &data, NULL); could use a paint function here, but I guess a timeout works.
Review of attachment 244314 [details] [review]: yeah, this is nicer.
Created attachment 244321 [details] [review] conform: add offscreen effects fbo size check
Review of attachment 244321 [details] [review]: looks good.
Attachment 244314 [details] pushed as 9c6f379 - offscreen-effect: limit offscreen fbo size to the stage's size Attachment 244321 [details] pushed as d1041e1 - conform: add offscreen effects fbo size check