GNOME Bugzilla – Bug 689858
St: optimize box-shadow rendering
Last modified: 2013-07-09 10:25:43 UTC
Currently the box-shadow is rendered using an offscreen buffer of the size of the actor. That means every time we resize an actor with a box-shadow, it creates a new offscreen buffer is rendered and blurred. This is proven to be problematic for animations : https://bugzilla.gnome.org/show_bug.cgi?id=687660
Created attachment 230986 [details] [review] Optimize box-shadow rendering
Created attachment 232042 [details] [review] Optimize box-shadow rendering v2 As pointed by folks on IRC, the previous patch had quite a few issues. This new patch should be a lot easier to read. It only uses a 9-slices when the allocation of the widget allows it and falls back to the previous rendering method if the size is too small.
Created attachment 232043 [details] [review] Animated box-shadow interactive test Here is a standalone patch to look at the performance difference after and before the optimization.
Review of attachment 232042 [details] [review]: Preliminary review. ::: src/st/st-theme-node-drawing.c @@ +1333,3 @@ _st_theme_node_free_drawing_state (node); + texture_cache = st_texture_cache_get_default (); Why the move? @@ +1837,3 @@ + bottom = MAX (bottom, shadow_blur_radius + box_shadow_spec->blur); + left = MAX (left, shadow_blur_radius + box_shadow_spec->blur); + right = MAX (right, shadow_blur_radius + box_shadow_spec->blur); Yikes, lots of fancy math here. @@ +1839,3 @@ + right = MAX (right, shadow_blur_radius + box_shadow_spec->blur); + + s_top = (top + shadow_blur_radius) / shadow_height; These need better variable names. @@ +2261,3 @@ + box-shadow size boundary, one way or another. If that's the case, + we need to recompute the box-shadow */ + if (pre_small_size ^ post_small_size) ^ is != Since post_small_size can't be TRUE, that means that post_small_size is FALSE, so the only way that this can pass is if pre_small_size is TRUE. Replace with: if (post_small_size || pre_small_size) return TRUE; Or there's a logic error.
Created attachment 232387 [details] [review] Optimize box-shadow rendering v3 Update after review.
Created attachment 232391 [details] [review] Optimize box-shadow rendering v4 Quick update to sample the center slice with only one pixel from the offscreen buffer.
Comment on attachment 232391 [details] [review] Optimize box-shadow rendering v4 I'm still losing border corners on alt-tab after the first use.
(In reply to comment #7) > (From update of attachment 232391 [details] [review]) > I'm still losing border corners on alt-tab after the first use. Ok, found the problem: you need to copy rendered_once inside copy_cached_paint_state, otherwise a theme node could think it has cached state while it hasn't. (Alternatively, I guess copy_cached_paint_state() could recognize if asked to copy from a node without state, and skip the copy althogheter)
Thanks a lot for spotting the problem. For some reason I couldn't reproduce it... I also added copies of a few variables describing the initial/minimal shadow sizes.
Created attachment 232435 [details] [review] Optimize box-shadow rendering v5
Review of attachment 232435 [details] [review]: Yet Another Shadow Renderer how many are we at? 4? 5? Not a detailed review. ::: src/st/st-theme-node-drawing.c @@ +1456,3 @@ } + + node->rendered_once = TRUE; How is rendered_once different from having a non-null prerendered material? @@ +1480,3 @@ + + if (node->border_slices_texture == COGL_INVALID_HANDLE && + node->box_shadow_material != COGL_INVALID_HANDLE) This is a lot of fancy complex state logic here. Can we try and make it simpler? @@ +1493,3 @@ + box_shadow_spec = st_theme_node_get_box_shadow (node); + + if (had_prerendered_texture) Why does whether we had a prerendered texture before matter? @@ +1497,3 @@ + node->prerendered_texture = st_theme_node_prerender_background (node); + + if (node->prerendered_texture) Could this happen? @@ +1851,3 @@ + s_bottom = shadow_blur_radius + box_shadow_spec->blur + + MAX (node->border_radius[ST_CORNER_TOPLEFT], + node->border_radius[ST_CORNER_TOPRIGHT]); Copy/paste error? @@ +2310,3 @@ + + pre_small_size = (node->alloc_width < node->box_shadow_min_width || + node->alloc_height < node->box_shadow_min_height); The names 'pre_small_size' and 'post_small_size' aren't very good names. Putting them directly in the conditions might be better. @@ +2347,3 @@ return; + if (st_theme_node_needs_new_box_shadow_for_size (node, width, height)) This really isn't box-size specific, so needs_new_resources is more appropriate.
Review of attachment 232435 [details] [review]: ::: src/st/st-theme-node-drawing.c @@ +1456,3 @@ } + + node->rendered_once = TRUE; The prerendered material can be NULL, that's why we need an additional variable. @@ +1480,3 @@ + + if (node->border_slices_texture == COGL_INVALID_HANDLE && + Let me unswap that problem in memory. @@ +1493,3 @@ + box_shadow_spec = st_theme_node_get_box_shadow (node); + + had_prerendered_texture = TRUE; The prerendered texture depends on the size of the widget. If we had one before, we need to rerender it. @@ +1497,3 @@ + node->prerendered_texture = st_theme_node_prerender_background (node); + + if (node->prerendered_material != COGL_INVALID_HANDLE) It's basically the same code that in the st_theme_node_render_resources() function. But I can see a potential problem here. Thanks for spotting this. @@ +1851,3 @@ + s_bottom = shadow_blur_radius + box_shadow_spec->blur + + MAX (node->border_radius[ST_CORNER_TOPLEFT], + * top ---------------------------- Yes indeed, thanks. @@ +2310,3 @@ + + pre_small_size = (node->alloc_width < node->box_shadow_min_width || + /* The allocation hasn't changed, no need to recompute a new Will do. @@ +2347,3 @@ return; + if (st_theme_node_needs_new_box_shadow_for_size (node, width, height)) It is box-size specific. Under a certain size (computed from rounded radius in the corners) we need to regenerate the shadow, the 9-slice method doesn't work anymore.
Created attachment 233748 [details] [review] Optimize box-shadow rendering v6
Regarding "Yet Another Shadow Renderer", I didn't write a new one. And to my knowledge the one in mutter is the only other one using cogl to paint the shadow at the end. Yes it would be great to have that in only one place. But is it going to happen? I suspect people don't want to remove the shadow code from mutter neither planning to render St shadows with the mutter API...
If background settings go here because that's where people are used to them being in Windows, should we also put some shortcuts to other display-related settings? Change resolution, change multi-monitor modes, that sort of thing?
(sorry, wrong bug)
Review of attachment 233748 [details] [review]: This would need rebasing for the new StThemeNode optimizations we've landed in master, but otherwise I think this looks OK.
Created attachment 248379 [details] [review] st: optimize box-shadow rendering
Created attachment 248380 [details] [review] tests: add animated box-shadow test to demo optimized painting
Created attachment 248381 [details] [review] st-theme-node: refactor calls to render css box
Created attachment 248382 [details] [review] st-theme-node: reuse box-shadow material between paint states when possible
Pushed to master.